Streaming aggregation implementation of ARRAY_AGG() - #4463
Open
robert-brunel wants to merge 4 commits into
Open
Conversation
robert-brunel
force-pushed
the
apple/robert-brunel/array_agg-1
branch
from
August 17, 2026 19:10
c4bc382 to
6951560
Compare
robert-brunel
changed the base branch from
main
to
apple/robert-brunel/nullable-arrays-tweaks
August 17, 2026 19:14
robert-brunel
changed the title
ARRAY_AGG()ARRAY_AGG()
robert-brunel
marked this pull request as ready for review
August 17, 2026 19:14
robert-brunel
force-pushed
the
apple/robert-brunel/array_agg-1
branch
from
August 18, 2026 10:14
6951560 to
31f7c37
Compare
Base automatically changed from
apple/robert-brunel/nullable-arrays-tweaks
to
main
August 18, 2026 18:33
This change implements the `ArrayAggValue` built-in to evaluate the `ARRAY_AGG()` aggregate function, as a `StreamableAggregateValue` (as opposed to an `IndexableAggregateValue`; indexing would require a new index type or maintainer and is not in scope).
The `DISTINCT` and `ORDER BY` clauses are not yet supported. The `{RESPECT|IGNORE} NULLS` clause is, in principle, supported, but subject to the limitation that nulls in arrays cannot currently be represented (Issue #3646).
The accumulator collects elements into a list and (since `OneOfTypedState` has no list slot) serializes that list for continuations through a wrapper record with a single `repeated` field.
robert-brunel
force-pushed
the
apple/robert-brunel/array_agg-1
branch
from
August 18, 2026 18:33
31f7c37 to
18da484
Compare
pengpeng-lu
reviewed
Aug 20, 2026
| Type.Record.Field.of(new Type.Array(false, elementType), | ||
| Optional.of(NullableArrayTypeUtils.getRepeatedFieldName())))); | ||
| final TypeRepository localRepository = TypeRepository.newBuilder().addTypeIfNeeded(wrapperType).build(); | ||
| return Verify.verifyNotNull(localRepository.getMessageDescriptor(wrapperType)); |
Contributor
There was a problem hiding this comment.
are you trying to reuse some logics in TypeRepository here? is it possible to extract the logic as some static method and call the static method here instead?
Contributor
Author
There was a problem hiding this comment.
Do you mean the construction of that wrapperType? Except for nullability, it’s indeed the same wrapper type that also gets constructed in Type.java. So I’ve now introduced a NullableArrayTypeUtils.wrapperTypeFor() helper and am calling it from here and from Type.java. What do you think?
Contributor
There was a problem hiding this comment.
adding NullableArrayTypeUtils.wrapperTypeFor() makes sense, but what looks weird to me is the localRepository object, the only reason it is created is that you want to get a messageDescriptor, not that you need a typeRepository, so I'm wondering if it is possible to extract the logic that creates a messageDescriptor into a static method, and call it from here.
pengpeng-lu
reviewed
Aug 21, 2026
pengpeng-lu
requested changes
Aug 21, 2026
Brings in the 20-odd commits that landed since this branch was opened, in particular the removal of `RecordQueryStreamingAggregationPlan.SerializationMode` (#4468) and the 4.13.2.0/4.13.3.0 release cuts. Three resolutions: * `FDBStreamAggregationTest` — `SerializationMode` is gone, so `partialAggregateArrayAgg()` becomes a plain `@Test` calling the one-argument `build()`, as `main` has already done for the scalar-aggregate tests it was modelled on. * `array-agg-tests.yamsql` — restore `supported_version: !current_version`. The 4.13.1.0 release cut had literalized it, which claims these tests run against a released server; since the executing form of `ARRAY_AGG()` is unreleased they cannot, and the mixed-version configurations against 4.13.3.0 failed accordingly. * `array-agg-tests.metrics.{yaml,binpb}` — regenerate. Only task counts and timings move, no explain strings, so this is the planner evolution on `main` rather than a change in these plans.
Brings the branch up to date with the 15 commits that landed since the previous merge, among them the gRPC 1.83.0 bump and the `MetaDataEvolutionValidator` fix for renamed types (#4475). The merge is clean, and no expectations needed adjusting.
📊 Metrics Diff Analysis ReportSummary
ℹ️ About this analysisThis automated analysis compares query planner metrics between the base branch and this PR. It categorizes changes into:
The last category in particular may indicate planner regressions that should be investigated. New QueriesCount of new queries by file:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This change implements the
ArrayAggValuebuilt-in to evaluate theARRAY_AGG()aggregate function, as aStreamableAggregateValue(as opposed to anIndexableAggregateValue; indexing would require a new index type or maintainer and is not in scope).The
DISTINCTandORDER BYclauses are not yet supported. The{RESPECT|IGNORE} NULLSclause is, in principle, supported, but subject to the limitation that nulls in arrays cannot currently be represented (Issue #3646).The accumulator collects elements into a list and (since
OneOfTypedStatehas no list slot) serializes that list for continuations through a wrapper record with a singlerepeatedfield.