ES|QL: fix LIMIT pushdown past MV_EXPAND #115624

luigidellaquila · 2024-10-25T07:56:43Z

Fixes #102084
Fixes #102061

Allow proper pushdown of LIMIT x command past MV_EXPAND, using a two-step strategy:

first push down the LIMIT x and keep the original limit information inside MV_EXPAND itself, ie. transform MV_EXPAND | LIMIT x into LIMIT x | MV_EXPAND(with limit x)
(do the normal planning, with batch loops that can trigger the above operation again if there are other LIMIT or MV_EXPAND commands)
only at the end, at mapping time, see if MV_EXPAND has an inner limit, and in case add the corresponding LimitExec

All this happens only at logical level, so that physical plan and operators can ignore it (mv_expand operator implementation is unbounded, this PR doesn't change it).
The inner limit is resolved locally:

the mapper replaces inner limit with a normal LimitExec, so the physical plan doesn't have it
in the physical plan -> logical plan fragment translation for data node routing, MV_EXPAND is created without inner limit

so there is no need to serialize it (ie. no impact on bwc problems, no changes to TransportVersions)

Q: Why adding a LIMIT to MV_EXPAND and not trying to just reason on the plan? LIMIT is not strictly related to MV_EXPAND, so it seems out of context there.
A: For many reasons:

because it avoids infinite loops: MV_EXPAND | LIMIT -> LIMIT | MV_EXPAND | LIMIT -> ... | MV_EXPAND | LIMIT
because reasoning on the whole plan is much more complicated and error prone than reasoning on the single MV_EXPAND

(it was previously #102545)

elasticsearchmachine · 2024-10-25T07:57:08Z

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

elasticsearchmachine · 2024-10-25T07:57:09Z

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

luigidellaquila · 2024-10-25T08:29:09Z

@elasticmachine update branch

craigtaverner

Seems like a good solution, but I did have some comments.

craigtaverner · 2024-10-25T10:06:05Z

...main/java/org/elasticsearch/xpack/esql/optimizer/rules/logical/PushDownAndCombineLimits.java

        } else if (limit.child() instanceof UnaryPlan unary) {
            if (unary instanceof Eval || unary instanceof Project || unary instanceof RegexExtract || unary instanceof Enrich) {
                return unary.replaceChild(limit.replaceChild(unary.child()));
+            } else if (unary instanceof MvExpand mvx) {


Interesting that this code previously did not push the limit down past the MvExpand at all, but the bug reports indicate that that was happening. Was it happening somewhere else? If so, should that location be modified?

Oh yeah, good point, I forgot about it...
We have another rule, called DuplicateLimitAfterMvExpand, that does exactly what I describe above:
MV_EXPAND | LIMIT -> LIMIT | MV_EXPAND | LIMIT

I think we can remove it

There is another rule, AddDefaultTopN, that is intrinsically wrong (it adds an arbitrary limit at the beginning of the query) and that IMHO should be removed as well, but I'm not sure this fix is enough to do it, maybe the problems it tries to solve are not only related to MV_EXPAND.

x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/planner/Mapper.java

…_limit_2' into esql/fix_sort_mv_expand_limit_2

craigtaverner · 2024-10-25T11:43:00Z

...n/java/org/elasticsearch/xpack/esql/optimizer/rules/logical/DuplicateLimitAfterMvExpand.java

-            || child instanceof Limit;
-
-        if (shouldSkip == false && child instanceof UnaryPlan unary) {
-            MvExpand mvExpand = descendantMvExpand(unary);


This rule seems to cover more cases than simply LIMIT->MvExpand, which the other rule now covers. Presumably the iterative application of the other rules will cover this scenario already, so this removal is of no consequence. And presumably test coverage will pick this up if that is not the case.

costin · 2024-10-28T05:25:33Z

The other PRs for this issue (#103080 and #103582) contain multiple tests - even if this PR doesn't fix them, please add them here and disable them to get a better understanding of the limitations.

costin

An inner limit is going to create its own problems however the original issue hasn't been addressed in over a year (I felt short) and this PR looks to improve the situation.

See my other comments on incorporating the tests (disabled them if they fail) from the other PRs .

costin · 2024-10-28T05:27:38Z

x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/action/EsqlCapabilities.java

+        /**
+         * Fix pushdown of LIMIT past MV_EXPAND
+         */
+        FIX_MV_EXPAND_LIMIT_PUSHDOWN;


Nit: Unless this PR fixes all issues around limit and mvExpand, add a PARTIAL_ prefix or describe what the PR does - ADD_LIMIT_INSIDE_MV_EXPAND.

I think it fully addresses the LIMIT pushdown problem, but I also think the name you are suggesting is more descriptive, I'll change it anyway

costin · 2024-10-28T05:31:03Z

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

            return false;
        }
-        return Objects.equals(target, ((MvExpand) obj).target) && Objects.equals(expanded, ((MvExpand) obj).expanded);
+        return Objects.equals(target, ((MvExpand) obj).target)


Extract the MvExpand casting inside equals.

costin · 2024-10-28T05:32:54Z

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

    private List<Attribute> output;

-    public MvExpand(Source source, LogicalPlan child, NamedExpression target, Attribute expanded) {
+    public MvExpand(Source source, LogicalPlan child, NamedExpression target, Attribute expanded, Integer limit) {


Since the limit is "artificial" and not serialized, consider keeping the old constructor which delegates to the extended one and just passes limit as null.
Makes the change less disruptive especially in most cases looks like null is passed anyway.

luigidellaquila · 2024-10-28T11:21:39Z

The other PRs for this issue (#103080 and #103582) contain multiple tests - even if this PR doesn't fix them, please add them here and disable them to get a better understanding of the limitations.

Good point, I'm adding some of the tests from these two PRs.
Some other tests don't really make sense in this context, because they tested solutions that were specific of those PRs, in particular the pushdown of MV_EXPAND past SORT (that unfortunately is not possible in all cases).

luigidellaquila · 2024-10-28T12:10:38Z

@elasticmachine update branch

bpintea

I

bpintea · 2024-10-28T12:06:14Z

x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/planner/Mapper.java

        if (p instanceof MvExpand mvExpand) {
-            return new MvExpandExec(mvExpand.source(), map(mvExpand.child()), mvExpand.target(), mvExpand.expanded());
+            MvExpandExec result = new MvExpandExec(mvExpand.source(), map(mvExpand.child()), mvExpand.target(), mvExpand.expanded());
+            if (mvExpand.limit() != null && mvExpand.limit() >= 0) {


Curious about the >= 0 bit. Is it needed, do we do it anywhere else?

I'm pretty sure we don't support a LIMIT -1... probably I should remove that check.

bpintea · 2024-10-28T12:13:19Z

...main/java/org/elasticsearch/xpack/esql/optimizer/rules/logical/PushDownAndCombineLimits.java

+                var limitSource = limit.limit();
+                var limitVal = (int) limitSource.fold();
+                Integer mvxLimit = mvx.limit();
+                if (mvxLimit == null || mvxLimit < 0 || mvxLimit > limitVal) {


same here: is the negative check needed?

bpintea · 2024-10-28T12:43:58Z

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

        out.writeNamedWriteable(child());
        out.writeNamedWriteable(target());
        out.writeNamedWriteable(expanded());
+        assert limit == null;


I guess this never triggers b/c there's currently always going to be a limit below MV_EXPAND, so it'll never be serialized in the fragment, right?

It never happens because we do the serialization after the physical planning, and the mapper will resolve an MvExpand(limit=n) into a LimitExec(n) + MvExpandExec; after physical planning, even if that fragment remains the same, it will be re-translated to Limit(n) + MvExpand and serialized.

To clarify, we don't need to serialize it now, but we could. It would be yet another transport version bump, and since it's a fix and it will be backported, I preferred to make the backport as smooth as possible

I think I get it.

To clarify, we don't need to serialize it now, but we could.

The fragment will contain the logical plan, the "original" one. So theoretically, we might need to serialise an MvExpand with a non-null limit. In which case the assertion would trip. But MvExpand's limit would be non-null only if a limit was pushed down over it and since limit is a pipeline breaker, the plan will be latest broken down at the limit, so MvExpand will, in current form, never make it onto the nodes. Right?
The translation to physical and the mapping wouldn't then influence this, I guess.

and since limit is a pipeline breaker, the plan will be latest broken down at the limit, so MvExpand will, in current form, never make it onto the nodes. Right?

Correct, and it's enough to guarantee that limited MV_EXPAND will never make it into the data nodes.

The translation to physical and the mapping wouldn't then influence this, I guess.

I think you're right

bpintea

Lgtm. I guess at some point we might want to review this, if we add multiple reducers, no-limit plans or similar, but now it's a good fix.

…_limit_2' into esql/fix_sort_mv_expand_limit_2

luigidellaquila · 2024-10-30T09:44:22Z

@elasticmachine update branch

luigidellaquila · 2024-10-30T14:37:02Z

@elasticmachine update branch

elasticsearchmachine · 2024-10-30T15:41:50Z

💚 Backport successful

Status	Branch	Result
✅	8.16
✅	8.x

ES|QL: fix LIMIT pushdown past MV_EXPAND

65096cb

luigidellaquila added >bug auto-backport Automatically create backport pull requests when merged :Analytics/ES|QL AKA ESQL v8.16.1 v8.17.0 labels Oct 25, 2024

elasticsearchmachine added v9.0.0 Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) labels Oct 25, 2024

Update docs/changelog/115624.yaml

280a4b5

Merge branch 'main' into esql/fix_sort_mv_expand_limit_2

f471d54

craigtaverner approved these changes Oct 25, 2024

View reviewed changes

luigidellaquila added 2 commits October 25, 2024 13:07

Remove DuplicateLimitAfterMvExpand and add comments

b03ef72

Merge remote-tracking branch 'luigidellaquila/esql/fix_sort_mv_expand…

5b3fd2e

…_limit_2' into esql/fix_sort_mv_expand_limit_2

craigtaverner reviewed Oct 25, 2024

View reviewed changes

costin self-requested a review October 25, 2024 21:09

costin approved these changes Oct 28, 2024

View reviewed changes

More tests

db86a6f

Spotless

e171cdf

Merge branch 'main' into esql/fix_sort_mv_expand_limit_2

5755968

bpintea reviewed Oct 28, 2024

View reviewed changes

bpintea approved these changes Oct 28, 2024

View reviewed changes

luigidellaquila added 3 commits October 28, 2024 16:03

Remove unnecessary checks

3b407e7

Merge remote-tracking branch 'luigidellaquila/esql/fix_sort_mv_expand…

2322b04

…_limit_2' into esql/fix_sort_mv_expand_limit_2

Merge branch 'main' into esql/fix_sort_mv_expand_limit_2

7ee5811

More tests

8f1c2d7

Merge branch 'main' into esql/fix_sort_mv_expand_limit_2

adc4d81

Merge branch 'main' into esql/fix_sort_mv_expand_limit_2

af70d28

luigidellaquila merged commit a3615f0 into elastic:main Oct 30, 2024
16 checks passed

This was referenced Oct 30, 2024

[8.16] ES|QL: fix LIMIT pushdown past MV_EXPAND (#115624) #115957

Merged

[8.x] ES|QL: fix LIMIT pushdown past MV_EXPAND (#115624) #115958

Merged

luigidellaquila added a commit to luigidellaquila/elasticsearch that referenced this pull request Oct 30, 2024

ES|QL: fix LIMIT pushdown past MV_EXPAND (elastic#115624)

c127e31

luigidellaquila added a commit to luigidellaquila/elasticsearch that referenced this pull request Oct 30, 2024

ES|QL: fix LIMIT pushdown past MV_EXPAND (elastic#115624)

9b526e7

elasticsearchmachine pushed a commit that referenced this pull request Oct 30, 2024

ES|QL: fix LIMIT pushdown past MV_EXPAND (#115624) (#115957)

0b86769

elasticsearchmachine pushed a commit that referenced this pull request Oct 30, 2024

ES|QL: fix LIMIT pushdown past MV_EXPAND (#115624) (#115958)

6a5a562

jfreden pushed a commit to jfreden/elasticsearch that referenced this pull request Nov 4, 2024

ES|QL: fix LIMIT pushdown past MV_EXPAND (elastic#115624)

43c8275

alex-spies mentioned this pull request Jan 21, 2025

ESQL: Fix LOOKUP JOIN with limit #120411

Merged

ES|QL: fix LIMIT pushdown past MV_EXPAND #115624

ES|QL: fix LIMIT pushdown past MV_EXPAND #115624

Uh oh!

Conversation

luigidellaquila commented Oct 25, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elasticsearchmachine commented Oct 25, 2024

Uh oh!

elasticsearchmachine commented Oct 25, 2024

Uh oh!

luigidellaquila commented Oct 25, 2024

Uh oh!

craigtaverner left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

luigidellaquila Oct 25, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

costin commented Oct 28, 2024

Uh oh!

costin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

luigidellaquila Oct 28, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

luigidellaquila commented Oct 28, 2024

Uh oh!

luigidellaquila commented Oct 28, 2024

Uh oh!

bpintea left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bpintea left a comment

Choose a reason for hiding this comment

Uh oh!

luigidellaquila commented Oct 30, 2024

Uh oh!

luigidellaquila commented Oct 30, 2024

Uh oh!

Uh oh!

elasticsearchmachine commented Oct 30, 2024

💚 Backport successful

Uh oh!

Reviewers

Assignees

Labels

Projects

luigidellaquila commented Oct 25, 2024 •

edited

Loading

luigidellaquila Oct 25, 2024 •

edited

Loading

luigidellaquila Oct 28, 2024 •

edited

Loading