ESQL: SUM long overflow fix and TransportVersionAware interface#141272
Merged
ncordon merged 56 commits intoelastic:mainfrom Mar 31, 2026
Merged
ESQL: SUM long overflow fix and TransportVersionAware interface#141272ncordon merged 56 commits intoelastic:mainfrom
ncordon merged 56 commits intoelastic:mainfrom
Conversation
# Conflicts: # server/src/main/resources/transport/upper_bounds/9.4.csv
Collaborator
|
Hi @ivancea, I've created a changelog YAML for you. |
# Conflicts: # server/src/main/resources/transport/upper_bounds/9.4.csv
ivancea
commented
Jan 28, 2026
Contributor
Author
There was a problem hiding this comment.
Added tests for a combination of:
- FROM and ROW
- Grouping and not grouping
- SUM and AVG (Surrogates to SUM)
ivancea
commented
Jan 28, 2026
| }; | ||
| case SUM -> switch (dataType) { | ||
| case LONGS -> new SumLongAggregatorFunctionSupplier(); | ||
| case LONGS -> new SumLongAggregatorFunctionSupplier(-1, -2, ""); |
Contributor
Author
There was a problem hiding this comment.
FYI because agg-generated code is in the compute project, it can't receive Source (esql/core) like evaluators do. So we pass the line, column and text directly.
This was made in the warnExceptions PR
alex-spies
reviewed
Jan 28, 2026
Contributor
alex-spies
left a comment
There was a problem hiding this comment.
This is currently blocked on the outcome of #138805.
That said, I like this approach. Looks nice and simple, and everything is clear after the substitutions step in the optimizer.
| readWindow(in), | ||
| readSummationMode(in) | ||
| readSummationMode(in), | ||
| in.getTransportVersion().supports(ESQL_SUM_LONG_OVERFLOW_FIX) ? in.readBoolean() : true |
| * Returns the expression to be replaced by or {@code null} if this cannot | ||
| * be replaced. | ||
| */ | ||
| Expression forTransportVersion(TransportVersion minTransportVersion); |
Contributor
There was a problem hiding this comment.
I think we need some more explanation here?
I think there are 3 cases.
- Replaced successfully
- Doesn't need replace (returned same as before)
- Cannot be replaced (but maybe it should and if unsuccessful it should throw about unsupported feature)?
It seems right now we don't have the error code/throwing in the API design, but we might need it. We should think about this when designing the new interface and provide comments here about the expected behavior in that case. I think we can just throw in the forTransportVersion implementation, maybe just improve the comment
| new ReplaceAliasingEvalWithProject(), | ||
| new SkipQueryOnEmptyMappings(), | ||
| new SubstituteSurrogateExpressions(), | ||
| new SubstituteTransportVersionAwareExpressions(), |
Contributor
There was a problem hiding this comment.
This is brittle. If there is a rule after this one that introduces sum and it will create a non-bwc sum and fail on old nodes (where before it would succeed unless there is an overflow). I reviewed the rules after, they seem OK right now (but @alex-spies you might know more details). But rules change from time to time.
Maybe it is safer to start with the old compatible sum version, and use this rule to make it new version of the sum?
ivancea
commented
Jan 30, 2026
Contributor
Author
There was a problem hiding this comment.
Add to toString to see it in the plan outputs?
# Conflicts: # server/src/main/resources/transport/upper_bounds/9.4.csv
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 72 out of 78 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
...VersionAwareGoldenTests/testSumGetsReplacedWithSafeLong/local_physical_optimization.expected
Outdated
Show resolved
Hide resolved
...t/java/org/elasticsearch/xpack/esql/expression/function/aggregate/SumSerializationTests.java
Outdated
Show resolved
Hide resolved
...t/java/org/elasticsearch/xpack/esql/expression/function/aggregate/SumSerializationTests.java
Outdated
Show resolved
Hide resolved
# Conflicts: # server/src/main/resources/transport/upper_bounds/9.4.csv
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 73 out of 79 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
.../test/java/org/elasticsearch/xpack/esql/expression/function/AbstractAggregationTestCase.java
Show resolved
Hide resolved
.../test/java/org/elasticsearch/xpack/esql/expression/function/AbstractAggregationTestCase.java
Outdated
Show resolved
Hide resolved
…end on the transport version
# Conflicts: # server/src/main/resources/transport/upper_bounds/9.4.csv
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (8)
📒 Files selected for processing (104)
📝 WalkthroughWalkthroughThis pull request addresses long overflow handling in ESQL SUM aggregation (issue ✨ Finishing Touches🧪 Generate unit tests (beta)
|
ncordon
pushed a commit
to ncordon/elasticsearch
that referenced
this pull request
Apr 1, 2026
not-napoleon
added a commit
that referenced
this pull request
Apr 6, 2026
This was referenced Apr 8, 2026
mromaios
pushed a commit
to mromaios/elasticsearch
that referenced
this pull request
Apr 9, 2026
mromaios
pushed a commit
to mromaios/elasticsearch
that referenced
this pull request
Apr 9, 2026
resolves elastic#145381 I think this is a sequencing conflict with elastic#141272, which applied a similar change to many other golden tests.
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
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.
Fixes #110437
Recovers an old PR now that we have
minTransportVersionavailable: #116170SUM(long)throws on an overflow, leading to a 500 error and fully breaking the query.This PR introduces a
warnExceptionsin the evaluator (Introduced here and never used yet: #111829) to instead show a warning and return a null for that group/aggregation.Because this is an aggregation intermediate state change, this isn't BWC. So a new
TransportVersionAwareinterface was introduced to let the function modify its behavior based on theminTransportVersionacross all the query clusters (This is the new bit that we didn't have in the original PR).