From 47de7465191f7759a08cd9f5f7796a3207e5cf3d Mon Sep 17 00:00:00 2001 From: Martin Traverso Date: Fri, 31 Mar 2023 08:39:40 -0700 Subject: [PATCH] Remove ValidateLimitWithPresortedInput It's not powerful enough to validate properties of plans that get modified by predicate pushdown after AddExchanges runs, resulting in false positives such as https://github.com/trinodb/trino/issues/16768 --- .../sql/planner/sanity/PlanSanityChecker.java | 1 - .../ValidateLimitWithPresortedInput.java | 120 ----------- .../TestPartialTopNWithPresortedInput.java | 69 ++++--- .../TestValidateLimitWithPresortedInput.java | 191 ------------------ 4 files changed, 36 insertions(+), 345 deletions(-) delete mode 100644 core/trino-main/src/main/java/io/trino/sql/planner/sanity/ValidateLimitWithPresortedInput.java delete mode 100644 core/trino-main/src/test/java/io/trino/sql/planner/sanity/TestValidateLimitWithPresortedInput.java diff --git a/core/trino-main/src/main/java/io/trino/sql/planner/sanity/PlanSanityChecker.java b/core/trino-main/src/main/java/io/trino/sql/planner/sanity/PlanSanityChecker.java index c609e2425d5e..a531d873234b 100644 --- a/core/trino-main/src/main/java/io/trino/sql/planner/sanity/PlanSanityChecker.java +++ b/core/trino-main/src/main/java/io/trino/sql/planner/sanity/PlanSanityChecker.java @@ -61,7 +61,6 @@ public PlanSanityChecker(boolean forceSingleNode) new ValidateAggregationsWithDefaultValues(forceSingleNode), new ValidateScaledWritersUsage(), new ValidateStreamingAggregations(), - new ValidateLimitWithPresortedInput(), new DynamicFiltersChecker(), new TableScanValidator(), new TableExecuteStructureValidator()) diff --git a/core/trino-main/src/main/java/io/trino/sql/planner/sanity/ValidateLimitWithPresortedInput.java b/core/trino-main/src/main/java/io/trino/sql/planner/sanity/ValidateLimitWithPresortedInput.java deleted file mode 100644 index 66b852155ef9..000000000000 --- a/core/trino-main/src/main/java/io/trino/sql/planner/sanity/ValidateLimitWithPresortedInput.java +++ /dev/null @@ -1,120 +0,0 @@ -/* - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ -package io.trino.sql.planner.sanity; - -import com.google.common.base.VerifyException; -import com.google.common.collect.PeekingIterator; -import io.trino.Session; -import io.trino.execution.warnings.WarningCollector; -import io.trino.spi.connector.ConstantProperty; -import io.trino.spi.connector.LocalProperty; -import io.trino.spi.connector.SortingProperty; -import io.trino.sql.PlannerContext; -import io.trino.sql.planner.Symbol; -import io.trino.sql.planner.TypeAnalyzer; -import io.trino.sql.planner.TypeProvider; -import io.trino.sql.planner.optimizations.StreamPropertyDerivations.StreamProperties; -import io.trino.sql.planner.plan.LimitNode; -import io.trino.sql.planner.plan.PlanNode; -import io.trino.sql.planner.plan.PlanVisitor; -import io.trino.sql.planner.sanity.PlanSanityChecker.Checker; - -import java.util.HashSet; -import java.util.Set; - -import static com.google.common.collect.Iterators.peekingIterator; -import static io.trino.sql.planner.optimizations.LocalProperties.normalizeAndPrune; -import static io.trino.sql.planner.optimizations.StreamPropertyDerivations.derivePropertiesRecursively; -import static java.lang.String.format; - -/** - * Verifies that input of order-sensitive Limit is ordered by the pre-sorted symbols - */ -public class ValidateLimitWithPresortedInput - implements Checker -{ - @Override - public void validate(PlanNode planNode, - Session session, - PlannerContext plannerContext, - TypeAnalyzer typeAnalyzer, - TypeProvider types, - WarningCollector warningCollector) - { - planNode.accept(new Visitor(session, plannerContext, typeAnalyzer, types), null); - } - - private static final class Visitor - extends PlanVisitor - { - private final Session session; - private final PlannerContext plannerContext; - private final TypeAnalyzer typeAnalyzer; - private final TypeProvider types; - - private Visitor(Session session, - PlannerContext plannerContext, - TypeAnalyzer typeAnalyzer, - TypeProvider types) - { - this.session = session; - this.plannerContext = plannerContext; - this.typeAnalyzer = typeAnalyzer; - this.types = types; - } - - @Override - protected Void visitPlan(PlanNode node, Void context) - { - node.getSources().forEach(source -> source.accept(this, context)); - return null; - } - - @Override - public Void visitLimit(LimitNode node, Void context) - { - PlanNode source = node.getSource(); - source.accept(this, context); // visit child - - if (node.getPreSortedInputs().isEmpty()) { - return null; - } - - StreamProperties properties = derivePropertiesRecursively(node.getSource(), plannerContext, session, types, typeAnalyzer); - - PeekingIterator> actuals = peekingIterator(normalizeAndPrune(properties.getLocalProperties()).iterator()); - - Set satisfied = new HashSet<>(); - for (Symbol expected : node.getPreSortedInputs()) { - while (actuals.hasNext()) { - LocalProperty actual = actuals.peek(); - if (actual instanceof ConstantProperty || - (actual instanceof SortingProperty && ((SortingProperty) actual).getColumn().equals(expected))) { - satisfied.addAll(actual.getColumns()); - actuals.next(); - } - else { - break; - } - } - - if (!satisfied.contains(expected)) { - throw new VerifyException(format("Expected Limit input to be sorted by: %s, but was %s", node.getPreSortedInputs(), properties.getLocalProperties())); - } - } - - return null; - } - } -} diff --git a/core/trino-main/src/test/java/io/trino/sql/planner/optimizations/TestPartialTopNWithPresortedInput.java b/core/trino-main/src/test/java/io/trino/sql/planner/optimizations/TestPartialTopNWithPresortedInput.java index 0184e2b0ecbf..bddccc0dbbbe 100644 --- a/core/trino-main/src/test/java/io/trino/sql/planner/optimizations/TestPartialTopNWithPresortedInput.java +++ b/core/trino-main/src/test/java/io/trino/sql/planner/optimizations/TestPartialTopNWithPresortedInput.java @@ -20,38 +20,34 @@ import io.trino.connector.MockConnectorColumnHandle; import io.trino.connector.MockConnectorFactory; import io.trino.connector.MockConnectorTableHandle; -import io.trino.execution.warnings.WarningCollector; import io.trino.spi.connector.ColumnHandle; import io.trino.spi.connector.ColumnMetadata; import io.trino.spi.connector.ConnectorTableProperties; import io.trino.spi.connector.SchemaTableName; import io.trino.spi.connector.SortingProperty; import io.trino.spi.predicate.TupleDomain; -import io.trino.sql.PlannerContext; -import io.trino.sql.planner.Plan; +import io.trino.spi.type.RowType; import io.trino.sql.planner.assertions.BasePlanTest; -import io.trino.sql.planner.assertions.PlanAssert; import io.trino.sql.planner.assertions.PlanMatchPattern; -import io.trino.sql.planner.sanity.ValidateLimitWithPresortedInput; import io.trino.sql.tree.LongLiteral; import io.trino.testing.LocalQueryRunner; -import org.intellij.lang.annotations.Language; import org.testng.annotations.Test; import java.util.List; import java.util.Optional; import static com.google.common.collect.ImmutableList.toImmutableList; -import static io.trino.execution.querystats.PlanOptimizersStatsCollector.createPlanOptimizersStatsCollector; import static io.trino.spi.connector.SortOrder.ASC_NULLS_FIRST; import static io.trino.spi.connector.SortOrder.ASC_NULLS_LAST; +import static io.trino.spi.type.IntegerType.INTEGER; import static io.trino.spi.type.VarcharType.VARCHAR; -import static io.trino.sql.planner.LogicalPlanner.Stage.OPTIMIZED_AND_VALIDATED; -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.exchange; +import static io.trino.sql.planner.assertions.PlanMatchPattern.expression; +import static io.trino.sql.planner.assertions.PlanMatchPattern.filter; import static io.trino.sql.planner.assertions.PlanMatchPattern.limit; import static io.trino.sql.planner.assertions.PlanMatchPattern.output; +import static io.trino.sql.planner.assertions.PlanMatchPattern.project; import static io.trino.sql.planner.assertions.PlanMatchPattern.sort; import static io.trino.sql.planner.assertions.PlanMatchPattern.tableScan; import static io.trino.sql.planner.assertions.PlanMatchPattern.topN; @@ -79,6 +75,8 @@ public class TestPartialTopNWithPresortedInput private static final ColumnHandle columnHandleA = new MockConnectorColumnHandle(columnNameA, VARCHAR); private static final String columnNameB = "col_b"; + private static final SchemaTableName nestedField = new SchemaTableName(TEST_SCHEMA, "with_nested_field"); + @Override protected LocalQueryRunner createLocalQueryRunner() { @@ -98,6 +96,9 @@ protected LocalQueryRunner createLocalQueryRunner() Optional.empty(), ImmutableList.of(new SortingProperty<>(columnHandleA, ASC_NULLS_FIRST))); } + else if (tableHandle.getTableName().equals(nestedField)) { + return new ConnectorTableProperties(); + } throw new IllegalArgumentException(); }) .withGetColumns(schemaTableName -> { @@ -106,6 +107,10 @@ protected LocalQueryRunner createLocalQueryRunner() new ColumnMetadata(columnNameA, VARCHAR), new ColumnMetadata(columnNameB, VARCHAR)); } + else if (schemaTableName.equals(nestedField)) { + return ImmutableList.of( + new ColumnMetadata("nested", RowType.from(ImmutableList.of(RowType.field("k", INTEGER))))); + } throw new IllegalArgumentException(); }) .build(); @@ -117,9 +122,7 @@ protected LocalQueryRunner createLocalQueryRunner() public void testWithSortedTable() { List orderBy = ImmutableList.of(sort("t_col_a", ASCENDING, FIRST)); - assertPlanWithValidation( - "SELECT col_a FROM table_a ORDER BY 1 ASC NULLS FIRST LIMIT 10", - output( + assertDistributedPlan("SELECT col_a FROM table_a ORDER BY 1 ASC NULLS FIRST LIMIT 10", output( topN(10, orderBy, FINAL, exchange(LOCAL, GATHER, ImmutableList.of(), exchange(REMOTE, GATHER, ImmutableList.of(), @@ -132,9 +135,7 @@ public void testWithSortedTable() .collect(toImmutableList()), tableScan("table_a", ImmutableMap.of("t_col_a", "col_a")))))))); - assertPlanWithValidation( - "SELECT col_a FROM table_a ORDER BY 1 ASC NULLS FIRST", - output( + assertDistributedPlan("SELECT col_a FROM table_a ORDER BY 1 ASC NULLS FIRST", output( exchange(REMOTE, GATHER, orderBy, exchange(LOCAL, GATHER, orderBy, sort(orderBy, @@ -142,9 +143,7 @@ public void testWithSortedTable() tableScan("table_a", ImmutableMap.of("t_col_a", "col_a")))))))); orderBy = ImmutableList.of(sort("t_col_a", ASCENDING, LAST)); - assertPlanWithValidation( - "SELECT col_a FROM table_a ORDER BY 1 ASC NULLS LAST LIMIT 10", - output( + assertDistributedPlan("SELECT col_a FROM table_a ORDER BY 1 ASC NULLS LAST LIMIT 10", output( topN(10, orderBy, FINAL, exchange(LOCAL, GATHER, ImmutableList.of(), exchange(REMOTE, GATHER, ImmutableList.of(), @@ -156,9 +155,7 @@ public void testWithSortedTable() public void testWithSortedWindowFunction() { List orderBy = ImmutableList.of(sort("col_b", ASCENDING, LAST)); - assertPlanWithValidation( - "SELECT col_b, COUNT(*) OVER (ORDER BY col_b) FROM table_a ORDER BY col_b LIMIT 5", - output( + assertDistributedPlan("SELECT col_b, COUNT(*) OVER (ORDER BY col_b) FROM table_a ORDER BY col_b LIMIT 5", output( topN(5, orderBy, FINAL, exchange(LOCAL, GATHER, ImmutableList.of(), limit( @@ -181,9 +178,7 @@ public void testWithSortedWindowFunction() @Test public void testWithConstantProperty() { - assertPlanWithValidation( - "SELECT * FROM (VALUES (1), (1)) AS t (id) WHERE id = 1 ORDER BY 1 LIMIT 1", - output( + assertDistributedPlan("SELECT * FROM (VALUES (1), (1)) AS t (id) WHERE id = 1 ORDER BY 1 LIMIT 1", output( topN(1, ImmutableList.of(sort("id", ASCENDING, LAST)), FINAL, exchange(LOCAL, GATHER, ImmutableList.of(), limit(1, ImmutableList.of(), true, ImmutableList.of("id"), @@ -195,15 +190,23 @@ public void testWithConstantProperty() ImmutableList.of(new LongLiteral("1")))))))))); } - private void assertPlanWithValidation(@Language("SQL") String sql, PlanMatchPattern pattern) + @Test + public void testNestedField() { - LocalQueryRunner queryRunner = getQueryRunner(); - queryRunner.inTransaction(queryRunner.getDefaultSession(), transactionSession -> { - Plan actualPlan = queryRunner.createPlan(transactionSession, sql, OPTIMIZED_AND_VALIDATED, false, WarningCollector.NOOP, createPlanOptimizersStatsCollector()); - PlanAssert.assertPlan(transactionSession, queryRunner.getMetadata(), queryRunner.getFunctionManager(), queryRunner.getStatsCalculator(), actualPlan, pattern); - PlannerContext plannerContext = queryRunner.getPlannerContext(); - new ValidateLimitWithPresortedInput().validate(actualPlan.getRoot(), transactionSession, plannerContext, createTestingTypeAnalyzer(plannerContext), actualPlan.getTypes(), WarningCollector.NOOP); - return null; - }); + assertDistributedPlan( + """ + SELECT nested.k + FROM with_nested_field + WHERE nested.k = 1 + ORDER BY nested.k + LIMIT 1 + """, + output( + topN(1, ImmutableList.of(sort("k", ASCENDING, LAST)), FINAL, + anyTree( + limit(1, ImmutableList.of(), true, ImmutableList.of("k"), + project(ImmutableMap.of("k", expression("nested[1]")), + filter("nested[1] = 1", + tableScan("with_nested_field", ImmutableMap.of("nested", "nested"))))))))); } } diff --git a/core/trino-main/src/test/java/io/trino/sql/planner/sanity/TestValidateLimitWithPresortedInput.java b/core/trino-main/src/test/java/io/trino/sql/planner/sanity/TestValidateLimitWithPresortedInput.java deleted file mode 100644 index 471320c0021e..000000000000 --- a/core/trino-main/src/test/java/io/trino/sql/planner/sanity/TestValidateLimitWithPresortedInput.java +++ /dev/null @@ -1,191 +0,0 @@ -/* - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ -package io.trino.sql.planner.sanity; - -import com.google.common.base.VerifyException; -import com.google.common.collect.ImmutableList; -import com.google.common.collect.ImmutableMap; -import io.trino.Session; -import io.trino.connector.MockConnectorColumnHandle; -import io.trino.connector.MockConnectorFactory; -import io.trino.connector.MockConnectorTableHandle; -import io.trino.execution.warnings.WarningCollector; -import io.trino.metadata.TableHandle; -import io.trino.spi.connector.ColumnHandle; -import io.trino.spi.connector.ColumnMetadata; -import io.trino.spi.connector.ConnectorTableProperties; -import io.trino.spi.connector.SchemaTableName; -import io.trino.spi.connector.SortingProperty; -import io.trino.spi.predicate.TupleDomain; -import io.trino.sql.planner.PlanNodeIdAllocator; -import io.trino.sql.planner.TypeAnalyzer; -import io.trino.sql.planner.TypeProvider; -import io.trino.sql.planner.assertions.BasePlanTest; -import io.trino.sql.planner.iterative.rule.test.PlanBuilder; -import io.trino.sql.planner.plan.PlanNode; -import io.trino.sql.tree.LongLiteral; -import io.trino.testing.LocalQueryRunner; -import org.testng.annotations.Test; - -import java.util.Optional; -import java.util.function.Function; - -import static io.trino.spi.connector.SortOrder.ASC_NULLS_FIRST; -import static io.trino.spi.type.BigintType.BIGINT; -import static io.trino.spi.type.VarcharType.VARCHAR; -import static io.trino.sql.planner.TypeAnalyzer.createTestingTypeAnalyzer; -import static io.trino.sql.planner.iterative.rule.test.PlanBuilder.expression; -import static io.trino.testing.TestingHandles.TEST_CATALOG_NAME; -import static io.trino.testing.TestingSession.testSessionBuilder; -import static org.assertj.core.api.Assertions.assertThatThrownBy; - -public class TestValidateLimitWithPresortedInput - extends BasePlanTest -{ - private final PlanNodeIdAllocator idAllocator = new PlanNodeIdAllocator(); - private static final String TEST_SCHEMA = "test_schema"; - private static final SchemaTableName MOCK_TABLE_NAME = new SchemaTableName(TEST_SCHEMA, "table_a"); - private static final String COLUMN_NAME_A = "col_a"; - private static final ColumnHandle COLUMN_HANDLE_A = new MockConnectorColumnHandle(COLUMN_NAME_A, VARCHAR); - private static final String COLUMN_NAME_B = "col_b"; - private static final ColumnHandle COLUMN_HANDLE_B = new MockConnectorColumnHandle(COLUMN_NAME_B, VARCHAR); - private static final String COLUMN_NAME_C = "col_c"; - private static final ColumnHandle COLUMN_HANDLE_C = new MockConnectorColumnHandle(COLUMN_NAME_C, VARCHAR); - - private TableHandle mockTableHandle; - - @Override - protected LocalQueryRunner createLocalQueryRunner() - { - Session session = testSessionBuilder() - .setCatalog(TEST_CATALOG_NAME) - .setSchema(TEST_SCHEMA) - .build(); - LocalQueryRunner queryRunner = LocalQueryRunner.builder(session).build(); - MockConnectorFactory mockFactory = MockConnectorFactory.builder() - .withGetTableProperties((connectorSession, handle) -> { - MockConnectorTableHandle tableHandle = (MockConnectorTableHandle) handle; - if (tableHandle.getTableName().equals(MOCK_TABLE_NAME)) { - return new ConnectorTableProperties( - TupleDomain.all(), - Optional.empty(), - Optional.empty(), - Optional.empty(), - ImmutableList.of( - new SortingProperty<>(COLUMN_HANDLE_A, ASC_NULLS_FIRST), - new SortingProperty<>(COLUMN_HANDLE_C, ASC_NULLS_FIRST))); - } - throw new IllegalArgumentException(); - }) - .withGetColumns(schemaTableName -> { - if (schemaTableName.equals(MOCK_TABLE_NAME)) { - return ImmutableList.of( - new ColumnMetadata(COLUMN_NAME_A, VARCHAR), - new ColumnMetadata(COLUMN_NAME_B, VARCHAR), - new ColumnMetadata(COLUMN_NAME_C, VARCHAR)); - } - throw new IllegalArgumentException(); - }) - .build(); - queryRunner.createCatalog(TEST_CATALOG_NAME, mockFactory, ImmutableMap.of()); - - mockTableHandle = queryRunner.getTableHandle(TEST_CATALOG_NAME, MOCK_TABLE_NAME.getSchemaName(), MOCK_TABLE_NAME.getTableName()); - return queryRunner; - } - - @Test - public void testValidateSuccessful() - { - validatePlan( - p -> p.limit( - 10, - ImmutableList.of(), - true, - ImmutableList.of(p.symbol(COLUMN_NAME_A, VARCHAR), p.symbol(COLUMN_NAME_C, VARCHAR)), - p.tableScan( - mockTableHandle, - ImmutableList.of(p.symbol(COLUMN_NAME_A, VARCHAR), p.symbol(COLUMN_NAME_B, VARCHAR), p.symbol(COLUMN_NAME_C, VARCHAR)), - ImmutableMap.of( - p.symbol(COLUMN_NAME_A, VARCHAR), COLUMN_HANDLE_A, - p.symbol(COLUMN_NAME_B, VARCHAR), COLUMN_HANDLE_B, - p.symbol(COLUMN_NAME_C, VARCHAR), COLUMN_HANDLE_C)))); - - validatePlan( - p -> p.limit( - 10, - ImmutableList.of(), - true, - ImmutableList.of(p.symbol(COLUMN_NAME_A, VARCHAR)), - p.tableScan( - mockTableHandle, - ImmutableList.of(p.symbol(COLUMN_NAME_A, VARCHAR), p.symbol(COLUMN_NAME_B, VARCHAR), p.symbol(COLUMN_NAME_C, VARCHAR)), - ImmutableMap.of( - p.symbol(COLUMN_NAME_A, VARCHAR), COLUMN_HANDLE_A, - p.symbol(COLUMN_NAME_B, VARCHAR), COLUMN_HANDLE_B, - p.symbol(COLUMN_NAME_C, VARCHAR), COLUMN_HANDLE_C)))); - } - - @Test - public void testValidateConstantProperty() - { - validatePlan( - p -> p.limit( - 10, - ImmutableList.of(), - true, - ImmutableList.of(p.symbol("a", BIGINT)), - p.filter( - expression("a = BIGINT '1'"), - p.values( - ImmutableList.of(p.symbol("a", BIGINT)), - ImmutableList.of( - ImmutableList.of(new LongLiteral("1")), - ImmutableList.of(new LongLiteral("1"))))))); - } - - @Test - public void testValidateFailed() - { - assertThatThrownBy(() -> validatePlan( - p -> p.limit( - 10, - ImmutableList.of(), - true, - ImmutableList.of(p.symbol(COLUMN_NAME_B, VARCHAR)), - p.tableScan( - mockTableHandle, - ImmutableList.of(p.symbol(COLUMN_NAME_A, VARCHAR), p.symbol(COLUMN_NAME_B, VARCHAR)), - ImmutableMap.of( - p.symbol(COLUMN_NAME_A, VARCHAR), COLUMN_HANDLE_A, - p.symbol(COLUMN_NAME_B, VARCHAR), COLUMN_HANDLE_B))))) - .isInstanceOf(VerifyException.class) - .hasMessageMatching("\\QExpected Limit input to be sorted by: [col_b], but was [Sā†‘ā†(col_a)]\\E"); - } - - private void validatePlan(Function planProvider) - { - LocalQueryRunner queryRunner = getQueryRunner(); - PlanBuilder builder = new PlanBuilder(idAllocator, queryRunner.getMetadata(), queryRunner.getDefaultSession()); - PlanNode planNode = planProvider.apply(builder); - TypeProvider types = builder.getTypes(); - - queryRunner.inTransaction(session -> { - // metadata.getCatalogHandle() registers the catalog for the transaction - session.getCatalog().ifPresent(catalog -> queryRunner.getMetadata().getCatalogHandle(session, catalog)); - TypeAnalyzer typeAnalyzer = createTestingTypeAnalyzer(queryRunner.getPlannerContext()); - new ValidateLimitWithPresortedInput().validate(planNode, session, queryRunner.getPlannerContext(), typeAnalyzer, types, WarningCollector.NOOP); - return null; - }); - } -}