Streaming aggregation implementation of ARRAY_AGG() - #4463
Open
robert-brunel wants to merge 1 commit 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
📊 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:
|
pengpeng-lu
reviewed
Aug 20, 2026
| * the element type will be non-nullable, since {@code NULL} values are then skipped rather than collected. | ||
| */ | ||
| public ArrayAggValue(@Nonnull final Value child, final boolean ignoreNulls) { | ||
| this(child, ignoreNulls ? child.getResultType().notNullable() : child.getResultType(), ignoreNulls); |
Contributor
There was a problem hiding this comment.
if child.getResultType() == Type.NULL, notNullable() will throw an exception. is this expected? Or should the method verify child.getResultType() before this line?
pengpeng-lu
reviewed
Aug 20, 2026
| this.elementType = elementType; | ||
| this.ignoreNulls = ignoreNulls; | ||
| this.wrapperDescriptorSupplier = Suppliers.memoize(() -> wrapperDescriptorFor(elementType)); | ||
| this.resultTypeSupplier = Suppliers.memoize(() -> new Type.Array(true, elementType)); |
Contributor
There was a problem hiding this comment.
so here the array's isNullable = true, while in line#145, it is constructed as Type.Array(false, elementType), is it intentional?
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?
pengpeng-lu
reviewed
Aug 20, 2026
| this.valuesField = Verify.verifyNotNull( | ||
| wrapperDescriptor.findFieldByName(NullableArrayTypeUtils.getRepeatedFieldName())); | ||
| this.elements = new ArrayList<>(); | ||
| this.ignoreNulls = ignoreNulls; |
Contributor
There was a problem hiding this comment.
this.seenAnyRow = false;
pengpeng-lu
reviewed
Aug 20, 2026
| // Under IGNORE NULLS a NULL is simply dropped. Under RESPECT NULLS (the SQL default) it would have to | ||
| // be collected. However, nulls are not representable currently. | ||
| SemanticException.check(ignoreNulls, SemanticException.ErrorCode.UNSUPPORTED, | ||
| "An ARRAY value cannot have NULL elements"); |
Contributor
There was a problem hiding this comment.
what happens if ignoreNulls = false?
pengpeng-lu
reviewed
Aug 21, 2026
| - maxRows: 1 | ||
| - result: [{10, [100, 200, 300]}] | ||
| - result: [{20, [400, 500]}] | ||
| - result: [{30, []}] |
Contributor
There was a problem hiding this comment.
if not ignore nulls, will this row be result: [{30, [null]}]?
pengpeng-lu
requested changes
Aug 21, 2026
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.