- Notifications
You must be signed in to change notification settings - Fork 25.7k
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.
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
Conversation
| Hi @ncordon, I've created a changelog YAML for you. |
967c2e9 to c7715fa Compare | Pinging @elastic/es-analytical-engine (Team:Analytics) |
ivancea left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, thanks!
x-pack/plugin/esql/qa/testFixtures/src/main/resources/stats.csv-spec Outdated Show resolved Hide resolved
x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/action/EsqlCapabilities.java Outdated Show resolved Hide resolved
| Expression expression = randomFrom( | ||
| buildLiteralExpression(testCase), | ||
| buildDeepCopyOfFieldExpression(testCase), | ||
| buildFieldExpression(testCase) | ||
| ); |
There was a problem hiding this comment.
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:
| Expression expression = randomFrom( | |
| buildLiteralExpression(testCase), | |
| buildDeepCopyOfFieldExpression(testCase), | |
| buildFieldExpression(testCase) | |
| ); | |
| Expression expression = randomFrom( | |
| random(), | |
| () -> buildLiteralExpression(testCase), | |
| () -> buildDeepCopyOfFieldExpression(testCase), | |
| () -> buildFieldExpression(testCase) | |
| ); |
.../test/java/org/elasticsearch/xpack/esql/expression/function/AbstractAggregationTestCase.java Outdated Show resolved Hide resolved
💔 Backport failed
You can use sqren/backport to manually backport by running |
) --------- Co-authored-by: Jan Kuipers <jan.kuipers@elastic.co> Co-authored-by: Jan Kuipers <148754765+jan-elastic@users.noreply.github.com> (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
) --------- Co-authored-by: Jan Kuipers <jan.kuipers@elastic.co> Co-authored-by: Jan Kuipers <148754765+jan-elastic@users.noreply.github.com> (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
💚 All backports created successfully
Questions ?Please refer to the Backport tool documentation |
) --------- Co-authored-by: Jan Kuipers <jan.kuipers@elastic.co> Co-authored-by: Jan Kuipers <148754765+jan-elastic@users.noreply.github.com> (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
) --------- Co-authored-by: Jan Kuipers <jan.kuipers@elastic.co> Co-authored-by: Jan Kuipers <148754765+jan-elastic@users.noreply.github.com> (cherry picked from commit 381fc8e)
) --------- Co-authored-by: Jan Kuipers <jan.kuipers@elastic.co> Co-authored-by: Jan Kuipers <148754765+jan-elastic@users.noreply.github.com> (cherry picked from commit 381fc8e)
) --------- Co-authored-by: Jan Kuipers <jan.kuipers@elastic.co> Co-authored-by: Jan Kuipers <148754765+jan-elastic@users.noreply.github.com> (cherry picked from commit 381fc8e)
) --------- Co-authored-by: Jan Kuipers <jan.kuipers@elastic.co> Co-authored-by: Jan Kuipers <148754765+jan-elastic@users.noreply.github.com> (cherry picked from commit 381fc8e)
) --------- Co-authored-by: Jan Kuipers <jan.kuipers@elastic.co> Co-authored-by: Jan Kuipers <148754765+jan-elastic@users.noreply.github.com> (cherry picked from commit 381fc8e)
) (#134725) --------- Co-authored-by: Jan Kuipers <jan.kuipers@elastic.co> Co-authored-by: Jan Kuipers <148754765+jan-elastic@users.noreply.github.com>
--------- Co-authored-by: Jan Kuipers <jan.kuipers@elastic.co> Co-authored-by: Jan Kuipers <148754765+jan-elastic@users.noreply.github.com>
--------- Co-authored-by: Jan Kuipers <jan.kuipers@elastic.co> Co-authored-by: Jan Kuipers <148754765+jan-elastic@users.noreply.github.com>
When an aggregate expression can get replaced by a surrogate in the planner, we have to propagate the original
filter()part. Example:gets replaced internally by a
MIN(field)but we were losing theWHEREpartFixes #134380