ESQL: mv_expand pushes down limit and project and keep the limit after it untouched #100782

astefan · 2023-10-12T21:38:12Z

Allow mv_expand to push down limit and project past it.
Have the limit before mv_expand to not override the limit after it, since mv_expand is special from this point of view in the sense that it creates more rows than the original ones as defined by the from command.

Fixes #99971
Fixes #100774

- properly accept a limit after mv_expand when there is also a second limit before the mv_expand

elasticsearchmachine · 2023-10-12T21:39:38Z

Pinging @elastic/es-ql (Team:QL)

elasticsearchmachine · 2023-10-12T21:39:38Z

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

elasticsearchmachine · 2023-10-12T21:39:39Z

Pinging @elastic/elasticsearch-esql (:Query Languages/ES|QL)

astefan · 2023-10-12T21:39:41Z

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

@@ -383,7 +385,8 @@ protected LogicalPlan rule(Limit limit) {
                // check if there's a 'visible' descendant limit lower than the current one
                // and if so, align the current limit since it adds no value
                // this applies for cases such as | limit 1 | sort field | limit 10
-                else {
+                // but NOT for mv_expand (ie | limit 1 | mv_expand x | limit 20) where we want that last "limit" to apply on expand results
+                else if (unary instanceof MvExpand == false) {


Don't combine the limits for mv_expand.

this works only if the limit is right above mv_expand - if it's hidden by another node (keep, where, etc..) the rule won't see it.
Move the check into descendantLimit so that if mv_expand is encountered, just like with an agg, no limit is returned.

Good point. Haven't thought of this scenario.

costin

The current approach can be improved and made more reliable - see my comments.

costin · 2023-10-12T23:49:11Z

...gin/esql/src/test/java/org/elasticsearch/xpack/esql/optimizer/LogicalPlanOptimizerTests.java

+    public void testCombineOrderByThroughMvExpand() {
+        LogicalPlan plan = optimizedPlan("""
+            from test
+            | sort emp_no
+            | mv_expand first_name
+            | sort first_name""");
+
+        var topN = as(plan, TopN.class);
+        assertThat(orderNames(topN), contains("first_name", "emp_no"));
+        var mvExpand = as(topN.child(), MvExpand.class);
+        as(mvExpand.child(), EsRelation.class);
+    }
+
+    public void testPushDownMvExpandPastProject() {
+        LogicalPlan plan = optimizedPlan("""
+            from test
+            | rename first_name as x
+            | keep x
+            | mv_expand x
+            """);
+
+        var keep = as(plan, Project.class);
+        var limit = as(keep.child(), Limit.class);
+        var mvExpand = as(limit.child(), MvExpand.class);
+        assertThat(as(mvExpand.target(), FieldAttribute.class).name(), is("first_name"));
+    }
+
+    public void testDontPushDownLimitPastMvExpand() {


Add as javadoc the expanded plans.

costin · 2023-10-12T23:51:46Z

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

@@ -653,8 +656,22 @@ protected LogicalPlan rule(Enrich re) {
        }
    }

-    protected static class PushDownAndCombineOrderBy extends OptimizerRules.OptimizerRule<OrderBy> {
+    protected static class PushDownMvExpand extends OptimizerRules.OptimizerRule<MvExpand> {


What's the advantage of pushing down MvExpand? I'd argue we want the opposite as this increases the amount of data earlier in the pipeline.

costin · 2023-10-12T23:55:47Z

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

@@ -383,7 +385,8 @@ protected LogicalPlan rule(Limit limit) {
                // check if there's a 'visible' descendant limit lower than the current one
                // and if so, align the current limit since it adds no value
                // this applies for cases such as | limit 1 | sort field | limit 10
-                else {
+                // but NOT for mv_expand (ie | limit 1 | mv_expand x | limit 20) where we want that last "limit" to apply on expand results
+                else if (unary instanceof MvExpand == false) {


this works only if the limit is right above mv_expand - if it's hidden by another node (keep, where, etc..) the rule won't see it.
Move the check into descendantLimit so that if mv_expand is encountered, just like with an agg, no limit is returned.

luigidellaquila

Let me add a couple of comments, because I think the situation here is a bit more convoluted:

pushing down mv_expand is a very peculiar operation: sort a, b | mv_expand c could be rewritten as mv_expand c | sort a, b (they have the same semantics), but this doesn't apply to sort a, b | mv_expand a (if a is expanded before the sort, the final result will be different)
pushing down limit vs mv_expand can still be done (and is necessary in some cases), but with a slightly different logic: mv_expand a | limit 10 is different from limit 10 | mv_expand a, but is equivalent to limit 10 | mv_expand a | limit 10. We will need this kind of logic especially when we have sort, eg. for sort a | mv_expand b | limit 10, if we don't push down limit, we won't be able to build a TopN.
if we apply the rule above (ie. push down the limit, but also maintain the original limit at the end) we have to be careful, the plan optimization could enter in an infinite loop, eg.
1. sort ... | keep ... | mv_expand a | limit 10
2. sort ... | keep ... | limit 10 | mv_expand a | limit 10
3. sort ... | limit 10 | keep ... | mv_expand a | limit 10
4. topN(10) | keep ... | mv_expand a | limit 10
5. topN(10) | keep ... | limit 10 | mv_expand a | limit 10
6. topN(10) | limit 10 | keep ... | mv_expand a | limit 10
  and so on
mv_expand is a pretty heavy command (mostly in terms of memory consumption), so having a limit after it could be a sub-optimal solution. It could be much better to enhance MvExpandOperator with an internal limit, like for TopN

…earch into mv_expand_fixes

…mv_expand_fixes

luigidellaquila

Thank you very much @astefan, I think we are much closer to a solution, but apparently there are still some cases that are not covered.
I managed to break the planning (unknown physical plan node [OrderExec]) with the following:

row a = 1 | sort a | mv_expand a | eval b = 100 | sort b | limit 10

I think the problem here is that MV_EXPAND between the two SORTs cuts out some optimization rules, so that the first sort (that is practically useless) does not get removed

astefan · 2023-10-17T22:25:33Z

Thank you very much @astefan, I think we are much closer to a solution, but apparently there are still some cases that are not covered. I managed to break the planning (unknown physical plan node [OrderExec]) with the following:
row a = 1 | sort a | mv_expand a | eval b = 100 | sort b | limit 10
I think the problem here is that MV_EXPAND between the two SORTs cuts out some optimization rules, so that the first sort (that is practically useless) does not get removed

@luigidellaquila thank you for this scenario. I didn't think of testing such a query.
The problem with this one is that the eval is not pushed down past mv_expand and sort a not pushed up until it reaches sort b. If mv_expand wouldn't have been there, the two sorts would have been combined in a single one: sort a, b and eventually would have became TopN(a, b).

costin

Left a small round of comments - a potential problem with the current approach is looking at things top-down vs bottom-up; the later might simplify the rule.

costin · 2023-10-17T23:31:57Z

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

+            while (plan instanceof Aggregate == false) {
+                if (plan instanceof Limit limit) {
+                    return limit;
+                } else if (plan instanceof MvExpand) {


Why not apply the same behavior for Aggregate - stop searching for a limit once an MvExpand is found?

costin · 2023-10-17T23:35:12Z

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

+                else if (unary instanceof MvExpand || (unary instanceof OrderBy orderBy && orderBy.child() instanceof MvExpand)) {
+                    MvExpand mvExpand = unary instanceof MvExpand mve ? mve : (MvExpand) (unary.child());
+                    Limit limitBeforeMvExpand = limitBeforeMvExpand(mvExpand);
+                    // if there is no "appropriate" limit before mv_expand, then push down a copy of the one after it so that:
+                    // - a possible TopN is properly built as low as possible in the tree (closed to Lucene)
+                    // - the input of mv_expand is as small as possible before it is expanded (less rows to inflate and occupy memory)
+                    if (limitBeforeMvExpand == null) {
+                        var duplicateLimit = new Limit(limit.source(), limit.limit(), mvExpand.child());
+                        if (unary instanceof OrderBy orderBy) {
+                            return limit.replaceChild(orderBy.replaceChild(mvExpand.replaceChild(duplicateLimit)));
+                        } else {
+                            return limit.replaceChild(mvExpand.replaceChild(duplicateLimit));
+                        }
+                    }
+                }


This block needs to be simplified as the main condition is checked 2-3 times.
do a simple boolean isMvExpand = unary instanceof MvExpand, boolean orderByChild = unary instanceof OrderBy && ...
then use those later in the code.

Add a default TopN for cases when there is only a sort at Lucene level

…mv_expand_fixes

astefan · 2023-10-20T16:30:59Z

I have another solution to how we handle mv_expand that is also adapted to further scenarios discovered in the meantime. But it has some bigger implications, non-ideal ones. This change is not final, but I am providing below these new points for awareness purposes and feedback.

from employees | sort emp_no | mv_expand job_positions | limit 5. In this case limit 5 should be pushed down past mv_expand until sort emp_no because we unnecessarily expand a lot of job_positions values if we only need 5 at the end. And because of ESQL: mv_expand after sort fails #99971.
from employees | sort emp_no | mv_expand job_positions | where job_positions like "*a*" | limit 5. In this case we shouldn't push limit 5 like we did for the first case because the limit happens on filtered job_positions. But we also need a limit for sort emp_no. And that limit is the "default one", the one that Analyzer.AddImplicitLimit rule defines.
- the code in current form it's using the esql.query.result_truncation_default_size value (500) but to be entirely correct it should use the one added by the analyzer
- the logical optimizer is now esqlconfiguration aware, which means it is parameterized and a new instance of it is created for each query. If we use the implicit limit from the analyzer, this change in the optimizer shouldn't be needed anymore.
from employees | mv_expand job_positions | limit 5 same argument as for the first scenario BUT differently from the first one we don't end up with an exception here because there is no sort that should be combined with a limit to form a TopN.

costin

LGTM.
The comments below can be done in a separate PR as they are mainly cosmetic and do NOT impact the 8.11 code.

The issue this ticket works on is dealing with generators, that is operators that create additional rows.
We only have them as a Source and thus no mechanism per se and thus makes this issue somewhat out of scope (one approach is to let the generator decide whether it can impose a limit or not internally).

My main comment is around moving the MvExpand handling from the CombineAndPushDownLimit into a separate rule - it shares little code with that rule and also complicated the logic.
Secondary, avoid making the Optimizer context aware and instead pick up the default limit from the plan (potentially through a separate path). If the limit was somehow optimized, try to incorporate that.
Again this is more of a tweak and can be done separately. On the flip side it would simplify the size of this PR.

costin · 2023-10-24T03:39:35Z

FTR, I've raised #101248 to get away from the weird gradle compilation errors in the CI (and I was able to reproduce). I've cherry picked the changes, squashed them and rebased them on main.
I think the issue might be related to some incomplete merges (that went wrong).

…mv_expand_fixes

Add OrderBy node type to the exceptions for duplicating the limit after mv_expand

astefan · 2023-10-24T12:50:10Z

Thanks @costin. I've found why the CI was complaining, as you guessed it was a faulty merge.

My main comment is around moving the MvExpand handling from the CombineAndPushDownLimit into a separate rule - it shares little code with that rule and also complicated the logic.

I've extracted that specific logic in a separate rule.
Also, I've found another use case where the default value should have been used instead of the one of the limit after mv_expand.

Secondary, avoid making the Optimizer context aware and instead pick up the default limit from the plan (potentially through a separate path). If the limit was somehow optimized, try to incorporate that.

I've created #101266 to track this.

elasticsearchmachine · 2023-10-24T13:20:09Z

💔 Backport failed

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

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

… untouched (elastic#100782) - allow mv_expand to push down limit and project past it - accept a limit after mv_expand when there is also a second limit before the mv_expand - adds a default TopN for cases when there is only a sort at Lucene level - adds OrderBy node type to the exceptions for duplicating the limit after mv_expand (cherry picked from commit 4679b09)

… untouched (#100782) (#101268) - allow mv_expand to push down limit and project past it - accept a limit after mv_expand when there is also a second limit before the mv_expand - adds a default TopN for cases when there is only a sort at Lucene level - adds OrderBy node type to the exceptions for duplicating the limit after mv_expand (cherry picked from commit 4679b09)

- allow mv_expand to push down limit and project past it

29a85c3

- properly accept a limit after mv_expand when there is also a second limit before the mv_expand

astefan added >bug auto-backport-and-merge :Analytics/ES|QL AKA ESQL v8.11.0 v8.12.0 labels Oct 12, 2023

astefan requested review from costin, luigidellaquila, bpintea and alex-spies October 12, 2023 21:38

astefan changed the title ~~ESQL:~~ ESQL: mv_expand pushes down limit and project and keep the limit after it untouched Oct 12, 2023

elasticsearchmachine added the Team:QL (Deprecated) Meta label for query languages team label Oct 12, 2023

Update docs/changelog/100782.yaml

7f8b9a0

astefan commented Oct 12, 2023

View reviewed changes

costin requested changes Oct 12, 2023

View reviewed changes

luigidellaquila reviewed Oct 13, 2023

View reviewed changes

astefan added 3 commits October 17, 2023 17:19

Address feedback

e303b50

Merge branch 'mv_expand_fixes' of https://github.com/astefan/elastics…

aadba96

…earch into mv_expand_fixes

Merge branch 'main' of https://github.com/elastic/elasticsearch into …

6259816

…mv_expand_fixes

astefan requested review from costin and luigidellaquila October 17, 2023 14:41

luigidellaquila reviewed Oct 17, 2023

View reviewed changes

costin reviewed Oct 17, 2023

View reviewed changes

astefan added 3 commits October 20, 2023 19:22

Change the optimization rule

c27f9c7

Add a default TopN for cases when there is only a sort at Lucene level

Merge branch 'main' of https://github.com/elastic/elasticsearch into …

4a67590

…mv_expand_fixes

Spotless

90f3edf

costin mentioned this pull request Oct 24, 2023

ESQL: Rebase for #100782 #101248

Closed

costin approved these changes Oct 24, 2023

View reviewed changes

astefan added 2 commits October 24, 2023 11:52

Merge branch 'main' of https://github.com/elastic/elasticsearch into …

8fafb07

…mv_expand_fixes

Extract the mv_expand specific logical plan optimizer

ecd71ef

Add OrderBy node type to the exceptions for duplicating the limit after mv_expand

astefan mentioned this pull request Oct 24, 2023

ESQL: use the correct upper limit for topN for mv_expand queries #101266

Open

astefan merged commit 4679b09 into elastic:main Oct 24, 2023

astefan deleted the mv_expand_fixes branch October 24, 2023 13:18

elasticsearchmachine added the backport pending label Oct 24, 2023

astefan mentioned this pull request Oct 24, 2023

ESQL: mv_expand pushes down a limit copy and keeps the limit after it untouched (#100782) #101268

Merged

mattc58 removed the backport pending label Oct 24, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ESQL: mv_expand pushes down limit and project and keep the limit after it untouched #100782

ESQL: mv_expand pushes down limit and project and keep the limit after it untouched #100782

astefan commented Oct 12, 2023

elasticsearchmachine commented Oct 12, 2023

elasticsearchmachine commented Oct 12, 2023

elasticsearchmachine commented Oct 12, 2023

astefan Oct 12, 2023

costin Oct 12, 2023

astefan Oct 13, 2023

costin left a comment

costin Oct 12, 2023

costin Oct 12, 2023

costin Oct 12, 2023

luigidellaquila left a comment •

edited

Loading

luigidellaquila left a comment

astefan commented Oct 17, 2023

costin left a comment

costin Oct 17, 2023

costin Oct 17, 2023

astefan commented Oct 20, 2023

costin left a comment

costin commented Oct 24, 2023

astefan commented Oct 24, 2023

elasticsearchmachine commented Oct 24, 2023

ESQL: mv_expand pushes down limit and project and keep the limit after it untouched #100782

ESQL: mv_expand pushes down limit and project and keep the limit after it untouched #100782

Conversation

astefan commented Oct 12, 2023

elasticsearchmachine commented Oct 12, 2023

elasticsearchmachine commented Oct 12, 2023

elasticsearchmachine commented Oct 12, 2023

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

luigidellaquila left a comment • edited Loading

Choose a reason for hiding this comment

luigidellaquila left a comment

Choose a reason for hiding this comment

astefan commented Oct 17, 2023

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

astefan commented Oct 20, 2023

costin left a comment

Choose a reason for hiding this comment

costin commented Oct 24, 2023

astefan commented Oct 24, 2023

elasticsearchmachine commented Oct 24, 2023

💔 Backport failed

luigidellaquila left a comment •

edited

Loading