Remove eliminating sort optimization

This bit of code is making the assumption that it's safe to remove the Sort because there are no exchanges yet. However, it's forgetting the fact that the data needs to be sorted, so later when the exchange is added this expectation is broken.
trinodb · Oct 19, 2023 · d9fbf8a · d9fbf8a
1 parent a098d04
commit d9fbf8a
Show file tree

Hide file tree

Showing 2 changed files with 0 additions and 28 deletions.
diff --git a/core/trino-main/src/main/java/io/trino/sql/planner/optimizations/AddExchanges.java b/core/trino-main/src/main/java/io/trino/sql/planner/optimizations/AddExchanges.java
@@ -608,18 +608,6 @@ public PlanWithProperties visitSort(SortNode node, PreferredProperties preferred
         {
             PlanWithProperties child = planChild(node, PreferredProperties.undistributed());
 
-            if (child.getProperties().isSingleNode()) {
-                // current plan so far is single node, so local properties are effectively global properties
-                // skip the SortNode if the local properties guarantee ordering on Sort keys
-                // TODO: This should be extracted as a separate optimizer once the planner is able to reason about the ordering of each operator
-                List<LocalProperty<Symbol>> desiredProperties = node.getOrderingScheme().toLocalProperties();
-
-                if (LocalProperties.match(child.getProperties().getLocalProperties(), desiredProperties).stream()
-                        .noneMatch(Optional::isPresent)) {
-                    return child;
-                }
-            }
-
             if (isDistributedSortEnabled(session)) {
                 child = planChild(node, PreferredProperties.any());
                 // insert round robin exchange to eliminate skewness issues

diff --git a/core/trino-main/src/test/java/io/trino/sql/planner/optimizations/TestEliminateSorts.java b/core/trino-main/src/test/java/io/trino/sql/planner/optimizations/TestEliminateSorts.java
@@ -36,7 +36,6 @@
 import static io.trino.sql.planner.TypeAnalyzer.createTestingTypeAnalyzer;
 import static io.trino.sql.planner.assertions.PlanMatchPattern.anyTree;
 import static io.trino.sql.planner.assertions.PlanMatchPattern.functionCall;
-import static io.trino.sql.planner.assertions.PlanMatchPattern.output;
 import static io.trino.sql.planner.assertions.PlanMatchPattern.sort;
 import static io.trino.sql.planner.assertions.PlanMatchPattern.specification;
 import static io.trino.sql.planner.assertions.PlanMatchPattern.tableScan;
@@ -56,21 +55,6 @@ public class TestEliminateSorts
             "lineitem",
             ImmutableMap.of(QUANTITY_ALIAS, "quantity"));
 
-    @Test
-    public void testEliminateSorts()
-    {
-        @Language("SQL") String sql = "SELECT quantity, row_number() OVER (ORDER BY quantity) FROM lineitem ORDER BY quantity";
-
-        PlanMatchPattern pattern =
-                output(
-                        window(windowMatcherBuilder -> windowMatcherBuilder
-                                        .specification(windowSpec)
-                                        .addFunction(functionCall("row_number", Optional.empty(), ImmutableList.of())),
-                                anyTree(LINEITEM_TABLESCAN_Q)));
-
-        assertUnitPlan(sql, pattern);
-    }
-
     @Test
     public void testNotEliminateSorts()
     {