feat(aggregate): add Java-compatible field aggregators by liangjie3138 · Pull Request #463 · alibaba/paimon-cpp

liangjie3138 · 2026-07-30T06:11:49Z

Purpose

Linked issue: close #457

Port the six practical field aggregators missing on the C++ side — collect,
merge_map, nested_update, rbm64, hll_sketch, theta_sketch .

Also adds GenericArray / GenericMap, the in-memory InternalArray /
InternalMap implementations these aggregators need to build ARRAY and MAP
results.

Tests

18 new unit test cases in paimon-core-test, covering each aggregator's
aggregate / retract / null / type-validation paths.

Three pin behaviour that is easy to "fix" incorrectly, and each was checked by
reverting the corresponding production change and confirming it fails:

BinaryAggMergeFunctionTest.OwnedAccumulatorSurvivesNullInput — drives a real
AggregateMergeFunction so the use-after-free is exercised through the
production path.
FieldNestedUpdateAggTest.CountLimitCountsNullElementsOfAccumulator — count
limit is measured against the raw element count, matching Java.
FieldIgnoreRetractAggTest.ReversedAggBypassesWrappedOverride — the wrapper
does not preserve the wrapped aggregator's reversed-aggregation override, also
matching Java.

No integration tests added.

API and Format

Additive only, no signatures changed:

include/paimon/defs.h: four new Options constants — NESTED_KEY,
NESTED_KEY_NULL_STRATEGY, NESTED_SEQUENCE_FIELD, COUNT_LIMIT.
CoreOptions: a NestedKeyNullStrategy enum plus accessors.
FieldAggregator: new virtuals default to the existing Agg / AggReversed.

New dependency: Apache DataSketches 5.2.0 (Apache-2.0), resolved through
resolve_dependency() like the other bundled dependencies.

Documentation

None.

Generative AI tooling

Generated-by: Claude Code 2.1.220 (Claude Opus 5)

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

lxy-9602 · 2026-08-03T02:48:11Z

+    PAIMON_RETURN_NOT_OK(input_bitmap.Deserialize(input_bytes.data(), input_bytes.size()));
+    accumulator_bitmap |= input_bitmap;
+    return VariantType(std::shared_ptr<Bytes>(accumulator_bitmap.Serialize(/*pool=*/nullptr)));
+}


There is an issue here: the bitmap serialization currently used in Java is not cross-language compatible. To ensure compatibility, it needs to use Roaring64NavigableMap.serializePortable() instead. In some scenarios, data written by Java cannot be deserialized by C++ or other languages, and vice versa. Should we temporarily remove bitmap64 aggregation support until Java serialization is adjusted for compatibility, to avoid cases where data written in one language cannot be read in another?

lxy-9602 · 2026-08-03T05:25:10Z

-            merged_field = aggregators_[i]->Agg(accumulator, input_field);
+            PAIMON_ASSIGN_OR_RAISE(merged_field,
+                                   aggregators_[i]->AggResult(accumulator, input_field));
        }


We can still use Agg in the naming here; the only difference is that it now has a return value.

lxy-9602 · 2026-08-03T05:57:26Z

+                                          const VariantType& input_field) {
+        return Agg(accumulator, input_field);
+    }
+


Could we remove the newly added AggResult / AggReversedResult functions and simply update the return values of Agg / AggReversed instead?

lxy-9602 · 2026-08-03T06:00:03Z

+    if (!lhs || !rhs || lhs->GetFieldCount() != rhs->GetFieldCount() ||
+        lhs->GetFieldCount() != type->num_fields()) {
+        return lhs == rhs;
+    }


Should we also add a comparison for getRowKind() here?

lxy-9602 · 2026-08-03T06:25:22Z

+                               FieldAggregateUtils::Equals(value, candidate, element_type));
+        if (equal) {
+            return true;
+        }


It looks like there may be some performance regression here. Given the size of this PR, maybe we could add a TODO or note documenting the current O(n²) complexity and optimize it in a follow-up.

lxy-9602 · 2026-08-03T07:19:26Z

 }

+Result<std::vector<std::string>> CoreOptions::FieldNestedUpdateAggNestedKey(
+    const std::string& field_name) const {


Please add tests for the newly added CoreOptions APIs to CoreOptionsTest.

lxy-9602 · 2026-08-03T07:42:35Z

+    pooled_unique_ptr<Bytes> result =
+        Bytes::AllocateBytes(serialized.size() * sizeof(T), GetDefaultPool().get());
+    if (!serialized.empty()) {
+        std::memcpy(result->data(), serialized.data(), result->size());


Just to clarify: I noticed many places are using GetDefaultPool(). Will the data volume during merging be relatively large? In paimon-cpp, batches are merged together, and data that has not yet been converted into Arrow arrays may stay held in memory for a while. Is it appropriate for that memory to be allocated from a default pool that is separate from the user-provided memory pool?

lxy-9602 · 2026-08-03T08:05:04Z

+    bool input_null = DataDefine::IsVariantNull(input_field);
+    if (accumulator_null || input_null) {
+        return accumulator_null ? input_field : FieldAggregateUtils::OwnedBinary(accumulator);
+    }


Annotation 1

[P1] Preserve binary ownership in the reversed null path

When PartialUpdateMergeFunction processes an older sequence-group record, it calls AggReversedResult(accumulator, field). The base implementation swaps the arguments and invokes AggResult(field, accumulator).

For hll_sketch, and theta_sketch, if the older field is null, the null branch returns input_field directly. This value is a non-owning std::string_view produced by the use_view=true getter. If the current accumulator was created by a previous aggregation, its buffer is owned solely by the shared_ptr<Bytes> stored in row_->fields_[i].

The subsequent row_->SetField(i, result) replaces that shared_ptr<Bytes> with the returned view, releases the underlying buffer, and leaves a dangling view in the row. Later access may cause corrupted data, deserialization failures, or a use-after-free.

A valid trigger sequence is:

seq=10, sketch=A seq=20, sketch=B // union creates owned Bytes seq=15, sketch=NULL // enters the reversed null path

Please ensure that either null-argument order returns an owning copy of the non-null binary value, while preserving null when both arguments are null. Also add a partial-update regression test covering a sequence group, a previously aggregated binary value, and an older record with a null field. The existing OwnedAccumulatorSurvivesNullInput test only covers the forward AggResult() path.

lxy-9602 · 2026-08-03T08:07:41Z

+                                               name, option_name, row_type->ToString()));
+        }
+        if (std::find(fields.begin(), fields.end(), index) != fields.end()) {
+            return Status::Invalid(


I recall that GetFieldIndex() also returns -1 when it encounters duplicate fields, so this find check seems unnecessary.

lxy-9602 · 2026-08-03T08:20:44Z

+}
+
+Result<bool> FieldNestedUpdateAgg::RowsEqual(const InternalRow& lhs, const InternalRow& rhs) const {
+    for (int32_t field = 0; field < row_type_->num_fields(); ++field) {


Why not use FieldAggregateUtils::Equals directly here?

lxy-9602 · 2026-08-03T08:56:00Z

+                         CoreOptions::FromMap({{"fields.f.count-limit", "-1"}}));
+    ASSERT_NOK(FieldNestedUpdateAgg::Create(NestedType(), negative_limit, "f"));
+}
+


[P1] Please align the new aggregator tests with the Java test matrix

The current C++ tests mainly cover basic happy paths and are substantially narrower than the corresponding Java tests.

The Java behavior is primarily covered by:

paimon-core/src/test/java/org/apache/paimon/mergetree/compact/aggregate/FieldAggregatorTest.java

paimon-core/src/test/java/org/apache/paimon/mergetree/compact/aggregate/FieldAggregatorRetractNullTest.java

Important missing cases include:

collect: distinct and non-distinct aggregation/retraction for primitive, ROW, ARRAY, and MAP elements.

nested_update: composite nested keys, multiple sequence fields, count-limit boundary cases, all null-key strategies on both accumulator and retract input, and retraction without a nested key.

rbm64, hll_sketch, and theta_sketch: the complete null-input matrix and the unsupported-retraction contract.

These gaps allow Java compatibility issues to go unnoticed. Please port the relevant Java cases or add equivalent parameterized tests.

Please also add end-to-end write/compaction/read tests for the new aggregators to verify the final data read from files, rather than testing only the in-memory aggregation result.

feat(aggregate): add Java-compatible field aggregators

9dac772

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

lxy-9602 reviewed Aug 3, 2026

View reviewed changes

revert field_roaring_bitmap64_agg

4407f5d

lxy-9602 reviewed Aug 3, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

feat(aggregate): add Java-compatible field aggregators - #463

feat(aggregate): add Java-compatible field aggregators#463
liangjie3138 wants to merge 2 commits into
alibaba:mainfrom
liangjie3138:feat/java-compatible-aggregators

liangjie3138 commented Jul 30, 2026

Uh oh!

lxy-9602 Aug 3, 2026

Uh oh!

lxy-9602 Aug 3, 2026

Uh oh!

lxy-9602 Aug 3, 2026

Uh oh!

lxy-9602 Aug 3, 2026

Uh oh!

lxy-9602 Aug 3, 2026

Uh oh!

lxy-9602 Aug 3, 2026

Uh oh!

lxy-9602 Aug 3, 2026

Uh oh!

lxy-9602 Aug 3, 2026

Uh oh!

lxy-9602 Aug 3, 2026

Uh oh!

lxy-9602 Aug 3, 2026

Uh oh!

lxy-9602 Aug 3, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

liangjie3138 commented Jul 30, 2026

Purpose

Tests

API and Format

Documentation

Generative AI tooling

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Annotation 1

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants