ESQL: Reduce the number of Evals ReplaceMissingFieldWithNull creates #104586

costin · 2024-01-20T03:24:09Z

Improve ReplaceMissingFieldWithNull to create just one eval for the
missing value and have the rest point to it. This reduces the amount
of EvalOperators created in the pipeline.

Fix #104583

Improve ReplaceMissingFieldWithNull to create just one eval for the missing value and have the rest point to it. This reduces the amount of EvalOperators created in the pipeline. Fix elastic#104583

elasticsearchmachine · 2024-01-20T03:24:33Z

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

elasticsearchmachine · 2024-01-20T03:24:33Z

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

alex-spies

This works and I couldn't find any queries that would break this. LGTM!

...sql/src/test/java/org/elasticsearch/xpack/esql/optimizer/LocalLogicalPlanOptimizerTests.java

alex-spies · 2024-01-22T11:06:12Z

...k/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/LogicalPlanOptimizer.java

+
+            java.util.function.Function<ReferenceAttribute, Expression> replaceReference = r -> collectRefs.resolve(r, r);
+
+            // collect aliases bottom-up
            plan.forEachExpressionUp(Alias.class, a -> {
                var c = a.child();
-                if (c.foldable()) {
-                    collectRefs.put(a.toAttribute(), c);
+                boolean shouldCollect = c.foldable();
+                // try to resolve the expression based on an existing foldables
+                if (shouldCollect == false) {
+                    c = c.transformUp(ReferenceAttribute.class, replaceReference);
+                    shouldCollect = c.foldable();
+                }
+                if (shouldCollect) {
+                    collectRefs.put(a.toAttribute(), Literal.of(c));


Why did we need to make changes to PropagateEvalFoldables?

Same ^^. Is it to accelerate the resolution? Wouldn't the first c.foldable() eventually return true?

Forgot to mention this - while running debugging I've noticed this rule is doing only one transform per run causing the optimizer to run multiple times. So I've modified it so it can do the replacement in one go instead of multiple method calls.
An example is this:

eval x = 1 eval y = x eval z = x + 3 eval w = z + y

Previously the rule had to run 4 times, now it does it in the first run.

astefan

I tried to reproduce the behavior reported in the original issue and, given the description of the issue and the one of the ReplaceMissingFieldWithNull rule, I failed to repro.

Debugging the code on a single debug-mode node showed that "missing" means "the field doesn't exist in the mapping" of an index, which is different than (what I originally thought) missing from the documents.

This means that the reproduction step involves at least two nodes, one index having its shards (probably only one) assigned to one node, the other index having the shard(s) on the second node. Also, one index has to have some fields that the other doesn't.
Given this, I would adjust the description of the rule to be a bit more exact and potentially add one-two comments in the code of the rule as well, to make sure there are no confusions about what "missing" means.

I would love to see an IT test with this one in action since it's changing the way the projected fields are treated (now having multiple aliases pointing to the same field = null expression). And the whole point of this rule is to help expedite queries that can end up not actually touching the indices. This means a query like from test* | project field3 with field3 "missing" from one of the indices would not even touch those shards on potentially several nodes and just return null. But the end result of those queries needs to be the exepected one.

We do have a multi-node qa project where this could potentially be tested.

bpintea

If we are to keep the rule, I think this is a better solution over the existing one.

bpintea · 2024-01-22T17:31:26Z

.../main/java/org/elasticsearch/xpack/esql/expression/function/scalar/conditional/Greatest.java

@@ -111,6 +111,8 @@ public Object fold() {

    @Override
    public ExpressionEvaluator.Factory toEvaluator(Function<Expression, ExpressionEvaluator.Factory> toEvaluator) {
+        // force datatype initialization
+        var dataType = dataType();


Wondering why this wasn't needed so far.
Nit: maybe just calling the function ignoring the return and not shadowing the instance variable might be cleaner.

This was a bug - the node was mutated but the new instance did not have its dataType()) called.
For example, the node was serialized, deserialized than the execution kicked in - since nobody called dataType(), the value was not initialized throwing a NPE - see the CI failures.

bpintea · 2024-01-22T17:36:04Z

...k/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/LogicalPlanOptimizer.java

+
+            java.util.function.Function<ReferenceAttribute, Expression> replaceReference = r -> collectRefs.resolve(r, r);
+
+            // collect aliases bottom-up
            plan.forEachExpressionUp(Alias.class, a -> {
                var c = a.child();
-                if (c.foldable()) {
-                    collectRefs.put(a.toAttribute(), c);
+                boolean shouldCollect = c.foldable();
+                // try to resolve the expression based on an existing foldables
+                if (shouldCollect == false) {
+                    c = c.transformUp(ReferenceAttribute.class, replaceReference);
+                    shouldCollect = c.foldable();
+                }
+                if (shouldCollect) {
+                    collectRefs.put(a.toAttribute(), Literal.of(c));


Same ^^. Is it to accelerate the resolution? Wouldn't the first c.foldable() eventually return true?

Fix subtle error in LocalPhysicalPlanOptimizer test Add more unit tests

costin · 2024-01-24T00:38:34Z

...ql/src/test/java/org/elasticsearch/xpack/esql/optimizer/LocalPhysicalPlanOptimizerTests.java

        // the real execution breaks the plan at the exchange and then decouples the plan
        // this is of no use in the unit tests, which checks the plan as a whole instead of each
        // individually hence why here the plan is kept as is

        var logicalTestOptimizer = new LocalLogicalPlanOptimizer(new LocalLogicalOptimizerContext(config, searchStats));
        var physicalTestOptimizer = new TestLocalPhysicalPlanOptimizer(new LocalPhysicalOptimizerContext(config, searchStats), true);
-        var l = PlannerUtils.localPlan(plan, logicalTestOptimizer, physicalTestOptimizer);
+        var l = PlannerUtils.localPlan(physicalPlan, logicalTestOptimizer, physicalTestOptimizer);


This was a bug that caused a lot of head scratching - the other tests where not failing since the local plan was not influenced for them.

elasticsearchmachine · 2024-01-24T00:41:20Z

💔 Backport failed

Status	Branch	Result
❌	8.12	Commit could not be cherrypicked due to conflicts

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

…lastic#104586) Improve ReplaceMissingFieldWithNull to create just one eval (per datatype) for the missing value and have the rest point to it. This reduces the amount of EvalOperators created in the pipeline. Preserve type information (one null eval per dataType) Fix subtle error in LocalPhysicalPlanOptimizer test Fix elastic#104583 (cherry picked from commit d6f900c)

…104586) (#104669) Improve ReplaceMissingFieldWithNull to create just one eval (per datatype) for the missing value and have the rest point to it. This reduces the amount of EvalOperators created in the pipeline. Preserve type information (one null eval per dataType) Fix subtle error in LocalPhysicalPlanOptimizer test Fix #104583 (cherry picked from commit d6f900c)

Reduce the number of Evals ReplaceMissingFieldWithNull creates

ff69a57

Improve ReplaceMissingFieldWithNull to create just one eval for the missing value and have the rest point to it. This reduces the amount of EvalOperators created in the pipeline. Fix elastic#104583

costin added >bug :Analytics/ES|QL AKA ESQL v8.12.1 v8.13.0 labels Jan 20, 2024

costin requested review from luigidellaquila, astefan, bpintea and alex-spies January 20, 2024 03:24

elasticsearchmachine added the Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) label Jan 20, 2024

costin and others added 2 commits January 19, 2024 19:24

Update docs/changelog/104586.yaml

027212d

Fix datatype resolution in Greatest/Least

f8f00d0

costin changed the title ~~Reduce the number of Evals ReplaceMissingFieldWithNull creates~~ ESQL: Reduce the number of Evals ReplaceMissingFieldWithNull creates Jan 22, 2024

alex-spies approved these changes Jan 22, 2024

View reviewed changes

alex-spies mentioned this pull request Jan 22, 2024

ReplaceMissingFieldWithNull rule lead to huge number of eval operators #104583

Closed

astefan reviewed Jan 22, 2024

View reviewed changes

bpintea approved these changes Jan 22, 2024

View reviewed changes

costin requested a review from fang-xing-esql January 23, 2024 01:23

costin added 4 commits January 23, 2024 14:05

Preserve type information (one null eval per dataType)

9ea4d96

Fix subtle error in LocalPhysicalPlanOptimizer test Add more unit tests

Merge branch 'main' into fix/104583

800890d

Fix long javadoc

2682ac6

Fix describe method

9ec8394

costin commented Jan 24, 2024

View reviewed changes

costin added the auto-backport-and-merge label Jan 24, 2024

costin merged commit d6f900c into elastic:main Jan 24, 2024
15 checks passed

costin deleted the fix/104583 branch January 24, 2024 00:40

elasticsearchmachine added the backport pending label Jan 24, 2024

costin mentioned this pull request Jan 24, 2024

ESQL: Reduce the number of Evals ReplaceMissingFieldWithNull creates #104669

Merged

costin removed the backport pending label Jan 25, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ESQL: Reduce the number of Evals ReplaceMissingFieldWithNull creates #104586

ESQL: Reduce the number of Evals ReplaceMissingFieldWithNull creates #104586

costin commented Jan 20, 2024

elasticsearchmachine commented Jan 20, 2024

elasticsearchmachine commented Jan 20, 2024

alex-spies left a comment

alex-spies Jan 22, 2024

bpintea Jan 22, 2024

costin Jan 22, 2024

astefan left a comment •

edited

Loading

bpintea left a comment

bpintea Jan 22, 2024

costin Jan 22, 2024 •

edited

Loading

bpintea Jan 22, 2024

costin Jan 24, 2024

elasticsearchmachine commented Jan 24, 2024

ESQL: Reduce the number of Evals ReplaceMissingFieldWithNull creates #104586

ESQL: Reduce the number of Evals ReplaceMissingFieldWithNull creates #104586

Conversation

costin commented Jan 20, 2024

elasticsearchmachine commented Jan 20, 2024

elasticsearchmachine commented Jan 20, 2024

alex-spies left a comment

Choose a reason for hiding this comment

alex-spies Jan 22, 2024

Choose a reason for hiding this comment

bpintea Jan 22, 2024

Choose a reason for hiding this comment

costin Jan 22, 2024

Choose a reason for hiding this comment

astefan left a comment • edited Loading

Choose a reason for hiding this comment

bpintea left a comment

Choose a reason for hiding this comment

bpintea Jan 22, 2024

Choose a reason for hiding this comment

costin Jan 22, 2024 • edited Loading

Choose a reason for hiding this comment

bpintea Jan 22, 2024

Choose a reason for hiding this comment

costin Jan 24, 2024

Choose a reason for hiding this comment

elasticsearchmachine commented Jan 24, 2024

💔 Backport failed

astefan left a comment •

edited

Loading

costin Jan 22, 2024 •

edited

Loading