Make OptimizerExpressionRule conditional #127500

idegtiarenko · 2025-04-29T12:17:31Z

This makes OptimizerExpressionRule conditional.
This should allow to skip expression traversal (as it might be quiet expensive, especially in case of multiple attributes) if we know beforehand that certain plan type is applicable for the rule or can not be changed by it.

According to QueryPlanningBenchmark.run, this change makes it twice faster to parse the query:

before        avgt   10  5.254 ± 0.079  ms/op
after         avgt   10  2.752 ± 0.085  ms/op

Closes: #124288

alex-spies

This is looking good. We need to double check all affected rules before merging, but this should work and I'm hoping for a nice perf boost on queries that have tons of field attributes in relations/projections (like FROM logs-*).

alex-spies · 2025-04-29T12:23:02Z

.../esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/rules/logical/OptimizerRules.java

+                case EsRelation esr -> false;
+                case Project p -> false;// this covers both keep and project


Suggestion: IMO we should document in the javadoc that relation + projection are getting skipped per default and that one should override shouldVisit to change that.

Fair point. I opened a draft at this point to see if this breaks a lot of tests (it did not 🎉 ) so now I will focus no documenting and testing it.

alex-spies · 2025-04-29T12:24:15Z

x-pack/plugin/esql-core/src/main/java/org/elasticsearch/xpack/esql/core/tree/Node.java

@@ -194,6 +193,12 @@ public <E extends T> T transformDown(Class<E> typeToken, Function<E, ? extends T
        return transformDown((t) -> (typeToken.isInstance(t) ? rule.apply((E) t) : t));
    }

+    @SuppressWarnings("unchecked")
+    public <E extends T> T transformDown(Predicate<Node<?>> tokenPredicate, Function<E, ? extends T> rule) {


nit:

Suggested change

public <E extends T> T transformDown(Predicate<Node<?>> tokenPredicate, Function<E, ? extends T> rule) {

public <E extends T> T transformDown(Predicate<Node<?>> nodePredicate, Function<E, ? extends T> rule) {

we don't have class tokens here.

elasticsearchmachine · 2025-04-30T14:12:52Z

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

astefan · 2025-04-30T14:58:46Z

.../esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/rules/logical/OptimizerRules.java

+        /**
+         * Defines if a node should be visited or not.
+         * Allows to skip nodes that are not applicable for the rule even if they contain expressions.
+         * By default that skips FROM, LIMIT, PROJECT, KEEP and DROP but this list could be extended or replaced in subclasses.


How did you come up with this list of commands/node types?

Those are commands that are either common but have not expressions (limit) or common, contain list of attributes but do not require any optimization or rearranging (such as keep/project/drop) if properly constructed they only contain plain list of attribute used.

For more context: @idegtiarenko found out that the simple FROM logs-* | LIMIT 10, which we want to make faster, seemed to spend significant amount of time just traversing plan nodes/expressions in case that the index pattern matched a ton of fields. Just not traversing the expressions of EsRelation every time, needlessly, should cut down on the planning/optimizing time for such queries.

costin

We might be making a trap for ourselves in the future due to implicit filtering however the PR is contained (thanks for that), the tests are passing and the perf numbers look great.
So much so that backporting this to 8.19 makes sense.

costin · 2025-04-30T16:14:50Z

x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/QueryPlan.java

Why remove the previous methods?

They are unused at the moment. Please let me know if we should keep them anyways

costin · 2025-04-30T16:18:40Z

.../esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/rules/logical/OptimizerRules.java

@@ -7,9 +7,14 @@
 package org.elasticsearch.xpack.esql.optimizer.rules.logical;


Isn't there a physical plan equivalent?

Yes, org.elasticsearch.xpack.esql.optimizer.PhysicalOptimizerRules, however it does not seem to have an rules for expressions.

alex-spies

LGTM. Nice one, @idegtiarenko !

alex-spies · 2025-04-30T16:27:34Z

.../esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/rules/logical/OptimizerRules.java

+        /**
+         * Defines if a node should be visited or not.
+         * Allows to skip nodes that are not applicable for the rule even if they contain expressions.
+         * By default that skips FROM, LIMIT, PROJECT, KEEP and DROP but this list could be extended or replaced in subclasses.


For more context: @idegtiarenko found out that the simple FROM logs-* | LIMIT 10, which we want to make faster, seemed to spend significant amount of time just traversing plan nodes/expressions in case that the index pattern matched a ton of fields. Just not traversing the expressions of EsRelation every time, needlessly, should cut down on the planning/optimizing time for such queries.

.../esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/rules/logical/OptimizerRules.java

alex-spies · 2025-04-30T16:29:10Z

...ck/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/optimizer/OptimizerRulesTests.java

+        };
+
+        rule.apply(
+            new EsqlParser().createStatement("FROM index | EVAL x=f1+1 | KEEP x, f2 | LIMIT 1"),


nit: could also add a query with DROP.

Please note, this test is only executed with a single test rule. As a result drop is not converted to project (as suggested here) and test would highlight rule execution with it.
I am not sure it is worth configuring attribute resolution here to check this aspect.

astefan

LGTM
I've checked some of the optimization rules that might interfere with the plan types excluded in the tree traversal in this PR and things seem to work (stats transforms itself sometimes in a project and stats do support expressions). Since I don't think we will introduce limit 1+2 any time soon, it does look good to me.

Make OptimizerExpressionRule conditional

22de40b

idegtiarenko requested a review from alex-spies April 29, 2025 12:17

elasticsearchmachine added the v9.1.0 label Apr 29, 2025

alex-spies reviewed Apr 29, 2025

View reviewed changes

idegtiarenko mentioned this pull request Apr 30, 2025

Query planning benchmark #127550

Merged

idegtiarenko added 4 commits April 30, 2025 14:35

Merge branch 'main' into conditional_OptimizerExpressionRule

8c7df30

upd

8a7fc3d

add a test

4f7ff09

remove unnecessary comments

1ef4f3a

idegtiarenko requested review from alex-spies and nik9000 April 30, 2025 14:11

idegtiarenko added >non-issue Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) :Analytics/ES|QL AKA ESQL labels Apr 30, 2025

idegtiarenko marked this pull request as ready for review April 30, 2025 14:12

Merge branch 'main' into conditional_OptimizerExpressionRule

0b1e2fb

astefan reviewed Apr 30, 2025

View reviewed changes

costin approved these changes Apr 30, 2025

View reviewed changes

alex-spies approved these changes Apr 30, 2025

View reviewed changes

astefan approved these changes May 1, 2025

View reviewed changes

idegtiarenko added auto-backport Automatically create backport pull requests when merged v8.19.0 labels May 5, 2025

idegtiarenko added 3 commits May 5, 2025 10:04

remove drop

cfe086e

Merge branch 'main' into conditional_OptimizerExpressionRule

2715306

Merge branch 'main' into conditional_OptimizerExpressionRule

ed7a3b8

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make OptimizerExpressionRule conditional #127500

Make OptimizerExpressionRule conditional #127500

idegtiarenko commented Apr 29, 2025 •

edited

Loading

alex-spies left a comment

alex-spies Apr 29, 2025

idegtiarenko Apr 29, 2025

alex-spies Apr 29, 2025

elasticsearchmachine commented Apr 30, 2025

astefan Apr 30, 2025

idegtiarenko Apr 30, 2025

alex-spies Apr 30, 2025

costin left a comment

costin Apr 30, 2025

idegtiarenko May 5, 2025

costin Apr 30, 2025

idegtiarenko May 5, 2025

alex-spies left a comment

alex-spies Apr 30, 2025

alex-spies Apr 30, 2025

idegtiarenko May 5, 2025 •

edited

Loading

astefan left a comment

		case EsRelation esr -> false;
		case Project p -> false;// this covers both keep and project

	public <E extends T> T transformDown(Predicate<Node<?>> tokenPredicate, Function<E, ? extends T> rule) {
	public <E extends T> T transformDown(Predicate<Node<?>> nodePredicate, Function<E, ? extends T> rule) {

		@@ -7,9 +7,14 @@
		package org.elasticsearch.xpack.esql.optimizer.rules.logical;

Make OptimizerExpressionRule conditional #127500

Are you sure you want to change the base?

Make OptimizerExpressionRule conditional #127500

Conversation

idegtiarenko commented Apr 29, 2025 • edited Loading

alex-spies left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elasticsearchmachine commented Apr 30, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

costin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alex-spies left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

idegtiarenko May 5, 2025 • edited Loading

Choose a reason for hiding this comment

astefan left a comment

Choose a reason for hiding this comment

idegtiarenko commented Apr 29, 2025 •

edited

Loading

idegtiarenko May 5, 2025 •

edited

Loading