Skip to content

ESQL: Change the order of the optimization rules #124335

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

Merged
merged 13 commits into from
Mar 13, 2025
Merged

ESQL: Change the order of the optimization rules #124335

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
9828afc
Change the order of the optimization rules
astefan Mar 3, 2025
8843569
Update docs/changelog/124335.yaml
astefan Mar 7, 2025
a2c33fa
[CI] Auto commit changes from spotless
Mar 7, 2025
18ebafe
Fix import
astefan Mar 7, 2025
8beb022
Merge branch 'inlinestats_pickup3' of https://github.com/astefan/elas…
astefan Mar 7, 2025
26f2349
More tests
astefan Mar 7, 2025
68c31b7
Merge branch 'main' of https://github.com/elastic/elasticsearch into …
astefan Mar 7, 2025
16311c2
[CI] Auto commit changes from spotless
Mar 7, 2025
a036535
Address reviews
astefan Mar 12, 2025
89e76ef
Merge branch 'main' of https://github.com/elastic/elasticsearch into …
astefan Mar 12, 2025
eddde2d
update
astefan Mar 12, 2025
dc8c35d
Merge branch 'main' into inlinestats_pickup3
astefan Mar 12, 2025
ef8303c
Merge branch 'main' into inlinestats_pickup3
astefan Mar 12, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions docs/changelog/124335.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
pr: 124335
summary: Change the order of the optimization rules
area: ES|QL
type: bug
issues: []
4 changes: 2 additions & 2 deletions ...i-clusters/src/javaRestTest/java/org/elasticsearch/xpack/esql/ccq/MultiClusterSpecIT.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@
import static org.elasticsearch.xpack.esql.EsqlTestUtils.classpathResources;
import static org.elasticsearch.xpack.esql.action.EsqlCapabilities.Cap.INLINESTATS;
import static org.elasticsearch.xpack.esql.action.EsqlCapabilities.Cap.INLINESTATS_V2;
import static org.elasticsearch.xpack.esql.action.EsqlCapabilities.Cap.INLINESTATS_V4;
import static org.elasticsearch.xpack.esql.action.EsqlCapabilities.Cap.INLINESTATS_V5;
import static org.elasticsearch.xpack.esql.action.EsqlCapabilities.Cap.JOIN_LOOKUP_V12;
import static org.elasticsearch.xpack.esql.action.EsqlCapabilities.Cap.JOIN_PLANNING_V1;
import static org.elasticsearch.xpack.esql.action.EsqlCapabilities.Cap.METADATA_FIELDS_REMOTE_TEST;
Expand Down Expand Up @@ -126,7 +126,7 @@ protected void shouldSkipTest(String testName) throws IOException {
assumeFalse("INLINESTATS not yet supported in CCS", testCase.requiredCapabilities.contains(INLINESTATS.capabilityName()));
assumeFalse("INLINESTATS not yet supported in CCS", testCase.requiredCapabilities.contains(INLINESTATS_V2.capabilityName()));
assumeFalse("INLINESTATS not yet supported in CCS", testCase.requiredCapabilities.contains(JOIN_PLANNING_V1.capabilityName()));
assumeFalse("INLINESTATS not yet supported in CCS", testCase.requiredCapabilities.contains(INLINESTATS_V4.capabilityName()));
assumeFalse("INLINESTATS not yet supported in CCS", testCase.requiredCapabilities.contains(INLINESTATS_V5.capabilityName()));
assumeFalse("LOOKUP JOIN not yet supported in CCS", testCase.requiredCapabilities.contains(JOIN_LOOKUP_V12.capabilityName()));
// Unmapped fields require a coorect capability response from every cluster, which isn't currently implemented.
assumeFalse("UNMAPPED FIELDS not yet supported in CCS", testCase.requiredCapabilities.contains(UNMAPPED_FIELDS.capabilityName()));
Expand Down
258 changes: 236 additions & 22 deletions x-pack/plugin/esql/qa/testFixtures/src/main/resources/inlinestats.csv-spec
View file
Open in desktop
Copy link
Contributor

@alex-spies alex-spies Mar 12, 2025

Choose a reason for hiding this comment

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

I just realized that it's probably a good idea to add tests for INLINESTATS ... BY x = bucket(...) and INLINESTATS ... BY x = CATEGORIZE(...). I think the latter cannot work because the join key in BY x = CATEGORIZE(...) is computed during the aggregation, whereas INLINESTATS requires the join key to be present before that.

Cc @jan-elastic , I think we'll have to start out with a limitation where INLINESTATS can't use CATEGORIZE, at least at first; to enable this, I think we'd somehow have to grab the categorizer from the first phase of the query (which computes the STATS) and make it available to the second phase of the query (which performs the joining with every row we see).

Copy link
Contributor

@jan-elastic jan-elastic Mar 13, 2025

Choose a reason for hiding this comment

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

FWIW, I can live with the fact that the first version of INLINESTATS doesn't work with CATEGORIZE.

Just open a GitHub issue for that and it can be resolved later.

Copy link
Contributor

@alex-spies alex-spies Mar 13, 2025

Choose a reason for hiding this comment

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

I opened #124717

Large diffs are not rendered by default.

2 changes: 1 addition & 1 deletion x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/action/EsqlCapabilities.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -835,7 +835,7 @@ public enum Cap {
* Fixes a series of issues with inlinestats which had an incomplete implementation after lookup and inlinestats
* were refactored.
*/
INLINESTATS_V4(EsqlPlugin.INLINESTATS_FEATURE_FLAG),
INLINESTATS_V5(EsqlPlugin.INLINESTATS_FEATURE_FLAG),

/**
* Support partial_results
Expand Down
6 changes: 3 additions & 3 deletions ...lugin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/LogicalPlanOptimizer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -142,11 +142,11 @@ protected static Batch<LogicalPlan> substitutions() {
new ReplaceAggregateNestedExpressionWithEval(),
new ReplaceRegexMatch(),
new ReplaceTrivialTypeConversions(),
new ReplaceOrderByExpressionWithEval(),
new PropagateInlineEvals(),
new ReplaceAliasingEvalWithProject(),
new SkipQueryOnEmptyMappings(),
new SubstituteSpatialSurrogates(),
new ReplaceOrderByExpressionWithEval(),
new PropagateInlineEvals()
new SubstituteSpatialSurrogates()
// new NormalizeAggregate(), - waits on https://github.com/elastic/elasticsearch/issues/100634
);
}
Expand Down
6 changes: 4 additions & 2 deletions .../main/java/org/elasticsearch/xpack/esql/optimizer/rules/logical/PropagateInlineEvals.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,6 @@ protected LogicalPlan rule(InlineJoin plan) {
// check if there's any grouping that uses a reference on the right side
Copy link
Contributor Author

@astefan astefan Mar 7, 2025

Choose a reason for hiding this comment

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

Only a small cleanup here.

// if so, look for the source until finding a StubReference
// then copy those on the left side as well

LogicalPlan left = plan.left();
LogicalPlan right = plan.right();

Expand All @@ -46,7 +45,6 @@ protected LogicalPlan rule(InlineJoin plan) {
// first checks any aggregate that declares expressions inside the grouping
// second that checks any found references to collect their declaration
right = right.transformDown(p -> {

if (p instanceof Aggregate aggregate) {
// collect references
for (Expression g : aggregate.groupings()) {
Expand All @@ -56,6 +54,10 @@ protected LogicalPlan rule(InlineJoin plan) {
}
}

if (groupingRefs.isEmpty()) {
return p;
}

// find their declaration and remove it
// TODO: this doesn't take into account aliasing
Copy link
Contributor

@alex-spies alex-spies Mar 11, 2025

Choose a reason for hiding this comment

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

This comment may be stale now.

if (p instanceof Eval eval) {
Expand Down
1 change: 0 additions & 1 deletion .../sql-action/src/main/java/org/elasticsearch/xpack/sql/action/AbstractSqlQueryRequest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@
*/
package org.elasticsearch.xpack.sql.action;

import org.elasticsearch.TransportVersions;
import org.elasticsearch.action.ActionRequestValidationException;
import org.elasticsearch.action.CompositeIndicesRequest;
import org.elasticsearch.common.Strings;
Expand Down