ES|QL: Remove redundant sorts from execution plan #121156

luigidellaquila · 2025-01-29T11:39:01Z

Fixes: #120817
Fixes: #120803

When we have multiple SORT commands in a query, some of them could be redundant.

Eg.

from test | sort x | mv_expand x | sort y | limit 1

from test | sort x | lookup join lookup on x | sort y | limit 1

in both cases, the first SORT is practically irrelevant, as the second SORT will reorder the results, and the commands in-between don't rely on the order of results.

In addition, since the second SORT "absorbes" the LIMIT, the first SORT will remain unbounded, and it is problematic, since ES|QL does not support unbounded sort.

This change adds an optimization rule that finds and removes redundant sorts.

A SORT is redundant if all the following conditions are met:

it is before another SORT.
all the commands between the two SORTs do not rely on the order of results.

because SORT y will reorder the results
but only if none of <command1>, <commandN> rely on the order of results

Types of commands that could rely on input order are:

LIMIT: it takes first N results, and if the input is ordered, that order matters
EVAL with window functions: eg. functions that, for a record, also consider the content of previous/next N records
STREAMSTATS
any commands with an inner limit (eg. like MV_EXPAND before this change)

~~At this stage we don't support window functions and STREAMSTATS, but in future we could, so to avoid regressions we consider EVAL and STATS as relevant for order for now.~~

Today we don't support window functions and STREAMSTATS, and we already had rules that do the same thing and don't consider these cases (the old PruneOrderByBeforeStats rule), so we'll consider EVAL, FILTER and STATS safe for now.

~~Given this limitation, AddDefaultTopN is still in place to make a couple of tests pass, but as reported here, it is most likely incorrect and needs a double-check.~~

Since in some cases we can still have unbounded sorts (eg. | sort | mv_expand | where), we also let OrderBy implement PostOptimizationVerificationAware and fail validation in a more graceful way.

elasticsearchmachine · 2025-01-29T11:39:53Z

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

…ort' into esql/remove_redundant_sort

elasticsearchmachine · 2025-01-30T10:31:07Z

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

astefan · 2025-01-30T13:48:44Z

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

@@ -1839,10 +1839,9 @@ public void testCombineOrderByThroughFilter() {

    /**
     * Expected
-     * TopN[[Order[first_name{f}#170,ASC,LAST]],1000[INTEGER]]
-     *  \_MvExpand[first_name{f}#170]
-     *    \_TopN[[Order[emp_no{f}#169,ASC,LAST]],1000[INTEGER]]


Notice that here you are losing a performance aspect: mv_expand is applied on all documents. The idea with a sort (and the default pushed down limit) in front of mv_expand was to have fewer documents to expand from because the expansion is, theoretically, creating even more rows. Before this PR, each node was getting 1000 docs, mv_expanded them and a final sort performed. With this PR, each node is expanding everything, then sorting.

Yes, but you were also discarding all the employees after the first 1000 (sorted by emp_no), that is not what the query says, so technically you got a faster execution, but a wrong result.

costin

It's great to allow more queries - left a few comments regarding the rule implementation.

costin · 2025-02-03T19:42:28Z

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

+ *
+ * This rule finds and removes redundant SORTs, making the plan executable.
+ */
+public class RemoveRedundantSort extends OptimizerRules.OptimizerRule<TopN> {


It makes sense to combine this with PruneOrderByBeforeStats since the logic is similar.
Remove -> Prune

Thanks Costin, I didn't realize the two rules were so similar.
I'll merge them in a single one.

A detail worth noting is that PruneOrderByBeforeStats currently considers Eval as a sort-agnostic plan (and it's correct now, since we don't have window functions yet). I'll keep the same logic for now, and I'll add a comment so that we don't forget.

The good thing is that, with this logic, we allow SORT pruning after all the currently supported plans (apart from LIMIT, but it will become a TopN anyway), so now we no longer have unbounded sort.

costin · 2025-02-03T19:43:14Z

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

+                p = lj.left();
+                // TODO do it also on the right-hand side?
+                continue;


Make the function recursive and let it run on both sides.

costin · 2025-02-03T19:44:06Z

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

+        });
+    }
+
+    private OrderBy findRedundantSort(TopN plan) {


Return a list to perform only one modification (and traversals) on the tree.

costin · 2025-02-03T19:46:11Z

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

+    protected LogicalPlan rule(TopN plan) {
+        OrderBy redundant = findRedundantSort(plan);
+        if (redundant == null) {
+            return plan;
+        }
+        return plan.transformDown(p -> {
+            if (p == redundant) {
+                return redundant.child();
+            }
+            return p;
+        });


A bottom up traversal would help collect all pruned sorts in one traversal instead of a top-down per sort:

those that occur before stats

those that occur before other sorts

costin · 2025-02-03T19:48:05Z

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

+
+    @Override
+    public void postOptimizationVerification(Failures failures) {
+        failures.add(fail(this, "The query cannot be executed because it would require unbounded sort"));


I'd rephrase the error message to include an action item for the user: `Unbounded sort not supported yet, please add a limit"

…ort' into esql/remove_redundant_sort

luigidellaquila · 2025-02-04T11:03:49Z

...rc/main/java/org/elasticsearch/xpack/esql/optimizer/rules/logical/PruneRedundantOrderBy.java

+                // IMPORTANT
+                // If we introduce window functions or order-sensitive aggs (eg. STREAMSTATS),
+                // the previous sort could actually become relevant
+                // so we have to be careful with plans that could use them, ie. the following
+                    || unary instanceof Filter
+                    || unary instanceof Eval
+                    || unary instanceof Aggregate) {


This is safe now, as long as we don't have window functions (and, in general, functions/aggs that rely on the order of the input).
We can decide to keep it like this for now and review it when we introduce such capabilities, or we can be more paranoid about future regressions and discard such cases, but in this case we won't be able to completely avoid unbounded sorts.

luigidellaquila · 2025-02-06T10:22:42Z

Thanks for the feedback @alex-spies @costin, I added another round of changes that should address your comments

alex-spies

This is super nice. I have only minor suggestions. Thanks Luigi!

alex-spies · 2025-02-07T13:04:32Z

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

+ * </code>
+ * <p>
+ *
+ * In all the other cases, eg. if the command does not implement this interface, or if dependsOnInputOrder() = true


++ I think this is really, really important. The pruning of previous sorts must be opt-in, not opt-out.

Costin and I also discussed flipping the default the other way around, i.e. making it so pruning previous sorts is allowed unless this interface is implemented. While Costin would prefer that for verbosity reasons/because most commands are okay with this optimization, I believe this would be really dangerous as it would lead to wrongly optimized plans per default - and we're in the process of adding new commands right now by people who can't be aware of this gotcha in the optimizer.

Sorting is expensive in a distributed system and commands and users should not depend on it.
At the moment, only Limit and the upcoming change point depend on it, all the other commands do not - hence my preference to opt in. That is the default is optimized for the majority of use-cases.

If it's opt in, the presence of the (marker) interface (SortAware) is enough - no need to implement a method, it's redundant.
If it's opt out, use the same marker interface but change its name to reflect the intent, e.g. SortIgnorant, SortIgnorant and simply use its presence.

Sorting is expensive in a distributed system and commands and users should not depend on it.
At the moment, only Limit and the upcoming change point depend on it, all the other commands do not - hence my preference to opt in. That is the default is optimized for the majority of use-cases.

@luigidellaquila and I discussed this and we both think that making the optimization opt-out is a bad idea.

Personally, I have a somewhat strong opinion here. I believe that we'd be really doing ourselves a disservice by making the optimization opt-out as this will, per default, silently break correct queries whenever we add a new command. One such command was just merged (#120998) today.

The key part here is the "silent". It's silent in the code by being opt-out, but more importantly it's silent to users when they get wrong query results. This increases the risk of rolling out buggy commands whose output doesn't make sense and finding the bug in production at a much later point in time.

Forgetting to ask for the opt-out marker when reviewing a PR with a new command is also something that's really easy to do, resp. the number of things that a reviewer needs to know and look out for is increased. And even for new commands that should be fine with removing upstream sorts, I think it's still better if the first implementation doesn't automatically enable the optimization, so that we can enable it later while simultaneously adding tests.

On the other hand, making the optimization opt-in only increases the verbosity a little bit because most plan nodes actually do opt in. I believe the trade offs here are therefore very much in favor of the opt-in solution. And if we want to keep verbosity down, there's still the possibility to create a new subclass of UnaryPlan called StreamingPlan, opt this one in, and have all our commands that operate in a row-based manner inherit from that.

If it's opt in, the presence of the (marker) interface (SortAware) is enough - no need to implement a method, it's redundant.

++, @luigidellaquila and I agreed on going this route.

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

...rc/main/java/org/elasticsearch/xpack/esql/optimizer/rules/logical/PruneRedundantOrderBy.java

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

alex-spies · 2025-02-07T14:20:13Z

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

+        var query = """
+              ROW x = [1,2,3], y = 1
+              | SORT y
+              | MV_EXPAND x
+              | WHERE x > 2
+            """;


suggestion: Could we have a simple test like this but for LOOKUP JOIN? This test is a nice minimal example.

alex-spies

nit: maybe it makes sense to move tests related to the new optimizer rule into a dedicated file. The LogicalPlanOptimizerTests file is already huge and it's hard to see what's tested and not.

alex-spies · 2025-02-07T14:25:30Z

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

+     *             | \_EsRelation[test][_meta_field{f}#26, emp_no{f}#20, first_name{f}#21, ..]
+     *             \_EsRelation[languages_lookup][LOOKUP][language_code{f}#31]
+     */
+    public void testRedundantSortOnMvExpandJoinEnrichGrokDissect() {


I don't see a test with SORT + DROP/KEEP/RENAME + SORT. Could we add one if they already exist?

Adding one.

nit: maybe it makes sense to move tests related to the new optimizer rule into a dedicated file.

Let's do it with a separate PR. LogicalPlanOptimizerTests has quite some initialization logic, it will need some refactoring.

costin

Small tweaks. LGTM

costin · 2025-02-10T10:47:43Z

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

+ * </code>
+ * <p>
+ *
+ * In all the other cases, eg. if the command does not implement this interface, or if dependsOnInputOrder() = true


Sorting is expensive in a distributed system and commands and users should not depend on it.
At the moment, only Limit and the upcoming change point depend on it, all the other commands do not - hence my preference to opt in. That is the default is optimized for the majority of use-cases.

If it's opt in, the presence of the (marker) interface (SortAware) is enough - no need to implement a method, it's redundant.
If it's opt out, use the same marker interface but change its name to reflect the intent, e.g. SortIgnorant, SortIgnorant and simply use its presence.

costin · 2025-02-10T10:51:10Z

...rc/main/java/org/elasticsearch/xpack/esql/optimizer/rules/logical/PruneRedundantOrderBy.java

+    /**
+     * breadth-first recursion to find redundant SORTs in the children tree.
+     */
+    private IdentityHashMap<OrderBy, Void> findRedundantSort(LogicalPlan plan) {


Hide the implementation by returning the keySet as a Collection<OrderBy> on which call contains

costin · 2025-02-10T10:52:19Z

...rc/main/java/org/elasticsearch/xpack/esql/optimizer/rules/logical/PruneRedundantOrderBy.java

+                if (redundant.containsKey(p)) {
+                    return ((OrderBy) p).child();
+                }
+                return p;


-> collection.contains(p) ? ((UnaryPlan) p).child() ? p

luigidellaquila · 2025-02-10T11:49:39Z

Thanks @costin

Sorting is expensive in a distributed system and commands and users should not depend on it.
At the moment, only Limit and the upcoming change point depend on it, all the other commands do not - hence my preference to opt in. That is the default is optimized for the majority of use-cases.

I'd prefer to keep it safe and have the optimization happen only when we know it's safe. ES|QL is evolving fast, and the risk of breaking things is pretty high

If it's opt in, the presence of the (marker) interface (SortAware) is enough - no need to implement a method, it's redundant.
If it's opt out, use the same marker interface but change its name to reflect the intent, e.g. SortIgnorant, SortIgnorant and simply use its presence.

👍 I renamed it SortAgnostic (it's the best I could think of, naming is hard...) and I removed the method (we don't need it for now, we'll add it in the future if we want more control on single commands)

costin · 2025-02-10T13:04:31Z

SortAgnostic
👍

luigidellaquila · 2025-02-10T13:15:03Z

Thanks folks!
Merging

elasticsearchmachine · 2025-02-10T13:17:13Z

💔 Backport failed

Status	Branch	Result
❌	8.18	Commit could not be cherrypicked due to conflicts
❌	8.x	Commit could not be cherrypicked due to conflicts
❌	9.0	Commit could not be cherrypicked due to conflicts

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

luigidellaquila · 2025-02-10T15:47:54Z

Manual backport #122187 #122231

Remove redundant sorts from execution plan

4910db5

elasticsearchmachine added the v9.0.0 label Jan 29, 2025

luigidellaquila added >bug auto-backport Automatically create backport pull requests when merged :Analytics/ES|QL AKA ESQL v8.18.0 and removed v9.0.0 labels Jan 29, 2025

luigidellaquila requested a review from alex-spies January 29, 2025 11:39

luigidellaquila added 6 commits January 29, 2025 12:39

Update docs/changelog/121156.yaml

a0b463a

More tests

78c86de

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

10117dc

…ort' into esql/remove_redundant_sort

Merge branch 'main' into esql/remove_redundant_sort

4d623d7

Verify OrderBy after optimization

0ec1539

Merge branch 'main' into esql/remove_redundant_sort

7ec3534

luigidellaquila marked this pull request as ready for review January 30, 2025 10:30

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

astefan reviewed Jan 30, 2025

View reviewed changes

Merge branch 'main' into esql/remove_redundant_sort

937e0dc

elasticsearchmachine added v8.19.0 and removed v8.18.0 labels Jan 30, 2025

luigidellaquila added 2 commits January 31, 2025 11:04

Merge branch 'main' into esql/remove_redundant_sort

3569a16

Merge branch 'main' into esql/remove_redundant_sort

df50502

costin reviewed Feb 3, 2025

View reviewed changes

luigidellaquila added 4 commits February 4, 2025 11:12

Implement review suggestions

3af3c11

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

16bfba0

…ort' into esql/remove_redundant_sort

Merge branch 'main' into esql/remove_redundant_sort

5c1ed70

Remove AddDefaultTopN rule

c73246d

luigidellaquila commented Feb 4, 2025

View reviewed changes

Merge branch 'main' into esql/remove_redundant_sort

7a9c8ab

alex-spies approved these changes Feb 7, 2025

View reviewed changes

alex-spies reviewed Feb 7, 2025

View reviewed changes

luigidellaquila added v9.0.1 v8.18.0 labels Feb 7, 2025

luigidellaquila added 3 commits February 7, 2025 16:33

Better description for SortAware

2d34e50

More tests

f2af414

Merge branch 'main' into esql/remove_redundant_sort

2303d8b

luigidellaquila added the v8.18.1 label Feb 10, 2025

costin approved these changes Feb 10, 2025

View reviewed changes

luigidellaquila added 2 commits February 10, 2025 12:44

SortAware -> SortAgnostic and simplify

9b22923

Merge branch 'main' into esql/remove_redundant_sort

9083477

luigidellaquila merged commit 1e5ac8b into elastic:main Feb 10, 2025
17 checks passed

elasticsearchmachine added the backport pending label Feb 10, 2025

luigidellaquila added a commit to luigidellaquila/elasticsearch that referenced this pull request Feb 10, 2025

ES|QL: Remove redundant sorts from execution plan (elastic#121156)

18629c3

luigidellaquila mentioned this pull request Feb 10, 2025

ES|QL: Remove redundant sorts from execution plan (#121156) #122187

Merged

elasticsearchmachine pushed a commit that referenced this pull request Feb 10, 2025

ES|QL: Remove redundant sorts from execution plan (#121156) (#122187)

d5d7937

luigidellaquila added a commit to luigidellaquila/elasticsearch that referenced this pull request Feb 11, 2025

ES|QL: Remove redundant sorts from execution plan (elastic#121156)

beb33fc

luigidellaquila mentioned this pull request Feb 11, 2025

ES|QL: Remove redundant sorts from execution plan (#121156) #122231

Merged

elasticsearchmachine pushed a commit that referenced this pull request Feb 11, 2025

ES|QL: Remove redundant sorts from execution plan (#121156) (#122231)

353f4c5

luigidellaquila added a commit to luigidellaquila/elasticsearch that referenced this pull request Feb 11, 2025

ES|QL: Remove redundant sorts from execution plan (elastic#121156)

adb0eee

luigidellaquila mentioned this pull request Feb 11, 2025

[8.18] ES|QL: Remove redundant sorts from execution plan (#121156) #122248

Merged

elasticsearchmachine pushed a commit that referenced this pull request Feb 11, 2025

ES|QL: Remove redundant sorts from execution plan (#121156) (#122248)

94b6406

alex-spies mentioned this pull request Apr 23, 2025

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

Closed

ES|QL: Remove redundant sorts from execution plan #121156

ES|QL: Remove redundant sorts from execution plan #121156

Uh oh!

Conversation

luigidellaquila commented Jan 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elasticsearchmachine commented Jan 29, 2025

Uh oh!

elasticsearchmachine commented Jan 30, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

costin 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!

luigidellaquila Feb 4, 2025 • 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!

luigidellaquila commented Feb 6, 2025

Uh oh!

alex-spies 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!

alex-spies Feb 10, 2025 • 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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alex-spies 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!

costin 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!

luigidellaquila commented Feb 10, 2025

Uh oh!

costin commented Feb 10, 2025

Uh oh!

luigidellaquila commented Feb 10, 2025

Uh oh!

Uh oh!

elasticsearchmachine commented Feb 10, 2025

💔 Backport failed

Uh oh!

luigidellaquila commented Jan 29, 2025 •

edited

Loading

luigidellaquila Feb 4, 2025 •

edited

Loading

alex-spies Feb 10, 2025 •

edited

Loading

luigidellaquila commented Feb 10, 2025 •

edited

Loading