Skip to content

Propagates filter() to aggregation functions' surrogates #134461

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 14 commits into from
Sep 15, 2025
Merged

Propagates filter() to aggregation functions' surrogates #134461

merged 14 commits into from
Sep 15, 2025
+247 −10

Conversation

ncordon
Copy link
Contributor

@ncordon ncordon commented Sep 10, 2025 •
edited
Loading

When an aggregate expression can get replaced by a surrogate in the planner, we have to propagate the original filter() part. Example:

TOP(field, 1, "ASC") WHERE {{ some condition on field }}

gets replaced internally by a MIN(field) but we were losing the WHERE part

Fixes #134380

@elasticsearchmachine
Copy link
Collaborator

Hi @ncordon, I've created a changelog YAML for you.

@ncordon ncordon added :Analytics/Aggregations Aggregations Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) and removed :Analytics/Compute Engine Analytics in ES|QL labels Sep 10, 2025
@ncordon ncordon force-pushed the fix-aggregation-surrogate-bug branch from 967c2e9 to c7715fa Compare September 11, 2025 07:47
@ncordon ncordon added the auto-backport Automatically create backport pull requests when merged label Sep 11, 2025
@ncordon ncordon marked this pull request as ready for review September 11, 2025 07:58
@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/es-analytical-engine (Team:Analytics)

@ivancea ivancea self-requested a review September 11, 2025 10:34
ivancea
ivancea approved these changes Sep 11, 2025
View reviewed changes
Copy link
Contributor

@ivancea ivancea left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, thanks!

.../test/java/org/elasticsearch/xpack/esql/expression/function/AbstractAggregationTestCase.java
Comment on lines +185 to +189
Expression expression = randomFrom(
buildLiteralExpression(testCase),
buildDeepCopyOfFieldExpression(testCase),
buildFieldExpression(testCase)
);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: I think there's an overload with suppliers here. Something like:

Suggested change
Expression expression = randomFrom(
buildLiteralExpression(testCase),
buildDeepCopyOfFieldExpression(testCase),
buildFieldExpression(testCase)
);
Expression expression = randomFrom(
random(),
() -> buildLiteralExpression(testCase),
() -> buildDeepCopyOfFieldExpression(testCase),
() -> buildFieldExpression(testCase)
);

@ivancea ivancea added :Analytics/ES|QL AKA ESQL and removed :Analytics/Aggregations Aggregations labels Sep 12, 2025
@ncordon ncordon merged commit 381fc8e into elastic:main Sep 15, 2025
35 checks passed
@elasticsearchmachine
Copy link
Collaborator

💔 Backport failed

Status Branch Result
❌ 8.19 Commit could not be cherrypicked due to conflicts
❌ 9.1 Commit could not be cherrypicked due to conflicts
❌ 8.18 Commit could not be cherrypicked due to conflicts
❌ 9.0 Commit could not be cherrypicked due to conflicts

You can use sqren/backport to manually backport by running backport --upstream elastic/elasticsearch --pr 134461

@ncordon ncordon mentioned this pull request Sep 15, 2025
Open
ncordon added a commit to ncordon/elasticsearch that referenced this pull request Sep 15, 2025
@ncordon
Propagates filter() to aggregation functions' surrogates (elastic#134461
20a2951 
)

---------

Co-authored-by: Jan Kuipers <[email protected]>
Co-authored-by: Jan Kuipers <[email protected]>
(cherry picked from commit 381fc8e)

# Conflicts:
#	x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/action/EsqlCapabilities.java
#	x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/expression/function/aggregate/Sum.java
@ncordon ncordon mentioned this pull request Sep 15, 2025
Open
ncordon added a commit to ncordon/elasticsearch that referenced this pull request Sep 15, 2025
@ncordon
Propagates filter() to aggregation functions' surrogates (elastic#134461
ca1e15c 
)

---------

Co-authored-by: Jan Kuipers <[email protected]>
Co-authored-by: Jan Kuipers <[email protected]>
(cherry picked from commit 381fc8e)

# Conflicts:
#	x-pack/plugin/esql/qa/testFixtures/src/main/resources/stats.csv-spec
#	x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/action/EsqlCapabilities.java
#	x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/expression/function/aggregate/Count.java
#	x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/expression/function/aggregate/Sum.java
This was referenced Sep 15, 2025
Open
Open
@ncordon
Copy link
Contributor Author

ncordon commented Sep 15, 2025

💚 All backports created successfully

Status Branch Result
✅ 9.1
✅ 9.0
✅ 8.19
✅ 8.18

Questions ?

Please refer to the Backport tool documentation

ncordon added a commit to ncordon/elasticsearch that referenced this pull request Sep 15, 2025
@ncordon
Propagates filter() to aggregation functions' surrogates (elastic#134461
6471c58 
)

---------

Co-authored-by: Jan Kuipers <[email protected]>
Co-authored-by: Jan Kuipers <[email protected]>
(cherry picked from commit 381fc8e)

# Conflicts:
#	x-pack/plugin/esql/qa/testFixtures/src/main/resources/stats.csv-spec
#	x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/action/EsqlCapabilities.java
#	x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/expression/function/aggregate/Count.java
#	x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/expression/function/aggregate/Sum.java
ncordon added a commit to ncordon/elasticsearch that referenced this pull request Sep 15, 2025
@ncordon @jan-elastic
Propagates filter() to aggregation functions' surrogates (elastic#134461
73ec0ac 
)

---------

Co-authored-by: Jan Kuipers <[email protected]>
Co-authored-by: Jan Kuipers <[email protected]>
(cherry picked from commit 381fc8e)
ncordon added a commit to ncordon/elasticsearch that referenced this pull request Sep 15, 2025
@ncordon @jan-elastic
Propagates filter() to aggregation functions' surrogates (elastic#134461
578b47d 
)

---------

Co-authored-by: Jan Kuipers <[email protected]>
Co-authored-by: Jan Kuipers <[email protected]>
(cherry picked from commit 381fc8e)
ncordon added a commit to ncordon/elasticsearch that referenced this pull request Sep 15, 2025
@ncordon @jan-elastic
Propagates filter() to aggregation functions' surrogates (elastic#134461
54a38d4 
)

---------

Co-authored-by: Jan Kuipers <[email protected]>
Co-authored-by: Jan Kuipers <[email protected]>
(cherry picked from commit 381fc8e)
ncordon added a commit to ncordon/elasticsearch that referenced this pull request Sep 15, 2025
@ncordon @jan-elastic
Propagates filter() to aggregation functions' surrogates (elastic#134461
bb3dd43 
)

---------

Co-authored-by: Jan Kuipers <[email protected]>
Co-authored-by: Jan Kuipers <[email protected]>
(cherry picked from commit 381fc8e)
ncordon added a commit to ncordon/elasticsearch that referenced this pull request Sep 15, 2025
@ncordon @jan-elastic
Propagates filter() to aggregation functions' surrogates (elastic#134461
2feeeb0 
)

---------

Co-authored-by: Jan Kuipers <[email protected]>
Co-authored-by: Jan Kuipers <[email protected]>
(cherry picked from commit 381fc8e)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ivancea ivancea ivancea approved these changes

Assignees
No one assigned
Labels
:Analytics/ES|QL AKA ESQL auto-backport Automatically create backport pull requests when merged backport pending >bug Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) v8.18.8 v8.19.5 v9.0.8 v9.1.5 v9.2.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

ESQL: Aggregates with surrogate() not propagating its filter()
4 participants
@ncordon @elasticsearchmachine @ivancea @jan-elastic