From 4910db51d6bcbc95047f72b2e7ad4268feb6c8bc Mon Sep 17 00:00:00 2001 From: Luigi Dell'Aquila Date: Wed, 29 Jan 2025 12:23:41 +0100 Subject: [PATCH 01/13] Remove redundant sorts from execution plan --- .../src/main/resources/lookup-join.csv-spec | 20 ++ .../src/main/resources/mv_expand.csv-spec | 14 + .../xpack/esql/action/EsqlCapabilities.java | 9 +- .../esql/optimizer/LogicalPlanOptimizer.java | 3 +- .../rules/logical/RemoveRedundantSort.java | 88 +++++++ .../optimizer/LogicalPlanOptimizerTests.java | 244 ++++++++++++------ 6 files changed, 293 insertions(+), 85 deletions(-) create mode 100644 x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/rules/logical/RemoveRedundantSort.java diff --git a/x-pack/plugin/esql/qa/testFixtures/src/main/resources/lookup-join.csv-spec b/x-pack/plugin/esql/qa/testFixtures/src/main/resources/lookup-join.csv-spec index d4a98fdc70a9a..31716bcf83848 100644 --- a/x-pack/plugin/esql/qa/testFixtures/src/main/resources/lookup-join.csv-spec +++ b/x-pack/plugin/esql/qa/testFixtures/src/main/resources/lookup-join.csv-spec @@ -1346,3 +1346,23 @@ language_code:integer | language_name:keyword | country:text 1 | English | United States of America 1 | English | null ; + + +sortBeforeAndAfterJoin +required_capability: join_lookup_v12 +required_capability: remove_redundant_sort + +FROM employees +| sort first_name +| EVAL language_code = languages +| LOOKUP JOIN languages_lookup ON language_code +| WHERE emp_no >= 10091 AND emp_no < 10094 +| SORT emp_no +| KEEP emp_no, language_code, language_name +; + +emp_no:integer | language_code:integer | language_name:keyword +10091 | 3 | Spanish +10092 | 1 | English +10093 | 3 | Spanish +; diff --git a/x-pack/plugin/esql/qa/testFixtures/src/main/resources/mv_expand.csv-spec b/x-pack/plugin/esql/qa/testFixtures/src/main/resources/mv_expand.csv-spec index 2a7c092798404..1b4c1f0bc2b6c 100644 --- a/x-pack/plugin/esql/qa/testFixtures/src/main/resources/mv_expand.csv-spec +++ b/x-pack/plugin/esql/qa/testFixtures/src/main/resources/mv_expand.csv-spec @@ -404,3 +404,17 @@ from employees | where emp_no == 10003 | mv_expand first_name | keep first_name first_name:keyword Parto ; + + +sortBeforeAndAfterMvExpand +from employees +| sort first_name +| mv_expand job_positions +| sort emp_no, job_positions +| keep emp_no, job_positions +| limit 2; + +emp_no:integer | job_positions:keyword +10001 | Accountant +10001 | Senior Python Developer +; diff --git a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/action/EsqlCapabilities.java b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/action/EsqlCapabilities.java index b8b911afe7fd4..0010ec2d350d5 100644 --- a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/action/EsqlCapabilities.java +++ b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/action/EsqlCapabilities.java @@ -779,7 +779,14 @@ public enum Cap { /** * Support match options in match function */ - MATCH_FUNCTION_OPTIONS; + MATCH_FUNCTION_OPTIONS, + + /** + * Fix for https://github.com/elastic/elasticsearch/issues/120817 + * and https://github.com/elastic/elasticsearch/issues/120803 + * Support for queries that have multiple SORTs that cannot become TopN + */ + REMOVE_REDUNDANT_SORT; private final boolean enabled; diff --git a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/LogicalPlanOptimizer.java b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/LogicalPlanOptimizer.java index 36150083daec0..b77f25c71fcf5 100644 --- a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/LogicalPlanOptimizer.java +++ b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/LogicalPlanOptimizer.java @@ -40,6 +40,7 @@ import org.elasticsearch.xpack.esql.optimizer.rules.logical.PushDownEnrich; import org.elasticsearch.xpack.esql.optimizer.rules.logical.PushDownEval; import org.elasticsearch.xpack.esql.optimizer.rules.logical.PushDownRegexExtract; +import org.elasticsearch.xpack.esql.optimizer.rules.logical.RemoveRedundantSort; import org.elasticsearch.xpack.esql.optimizer.rules.logical.RemoveStatsOverride; import org.elasticsearch.xpack.esql.optimizer.rules.logical.ReplaceAggregateAggExpressionWithEval; import org.elasticsearch.xpack.esql.optimizer.rules.logical.ReplaceAggregateNestedExpressionWithEval; @@ -195,6 +196,6 @@ protected static Batch operators() { } protected static Batch cleanup() { - return new Batch<>("Clean Up", new ReplaceLimitAndSortAsTopN(), new ReplaceRowAsLocalRelation()); + return new Batch<>("Clean Up", new ReplaceLimitAndSortAsTopN(), new ReplaceRowAsLocalRelation(), new RemoveRedundantSort()); } } diff --git a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/rules/logical/RemoveRedundantSort.java b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/rules/logical/RemoveRedundantSort.java new file mode 100644 index 0000000000000..b7b19977e3ff3 --- /dev/null +++ b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/rules/logical/RemoveRedundantSort.java @@ -0,0 +1,88 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the Elastic License + * 2.0; you may not use this file except in compliance with the Elastic License + * 2.0. + */ + +package org.elasticsearch.xpack.esql.optimizer.rules.logical; + +import org.elasticsearch.xpack.esql.plan.logical.Dissect; +import org.elasticsearch.xpack.esql.plan.logical.Enrich; +import org.elasticsearch.xpack.esql.plan.logical.Filter; +import org.elasticsearch.xpack.esql.plan.logical.Grok; +import org.elasticsearch.xpack.esql.plan.logical.LogicalPlan; +import org.elasticsearch.xpack.esql.plan.logical.MvExpand; +import org.elasticsearch.xpack.esql.plan.logical.OrderBy; +import org.elasticsearch.xpack.esql.plan.logical.Project; +import org.elasticsearch.xpack.esql.plan.logical.Rename; +import org.elasticsearch.xpack.esql.plan.logical.TopN; +import org.elasticsearch.xpack.esql.plan.logical.UnaryPlan; +import org.elasticsearch.xpack.esql.plan.logical.join.Join; + +/** + * SORT cannot be executed without a LIMIT, as ES|QL doesn't support unbounded sort (yet). + * + * The planner tries to push down LIMIT and transform all the unbounded sorts into a TopN. + * In some cases it's not possible though, eg. + * + * from test | sort x | lookup join lookup on x | sort y + * + * from test | sort x | mv_expand x | sort y + * + * "sort y" will become a TopN, but "sort x" will remain unbounded, so the query could not be executed. + * + * In most cases though, last SORT make the previous SORTs redundant, + * ie. it will re-sort previously sorted results + * often with a different order. + * + * This rule finds and removes redundant SORTs, making the plan executable. + */ +public class RemoveRedundantSort extends OptimizerRules.OptimizerRule { + + @Override + 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; + }); + } + + private OrderBy findRedundantSort(TopN plan) { + LogicalPlan p = plan.child(); + while (true) { + if (p instanceof OrderBy ob) { + return ob; + } + if (p instanceof UnaryPlan unary) { + if (unary instanceof Filter + || unary instanceof Project + || unary instanceof Rename + || unary instanceof MvExpand + || unary instanceof Enrich + || unary instanceof Grok + || unary instanceof Dissect + // If we introduce window functions, the previous sort could actually become relevant + // so to be sure we don't introduce regressions, we'll have to exclude places where these functions could be used + // || unary instanceof Eval + // || unary instanceof Aggregate + ) { + p = unary.child(); + continue; + } + } else if (p instanceof Join lj) { + p = lj.left(); + // TODO do it also on the right-hand side? + continue; + } + return null; + } + } + +} diff --git a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/optimizer/LogicalPlanOptimizerTests.java b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/optimizer/LogicalPlanOptimizerTests.java index c80e374540d09..f949b23d5898d 100644 --- a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/optimizer/LogicalPlanOptimizerTests.java +++ b/x-pack/plugin/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]] - * \_EsRelation[test][avg_worked_seconds{f}#167, birth_date{f}#168, emp_n..] + * TopN[[Order[first_name{r}#5575,ASC,LAST]],1000[INTEGER]] + * \_MvExpand[first_name{f}#5565,first_name{r}#5575,null] + * \_EsRelation[test][_meta_field{f}#5570, emp_no{f}#5564, first_name{f}#..] */ public void testDontCombineOrderByThroughMvExpand() { LogicalPlan plan = optimizedPlan(""" @@ -1854,9 +1853,7 @@ public void testDontCombineOrderByThroughMvExpand() { var topN = as(plan, TopN.class); assertThat(orderNames(topN), contains("first_name")); var mvExpand = as(topN.child(), MvExpand.class); - topN = as(mvExpand.child(), TopN.class); - assertThat(orderNames(topN), contains("emp_no")); - as(topN.child(), EsRelation.class); + as(mvExpand.child(), EsRelation.class); } /** @@ -2065,12 +2062,10 @@ public void testMultipleLookupJoinWithSortAndLimit() { } /** - * Expected - * EsqlProject[[emp_no{f}#350, first_name{f}#351, salary{f}#352]] - * \_TopN[[Order[salary{f}#352,ASC,LAST], Order[first_name{f}#351,ASC,LAST]],5[INTEGER]] - * \_MvExpand[first_name{f}#351] - * \_TopN[[Order[emp_no{f}#350,ASC,LAST]],10000[INTEGER]] - * \_EsRelation[employees][emp_no{f}#350, first_name{f}#351, salary{f}#352] + * EsqlProject[[emp_no{f}#10, first_name{r}#21, salary{f}#15]] + * \_TopN[[Order[salary{f}#15,ASC,LAST], Order[first_name{r}#21,ASC,LAST]],5[INTEGER]] + * \_MvExpand[first_name{f}#11,first_name{r}#21,null] + * \_EsRelation[test][_meta_field{f}#16, emp_no{f}#10, first_name{f}#11, ..] */ public void testPushDownLimitThroughMultipleSort_AfterMvExpand() { LogicalPlan plan = optimizedPlan(""" @@ -2086,20 +2081,16 @@ public void testPushDownLimitThroughMultipleSort_AfterMvExpand() { assertThat(topN.limit().fold(FoldContext.small()), equalTo(5)); assertThat(orderNames(topN), contains("salary", "first_name")); var mvExp = as(topN.child(), MvExpand.class); - topN = as(mvExp.child(), TopN.class); - assertThat(topN.limit().fold(FoldContext.small()), equalTo(10000)); - assertThat(orderNames(topN), contains("emp_no")); - as(topN.child(), EsRelation.class); + as(mvExp.child(), EsRelation.class); } /** * Expected - * EsqlProject[[emp_no{f}#361, first_name{f}#362, salary{f}#363]] - * \_TopN[[Order[first_name{f}#362,ASC,LAST]],5[INTEGER]] - * \_TopN[[Order[salary{f}#363,ASC,LAST]],5[INTEGER]] - * \_MvExpand[first_name{f}#362] - * \_TopN[[Order[emp_no{f}#361,ASC,LAST]],10000[INTEGER]] - * \_EsRelation[employees][emp_no{f}#361, first_name{f}#362, salary{f}#363] + * EsqlProject[[emp_no{f}#2560, first_name{r}#2571, salary{f}#2565]] + * \_TopN[[Order[first_name{r}#2571,ASC,LAST]],5[INTEGER]] + * \_TopN[[Order[salary{f}#2565,ASC,LAST]],5[INTEGER]] + * \_MvExpand[first_name{f}#2561,first_name{r}#2571,null] + * \_EsRelation[test][_meta_field{f}#2566, emp_no{f}#2560, first_name{f}#..] */ public void testPushDownLimitThroughMultipleSort_AfterMvExpand2() { LogicalPlan plan = optimizedPlan(""" @@ -2119,10 +2110,7 @@ public void testPushDownLimitThroughMultipleSort_AfterMvExpand2() { assertThat(topN.limit().fold(FoldContext.small()), equalTo(5)); assertThat(orderNames(topN), contains("salary")); var mvExp = as(topN.child(), MvExpand.class); - topN = as(mvExp.child(), TopN.class); - assertThat(topN.limit().fold(FoldContext.small()), equalTo(10000)); - assertThat(orderNames(topN), contains("emp_no")); - as(topN.child(), EsRelation.class); + as(mvExp.child(), EsRelation.class); } /** @@ -2258,14 +2246,13 @@ public void testPushDownLimit_PastEvalAndMvExpand() { /** * Expected - * EsqlProject[[emp_no{f}#12, first_name{r}#22, salary{f}#17]] - * \_TopN[[Order[salary{f}#17,ASC,LAST], Order[first_name{r}#22,ASC,LAST]],1000[INTEGER]] - * \_Filter[gender{f}#14 == [46][KEYWORD] AND WILDCARDLIKE(first_name{r}#22)] - * \_MvExpand[first_name{f}#13,first_name{r}#22,null] - * \_TopN[[Order[emp_no{f}#12,ASC,LAST]],10000[INTEGER]] - * \_EsRelation[test][_meta_field{f}#18, emp_no{f}#12, first_name{f}#13, ..] - */ - public void testAddDefaultLimit_BeforeMvExpand_WithFilterOnExpandedField_ResultTruncationDefaultSize() { + * EsqlProject[[emp_no{f}#5885, first_name{r}#5896, salary{f}#5890]] + * \_TopN[[Order[salary{f}#5890,ASC,LAST], Order[first_name{r}#5896,ASC,LAST]],1000[INTEGER]] + * \_Filter[gender{f}#5887 == [46][KEYWORD] AND WILDCARDLIKE(first_name{r}#5896)] + * \_MvExpand[first_name{f}#5886,first_name{r}#5896,null] + * \_EsRelation[test][_meta_field{f}#5891, emp_no{f}#5885, first_name{f}#..] + */ + public void testRedundantSort_BeforeMvExpand_WithFilterOnExpandedField_ResultTruncationDefaultSize() { LogicalPlan plan = optimizedPlan(""" from test | sort emp_no @@ -2282,9 +2269,7 @@ public void testAddDefaultLimit_BeforeMvExpand_WithFilterOnExpandedField_ResultT var filter = as(topN.child(), Filter.class); assertThat(filter.condition(), instanceOf(And.class)); var mvExp = as(filter.child(), MvExpand.class); - topN = as(mvExp.child(), TopN.class); // TODO is it correct? Double-check AddDefaultTopN rule - assertThat(orderNames(topN), contains("emp_no")); - as(topN.child(), EsRelation.class); + as(mvExp.child(), EsRelation.class); } /** @@ -2367,8 +2352,7 @@ public void testMultiMvExpand_SortDownBelow() { var mvExpand = as(topN.child(), MvExpand.class); var filter = as(mvExpand.child(), Filter.class); mvExpand = as(filter.child(), MvExpand.class); - var topN2 = as(mvExpand.child(), TopN.class); // TODO is it correct? Double-check AddDefaultTopN rule - as(topN2.child(), EsRelation.class); + as(mvExpand.child(), EsRelation.class); } /** @@ -2463,20 +2447,18 @@ public void testRemoveUnusedSortBeforeMvExpand_DefaultLimit10000() { assertThat(orderNames(topN), contains("first_name")); assertThat(topN.limit().fold(FoldContext.small()), equalTo(10000)); var mvExpand = as(topN.child(), MvExpand.class); - var topN2 = as(mvExpand.child(), TopN.class); // TODO is it correct? Double-check AddDefaultTopN rule - as(topN2.child(), EsRelation.class); + as(mvExpand.child(), EsRelation.class); } /** * Expected - * EsqlProject[[emp_no{f}#104, first_name{f}#105, salary{f}#106]] - * \_TopN[[Order[salary{f}#106,ASC,LAST], Order[first_name{f}#105,ASC,LAST]],15[INTEGER]] - * \_Filter[gender{f}#215 == [46][KEYWORD] AND WILDCARDLIKE(first_name{f}#105)] - * \_MvExpand[first_name{f}#105] - * \_TopN[[Order[emp_no{f}#104,ASC,LAST]],10000[INTEGER]] - * \_EsRelation[employees][emp_no{f}#104, first_name{f}#105, salary{f}#106] - */ - public void testAddDefaultLimit_BeforeMvExpand_WithFilterOnExpandedField() { + * EsqlProject[[emp_no{f}#3517, first_name{r}#3528, salary{f}#3522]] + * \_TopN[[Order[salary{f}#3522,ASC,LAST], Order[first_name{r}#3528,ASC,LAST]],15[INTEGER]] + * \_Filter[gender{f}#3519 == [46][KEYWORD] AND WILDCARDLIKE(first_name{r}#3528)] + * \_MvExpand[first_name{f}#3518,first_name{r}#3528,null] + * \_EsRelation[test][_meta_field{f}#3523, emp_no{f}#3517, first_name{f}#..] + */ + public void testRedundantSort_BeforeMvExpand_WithFilterOnExpandedField() { LogicalPlan plan = optimizedPlan(""" from test | sort emp_no @@ -2494,24 +2476,18 @@ public void testAddDefaultLimit_BeforeMvExpand_WithFilterOnExpandedField() { var filter = as(topN.child(), Filter.class); assertThat(filter.condition(), instanceOf(And.class)); var mvExp = as(filter.child(), MvExpand.class); - topN = as(mvExp.child(), TopN.class); - // the filter acts on first_name (the one used in mv_expand), so the limit 15 is not pushed down past mv_expand - // instead the default limit is added - assertThat(topN.limit().fold(FoldContext.small()), equalTo(10000)); - assertThat(orderNames(topN), contains("emp_no")); - as(topN.child(), EsRelation.class); + as(mvExp.child(), EsRelation.class); } /** * Expected - * EsqlProject[[emp_no{f}#104, first_name{f}#105, salary{f}#106]] - * \_TopN[[Order[salary{f}#106,ASC,LAST], Order[first_name{f}#105,ASC,LAST]],15[INTEGER]] - * \_Filter[gender{f}#215 == [46][KEYWORD] AND salary{f}#106 > 60000[INTEGER]] - * \_MvExpand[first_name{f}#105] - * \_TopN[[Order[emp_no{f}#104,ASC,LAST]],10000[INTEGER]] - * \_EsRelation[employees][emp_no{f}#104, first_name{f}#105, salary{f}#106] - */ - public void testAddDefaultLimit_BeforeMvExpand_WithFilter_NOT_OnExpandedField() { + * EsqlProject[[emp_no{f}#3421, first_name{r}#3432, salary{f}#3426]] + * \_TopN[[Order[salary{f}#3426,ASC,LAST], Order[first_name{r}#3432,ASC,LAST]],15[INTEGER]] + * \_Filter[gender{f}#3423 == [46][KEYWORD] AND salary{f}#3426 > 60000[INTEGER]] + * \_MvExpand[first_name{f}#3422,first_name{r}#3432,null] + * \_EsRelation[test][_meta_field{f}#3427, emp_no{f}#3421, first_name{f}#..] + */ + public void testRedundantSort_BeforeMvExpand_WithFilter_NOT_OnExpandedField() { LogicalPlan plan = optimizedPlan(""" from test | sort emp_no @@ -2529,24 +2505,18 @@ public void testAddDefaultLimit_BeforeMvExpand_WithFilter_NOT_OnExpandedField() var filter = as(topN.child(), Filter.class); assertThat(filter.condition(), instanceOf(And.class)); var mvExp = as(filter.child(), MvExpand.class); - topN = as(mvExp.child(), TopN.class); - // the filters after mv_expand do not act on the expanded field values, as such the limit 15 is the one being pushed down - // otherwise that limit wouldn't have pushed down and the default limit was instead being added by default before mv_expanded - assertThat(topN.limit().fold(FoldContext.small()), equalTo(10000)); - assertThat(orderNames(topN), contains("emp_no")); - as(topN.child(), EsRelation.class); + as(mvExp.child(), EsRelation.class); } /** * Expected - * EsqlProject[[emp_no{f}#116, first_name{f}#117 AS x, salary{f}#119]] - * \_TopN[[Order[salary{f}#119,ASC,LAST], Order[first_name{f}#117,ASC,LAST]],15[INTEGER]] - * \_Filter[gender{f}#118 == [46][KEYWORD] AND WILDCARDLIKE(first_name{f}#117)] - * \_MvExpand[first_name{f}#117] - * \_TopN[[Order[gender{f}#118,ASC,LAST]],10000[INTEGER]] - * \_EsRelation[employees][emp_no{f}#116, first_name{f}#117, gender{f}#118, sa..] - */ - public void testAddDefaultLimit_BeforeMvExpand_WithFilterOnExpandedFieldAlias() { + * EsqlProject[[emp_no{f}#2085, first_name{r}#2096 AS x, salary{f}#2090]] + * \_TopN[[Order[salary{f}#2090,ASC,LAST], Order[first_name{r}#2096,ASC,LAST]],15[INTEGER]] + * \_Filter[gender{f}#2087 == [46][KEYWORD] AND WILDCARDLIKE(first_name{r}#2096)] + * \_MvExpand[first_name{f}#2086,first_name{r}#2096,null] + * \_EsRelation[test][_meta_field{f}#2091, emp_no{f}#2085, first_name{f}#..] + */ + public void testRedundantSort_BeforeMvExpand_WithFilterOnExpandedFieldAlias() { LogicalPlan plan = optimizedPlan(""" from test | sort gender @@ -2565,11 +2535,7 @@ public void testAddDefaultLimit_BeforeMvExpand_WithFilterOnExpandedFieldAlias() var filter = as(topN.child(), Filter.class); assertThat(filter.condition(), instanceOf(And.class)); var mvExp = as(filter.child(), MvExpand.class); - topN = as(mvExp.child(), TopN.class); - // the filter uses an alias ("x") to the expanded field ("first_name"), so the default limit is used and not the one provided - assertThat(topN.limit().fold(FoldContext.small()), equalTo(10000)); - assertThat(orderNames(topN), contains("gender")); - as(topN.child(), EsRelation.class); + as(mvExp.child(), EsRelation.class); } /** @@ -7302,4 +7268,116 @@ public void testFunctionNamedParamsAsFunctionArgument() { assertEquals(new Literal(EMPTY, 2.0, DataType.DOUBLE), ee.value()); assertEquals(DataType.DOUBLE, ee.dataType()); } + + /** + * TopN[[Order[emp_no{f}#11,ASC,LAST]],1000[INTEGER]] + * \_Join[LEFT,[language_code{r}#5],[language_code{r}#5],[language_code{f}#22]] + * |_EsqlProject[[_meta_field{f}#17, emp_no{f}#11, first_name{f}#12, gender{f}#13, hire_date{f}#18, job{f}#19, job.raw{f}#20, l + * anguages{f}#14 AS language_code, last_name{f}#15, long_noidx{f}#21, salary{f}#16, foo{r}#7]] + * | \_Eval[[[62 61 72][KEYWORD] AS foo]] + * | \_Filter[languages{f}#14 > 1[INTEGER]] + * | \_EsRelation[test][_meta_field{f}#17, emp_no{f}#11, first_name{f}#12, ..] + * \_EsRelation[languages_lookup][LOOKUP][language_code{f}#22, language_name{f}#23] + */ + public void testRedundantSortOnJoin() throws Exception { + assumeTrue("Requires LOOKUP JOIN", EsqlCapabilities.Cap.JOIN_LOOKUP_V12.isEnabled()); + + var plan = optimizedPlan(""" + FROM test + | SORT languages + | RENAME languages AS language_code + | EVAL foo = "bar" + | LOOKUP JOIN languages_lookup ON language_code + | WHERE language_code > 1 + | SORT emp_no + """); + + var topN = as(plan, TopN.class); + var join = as(topN.child(), Join.class); + var project = as(join.left(), EsqlProject.class); + var eval = as(project.child(), Eval.class); + var filter = as(eval.child(), Filter.class); + as(filter.child(), EsRelation.class); + } + + /** + * TopN[[Order[emp_no{f}#9,ASC,LAST]],1000[INTEGER]] + * \_Filter[emp_no{f}#9 > 1[INTEGER]] + * \_MvExpand[languages{f}#12,languages{r}#20,null] + * \_Eval[[[62 61 72][KEYWORD] AS foo]] + * \_EsRelation[test][_meta_field{f}#15, emp_no{f}#9, first_name{f}#10, g..] + */ + public void testRedundantSortOnMvExpand() throws Exception { + var plan = optimizedPlan(""" + FROM test + | SORT languages + | EVAL foo = "bar" + | MV_EXPAND languages + | WHERE emp_no > 1 + | SORT emp_no + """); + + var topN = as(plan, TopN.class); + var filter = as(topN.child(), Filter.class); + var mvExpand = as(filter.child(), MvExpand.class); + var eval = as(mvExpand.child(), Eval.class); + as(eval.child(), EsRelation.class); + } + + /** + * TopN[[Order[emp_no{f}#11,ASC,LAST]],1000[INTEGER]] + * \_Join[LEFT,[language_code{r}#5],[language_code{r}#5],[language_code{f}#22]] + * |_Filter[emp_no{f}#11 > 1[INTEGER]] + * | \_MvExpand[languages{f}#14,languages{r}#24,null] + * | \_Eval[[languages{f}#14 AS language_code]] + * | \_EsRelation[test][_meta_field{f}#17, emp_no{f}#11, first_name{f}#12, ..] + * \_EsRelation[languages_lookup][LOOKUP][language_code{f}#22, language_name{f}#23] + */ + public void testRedundantSortOnMvExpandAndJoin() throws Exception { + var plan = optimizedPlan(""" + FROM test + | SORT languages + | EVAL language_code = languages + | MV_EXPAND languages + | WHERE emp_no > 1 + | LOOKUP JOIN languages_lookup ON language_code + | SORT emp_no + """); + + var topN = as(plan, TopN.class); + var join = as(topN.child(), Join.class); + var filter = as(join.left(), Filter.class); + var mvExpand = as(filter.child(), MvExpand.class); + var eval = as(mvExpand.child(), Eval.class); + as(eval.child(), EsRelation.class); + } + + /** + * TopN[[Order[emp_no{f}#12,ASC,LAST]],1000[INTEGER]] + * \_Join[LEFT,[language_code{r}#5],[language_code{r}#5],[language_code{f}#23]] + * |_Filter[emp_no{f}#12 > 1[INTEGER]] + * | \_MvExpand[languages{f}#15,languages{r}#25,null] + * | \_Eval[[languages{f}#15 AS language_code]] + * | \_EsRelation[test][_meta_field{f}#18, emp_no{f}#12, first_name{f}#13, ..] + * \_EsRelation[languages_lookup][LOOKUP][language_code{f}#23, language_name{f}#24] + */ + public void testMultlipleRedundantSortOnMvExpandAndJoin() throws Exception { + var plan = optimizedPlan(""" + FROM test + | SORT first_name + | EVAL language_code = languages + | MV_EXPAND languages + | sort last_name + | WHERE emp_no > 1 + | LOOKUP JOIN languages_lookup ON language_code + | SORT emp_no + """); + + var topN = as(plan, TopN.class); + var join = as(topN.child(), Join.class); + var filter = as(join.left(), Filter.class); + var mvExpand = as(filter.child(), MvExpand.class); + var eval = as(mvExpand.child(), Eval.class); + as(eval.child(), EsRelation.class); + } } From a0b463a3f32856e7798b9fec66f1914e1e67147e Mon Sep 17 00:00:00 2001 From: Luigi Dell'Aquila Date: Wed, 29 Jan 2025 12:39:53 +0100 Subject: [PATCH 02/13] Update docs/changelog/121156.yaml --- docs/changelog/121156.yaml | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 docs/changelog/121156.yaml diff --git a/docs/changelog/121156.yaml b/docs/changelog/121156.yaml new file mode 100644 index 0000000000000..8f9c5ccdb38c9 --- /dev/null +++ b/docs/changelog/121156.yaml @@ -0,0 +1,5 @@ +pr: 121156 +summary: Remove redundant sorts from execution plan +area: ES|QL +type: bug +issues: [] From 78c86dea1f247add43e04cc5598c2034e8273eca Mon Sep 17 00:00:00 2001 From: Luigi Dell'Aquila Date: Wed, 29 Jan 2025 14:02:12 +0100 Subject: [PATCH 03/13] More tests --- .../src/main/resources/lookup-join.csv-spec | 27 +++++++ .../optimizer/LogicalPlanOptimizerTests.java | 74 +++++++++++++++++++ 2 files changed, 101 insertions(+) diff --git a/x-pack/plugin/esql/qa/testFixtures/src/main/resources/lookup-join.csv-spec b/x-pack/plugin/esql/qa/testFixtures/src/main/resources/lookup-join.csv-spec index 31716bcf83848..8ca4292f97faa 100644 --- a/x-pack/plugin/esql/qa/testFixtures/src/main/resources/lookup-join.csv-spec +++ b/x-pack/plugin/esql/qa/testFixtures/src/main/resources/lookup-join.csv-spec @@ -1366,3 +1366,30 @@ emp_no:integer | language_code:integer | language_name:keyword 10092 | 1 | English 10093 | 3 | Spanish ; + + + +sortBeforeAndAfterMultipleJoinAndMvExpand +required_capability: join_lookup_v12 +required_capability: remove_redundant_sort + +FROM employees +| sort first_name +| EVAL language_code = languages +| LOOKUP JOIN languages_lookup ON language_code +| WHERE emp_no >= 10091 AND emp_no < 10094 +| SORT language_name +| MV_EXPAND first_name +| SORT first_name +| MV_EXPAND last_name +| SORT last_name +| LOOKUP JOIN languages_lookup ON language_code +| SORT emp_no +| KEEP emp_no, language_code, language_name +; + +emp_no:integer | language_code:integer | language_name:keyword +10091 | 3 | Spanish +10092 | 1 | English +10093 | 3 | Spanish +; diff --git a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/optimizer/LogicalPlanOptimizerTests.java b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/optimizer/LogicalPlanOptimizerTests.java index f949b23d5898d..fd3f9c8c736fc 100644 --- a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/optimizer/LogicalPlanOptimizerTests.java +++ b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/optimizer/LogicalPlanOptimizerTests.java @@ -7380,4 +7380,78 @@ public void testMultlipleRedundantSortOnMvExpandAndJoin() throws Exception { var eval = as(mvExpand.child(), Eval.class); as(eval.child(), EsRelation.class); } + + /** + * TopN[[Order[emp_no{f}#16,ASC,LAST]],1000[INTEGER]] + * \_Filter[emp_no{f}#16 > 1[INTEGER]] + * \_MvExpand[languages{f}#19,languages{r}#31] + * \_Dissect[foo{r}#5,Parser[pattern=%{z}, appendSeparator=, parser=org.elasticsearch.dissect.DissectParser@26f2cab],[z{r}#10 + * ]] + * \_Grok[foo{r}#5,Parser[pattern=%{WORD:y}, grok=org.elasticsearch.grok.Grok@6ea44ccd],[y{r}#9]] + * \_Enrich[ANY,[6c 61 6e 67 75 61 67 65 73 5f 69 64 78][KEYWORD],foo{r}#5,{"match":{"indices":[],"match_field":"id","enrich_ + * fields":["language_code","language_name"]}},{=languages_idx},[language_code{r}#29, language_name{r}#30]] + * \_Eval[[TOSTRING(languages{f}#19) AS foo]] + * \_EsRelation[test][_meta_field{f}#22, emp_no{f}#16, first_name{f}#17, ..] + */ + public void testRedundantSortOnMvExpandEnrichGrokDissect() throws Exception { + var plan = optimizedPlan(""" + FROM test + | SORT languages + | EVAL foo = to_string(languages) + | ENRICH languages_idx on foo + | GROK foo "%{WORD:y}" + | DISSECT foo "%{z}" + | MV_EXPAND languages + | WHERE emp_no > 1 + | SORT emp_no + """); + + var topN = as(plan, TopN.class); + var filter = as(topN.child(), Filter.class); + var mvExpand = as(filter.child(), MvExpand.class); + var dissect = as(mvExpand.child(), Dissect.class); + var grok = as(dissect.child(), Grok.class); + var enrich = as(grok.child(), Enrich.class); + var eval = as(enrich.child(), Eval.class); + as(eval.child(), EsRelation.class); + } + + /** + * TopN[[Order[emp_no{f}#20,ASC,LAST]],1000[INTEGER]] + * \_Filter[emp_no{f}#20 > 1[INTEGER]] + * \_MvExpand[languages{f}#23,languages{r}#37] + * \_Dissect[foo{r}#5,Parser[pattern=%{z}, appendSeparator=, parser=org.elasticsearch.dissect.DissectParser@3e922db0],[z{r}#1 + * 4]] + * \_Grok[foo{r}#5,Parser[pattern=%{WORD:y}, grok=org.elasticsearch.grok.Grok@4d6ad024],[y{r}#13]] + * \_Enrich[ANY,[6c 61 6e 67 75 61 67 65 73 5f 69 64 78][KEYWORD],foo{r}#5,{"match":{"indices":[],"match_field":"id","enrich_ + * fields":["language_code","language_name"]}},{=languages_idx},[language_code{r}#35, language_name{r}#36]] + * \_Join[LEFT,[language_code{r}#8],[language_code{r}#8],[language_code{f}#31]] + * |_Eval[[TOSTRING(languages{f}#23) AS foo, languages{f}#23 AS language_code]] + * | \_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() throws Exception { + var plan = optimizedPlan(""" + FROM test + | SORT languages + | EVAL foo = to_string(languages), language_code = languages + | LOOKUP JOIN languages_lookup ON language_code + | ENRICH languages_idx on foo + | GROK foo "%{WORD:y}" + | DISSECT foo "%{z}" + | MV_EXPAND languages + | WHERE emp_no > 1 + | SORT emp_no + """); + + var topN = as(plan, TopN.class); + var filter = as(topN.child(), Filter.class); + var mvExpand = as(filter.child(), MvExpand.class); + var dissect = as(mvExpand.child(), Dissect.class); + var grok = as(dissect.child(), Grok.class); + var enrich = as(grok.child(), Enrich.class); + var join = as(enrich.child(), Join.class); + var eval = as(join.left(), Eval.class); + as(eval.child(), EsRelation.class); + } } From 0ec1539f4b30be46ea7fdffe1942cb25ae4b2fd5 Mon Sep 17 00:00:00 2001 From: Luigi Dell'Aquila Date: Thu, 30 Jan 2025 11:00:27 +0100 Subject: [PATCH 04/13] Verify OrderBy after optimization --- muted-tests.yml | 2 -- .../xpack/esql/optimizer/LogicalVerifier.java | 3 ++ .../xpack/esql/plan/logical/OrderBy.java | 8 ++++- .../optimizer/LogicalPlanOptimizerTests.java | 29 +++++++++++++++---- 4 files changed, 33 insertions(+), 9 deletions(-) diff --git a/muted-tests.yml b/muted-tests.yml index 1b492c4efaf3d..6915fb9503cb6 100644 --- a/muted-tests.yml +++ b/muted-tests.yml @@ -314,8 +314,6 @@ tests: - class: org.elasticsearch.xpack.security.profile.ProfileIntegTests method: testSetEnabled issue: https://github.com/elastic/elasticsearch/issues/121183 -- class: org.elasticsearch.xpack.esql.optimizer.LogicalPlanOptimizerTests - issue: https://github.com/elastic/elasticsearch/issues/121185 - class: org.elasticsearch.xpack.security.CoreWithSecurityClientYamlTestSuiteIT method: test {yaml=cat.aliases/10_basic/Simple alias} issue: https://github.com/elastic/elasticsearch/issues/121186 diff --git a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/LogicalVerifier.java b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/LogicalVerifier.java index 94248ce2ecd0a..c474c48d6d96b 100644 --- a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/LogicalVerifier.java +++ b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/LogicalVerifier.java @@ -27,6 +27,9 @@ public Failures verify(LogicalPlan plan) { PlanConsistencyChecker.checkPlan(p, dependencyFailures); if (failures.hasFailures() == false) { + if (p instanceof PostOptimizationVerificationAware pova) { + pova.postOptimizationVerification(failures); + } p.forEachExpression(ex -> { if (ex instanceof PostOptimizationVerificationAware va) { va.postOptimizationVerification(failures); diff --git a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/logical/OrderBy.java b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/logical/OrderBy.java index 051e2c7769bde..0f359f815ead1 100644 --- a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/logical/OrderBy.java +++ b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/logical/OrderBy.java @@ -10,6 +10,7 @@ import org.elasticsearch.common.io.stream.StreamInput; import org.elasticsearch.common.io.stream.StreamOutput; import org.elasticsearch.xpack.esql.capabilities.PostAnalysisVerificationAware; +import org.elasticsearch.xpack.esql.capabilities.PostOptimizationVerificationAware; import org.elasticsearch.xpack.esql.capabilities.TelemetryAware; import org.elasticsearch.xpack.esql.common.Failures; import org.elasticsearch.xpack.esql.core.capabilities.Resolvables; @@ -25,7 +26,7 @@ import static org.elasticsearch.xpack.esql.common.Failure.fail; -public class OrderBy extends UnaryPlan implements PostAnalysisVerificationAware, TelemetryAware { +public class OrderBy extends UnaryPlan implements PostAnalysisVerificationAware, PostOptimizationVerificationAware, TelemetryAware { public static final NamedWriteableRegistry.Entry ENTRY = new NamedWriteableRegistry.Entry(LogicalPlan.class, "OrderBy", OrderBy::new); private final List order; @@ -109,4 +110,9 @@ public void postAnalysisVerification(Failures failures) { } }); } + + @Override + public void postOptimizationVerification(Failures failures) { + failures.add(fail(this, "The query cannot be executed because it would require unbounded sort")); + } } diff --git a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/optimizer/LogicalPlanOptimizerTests.java b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/optimizer/LogicalPlanOptimizerTests.java index fd3f9c8c736fc..365879c0c83e5 100644 --- a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/optimizer/LogicalPlanOptimizerTests.java +++ b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/optimizer/LogicalPlanOptimizerTests.java @@ -7279,7 +7279,7 @@ public void testFunctionNamedParamsAsFunctionArgument() { * | \_EsRelation[test][_meta_field{f}#17, emp_no{f}#11, first_name{f}#12, ..] * \_EsRelation[languages_lookup][LOOKUP][language_code{f}#22, language_name{f}#23] */ - public void testRedundantSortOnJoin() throws Exception { + public void testRedundantSortOnJoin() { assumeTrue("Requires LOOKUP JOIN", EsqlCapabilities.Cap.JOIN_LOOKUP_V12.isEnabled()); var plan = optimizedPlan(""" @@ -7307,7 +7307,7 @@ public void testRedundantSortOnJoin() throws Exception { * \_Eval[[[62 61 72][KEYWORD] AS foo]] * \_EsRelation[test][_meta_field{f}#15, emp_no{f}#9, first_name{f}#10, g..] */ - public void testRedundantSortOnMvExpand() throws Exception { + public void testRedundantSortOnMvExpand() { var plan = optimizedPlan(""" FROM test | SORT languages @@ -7333,7 +7333,7 @@ public void testRedundantSortOnMvExpand() throws Exception { * | \_EsRelation[test][_meta_field{f}#17, emp_no{f}#11, first_name{f}#12, ..] * \_EsRelation[languages_lookup][LOOKUP][language_code{f}#22, language_name{f}#23] */ - public void testRedundantSortOnMvExpandAndJoin() throws Exception { + public void testRedundantSortOnMvExpandAndJoin() { var plan = optimizedPlan(""" FROM test | SORT languages @@ -7361,7 +7361,7 @@ public void testRedundantSortOnMvExpandAndJoin() throws Exception { * | \_EsRelation[test][_meta_field{f}#18, emp_no{f}#12, first_name{f}#13, ..] * \_EsRelation[languages_lookup][LOOKUP][language_code{f}#23, language_name{f}#24] */ - public void testMultlipleRedundantSortOnMvExpandAndJoin() throws Exception { + public void testMultlipleRedundantSortOnMvExpandAndJoin() { var plan = optimizedPlan(""" FROM test | SORT first_name @@ -7393,7 +7393,7 @@ public void testMultlipleRedundantSortOnMvExpandAndJoin() throws Exception { * \_Eval[[TOSTRING(languages{f}#19) AS foo]] * \_EsRelation[test][_meta_field{f}#22, emp_no{f}#16, first_name{f}#17, ..] */ - public void testRedundantSortOnMvExpandEnrichGrokDissect() throws Exception { + public void testRedundantSortOnMvExpandEnrichGrokDissect() { var plan = optimizedPlan(""" FROM test | SORT languages @@ -7430,7 +7430,7 @@ public void testRedundantSortOnMvExpandEnrichGrokDissect() throws Exception { * | \_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() throws Exception { + public void testRedundantSortOnMvExpandJoinEnrichGrokDissect() { var plan = optimizedPlan(""" FROM test | SORT languages @@ -7454,4 +7454,21 @@ public void testRedundantSortOnMvExpandJoinEnrichGrokDissect() throws Exception var eval = as(join.left(), Eval.class); as(eval.child(), EsRelation.class); } + + public void testUnboundedSort() throws Exception { + String query = """ + FROM test + | EVAL language_code = 1 + | LOOKUP JOIN languages_lookup ON language_code + | SORT language_name + | MV_EXPAND language_name + | EVAL foo = concat(language_name, "foo") + | MV_EXPAND foo + | WHERE emp_no > 1 + | SORT emp_no + """; + + var e = expectThrows(VerificationException.class, () -> plan(query)); + assertThat(e.getMessage(), is("Found 1 problem\nline 4:3: The query cannot be executed because it would require unbounded sort")); + } } From 3af3c11909640a7b17b3f421c46a8db5e854d79c Mon Sep 17 00:00:00 2001 From: Luigi Dell'Aquila Date: Tue, 4 Feb 2025 11:12:11 +0100 Subject: [PATCH 05/13] Implement review suggestions --- .../esql/optimizer/LogicalPlanOptimizer.java | 7 +- .../logical/PruneOrderByBeforeStats.java | 47 ------- .../rules/logical/PruneRedundantOrderBy.java | 116 ++++++++++++++++++ .../rules/logical/RemoveRedundantSort.java | 88 ------------- .../xpack/esql/plan/logical/OrderBy.java | 2 +- .../optimizer/LogicalPlanOptimizerTests.java | 36 ++++-- 6 files changed, 145 insertions(+), 151 deletions(-) delete mode 100644 x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/rules/logical/PruneOrderByBeforeStats.java create mode 100644 x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/rules/logical/PruneRedundantOrderBy.java delete mode 100644 x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/rules/logical/RemoveRedundantSort.java diff --git a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/LogicalPlanOptimizer.java b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/LogicalPlanOptimizer.java index b77f25c71fcf5..2eb2259d71d76 100644 --- a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/LogicalPlanOptimizer.java +++ b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/LogicalPlanOptimizer.java @@ -32,7 +32,7 @@ import org.elasticsearch.xpack.esql.optimizer.rules.logical.PruneEmptyPlans; import org.elasticsearch.xpack.esql.optimizer.rules.logical.PruneFilters; import org.elasticsearch.xpack.esql.optimizer.rules.logical.PruneLiteralsInOrderBy; -import org.elasticsearch.xpack.esql.optimizer.rules.logical.PruneOrderByBeforeStats; +import org.elasticsearch.xpack.esql.optimizer.rules.logical.PruneRedundantOrderBy; import org.elasticsearch.xpack.esql.optimizer.rules.logical.PruneRedundantSortClauses; import org.elasticsearch.xpack.esql.optimizer.rules.logical.PushDownAndCombineFilters; import org.elasticsearch.xpack.esql.optimizer.rules.logical.PushDownAndCombineLimits; @@ -40,7 +40,6 @@ import org.elasticsearch.xpack.esql.optimizer.rules.logical.PushDownEnrich; import org.elasticsearch.xpack.esql.optimizer.rules.logical.PushDownEval; import org.elasticsearch.xpack.esql.optimizer.rules.logical.PushDownRegexExtract; -import org.elasticsearch.xpack.esql.optimizer.rules.logical.RemoveRedundantSort; import org.elasticsearch.xpack.esql.optimizer.rules.logical.RemoveStatsOverride; import org.elasticsearch.xpack.esql.optimizer.rules.logical.ReplaceAggregateAggExpressionWithEval; import org.elasticsearch.xpack.esql.optimizer.rules.logical.ReplaceAggregateNestedExpressionWithEval; @@ -190,12 +189,12 @@ protected static Batch operators() { new PushDownRegexExtract(), new PushDownEnrich(), new PushDownAndCombineOrderBy(), - new PruneOrderByBeforeStats(), + new PruneRedundantOrderBy(), new PruneRedundantSortClauses() ); } protected static Batch cleanup() { - return new Batch<>("Clean Up", new ReplaceLimitAndSortAsTopN(), new ReplaceRowAsLocalRelation(), new RemoveRedundantSort()); + return new Batch<>("Clean Up", new ReplaceLimitAndSortAsTopN(), new ReplaceRowAsLocalRelation()); } } diff --git a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/rules/logical/PruneOrderByBeforeStats.java b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/rules/logical/PruneOrderByBeforeStats.java deleted file mode 100644 index 24fb8971487d5..0000000000000 --- a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/rules/logical/PruneOrderByBeforeStats.java +++ /dev/null @@ -1,47 +0,0 @@ -/* - * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one - * or more contributor license agreements. Licensed under the Elastic License - * 2.0; you may not use this file except in compliance with the Elastic License - * 2.0. - */ - -package org.elasticsearch.xpack.esql.optimizer.rules.logical; - -import org.elasticsearch.xpack.esql.plan.logical.Aggregate; -import org.elasticsearch.xpack.esql.plan.logical.Enrich; -import org.elasticsearch.xpack.esql.plan.logical.Eval; -import org.elasticsearch.xpack.esql.plan.logical.Filter; -import org.elasticsearch.xpack.esql.plan.logical.LogicalPlan; -import org.elasticsearch.xpack.esql.plan.logical.OrderBy; -import org.elasticsearch.xpack.esql.plan.logical.Project; -import org.elasticsearch.xpack.esql.plan.logical.RegexExtract; -import org.elasticsearch.xpack.esql.plan.logical.UnaryPlan; - -public final class PruneOrderByBeforeStats extends OptimizerRules.OptimizerRule { - - @Override - protected LogicalPlan rule(Aggregate agg) { - OrderBy order = findPullableOrderBy(agg.child()); - - LogicalPlan p = agg; - if (order != null) { - p = agg.transformDown(OrderBy.class, o -> o == order ? order.child() : o); - } - return p; - } - - private static OrderBy findPullableOrderBy(LogicalPlan plan) { - OrderBy pullable = null; - if (plan instanceof OrderBy o) { - pullable = o; - } else if (plan instanceof Eval - || plan instanceof Filter - || plan instanceof Project - || plan instanceof RegexExtract - || plan instanceof Enrich) { - pullable = findPullableOrderBy(((UnaryPlan) plan).child()); - } - return pullable; - } - -} diff --git a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/rules/logical/PruneRedundantOrderBy.java b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/rules/logical/PruneRedundantOrderBy.java new file mode 100644 index 0000000000000..b560e01ed2424 --- /dev/null +++ b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/rules/logical/PruneRedundantOrderBy.java @@ -0,0 +1,116 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the Elastic License + * 2.0; you may not use this file except in compliance with the Elastic License + * 2.0. + */ + +package org.elasticsearch.xpack.esql.optimizer.rules.logical; + +import org.elasticsearch.xpack.esql.plan.logical.Aggregate; +import org.elasticsearch.xpack.esql.plan.logical.Drop; +import org.elasticsearch.xpack.esql.plan.logical.Enrich; +import org.elasticsearch.xpack.esql.plan.logical.Eval; +import org.elasticsearch.xpack.esql.plan.logical.Filter; +import org.elasticsearch.xpack.esql.plan.logical.InlineStats; +import org.elasticsearch.xpack.esql.plan.logical.LogicalPlan; +import org.elasticsearch.xpack.esql.plan.logical.Lookup; +import org.elasticsearch.xpack.esql.plan.logical.MvExpand; +import org.elasticsearch.xpack.esql.plan.logical.OrderBy; +import org.elasticsearch.xpack.esql.plan.logical.Project; +import org.elasticsearch.xpack.esql.plan.logical.RegexExtract; +import org.elasticsearch.xpack.esql.plan.logical.Rename; +import org.elasticsearch.xpack.esql.plan.logical.TopN; +import org.elasticsearch.xpack.esql.plan.logical.UnaryPlan; +import org.elasticsearch.xpack.esql.plan.logical.join.Join; + +import java.util.ArrayList; +import java.util.IdentityHashMap; +import java.util.List; + +/** + * SORT cannot be executed without a LIMIT, as ES|QL doesn't support unbounded sort (yet). + *

+ * The planner tries to push down LIMIT and transform all the unbounded sorts into a TopN. + * In some cases it's not possible though, eg. + *

+ * from test | sort x | lookup join lookup on x | sort y + *

+ * from test | sort x | mv_expand x | sort y + *

+ * "sort y" will become a TopN, but "sort x" will remain unbounded, so the query could not be executed. + *

+ * In most cases though, following commands can make the previous SORTs redundant, + * because it will re-sort previously sorted results (eg. if there is another SORT) + * or because the order will be scrambled by another command (eg. a STATS) + *

+ * This rule finds and prunes redundant SORTs, attempting to make the plan executable. + */ +public class PruneRedundantOrderBy extends OptimizerRules.OptimizerRule { + + @Override + protected LogicalPlan rule(LogicalPlan plan) { + if (plan instanceof OrderBy || plan instanceof TopN || plan instanceof Aggregate) { + IdentityHashMap redundant = findRedundantSort(((UnaryPlan) plan).child()); + if (redundant.isEmpty()) { + return plan; + } + return plan.transformUp(p -> { + if (redundant.containsKey(p)) { + return ((OrderBy) p).child(); + } + return p; + }); + } else { + return plan; + } + } + + private IdentityHashMap findRedundantSort(LogicalPlan plan) { + List toCheck = new ArrayList<>(); + toCheck.add(plan); + + IdentityHashMap result = new IdentityHashMap<>(); + LogicalPlan p = null; + while (true) { + if (p == null) { + if (toCheck.isEmpty()) { + return result; + } else { + p = toCheck.remove(0); + } + } else if (p instanceof OrderBy ob) { + result.put(ob, null); + p = ob.child(); + } else if (p instanceof UnaryPlan unary) { + if (unary instanceof Project + || unary instanceof Drop + || unary instanceof Rename + || unary instanceof MvExpand + || unary instanceof Enrich + || unary instanceof RegexExtract + || unary instanceof InlineStats + || unary instanceof Lookup + // 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) { + p = unary.child(); + } else { + // stop here, other unary plans could be sensitive to SORT + p = null; + } + } else if (p instanceof Join lj) { + toCheck.add(lj.left()); + toCheck.add(lj.right()); + p = null; + } else { + // stop here, other unary plans could be sensitive to SORT + p = null; + } + } + } +} diff --git a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/rules/logical/RemoveRedundantSort.java b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/rules/logical/RemoveRedundantSort.java deleted file mode 100644 index b7b19977e3ff3..0000000000000 --- a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/rules/logical/RemoveRedundantSort.java +++ /dev/null @@ -1,88 +0,0 @@ -/* - * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one - * or more contributor license agreements. Licensed under the Elastic License - * 2.0; you may not use this file except in compliance with the Elastic License - * 2.0. - */ - -package org.elasticsearch.xpack.esql.optimizer.rules.logical; - -import org.elasticsearch.xpack.esql.plan.logical.Dissect; -import org.elasticsearch.xpack.esql.plan.logical.Enrich; -import org.elasticsearch.xpack.esql.plan.logical.Filter; -import org.elasticsearch.xpack.esql.plan.logical.Grok; -import org.elasticsearch.xpack.esql.plan.logical.LogicalPlan; -import org.elasticsearch.xpack.esql.plan.logical.MvExpand; -import org.elasticsearch.xpack.esql.plan.logical.OrderBy; -import org.elasticsearch.xpack.esql.plan.logical.Project; -import org.elasticsearch.xpack.esql.plan.logical.Rename; -import org.elasticsearch.xpack.esql.plan.logical.TopN; -import org.elasticsearch.xpack.esql.plan.logical.UnaryPlan; -import org.elasticsearch.xpack.esql.plan.logical.join.Join; - -/** - * SORT cannot be executed without a LIMIT, as ES|QL doesn't support unbounded sort (yet). - * - * The planner tries to push down LIMIT and transform all the unbounded sorts into a TopN. - * In some cases it's not possible though, eg. - * - * from test | sort x | lookup join lookup on x | sort y - * - * from test | sort x | mv_expand x | sort y - * - * "sort y" will become a TopN, but "sort x" will remain unbounded, so the query could not be executed. - * - * In most cases though, last SORT make the previous SORTs redundant, - * ie. it will re-sort previously sorted results - * often with a different order. - * - * This rule finds and removes redundant SORTs, making the plan executable. - */ -public class RemoveRedundantSort extends OptimizerRules.OptimizerRule { - - @Override - 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; - }); - } - - private OrderBy findRedundantSort(TopN plan) { - LogicalPlan p = plan.child(); - while (true) { - if (p instanceof OrderBy ob) { - return ob; - } - if (p instanceof UnaryPlan unary) { - if (unary instanceof Filter - || unary instanceof Project - || unary instanceof Rename - || unary instanceof MvExpand - || unary instanceof Enrich - || unary instanceof Grok - || unary instanceof Dissect - // If we introduce window functions, the previous sort could actually become relevant - // so to be sure we don't introduce regressions, we'll have to exclude places where these functions could be used - // || unary instanceof Eval - // || unary instanceof Aggregate - ) { - p = unary.child(); - continue; - } - } else if (p instanceof Join lj) { - p = lj.left(); - // TODO do it also on the right-hand side? - continue; - } - return null; - } - } - -} diff --git a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/logical/OrderBy.java b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/logical/OrderBy.java index 0f359f815ead1..1660dbc63ffd9 100644 --- a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/logical/OrderBy.java +++ b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/logical/OrderBy.java @@ -113,6 +113,6 @@ public void postAnalysisVerification(Failures failures) { @Override public void postOptimizationVerification(Failures failures) { - failures.add(fail(this, "The query cannot be executed because it would require unbounded sort")); + failures.add(fail(this, "Unbounded sort not supported yet, please add a limit")); } } diff --git a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/optimizer/LogicalPlanOptimizerTests.java b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/optimizer/LogicalPlanOptimizerTests.java index 365879c0c83e5..3693ea1704b46 100644 --- a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/optimizer/LogicalPlanOptimizerTests.java +++ b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/optimizer/LogicalPlanOptimizerTests.java @@ -2219,8 +2219,7 @@ public void testPushDown_TheRightLimit_PastLookupJoin() { * \_TopN[[Order[salary{f}#12,ASC,LAST]],5[INTEGER]] * \_Eval[[100[INTEGER] AS b]] * \_MvExpand[first_name{f}#11] - * \_TopN[[Order[first_name{f}#11,ASC,LAST]],10000[INTEGER]] - * \_EsRelation[employees][emp_no{f}#10, first_name{f}#11, salary{f}#12] + * \_EsRelation[employees][emp_no{f}#10, first_name{f}#11, salary{f}#12] */ public void testPushDownLimit_PastEvalAndMvExpand() { LogicalPlan plan = optimizedPlan(""" @@ -2238,10 +2237,7 @@ public void testPushDownLimit_PastEvalAndMvExpand() { assertThat(orderNames(topN), contains("salary")); var eval = as(topN.child(), Eval.class); var mvExp = as(eval.child(), MvExpand.class); - topN = as(mvExp.child(), TopN.class); - assertThat(topN.limit().fold(FoldContext.small()), equalTo(10000)); - assertThat(orderNames(topN), contains("first_name")); - as(topN.child(), EsRelation.class); + as(mvExp.child(), EsRelation.class); } /** @@ -7455,8 +7451,19 @@ public void testRedundantSortOnMvExpandJoinEnrichGrokDissect() { as(eval.child(), EsRelation.class); } - public void testUnboundedSort() throws Exception { - String query = """ + /** + * TopN[[Order[emp_no{f}#15,ASC,LAST]],1000[INTEGER]] + * \_Filter[emp_no{f}#15 > 1[INTEGER]] + * \_MvExpand[foo{r}#10,foo{r}#29] + * \_Eval[[CONCAT(language_name{r}#28,[66 6f 6f][KEYWORD]) AS foo]] + * \_MvExpand[language_name{f}#27,language_name{r}#28] + * \_Join[LEFT,[language_code{r}#3],[language_code{r}#3],[language_code{f}#26]] + * |_Eval[[1[INTEGER] AS language_code]] + * | \_EsRelation[test][_meta_field{f}#21, emp_no{f}#15, first_name{f}#16, ..] + * \_EsRelation[languages_lookup][LOOKUP][language_code{f}#26, language_name{f}#27] + */ + public void testEvalLookupMultipleSorts() { + var plan = optimizedPlan(""" FROM test | EVAL language_code = 1 | LOOKUP JOIN languages_lookup ON language_code @@ -7466,9 +7473,16 @@ public void testUnboundedSort() throws Exception { | MV_EXPAND foo | WHERE emp_no > 1 | SORT emp_no - """; + """); + + var topN = as(plan, TopN.class); + var filter = as(topN.child(), Filter.class); + var mvExpand = as(filter.child(), MvExpand.class); + var eval = as(mvExpand.child(), Eval.class); + mvExpand = as(eval.child(), MvExpand.class); + var join = as(mvExpand.child(), Join.class); + eval = as(join.left(), Eval.class); + as(eval.child(), EsRelation.class); - var e = expectThrows(VerificationException.class, () -> plan(query)); - assertThat(e.getMessage(), is("Found 1 problem\nline 4:3: The query cannot be executed because it would require unbounded sort")); } } From c73246d378aa26283b82fd9085b1c46ee10797d5 Mon Sep 17 00:00:00 2001 From: Luigi Dell'Aquila Date: Tue, 4 Feb 2025 11:39:57 +0100 Subject: [PATCH 06/13] Remove AddDefaultTopN rule --- .../esql/optimizer/LogicalPlanOptimizer.java | 4 +- .../rules/logical/AddDefaultTopN.java | 54 ------------------- 2 files changed, 1 insertion(+), 57 deletions(-) delete mode 100644 x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/rules/logical/AddDefaultTopN.java diff --git a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/LogicalPlanOptimizer.java b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/LogicalPlanOptimizer.java index 2eb2259d71d76..bc32945d73eb5 100644 --- a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/LogicalPlanOptimizer.java +++ b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/LogicalPlanOptimizer.java @@ -10,7 +10,6 @@ import org.elasticsearch.xpack.esql.VerificationException; import org.elasticsearch.xpack.esql.common.Failures; import org.elasticsearch.xpack.esql.core.type.DataType; -import org.elasticsearch.xpack.esql.optimizer.rules.logical.AddDefaultTopN; import org.elasticsearch.xpack.esql.optimizer.rules.logical.BooleanFunctionEqualsElimination; import org.elasticsearch.xpack.esql.optimizer.rules.logical.BooleanSimplification; import org.elasticsearch.xpack.esql.optimizer.rules.logical.CombineBinaryComparisons; @@ -116,10 +115,9 @@ protected List> batches() { protected static List> rules() { var skip = new Batch<>("Skip Compute", new SkipQueryOnLimitZero()); - var defaultTopN = new Batch<>("Add default TopN", new AddDefaultTopN()); var label = new Batch<>("Set as Optimized", Limiter.ONCE, new SetAsOptimized()); - return asList(substitutions(), operators(), skip, cleanup(), defaultTopN, label); + return asList(substitutions(), operators(), skip, cleanup(), label); } protected static Batch substitutions() { diff --git a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/rules/logical/AddDefaultTopN.java b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/rules/logical/AddDefaultTopN.java deleted file mode 100644 index ef091686a4b38..0000000000000 --- a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/rules/logical/AddDefaultTopN.java +++ /dev/null @@ -1,54 +0,0 @@ -/* - * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one - * or more contributor license agreements. Licensed under the Elastic License - * 2.0; you may not use this file except in compliance with the Elastic License - * 2.0. - */ - -package org.elasticsearch.xpack.esql.optimizer.rules.logical; - -import org.elasticsearch.xpack.esql.core.expression.Literal; -import org.elasticsearch.xpack.esql.core.type.DataType; -import org.elasticsearch.xpack.esql.optimizer.LogicalOptimizerContext; -import org.elasticsearch.xpack.esql.plan.logical.EsRelation; -import org.elasticsearch.xpack.esql.plan.logical.LogicalPlan; -import org.elasticsearch.xpack.esql.plan.logical.OrderBy; -import org.elasticsearch.xpack.esql.plan.logical.TopN; -import org.elasticsearch.xpack.esql.plan.logical.UnaryPlan; - -/** - * This adds an explicit TopN node to a plan that only has an OrderBy right before Lucene. - * To date, the only known use case that "needs" this is a query of the form - * from test - * | sort emp_no - * | mv_expand first_name - * | rename first_name AS x - * | where x LIKE "*a*" - * | limit 15 - *

- * or - *

- * from test - * | sort emp_no - * | mv_expand first_name - * | sort first_name - * | limit 15 - *

- * {@link PushDownAndCombineLimits} will copy the "limit 15" after "sort emp_no" if there is no filter on the expanded values - * OR if there is no sort between "limit" and "mv_expand". - * But, since this type of query has such a filter, the "sort emp_no" will have no limit when it reaches the current rule. - */ -public final class AddDefaultTopN extends OptimizerRules.ParameterizedOptimizerRule { - public AddDefaultTopN() { - super(OptimizerRules.TransformDirection.DOWN); - } - - @Override - protected LogicalPlan rule(LogicalPlan plan, LogicalOptimizerContext context) { - if (plan instanceof UnaryPlan unary && unary.child() instanceof OrderBy order && order.child() instanceof EsRelation relation) { - var limit = new Literal(plan.source(), context.configuration().resultTruncationMaxSize(), DataType.INTEGER); - return unary.replaceChild(new TopN(plan.source(), relation, order.order(), limit)); - } - return plan; - } -} From 1c260e17c1c0bb0773ffe4ca70ceab99052d4e90 Mon Sep 17 00:00:00 2001 From: Luigi Dell'Aquila Date: Thu, 6 Feb 2025 09:40:31 +0100 Subject: [PATCH 07/13] More tests --- .../optimizer/LogicalPlanOptimizerTests.java | 43 +++++++++++++++++++ 1 file changed, 43 insertions(+) diff --git a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/optimizer/LogicalPlanOptimizerTests.java b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/optimizer/LogicalPlanOptimizerTests.java index f857ac70d7125..d9c9146232558 100644 --- a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/optimizer/LogicalPlanOptimizerTests.java +++ b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/optimizer/LogicalPlanOptimizerTests.java @@ -7453,4 +7453,47 @@ public void testEvalLookupMultipleSorts() { as(eval.child(), EsRelation.class); } + + public void testUnboundedSortSimple() { + var query = """ + ROW x = [1,2,3], y = 1 + | SORT y + | MV_EXPAND x + | WHERE x > 2 + """; + + VerificationException e = expectThrows(VerificationException.class, () -> plan(query)); + assertThat(e.getMessage(), containsString("line 2:5: Unbounded sort not supported yet, please add a limit")); + } + + public void testUnboundedSortWithMvExpandAndFilter() { + var query = """ + FROM test + | EVAL language_code = 1 + | LOOKUP JOIN languages_lookup ON language_code + | SORT language_name + | EVAL foo = concat(language_name, "foo") + | MV_EXPAND foo + | WHERE foo == "foo" + """; + + VerificationException e = expectThrows(VerificationException.class, () -> plan(query)); + assertThat(e.getMessage(), containsString("line 4:3: Unbounded sort not supported yet, please add a limit")); + } + + public void testUnboundedSortWithLookupJoinAndFilter() { + var query = """ + FROM test + | EVAL language_code = 1 + | EVAL foo = concat(language_code::string, "foo") + | MV_EXPAND foo + | SORT foo + | LOOKUP JOIN languages_lookup ON language_code + | WHERE language_name == "foo" + """; + + VerificationException e = expectThrows(VerificationException.class, () -> plan(query)); + assertThat(e.getMessage(), containsString("line 5:3: Unbounded sort not supported yet, please add a limit")); + } + } From a5d30efb17fa66b6bcbe447c1890263b40a2df7b Mon Sep 17 00:00:00 2001 From: Luigi Dell'Aquila Date: Thu, 6 Feb 2025 09:58:12 +0100 Subject: [PATCH 08/13] Better error message --- .../xpack/esql/plan/logical/OrderBy.java | 2 +- .../optimizer/LogicalPlanOptimizerTests.java | 18 +++++++++++++++--- 2 files changed, 16 insertions(+), 4 deletions(-) diff --git a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/logical/OrderBy.java b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/logical/OrderBy.java index 1660dbc63ffd9..ef36088d9f5ce 100644 --- a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/logical/OrderBy.java +++ b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/logical/OrderBy.java @@ -113,6 +113,6 @@ public void postAnalysisVerification(Failures failures) { @Override public void postOptimizationVerification(Failures failures) { - failures.add(fail(this, "Unbounded sort not supported yet, please add a limit")); + failures.add(fail(this, "Unbounded sort not supported yet [{}] please add a limit", this.sourceText())); } } diff --git a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/optimizer/LogicalPlanOptimizerTests.java b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/optimizer/LogicalPlanOptimizerTests.java index d9c9146232558..dedc6595266e7 100644 --- a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/optimizer/LogicalPlanOptimizerTests.java +++ b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/optimizer/LogicalPlanOptimizerTests.java @@ -7463,7 +7463,7 @@ public void testUnboundedSortSimple() { """; VerificationException e = expectThrows(VerificationException.class, () -> plan(query)); - assertThat(e.getMessage(), containsString("line 2:5: Unbounded sort not supported yet, please add a limit")); + assertThat(e.getMessage(), containsString("line 2:5: Unbounded sort not supported yet [SORT y] please add a limit")); } public void testUnboundedSortWithMvExpandAndFilter() { @@ -7478,7 +7478,7 @@ public void testUnboundedSortWithMvExpandAndFilter() { """; VerificationException e = expectThrows(VerificationException.class, () -> plan(query)); - assertThat(e.getMessage(), containsString("line 4:3: Unbounded sort not supported yet, please add a limit")); + assertThat(e.getMessage(), containsString("line 4:3: Unbounded sort not supported yet [SORT language_name] please add a limit")); } public void testUnboundedSortWithLookupJoinAndFilter() { @@ -7493,7 +7493,19 @@ public void testUnboundedSortWithLookupJoinAndFilter() { """; VerificationException e = expectThrows(VerificationException.class, () -> plan(query)); - assertThat(e.getMessage(), containsString("line 5:3: Unbounded sort not supported yet, please add a limit")); + assertThat(e.getMessage(), containsString("line 5:3: Unbounded sort not supported yet [SORT foo] please add a limit")); + } + + public void testUnboundedSortExpandFilter() { + var query = """ + ROW x = [1,2,3], y = 1 + | SORT x + | MV_EXPAND x + | WHERE x > 2 + """; + + VerificationException e = expectThrows(VerificationException.class, () -> plan(query)); + assertThat(e.getMessage(), containsString("line 2:5: Unbounded sort not supported yet [SORT x] please add a limit")); } } From f881d2d048f77212676fe08575dded96b4f4fc5e Mon Sep 17 00:00:00 2001 From: Luigi Dell'Aquila Date: Thu, 6 Feb 2025 10:07:48 +0100 Subject: [PATCH 09/13] Delete OrderExec --- .../rules/logical/PruneRedundantOrderBy.java | 3 +- .../xpack/esql/plan/PlanWritables.java | 2 - .../xpack/esql/plan/physical/OrderExec.java | 88 ------------------- .../esql/planner/mapper/LocalMapper.java | 6 -- .../xpack/esql/planner/mapper/Mapper.java | 8 +- .../physical/OrderExecSerializationTests.java | 46 ---------- 6 files changed, 3 insertions(+), 150 deletions(-) delete mode 100644 x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/physical/OrderExec.java delete mode 100644 x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/plan/physical/OrderExecSerializationTests.java diff --git a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/rules/logical/PruneRedundantOrderBy.java b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/rules/logical/PruneRedundantOrderBy.java index b560e01ed2424..e2387a59ebdef 100644 --- a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/rules/logical/PruneRedundantOrderBy.java +++ b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/rules/logical/PruneRedundantOrderBy.java @@ -38,7 +38,8 @@ *

* from test | sort x | mv_expand x | sort y *

- * "sort y" will become a TopN, but "sort x" will remain unbounded, so the query could not be executed. + * "sort y" will become a TopN due to the addition of the default Limit, but "sort x" will remain unbounded, + * so the query could not be executed. *

* In most cases though, following commands can make the previous SORTs redundant, * because it will re-sort previously sorted results (eg. if there is another SORT) diff --git a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/PlanWritables.java b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/PlanWritables.java index b3c273cbfa1bb..a345f69af0247 100644 --- a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/PlanWritables.java +++ b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/PlanWritables.java @@ -43,7 +43,6 @@ import org.elasticsearch.xpack.esql.plan.physical.LimitExec; import org.elasticsearch.xpack.esql.plan.physical.LocalSourceExec; import org.elasticsearch.xpack.esql.plan.physical.MvExpandExec; -import org.elasticsearch.xpack.esql.plan.physical.OrderExec; import org.elasticsearch.xpack.esql.plan.physical.ProjectExec; import org.elasticsearch.xpack.esql.plan.physical.ShowExec; import org.elasticsearch.xpack.esql.plan.physical.SubqueryExec; @@ -103,7 +102,6 @@ public static List phsyical() { LimitExec.ENTRY, LocalSourceExec.ENTRY, MvExpandExec.ENTRY, - OrderExec.ENTRY, ProjectExec.ENTRY, ShowExec.ENTRY, SubqueryExec.ENTRY, diff --git a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/physical/OrderExec.java b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/physical/OrderExec.java deleted file mode 100644 index 9d53e828f4f81..0000000000000 --- a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/physical/OrderExec.java +++ /dev/null @@ -1,88 +0,0 @@ -/* - * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one - * or more contributor license agreements. Licensed under the Elastic License - * 2.0; you may not use this file except in compliance with the Elastic License - * 2.0. - */ - -package org.elasticsearch.xpack.esql.plan.physical; - -import org.elasticsearch.common.io.stream.NamedWriteableRegistry; -import org.elasticsearch.common.io.stream.StreamInput; -import org.elasticsearch.common.io.stream.StreamOutput; -import org.elasticsearch.xpack.esql.core.tree.NodeInfo; -import org.elasticsearch.xpack.esql.core.tree.Source; -import org.elasticsearch.xpack.esql.expression.Order; -import org.elasticsearch.xpack.esql.io.stream.PlanStreamInput; - -import java.io.IOException; -import java.util.List; -import java.util.Objects; - -public class OrderExec extends UnaryExec { - public static final NamedWriteableRegistry.Entry ENTRY = new NamedWriteableRegistry.Entry( - PhysicalPlan.class, - "OrderExec", - OrderExec::new - ); - - private final List order; - - public OrderExec(Source source, PhysicalPlan child, List order) { - super(source, child); - this.order = order; - } - - private OrderExec(StreamInput in) throws IOException { - this( - Source.readFrom((PlanStreamInput) in), - in.readNamedWriteable(PhysicalPlan.class), - in.readCollectionAsList(org.elasticsearch.xpack.esql.expression.Order::new) - ); - } - - @Override - public void writeTo(StreamOutput out) throws IOException { - Source.EMPTY.writeTo(out); - out.writeNamedWriteable(child()); - out.writeCollection(order()); - } - - @Override - public String getWriteableName() { - return ENTRY.name; - } - - @Override - protected NodeInfo info() { - return NodeInfo.create(this, OrderExec::new, child(), order); - } - - @Override - public OrderExec replaceChild(PhysicalPlan newChild) { - return new OrderExec(source(), newChild, order); - } - - public List order() { - return order; - } - - @Override - public int hashCode() { - return Objects.hash(order, child()); - } - - @Override - public boolean equals(Object obj) { - if (this == obj) { - return true; - } - if (obj == null || getClass() != obj.getClass()) { - return false; - } - - OrderExec other = (OrderExec) obj; - - return Objects.equals(order, other.order) && Objects.equals(child(), other.child()); - } -} diff --git a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/planner/mapper/LocalMapper.java b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/planner/mapper/LocalMapper.java index f95ae0e0783e5..217737de5309b 100644 --- a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/planner/mapper/LocalMapper.java +++ b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/planner/mapper/LocalMapper.java @@ -17,7 +17,6 @@ import org.elasticsearch.xpack.esql.plan.logical.LeafPlan; import org.elasticsearch.xpack.esql.plan.logical.Limit; import org.elasticsearch.xpack.esql.plan.logical.LogicalPlan; -import org.elasticsearch.xpack.esql.plan.logical.OrderBy; import org.elasticsearch.xpack.esql.plan.logical.TopN; import org.elasticsearch.xpack.esql.plan.logical.UnaryPlan; import org.elasticsearch.xpack.esql.plan.logical.join.Join; @@ -28,7 +27,6 @@ import org.elasticsearch.xpack.esql.plan.physical.LimitExec; import org.elasticsearch.xpack.esql.plan.physical.LocalSourceExec; import org.elasticsearch.xpack.esql.plan.physical.LookupJoinExec; -import org.elasticsearch.xpack.esql.plan.physical.OrderExec; import org.elasticsearch.xpack.esql.plan.physical.PhysicalPlan; import org.elasticsearch.xpack.esql.plan.physical.TopNExec; @@ -81,10 +79,6 @@ private PhysicalPlan mapUnary(UnaryPlan unary) { return new LimitExec(limit.source(), mappedChild, limit.limit()); } - if (unary instanceof OrderBy o) { - return new OrderExec(o.source(), mappedChild, o.order()); - } - if (unary instanceof TopN topN) { return new TopNExec(topN.source(), mappedChild, topN.order(), topN.limit(), null); } diff --git a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/planner/mapper/Mapper.java b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/planner/mapper/Mapper.java index 8a4325ed84b2a..8ea19f545e67b 100644 --- a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/planner/mapper/Mapper.java +++ b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/planner/mapper/Mapper.java @@ -32,7 +32,6 @@ import org.elasticsearch.xpack.esql.plan.physical.LimitExec; import org.elasticsearch.xpack.esql.plan.physical.LocalSourceExec; import org.elasticsearch.xpack.esql.plan.physical.LookupJoinExec; -import org.elasticsearch.xpack.esql.plan.physical.OrderExec; import org.elasticsearch.xpack.esql.plan.physical.PhysicalPlan; import org.elasticsearch.xpack.esql.plan.physical.TopNExec; import org.elasticsearch.xpack.esql.plan.physical.UnaryExec; @@ -105,7 +104,7 @@ private PhysicalPlan mapUnary(UnaryPlan unary) { return enrichExec.child(); } if (f instanceof UnaryExec unaryExec) { - if (f instanceof LimitExec || f instanceof ExchangeExec || f instanceof OrderExec || f instanceof TopNExec) { + if (f instanceof LimitExec || f instanceof ExchangeExec || f instanceof TopNExec) { return f; } else { return unaryExec.child(); @@ -161,11 +160,6 @@ private PhysicalPlan mapUnary(UnaryPlan unary) { return new LimitExec(limit.source(), mappedChild, limit.limit()); } - if (unary instanceof OrderBy o) { - mappedChild = addExchangeForFragment(o, mappedChild); - return new OrderExec(o.source(), mappedChild, o.order()); - } - if (unary instanceof TopN topN) { mappedChild = addExchangeForFragment(topN, mappedChild); return new TopNExec(topN.source(), mappedChild, topN.order(), topN.limit(), null); diff --git a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/plan/physical/OrderExecSerializationTests.java b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/plan/physical/OrderExecSerializationTests.java deleted file mode 100644 index 755f1cd4f52da..0000000000000 --- a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/plan/physical/OrderExecSerializationTests.java +++ /dev/null @@ -1,46 +0,0 @@ -/* - * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one - * or more contributor license agreements. Licensed under the Elastic License - * 2.0; you may not use this file except in compliance with the Elastic License - * 2.0. - */ - -package org.elasticsearch.xpack.esql.plan.physical; - -import org.elasticsearch.xpack.esql.core.tree.Source; -import org.elasticsearch.xpack.esql.expression.Order; -import org.elasticsearch.xpack.esql.expression.OrderSerializationTests; - -import java.io.IOException; -import java.util.List; - -public class OrderExecSerializationTests extends AbstractPhysicalPlanSerializationTests { - public static OrderExec randomOrderExec(int depth) { - Source source = randomSource(); - PhysicalPlan child = randomChild(depth); - List order = randomList(1, 10, OrderSerializationTests::randomOrder); - return new OrderExec(source, child, order); - } - - @Override - protected OrderExec createTestInstance() { - return randomOrderExec(0); - } - - @Override - protected OrderExec mutateInstance(OrderExec instance) throws IOException { - PhysicalPlan child = instance.child(); - List order = instance.order(); - if (randomBoolean()) { - child = randomValueOtherThan(child, () -> randomChild(0)); - } else { - order = randomValueOtherThan(order, () -> randomList(1, 10, OrderSerializationTests::randomOrder)); - } - return new OrderExec(instance.source(), child, order); - } - - @Override - protected boolean alwaysEmptySource() { - return true; - } -} From 319cc69412df2d41588c1ec9af41d3f3793905a8 Mon Sep 17 00:00:00 2001 From: Luigi Dell'Aquila Date: Thu, 6 Feb 2025 11:19:49 +0100 Subject: [PATCH 10/13] Add SortAware interface --- .../rules/logical/PruneRedundantOrderBy.java | 73 +++++-------------- .../xpack/esql/plan/logical/Aggregate.java | 8 +- .../xpack/esql/plan/logical/Drop.java | 2 +- .../xpack/esql/plan/logical/Enrich.java | 2 +- .../xpack/esql/plan/logical/Eval.java | 8 +- .../xpack/esql/plan/logical/Filter.java | 8 +- .../xpack/esql/plan/logical/InlineStats.java | 8 +- .../xpack/esql/plan/logical/Keep.java | 2 +- .../xpack/esql/plan/logical/Lookup.java | 2 +- .../xpack/esql/plan/logical/MvExpand.java | 2 +- .../xpack/esql/plan/logical/OrderBy.java | 7 +- .../xpack/esql/plan/logical/Project.java | 2 +- .../xpack/esql/plan/logical/RegexExtract.java | 2 +- .../xpack/esql/plan/logical/Rename.java | 2 +- .../xpack/esql/plan/logical/SortAware.java | 38 ++++++++++ .../xpack/esql/plan/logical/join/Join.java | 3 +- .../optimizer/LogicalPlanOptimizerTests.java | 22 ++++++ 17 files changed, 124 insertions(+), 67 deletions(-) create mode 100644 x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/logical/SortAware.java diff --git a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/rules/logical/PruneRedundantOrderBy.java b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/rules/logical/PruneRedundantOrderBy.java index e2387a59ebdef..7ea633b55388b 100644 --- a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/rules/logical/PruneRedundantOrderBy.java +++ b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/rules/logical/PruneRedundantOrderBy.java @@ -8,25 +8,15 @@ package org.elasticsearch.xpack.esql.optimizer.rules.logical; import org.elasticsearch.xpack.esql.plan.logical.Aggregate; -import org.elasticsearch.xpack.esql.plan.logical.Drop; -import org.elasticsearch.xpack.esql.plan.logical.Enrich; -import org.elasticsearch.xpack.esql.plan.logical.Eval; -import org.elasticsearch.xpack.esql.plan.logical.Filter; -import org.elasticsearch.xpack.esql.plan.logical.InlineStats; import org.elasticsearch.xpack.esql.plan.logical.LogicalPlan; -import org.elasticsearch.xpack.esql.plan.logical.Lookup; -import org.elasticsearch.xpack.esql.plan.logical.MvExpand; import org.elasticsearch.xpack.esql.plan.logical.OrderBy; -import org.elasticsearch.xpack.esql.plan.logical.Project; -import org.elasticsearch.xpack.esql.plan.logical.RegexExtract; -import org.elasticsearch.xpack.esql.plan.logical.Rename; +import org.elasticsearch.xpack.esql.plan.logical.SortAware; import org.elasticsearch.xpack.esql.plan.logical.TopN; import org.elasticsearch.xpack.esql.plan.logical.UnaryPlan; -import org.elasticsearch.xpack.esql.plan.logical.join.Join; -import java.util.ArrayList; +import java.util.ArrayDeque; +import java.util.Deque; import java.util.IdentityHashMap; -import java.util.List; /** * SORT cannot be executed without a LIMIT, as ES|QL doesn't support unbounded sort (yet). @@ -56,7 +46,7 @@ protected LogicalPlan rule(LogicalPlan plan) { if (redundant.isEmpty()) { return plan; } - return plan.transformUp(p -> { + return plan.transformDown(p -> { if (redundant.containsKey(p)) { return ((OrderBy) p).child(); } @@ -67,50 +57,27 @@ protected LogicalPlan rule(LogicalPlan plan) { } } + /** + * breadth-first recursion to find redundant SORTs in the children tree. + */ private IdentityHashMap findRedundantSort(LogicalPlan plan) { - List toCheck = new ArrayList<>(); - toCheck.add(plan); - IdentityHashMap result = new IdentityHashMap<>(); - LogicalPlan p = null; + + Deque toCheck = new ArrayDeque<>(); + toCheck.push(plan); + while (true) { - if (p == null) { - if (toCheck.isEmpty()) { - return result; - } else { - p = toCheck.remove(0); - } - } else if (p instanceof OrderBy ob) { + if (toCheck.isEmpty()) { + return result; + } + LogicalPlan p = toCheck.pop(); + if (p instanceof OrderBy ob) { result.put(ob, null); - p = ob.child(); - } else if (p instanceof UnaryPlan unary) { - if (unary instanceof Project - || unary instanceof Drop - || unary instanceof Rename - || unary instanceof MvExpand - || unary instanceof Enrich - || unary instanceof RegexExtract - || unary instanceof InlineStats - || unary instanceof Lookup - // 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) { - p = unary.child(); - } else { - // stop here, other unary plans could be sensitive to SORT - p = null; + toCheck.push(ob.child()); + } else if (p instanceof SortAware sa && sa.dependsOnInputOrder() == false) { + for (LogicalPlan child : p.children()) { + toCheck.push(child); } - } else if (p instanceof Join lj) { - toCheck.add(lj.left()); - toCheck.add(lj.right()); - p = null; - } else { - // stop here, other unary plans could be sensitive to SORT - p = null; } } } diff --git a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/logical/Aggregate.java b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/logical/Aggregate.java index 5c40bfce32064..df174859067a4 100644 --- a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/logical/Aggregate.java +++ b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/logical/Aggregate.java @@ -40,7 +40,7 @@ import static org.elasticsearch.xpack.esql.expression.NamedExpressions.mergeOutputAttributes; import static org.elasticsearch.xpack.esql.plan.logical.Filter.checkFilterConditionDataType; -public class Aggregate extends UnaryPlan implements PostAnalysisVerificationAware, TelemetryAware { +public class Aggregate extends UnaryPlan implements PostAnalysisVerificationAware, TelemetryAware, SortAware { public static final NamedWriteableRegistry.Entry ENTRY = new NamedWriteableRegistry.Entry( LogicalPlan.class, "Aggregate", @@ -459,4 +459,10 @@ private static void addFailureOnGroupingUsedNakedInAggs(Failures failures, Expre fail(e, "grouping {} [{}] cannot be used as an aggregate once declared in the STATS BY clause", element, e.sourceText()) ); } + + @Override + public boolean dependsOnInputOrder() { + // TODO review this if we introduce expressions that depend on input order (eg. window functions) + return SortAware.super.dependsOnInputOrder(); + } } diff --git a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/logical/Drop.java b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/logical/Drop.java index 483c3508013ab..2410790c506cd 100644 --- a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/logical/Drop.java +++ b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/logical/Drop.java @@ -17,7 +17,7 @@ import java.util.List; import java.util.Objects; -public class Drop extends UnaryPlan implements TelemetryAware { +public class Drop extends UnaryPlan implements TelemetryAware, SortAware { private final List removals; public Drop(Source source, LogicalPlan child, List removals) { diff --git a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/logical/Enrich.java b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/logical/Enrich.java index 4e9fc87318029..865aae23b96f1 100644 --- a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/logical/Enrich.java +++ b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/logical/Enrich.java @@ -49,7 +49,7 @@ import static org.elasticsearch.xpack.esql.core.expression.Expressions.asAttributes; import static org.elasticsearch.xpack.esql.expression.NamedExpressions.mergeOutputAttributes; -public class Enrich extends UnaryPlan implements GeneratingPlan, PostAnalysisPlanVerificationAware, TelemetryAware { +public class Enrich extends UnaryPlan implements GeneratingPlan, PostAnalysisPlanVerificationAware, TelemetryAware, SortAware { public static final NamedWriteableRegistry.Entry ENTRY = new NamedWriteableRegistry.Entry( LogicalPlan.class, "Enrich", diff --git a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/logical/Eval.java b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/logical/Eval.java index 7c437dac03409..a1797b90e9949 100644 --- a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/logical/Eval.java +++ b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/logical/Eval.java @@ -38,7 +38,7 @@ import static org.elasticsearch.xpack.esql.core.expression.Expressions.asAttributes; import static org.elasticsearch.xpack.esql.expression.NamedExpressions.mergeOutputAttributes; -public class Eval extends UnaryPlan implements GeneratingPlan, PostAnalysisVerificationAware, TelemetryAware { +public class Eval extends UnaryPlan implements GeneratingPlan, PostAnalysisVerificationAware, TelemetryAware, SortAware { public static final NamedWriteableRegistry.Entry ENTRY = new NamedWriteableRegistry.Entry(LogicalPlan.class, "Eval", Eval::new); private final List fields; @@ -189,4 +189,10 @@ public void postAnalysisVerification(Failures failures) { }); }); } + + @Override + public boolean dependsOnInputOrder() { + // TODO review this if we introduce expressions that depend on input order (eg. window functions) + return SortAware.super.dependsOnInputOrder(); + } } diff --git a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/logical/Filter.java b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/logical/Filter.java index 6931c320007fe..7c9af98259c8c 100644 --- a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/logical/Filter.java +++ b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/logical/Filter.java @@ -29,7 +29,7 @@ * {@code SELECT x FROM y WHERE z ..} the "WHERE" clause is a Filter. A * {@code Filter} has a "condition" Expression that does the filtering. */ -public class Filter extends UnaryPlan implements PostAnalysisVerificationAware, TelemetryAware { +public class Filter extends UnaryPlan implements PostAnalysisVerificationAware, TelemetryAware, SortAware { public static final NamedWriteableRegistry.Entry ENTRY = new NamedWriteableRegistry.Entry(LogicalPlan.class, "Filter", Filter::new); private final Expression condition; @@ -116,4 +116,10 @@ public static void checkFilterConditionDataType(Expression expression, Failures failures.add(fail(expression, "Condition expression needs to be boolean, found [{}]", expression.dataType())); } } + + @Override + public boolean dependsOnInputOrder() { + // TODO review this if we introduce expressions that depend on input order (eg. window functions) + return SortAware.super.dependsOnInputOrder(); + } } diff --git a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/logical/InlineStats.java b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/logical/InlineStats.java index 527ba28d377f1..b7952a56610af 100644 --- a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/logical/InlineStats.java +++ b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/logical/InlineStats.java @@ -37,7 +37,7 @@ * underlying aggregate. *

*/ -public class InlineStats extends UnaryPlan implements NamedWriteable, SurrogateLogicalPlan, TelemetryAware { +public class InlineStats extends UnaryPlan implements NamedWriteable, SurrogateLogicalPlan, TelemetryAware, SortAware { public static final NamedWriteableRegistry.Entry ENTRY = new NamedWriteableRegistry.Entry( LogicalPlan.class, "InlineStats", @@ -143,4 +143,10 @@ public boolean equals(Object obj) { InlineStats other = (InlineStats) obj; return Objects.equals(aggregate, other.aggregate); } + + @Override + public boolean dependsOnInputOrder() { + // TODO review this if we introduce expressions that depend on input order (eg. window functions) + return SortAware.super.dependsOnInputOrder(); + } } diff --git a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/logical/Keep.java b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/logical/Keep.java index 67108afb94668..5568417efa90e 100644 --- a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/logical/Keep.java +++ b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/logical/Keep.java @@ -15,7 +15,7 @@ import java.util.List; import java.util.Objects; -public class Keep extends Project implements TelemetryAware { +public class Keep extends Project implements TelemetryAware, SortAware { public Keep(Source source, LogicalPlan child, List projections) { super(source, child, projections); diff --git a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/logical/Lookup.java b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/logical/Lookup.java index 1c05ceb124529..84d3312feeebb 100644 --- a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/logical/Lookup.java +++ b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/logical/Lookup.java @@ -32,7 +32,7 @@ * Looks up values from the associated {@code tables}. * The class is supposed to be substituted by a {@link Join}. */ -public class Lookup extends UnaryPlan implements SurrogateLogicalPlan, TelemetryAware { +public class Lookup extends UnaryPlan implements SurrogateLogicalPlan, TelemetryAware, SortAware { public static final NamedWriteableRegistry.Entry ENTRY = new NamedWriteableRegistry.Entry(LogicalPlan.class, "Lookup", Lookup::new); private final Expression tableName; diff --git a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/logical/MvExpand.java b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/logical/MvExpand.java index e700ad90afdab..186556869c9ea 100644 --- a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/logical/MvExpand.java +++ b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/logical/MvExpand.java @@ -23,7 +23,7 @@ import java.util.List; import java.util.Objects; -public class MvExpand extends UnaryPlan implements TelemetryAware { +public class MvExpand extends UnaryPlan implements TelemetryAware, SortAware { public static final NamedWriteableRegistry.Entry ENTRY = new NamedWriteableRegistry.Entry(LogicalPlan.class, "MvExpand", MvExpand::new); private final NamedExpression target; diff --git a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/logical/OrderBy.java b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/logical/OrderBy.java index ef36088d9f5ce..9a9aaaea989bb 100644 --- a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/logical/OrderBy.java +++ b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/logical/OrderBy.java @@ -26,7 +26,12 @@ import static org.elasticsearch.xpack.esql.common.Failure.fail; -public class OrderBy extends UnaryPlan implements PostAnalysisVerificationAware, PostOptimizationVerificationAware, TelemetryAware { +public class OrderBy extends UnaryPlan + implements + PostAnalysisVerificationAware, + PostOptimizationVerificationAware, + TelemetryAware, + SortAware { public static final NamedWriteableRegistry.Entry ENTRY = new NamedWriteableRegistry.Entry(LogicalPlan.class, "OrderBy", OrderBy::new); private final List order; diff --git a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/logical/Project.java b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/logical/Project.java index e12a8cb557fde..0100b631c1051 100644 --- a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/logical/Project.java +++ b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/logical/Project.java @@ -25,7 +25,7 @@ /** * A {@code Project} is a {@code Plan} with one child. In {@code SELECT x FROM y}, the "SELECT" statement is a Project. */ -public class Project extends UnaryPlan { +public class Project extends UnaryPlan implements SortAware { public static final NamedWriteableRegistry.Entry ENTRY = new NamedWriteableRegistry.Entry(LogicalPlan.class, "Project", Project::new); private final List projections; diff --git a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/logical/RegexExtract.java b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/logical/RegexExtract.java index d691507b62cb3..d0e1908b6434a 100644 --- a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/logical/RegexExtract.java +++ b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/logical/RegexExtract.java @@ -24,7 +24,7 @@ import static org.elasticsearch.xpack.esql.common.Failure.fail; import static org.elasticsearch.xpack.esql.expression.NamedExpressions.mergeOutputAttributes; -public abstract class RegexExtract extends UnaryPlan implements GeneratingPlan, PostAnalysisVerificationAware { +public abstract class RegexExtract extends UnaryPlan implements GeneratingPlan, PostAnalysisVerificationAware, SortAware { protected final Expression input; protected final List extractedFields; diff --git a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/logical/Rename.java b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/logical/Rename.java index 7887d8ed66b99..48d8f85e91339 100644 --- a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/logical/Rename.java +++ b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/logical/Rename.java @@ -21,7 +21,7 @@ import java.util.List; import java.util.Objects; -public class Rename extends UnaryPlan implements TelemetryAware { +public class Rename extends UnaryPlan implements TelemetryAware, SortAware { private final List renamings; diff --git a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/logical/SortAware.java b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/logical/SortAware.java new file mode 100644 index 0000000000000..f48a7a530ce7f --- /dev/null +++ b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/logical/SortAware.java @@ -0,0 +1,38 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the Elastic License + * 2.0; you may not use this file except in compliance with the Elastic License + * 2.0. + */ + +package org.elasticsearch.xpack.esql.plan.logical; + +/** + * This interface is intended to check redundancy of a previous SORT. + * + * Eg. if a MY_COMMAND that implements this interface is used after a sort, and if dependsOnInputOrder() = false, + * then we can assume that + *

+ * + * | SORT x | MY_COMMAND + * + *

+ * is equivalent to + *

+ * + * | MY_COMMAND + * + *

+ * + * In all the other cases, eg. if the command does not implement this interface, or if dependsOnInputOrder() = true + * then we assume that the SORT is still relevant and cannot be pruned. + */ +public interface SortAware { + + /** + * Returns true if the logic of this command could depend on the order of the input. + */ + default boolean dependsOnInputOrder() { + return false; + } +} diff --git a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/logical/join/Join.java b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/logical/join/Join.java index 997bff70663bd..cf11ef08580c9 100644 --- a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/logical/join/Join.java +++ b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/logical/join/Join.java @@ -21,6 +21,7 @@ import org.elasticsearch.xpack.esql.io.stream.PlanStreamInput; import org.elasticsearch.xpack.esql.plan.logical.BinaryPlan; import org.elasticsearch.xpack.esql.plan.logical.LogicalPlan; +import org.elasticsearch.xpack.esql.plan.logical.SortAware; import java.io.IOException; import java.util.ArrayList; @@ -32,7 +33,7 @@ import static org.elasticsearch.xpack.esql.expression.NamedExpressions.mergeOutputAttributes; import static org.elasticsearch.xpack.esql.plan.logical.join.JoinTypes.LEFT; -public class Join extends BinaryPlan implements PostAnalysisVerificationAware { +public class Join extends BinaryPlan implements PostAnalysisVerificationAware, SortAware { public static final NamedWriteableRegistry.Entry ENTRY = new NamedWriteableRegistry.Entry(LogicalPlan.class, "Join", Join::new); private final JoinConfig config; diff --git a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/optimizer/LogicalPlanOptimizerTests.java b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/optimizer/LogicalPlanOptimizerTests.java index dedc6595266e7..049fb92aa5519 100644 --- a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/optimizer/LogicalPlanOptimizerTests.java +++ b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/optimizer/LogicalPlanOptimizerTests.java @@ -96,6 +96,7 @@ import org.elasticsearch.xpack.esql.index.IndexResolution; import org.elasticsearch.xpack.esql.optimizer.rules.logical.LiteralsOnTheRight; import org.elasticsearch.xpack.esql.optimizer.rules.logical.OptimizerRules; +import org.elasticsearch.xpack.esql.optimizer.rules.logical.PruneRedundantOrderBy; import org.elasticsearch.xpack.esql.optimizer.rules.logical.PushDownAndCombineLimits; import org.elasticsearch.xpack.esql.optimizer.rules.logical.PushDownEnrich; import org.elasticsearch.xpack.esql.optimizer.rules.logical.PushDownEval; @@ -7508,4 +7509,25 @@ public void testUnboundedSortExpandFilter() { assertThat(e.getMessage(), containsString("line 2:5: Unbounded sort not supported yet [SORT x] please add a limit")); } + public void testPruneRedundantOrderBy() { + var rule = new PruneRedundantOrderBy(); + + var query = """ + row x = [1,2,3], y = 1 + | sort x + | mv_expand x + | sort x + | mv_expand x + | sort y + """; + LogicalPlan analyzed = analyzer.analyze(parser.createStatement(query)); + LogicalPlan optimized = rule.apply(analyzed); + + // check that all the redundant SORTs are removed in a single run + var limit = as(optimized, Limit.class); + var orderBy = as(limit.child(), OrderBy.class); + var mvExpand = as(orderBy.child(), MvExpand.class); + var mvExpand2 = as(mvExpand.child(), MvExpand.class); + as(mvExpand2.child(), Row.class); + } } From 2d34e50d800c2463f98d3216b7a41670187aa31a Mon Sep 17 00:00:00 2001 From: Luigi Dell'Aquila Date: Fri, 7 Feb 2025 16:33:49 +0100 Subject: [PATCH 11/13] Better description for SortAware --- .../xpack/esql/plan/logical/SortAware.java | 74 ++++++++++++++++++- 1 file changed, 70 insertions(+), 4 deletions(-) diff --git a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/logical/SortAware.java b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/logical/SortAware.java index f48a7a530ce7f..fdf562debddb3 100644 --- a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/logical/SortAware.java +++ b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/logical/SortAware.java @@ -9,23 +9,89 @@ /** * This interface is intended to check redundancy of a previous SORT. + *

* - * Eg. if a MY_COMMAND that implements this interface is used after a sort, and if dependsOnInputOrder() = false, + * An example is with commands that compute values record by record, regardless of the input order + * and that don't rely on the context (intended as previous/next records). + * + *

+ *

+ * + * Example 1: if a MY_COMMAND that implements this interface is used between two sorts, and if dependsOnInputOrder() = false, * then we can assume that *

* - * | SORT x | MY_COMMAND + * | SORT x, y, z | MY_COMMAND | SORT a, b, c * *

* is equivalent to *

* - * | MY_COMMAND + * | MY_COMMAND | SORT a, b, c * + * + *

+ *

+ * + * Example 2: commands that make previous order irrelevant, eg. because they collapse the results; + * STATS is one of them, eg. + * + *

+ * + * | SORT x, y, z | STATS count(*) + * + *

+ * is equivalent to + *

+ * + * | STATS count(*) + * + *

+ * + * and if MY_COMMAND implements this interface and if dependsOnInputOrder() = false, then + * + *

+ * + * | SORT x, y, z | MY_COMMAND | STATS count(*) + * + *

+ * is equivalent to + *

+ * + * | MY_COMMAND | STATS count(*) + * + * + *

*

* * In all the other cases, eg. if the command does not implement this interface, or if dependsOnInputOrder() = true - * then we assume that the SORT is still relevant and cannot be pruned. + * then we assume that the previous SORT is still relevant and cannot be pruned. + * + *

+ *

+ * + * Eg. LIMIT does not implement this interface, because + * + *

+ * + * | SORT x, y, z | LIMIT 10 | SORT a, b, c + * + *

+ * is NOT equivalent to + *

+ * + * | LIMIT 10 | SORT a, b, c + * + * + *

+ *

+ * + * For n-ary plans that implement this interface, + * we assume that the above applies to all the children, ie. + *

    + *
  • if dependsOnInputOrder() = false, sorts can be pruned on all the children
    • + *
  • if dependsOnInputOrder() = true, sorts cannot be pruned on any of the children
    • + *
*/ public interface SortAware { From f2af41401c5da7712d6e7829a8740ff0612a16c5 Mon Sep 17 00:00:00 2001 From: Luigi Dell'Aquila Date: Fri, 7 Feb 2025 16:55:26 +0100 Subject: [PATCH 12/13] More tests --- .../optimizer/LogicalPlanOptimizerTests.java | 48 +++++++++++++++++++ 1 file changed, 48 insertions(+) diff --git a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/optimizer/LogicalPlanOptimizerTests.java b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/optimizer/LogicalPlanOptimizerTests.java index 049fb92aa5519..6fb1cb9ca14ef 100644 --- a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/optimizer/LogicalPlanOptimizerTests.java +++ b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/optimizer/LogicalPlanOptimizerTests.java @@ -7420,6 +7420,42 @@ public void testRedundantSortOnMvExpandJoinEnrichGrokDissect() { as(eval.child(), EsRelation.class); } + /** + * TopN[[Order[emp_no{f}#23,ASC,LAST]],1000[INTEGER]] + * \_Filter[emp_no{f}#23 > 1[INTEGER]] + * \_MvExpand[languages{f}#26,languages{r}#36] + * \_EsqlProject[[language_name{f}#35, foo{r}#5 AS bar, languages{f}#26, emp_no{f}#23]] + * \_Join[LEFT,[language_code{r}#8],[language_code{r}#8],[language_code{f}#34]] + * |_Project[[_meta_field{f}#29, emp_no{f}#23, first_name{f}#24, gender{f}#25, hire_date{f}#30, job{f}#31, job.raw{f}#32, l + * anguages{f}#26, last_name{f}#27, long_noidx{f}#33, salary{f}#28, foo{r}#5, languages{f}#26 AS language_code]] + * | \_Eval[[TOSTRING(languages{f}#26) AS foo]] + * | \_EsRelation[test][_meta_field{f}#29, emp_no{f}#23, first_name{f}#24, ..] + * \_EsRelation[languages_lookup][LOOKUP][language_code{f}#34, language_name{f}#35] + */ + public void testRedundantSortOnMvExpandJoinKeepDropRename() { + var plan = optimizedPlan(""" + FROM test + | SORT languages + | EVAL foo = to_string(languages), language_code = languages + | LOOKUP JOIN languages_lookup ON language_code + | KEEP language_name, language_code, foo, languages, emp_no + | DROP language_code + | RENAME foo AS bar + | MV_EXPAND languages + | WHERE emp_no > 1 + | SORT emp_no + """); + + var topN = as(plan, TopN.class); + var filter = as(topN.child(), Filter.class); + var mvExpand = as(filter.child(), MvExpand.class); + var project = as(mvExpand.child(), Project.class); + var join = as(project.child(), Join.class); + var project2 = as(join.left(), Project.class); + var eval = as(project2.child(), Eval.class); + as(eval.child(), EsRelation.class); + } + /** * TopN[[Order[emp_no{f}#15,ASC,LAST]],1000[INTEGER]] * \_Filter[emp_no{f}#15 > 1[INTEGER]] @@ -7467,6 +7503,18 @@ public void testUnboundedSortSimple() { assertThat(e.getMessage(), containsString("line 2:5: Unbounded sort not supported yet [SORT y] please add a limit")); } + public void testUnboundedSortJoin() { + var query = """ + ROW x = [1,2,3], y = 2, language_code = 1 + | SORT y + | LOOKUP JOIN languages_lookup ON language_code + | WHERE language_name == "foo" + """; + + VerificationException e = expectThrows(VerificationException.class, () -> plan(query)); + assertThat(e.getMessage(), containsString("line 2:5: Unbounded sort not supported yet [SORT y] please add a limit")); + } + public void testUnboundedSortWithMvExpandAndFilter() { var query = """ FROM test From 9b22923a553fe418d201518250d2408d59cd73f9 Mon Sep 17 00:00:00 2001 From: Luigi Dell'Aquila Date: Mon, 10 Feb 2025 12:44:14 +0100 Subject: [PATCH 13/13] SortAware -> SortAgnostic and simplify --- .../rules/logical/PruneRedundantOrderBy.java | 22 ++++++++---------- .../xpack/esql/plan/logical/Aggregate.java | 8 +------ .../xpack/esql/plan/logical/Drop.java | 2 +- .../xpack/esql/plan/logical/Enrich.java | 2 +- .../xpack/esql/plan/logical/Eval.java | 8 +------ .../xpack/esql/plan/logical/Filter.java | 8 +------ .../xpack/esql/plan/logical/InlineStats.java | 8 +------ .../xpack/esql/plan/logical/Keep.java | 2 +- .../xpack/esql/plan/logical/Lookup.java | 2 +- .../xpack/esql/plan/logical/MvExpand.java | 2 +- .../xpack/esql/plan/logical/OrderBy.java | 2 +- .../xpack/esql/plan/logical/Project.java | 2 +- .../xpack/esql/plan/logical/RegexExtract.java | 2 +- .../xpack/esql/plan/logical/Rename.java | 2 +- .../{SortAware.java => SortAgnostic.java} | 23 +++++-------------- .../xpack/esql/plan/logical/join/Join.java | 4 ++-- 16 files changed, 31 insertions(+), 68 deletions(-) rename x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/logical/{SortAware.java => SortAgnostic.java} (72%) diff --git a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/rules/logical/PruneRedundantOrderBy.java b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/rules/logical/PruneRedundantOrderBy.java index 7ea633b55388b..2495f72864d1c 100644 --- a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/rules/logical/PruneRedundantOrderBy.java +++ b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/rules/logical/PruneRedundantOrderBy.java @@ -10,13 +10,15 @@ import org.elasticsearch.xpack.esql.plan.logical.Aggregate; import org.elasticsearch.xpack.esql.plan.logical.LogicalPlan; import org.elasticsearch.xpack.esql.plan.logical.OrderBy; -import org.elasticsearch.xpack.esql.plan.logical.SortAware; +import org.elasticsearch.xpack.esql.plan.logical.SortAgnostic; import org.elasticsearch.xpack.esql.plan.logical.TopN; import org.elasticsearch.xpack.esql.plan.logical.UnaryPlan; import java.util.ArrayDeque; +import java.util.Collections; import java.util.Deque; import java.util.IdentityHashMap; +import java.util.Set; /** * SORT cannot be executed without a LIMIT, as ES|QL doesn't support unbounded sort (yet). @@ -42,16 +44,11 @@ public class PruneRedundantOrderBy extends OptimizerRules.OptimizerRule redundant = findRedundantSort(((UnaryPlan) plan).child()); + Set redundant = findRedundantSort(((UnaryPlan) plan).child()); if (redundant.isEmpty()) { return plan; } - return plan.transformDown(p -> { - if (redundant.containsKey(p)) { - return ((OrderBy) p).child(); - } - return p; - }); + return plan.transformDown(p -> redundant.contains(p) ? ((UnaryPlan) p).child() : p); } else { return plan; } @@ -59,9 +56,10 @@ protected LogicalPlan rule(LogicalPlan plan) { /** * breadth-first recursion to find redundant SORTs in the children tree. + * Returns an identity set (we need to compare and prune the exact instances) */ - private IdentityHashMap findRedundantSort(LogicalPlan plan) { - IdentityHashMap result = new IdentityHashMap<>(); + private Set findRedundantSort(LogicalPlan plan) { + Set result = Collections.newSetFromMap(new IdentityHashMap<>()); Deque toCheck = new ArrayDeque<>(); toCheck.push(plan); @@ -72,9 +70,9 @@ private IdentityHashMap findRedundantSort(LogicalPlan plan) { } LogicalPlan p = toCheck.pop(); if (p instanceof OrderBy ob) { - result.put(ob, null); + result.add(ob); toCheck.push(ob.child()); - } else if (p instanceof SortAware sa && sa.dependsOnInputOrder() == false) { + } else if (p instanceof SortAgnostic) { for (LogicalPlan child : p.children()) { toCheck.push(child); } diff --git a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/logical/Aggregate.java b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/logical/Aggregate.java index df174859067a4..8cff1d4c88e90 100644 --- a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/logical/Aggregate.java +++ b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/logical/Aggregate.java @@ -40,7 +40,7 @@ import static org.elasticsearch.xpack.esql.expression.NamedExpressions.mergeOutputAttributes; import static org.elasticsearch.xpack.esql.plan.logical.Filter.checkFilterConditionDataType; -public class Aggregate extends UnaryPlan implements PostAnalysisVerificationAware, TelemetryAware, SortAware { +public class Aggregate extends UnaryPlan implements PostAnalysisVerificationAware, TelemetryAware, SortAgnostic { public static final NamedWriteableRegistry.Entry ENTRY = new NamedWriteableRegistry.Entry( LogicalPlan.class, "Aggregate", @@ -459,10 +459,4 @@ private static void addFailureOnGroupingUsedNakedInAggs(Failures failures, Expre fail(e, "grouping {} [{}] cannot be used as an aggregate once declared in the STATS BY clause", element, e.sourceText()) ); } - - @Override - public boolean dependsOnInputOrder() { - // TODO review this if we introduce expressions that depend on input order (eg. window functions) - return SortAware.super.dependsOnInputOrder(); - } } diff --git a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/logical/Drop.java b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/logical/Drop.java index 2410790c506cd..c8668f58ab5c0 100644 --- a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/logical/Drop.java +++ b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/logical/Drop.java @@ -17,7 +17,7 @@ import java.util.List; import java.util.Objects; -public class Drop extends UnaryPlan implements TelemetryAware, SortAware { +public class Drop extends UnaryPlan implements TelemetryAware, SortAgnostic { private final List removals; public Drop(Source source, LogicalPlan child, List removals) { diff --git a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/logical/Enrich.java b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/logical/Enrich.java index 865aae23b96f1..11e9a57064e5b 100644 --- a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/logical/Enrich.java +++ b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/logical/Enrich.java @@ -49,7 +49,7 @@ import static org.elasticsearch.xpack.esql.core.expression.Expressions.asAttributes; import static org.elasticsearch.xpack.esql.expression.NamedExpressions.mergeOutputAttributes; -public class Enrich extends UnaryPlan implements GeneratingPlan, PostAnalysisPlanVerificationAware, TelemetryAware, SortAware { +public class Enrich extends UnaryPlan implements GeneratingPlan, PostAnalysisPlanVerificationAware, TelemetryAware, SortAgnostic { public static final NamedWriteableRegistry.Entry ENTRY = new NamedWriteableRegistry.Entry( LogicalPlan.class, "Enrich", diff --git a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/logical/Eval.java b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/logical/Eval.java index a1797b90e9949..af81e26d57c60 100644 --- a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/logical/Eval.java +++ b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/logical/Eval.java @@ -38,7 +38,7 @@ import static org.elasticsearch.xpack.esql.core.expression.Expressions.asAttributes; import static org.elasticsearch.xpack.esql.expression.NamedExpressions.mergeOutputAttributes; -public class Eval extends UnaryPlan implements GeneratingPlan, PostAnalysisVerificationAware, TelemetryAware, SortAware { +public class Eval extends UnaryPlan implements GeneratingPlan, PostAnalysisVerificationAware, TelemetryAware, SortAgnostic { public static final NamedWriteableRegistry.Entry ENTRY = new NamedWriteableRegistry.Entry(LogicalPlan.class, "Eval", Eval::new); private final List fields; @@ -189,10 +189,4 @@ public void postAnalysisVerification(Failures failures) { }); }); } - - @Override - public boolean dependsOnInputOrder() { - // TODO review this if we introduce expressions that depend on input order (eg. window functions) - return SortAware.super.dependsOnInputOrder(); - } } diff --git a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/logical/Filter.java b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/logical/Filter.java index 7c9af98259c8c..7a1726ea59e97 100644 --- a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/logical/Filter.java +++ b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/logical/Filter.java @@ -29,7 +29,7 @@ * {@code SELECT x FROM y WHERE z ..} the "WHERE" clause is a Filter. A * {@code Filter} has a "condition" Expression that does the filtering. */ -public class Filter extends UnaryPlan implements PostAnalysisVerificationAware, TelemetryAware, SortAware { +public class Filter extends UnaryPlan implements PostAnalysisVerificationAware, TelemetryAware, SortAgnostic { public static final NamedWriteableRegistry.Entry ENTRY = new NamedWriteableRegistry.Entry(LogicalPlan.class, "Filter", Filter::new); private final Expression condition; @@ -116,10 +116,4 @@ public static void checkFilterConditionDataType(Expression expression, Failures failures.add(fail(expression, "Condition expression needs to be boolean, found [{}]", expression.dataType())); } } - - @Override - public boolean dependsOnInputOrder() { - // TODO review this if we introduce expressions that depend on input order (eg. window functions) - return SortAware.super.dependsOnInputOrder(); - } } diff --git a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/logical/InlineStats.java b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/logical/InlineStats.java index b7952a56610af..724aa2da25983 100644 --- a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/logical/InlineStats.java +++ b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/logical/InlineStats.java @@ -37,7 +37,7 @@ * underlying aggregate. *

*/ -public class InlineStats extends UnaryPlan implements NamedWriteable, SurrogateLogicalPlan, TelemetryAware, SortAware { +public class InlineStats extends UnaryPlan implements NamedWriteable, SurrogateLogicalPlan, TelemetryAware, SortAgnostic { public static final NamedWriteableRegistry.Entry ENTRY = new NamedWriteableRegistry.Entry( LogicalPlan.class, "InlineStats", @@ -143,10 +143,4 @@ public boolean equals(Object obj) { InlineStats other = (InlineStats) obj; return Objects.equals(aggregate, other.aggregate); } - - @Override - public boolean dependsOnInputOrder() { - // TODO review this if we introduce expressions that depend on input order (eg. window functions) - return SortAware.super.dependsOnInputOrder(); - } } diff --git a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/logical/Keep.java b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/logical/Keep.java index 5568417efa90e..268c6bbe17242 100644 --- a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/logical/Keep.java +++ b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/logical/Keep.java @@ -15,7 +15,7 @@ import java.util.List; import java.util.Objects; -public class Keep extends Project implements TelemetryAware, SortAware { +public class Keep extends Project implements TelemetryAware, SortAgnostic { public Keep(Source source, LogicalPlan child, List projections) { super(source, child, projections); diff --git a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/logical/Lookup.java b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/logical/Lookup.java index 84d3312feeebb..56dae7b1f16c0 100644 --- a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/logical/Lookup.java +++ b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/logical/Lookup.java @@ -32,7 +32,7 @@ * Looks up values from the associated {@code tables}. * The class is supposed to be substituted by a {@link Join}. */ -public class Lookup extends UnaryPlan implements SurrogateLogicalPlan, TelemetryAware, SortAware { +public class Lookup extends UnaryPlan implements SurrogateLogicalPlan, TelemetryAware, SortAgnostic { public static final NamedWriteableRegistry.Entry ENTRY = new NamedWriteableRegistry.Entry(LogicalPlan.class, "Lookup", Lookup::new); private final Expression tableName; diff --git a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/logical/MvExpand.java b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/logical/MvExpand.java index 186556869c9ea..f65811fc26526 100644 --- a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/logical/MvExpand.java +++ b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/logical/MvExpand.java @@ -23,7 +23,7 @@ import java.util.List; import java.util.Objects; -public class MvExpand extends UnaryPlan implements TelemetryAware, SortAware { +public class MvExpand extends UnaryPlan implements TelemetryAware, SortAgnostic { public static final NamedWriteableRegistry.Entry ENTRY = new NamedWriteableRegistry.Entry(LogicalPlan.class, "MvExpand", MvExpand::new); private final NamedExpression target; diff --git a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/logical/OrderBy.java b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/logical/OrderBy.java index 9a9aaaea989bb..ddb07e0490db3 100644 --- a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/logical/OrderBy.java +++ b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/logical/OrderBy.java @@ -31,7 +31,7 @@ public class OrderBy extends UnaryPlan PostAnalysisVerificationAware, PostOptimizationVerificationAware, TelemetryAware, - SortAware { + SortAgnostic { public static final NamedWriteableRegistry.Entry ENTRY = new NamedWriteableRegistry.Entry(LogicalPlan.class, "OrderBy", OrderBy::new); private final List order; diff --git a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/logical/Project.java b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/logical/Project.java index 0100b631c1051..a36341f60525a 100644 --- a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/logical/Project.java +++ b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/logical/Project.java @@ -25,7 +25,7 @@ /** * A {@code Project} is a {@code Plan} with one child. In {@code SELECT x FROM y}, the "SELECT" statement is a Project. */ -public class Project extends UnaryPlan implements SortAware { +public class Project extends UnaryPlan implements SortAgnostic { public static final NamedWriteableRegistry.Entry ENTRY = new NamedWriteableRegistry.Entry(LogicalPlan.class, "Project", Project::new); private final List projections; diff --git a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/logical/RegexExtract.java b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/logical/RegexExtract.java index d0e1908b6434a..f111b5d03edb3 100644 --- a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/logical/RegexExtract.java +++ b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/logical/RegexExtract.java @@ -24,7 +24,7 @@ import static org.elasticsearch.xpack.esql.common.Failure.fail; import static org.elasticsearch.xpack.esql.expression.NamedExpressions.mergeOutputAttributes; -public abstract class RegexExtract extends UnaryPlan implements GeneratingPlan, PostAnalysisVerificationAware, SortAware { +public abstract class RegexExtract extends UnaryPlan implements GeneratingPlan, PostAnalysisVerificationAware, SortAgnostic { protected final Expression input; protected final List extractedFields; diff --git a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/logical/Rename.java b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/logical/Rename.java index 48d8f85e91339..c609bfdae87e7 100644 --- a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/logical/Rename.java +++ b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/logical/Rename.java @@ -21,7 +21,7 @@ import java.util.List; import java.util.Objects; -public class Rename extends UnaryPlan implements TelemetryAware, SortAware { +public class Rename extends UnaryPlan implements TelemetryAware, SortAgnostic { private final List renamings; diff --git a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/logical/SortAware.java b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/logical/SortAgnostic.java similarity index 72% rename from x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/logical/SortAware.java rename to x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/logical/SortAgnostic.java index fdf562debddb3..3955b542ca496 100644 --- a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/logical/SortAware.java +++ b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/logical/SortAgnostic.java @@ -17,7 +17,7 @@ *
*

* - * Example 1: if a MY_COMMAND that implements this interface is used between two sorts, and if dependsOnInputOrder() = false, + * Example 1: if a MY_COMMAND that implements this interface is used between two sorts, * then we can assume that *

* @@ -48,7 +48,7 @@ * *

* - * and if MY_COMMAND implements this interface and if dependsOnInputOrder() = false, then + * and if MY_COMMAND implements this interface, then * *

* @@ -64,7 +64,7 @@ *

*

* - * In all the other cases, eg. if the command does not implement this interface, or if dependsOnInputOrder() = true + * In all the other cases, eg. if the command does not implement this interface * then we assume that the previous SORT is still relevant and cannot be pruned. * *

@@ -87,18 +87,7 @@ *

* * For n-ary plans that implement this interface, - * we assume that the above applies to all the children, ie. - *

    - *
  • if dependsOnInputOrder() = false, sorts can be pruned on all the children
    • - *
  • if dependsOnInputOrder() = true, sorts cannot be pruned on any of the children
    • - *
+ * we assume that the above applies to all the children + * */ -public interface SortAware { - - /** - * Returns true if the logic of this command could depend on the order of the input. - */ - default boolean dependsOnInputOrder() { - return false; - } -} +public interface SortAgnostic {} diff --git a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/logical/join/Join.java b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/logical/join/Join.java index cf11ef08580c9..14877abb62272 100644 --- a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/logical/join/Join.java +++ b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/logical/join/Join.java @@ -21,7 +21,7 @@ import org.elasticsearch.xpack.esql.io.stream.PlanStreamInput; import org.elasticsearch.xpack.esql.plan.logical.BinaryPlan; import org.elasticsearch.xpack.esql.plan.logical.LogicalPlan; -import org.elasticsearch.xpack.esql.plan.logical.SortAware; +import org.elasticsearch.xpack.esql.plan.logical.SortAgnostic; import java.io.IOException; import java.util.ArrayList; @@ -33,7 +33,7 @@ import static org.elasticsearch.xpack.esql.expression.NamedExpressions.mergeOutputAttributes; import static org.elasticsearch.xpack.esql.plan.logical.join.JoinTypes.LEFT; -public class Join extends BinaryPlan implements PostAnalysisVerificationAware, SortAware { +public class Join extends BinaryPlan implements PostAnalysisVerificationAware, SortAgnostic { public static final NamedWriteableRegistry.Entry ENTRY = new NamedWriteableRegistry.Entry(LogicalPlan.class, "Join", Join::new); private final JoinConfig config;