From d9fbf8a881ae0c59c1c0e778aee5bfd7bf559489 Mon Sep 17 00:00:00 2001 From: Naoki Takezoe Date: Tue, 17 Oct 2023 01:17:23 +0900 Subject: [PATCH] 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. --- .../sql/planner/optimizations/AddExchanges.java | 12 ------------ .../optimizations/TestEliminateSorts.java | 16 ---------------- 2 files changed, 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 index ba10bd677e50..aecf89185fc2 100644 --- 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> 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 index 224403807f3d..f1892a5edc47 100644 --- 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() {