ESQL: Standardize block args #135160
Merged
ESQL: Standardize block args #135160
Conversation
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
Standardizes how aggs and scalars work with arguments that take all values at a position. Aggs were using an arrays of values that we had to copy all of the values into the array. And allocate it. Scalars passed down the `Block` and the scalar read from the block on it's own. That's generally more efficient and not a lot harder. So I standardized on that. Previously scalars that took a `Block` parameter also took an implicit builder and position parameter. But aggs don't need the builder. And *do* need the position. This makes both of those parameters explicit rather than implicit.
elasticsearchmachine
added
the
Team:Analytics
|
Pinging @elastic/es-analytical-engine (Team:Analytics) |
nik9000
commented
Sep 21, 2025
| case StandardArgument sa -> Stream.of(new AggregationParameter(sa.name(), sa.type(), false)); | ||
| case BlockArgument ba -> Stream.of(new AggregationParameter(ba.name(), Types.elementType(ba.type()), true)); | ||
| case PositionArgument pa -> Stream.of(); | ||
| default -> throw new IllegalArgumentException("unsupported argument [" + declarationType + "][" + a + "]"); |
There was a problem hiding this comment.
In the next PR I should be ale to remove this instanceof and just us the Argument directly. It'll need a few more methods, but should be fine.
| current.add(values); | ||
| public static void combine(SpatialExtentState current, @Position int p, IntBlock values) { | ||
| current.add(p, values); | ||
| } |
There was a problem hiding this comment.
Here's the shift from arrays to Block.
|
|
||
| @Evaluator(extraName = "Int") | ||
| static void process(IntBlock.Builder builder, int position, IntBlock field1, IntBlock field2) { | ||
| static void process(IntBlock.Builder builder, @Position int position, IntBlock field1, IntBlock field2) { |
There was a problem hiding this comment.
And here's the explicit @Position parameter.
dnhatn
approved these changes
Sep 22, 2025
| "int valuesEnd = valuesStart + $L.getValueCount(valuesPosition)", | ||
| aggParams.getFirst().blockName() | ||
| ); | ||
| builder.addStatement("$L[] valuesArray = new $L[valuesEnd - valuesStart]", arrayType, arrayType); |
|
Thanks @dnhatn ! |
gmjehovich
pushed a commit
to gmjehovich/elasticsearch
that referenced
this pull request
Sep 22, 2025
Standardizes how aggs and scalars work with arguments that take all values at a position. Aggs were using an arrays of values that we had to copy all of the values into the array. And allocate it. Scalars passed down the `Block` and the scalar read from the block on it's own. That's generally more efficient and not a lot harder. So I standardized on that. Previously scalars that took a `Block` parameter also took an implicit builder and position parameter. But aggs don't need the builder. And *do* need the position. This makes both of those parameters explicit rather than implicit.
DonalEvans
pushed a commit
to DonalEvans/elasticsearch
that referenced
this pull request
Sep 22, 2025
Standardizes how aggs and scalars work with arguments that take all values at a position. Aggs were using an arrays of values that we had to copy all of the values into the array. And allocate it. Scalars passed down the `Block` and the scalar read from the block on it's own. That's generally more efficient and not a lot harder. So I standardized on that. Previously scalars that took a `Block` parameter also took an implicit builder and position parameter. But aggs don't need the builder. And *do* need the position. This makes both of those parameters explicit rather than implicit.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Standardizes how aggs and scalars work with arguments that take all
values at a position.
Aggs were using an arrays of values that we had to copy all of the
values into the array. And allocate it. Scalars passed down the
Blockand the scalar read from the block on it's own. That's generally more
efficient and not a lot harder. So I standardized on that.
Previously scalars that took a
Blockparameter also took an implicitbuilder and position parameter. But aggs don't need the builder. And
do need the position. This makes both of those parameters explicit
rather than implicit.
Relates to #108385