diff --git a/core/trino-main/src/main/java/io/trino/sql/planner/PlanOptimizers.java b/core/trino-main/src/main/java/io/trino/sql/planner/PlanOptimizers.java index ca534c4a696c..85416fc5a51d 100644 --- a/core/trino-main/src/main/java/io/trino/sql/planner/PlanOptimizers.java +++ b/core/trino-main/src/main/java/io/trino/sql/planner/PlanOptimizers.java @@ -216,6 +216,7 @@ import io.trino.sql.planner.iterative.rule.RewriteTableFunctionToTableScan; import io.trino.sql.planner.iterative.rule.SimplifyCountOverConstant; import io.trino.sql.planner.iterative.rule.SimplifyExpressions; +import io.trino.sql.planner.iterative.rule.SimplifyFalseConditions; import io.trino.sql.planner.iterative.rule.SimplifyFilterPredicate; import io.trino.sql.planner.iterative.rule.SingleDistinctAggregationToGroupBy; import io.trino.sql.planner.iterative.rule.TransformCorrelatedDistinctAggregationWithProjection; @@ -374,6 +375,7 @@ public PlanOptimizers( .addAll(new CanonicalizeExpressions(plannerContext, typeAnalyzer).rules()) .addAll(new RemoveRedundantDateTrunc(plannerContext, typeAnalyzer).rules()) .addAll(new ArraySortAfterArrayDistinct(plannerContext).rules()) + .addAll(new SimplifyFalseConditions(plannerContext).rules()) .add(new RemoveTrivialFilters()) .build(); IterativeOptimizer simplifyOptimizer = new IterativeOptimizer( @@ -420,6 +422,7 @@ public PlanOptimizers( .addAll(limitPushdownRules) .addAll(new UnwrapRowSubscript().rules()) .addAll(new PushCastIntoRow().rules()) + .addAll(new SimplifyFalseConditions(plannerContext).rules()) .addAll(ImmutableSet.of( new ImplementTableFunctionSource(metadata), new UnwrapSingleColumnRowInApply(typeAnalyzer), @@ -579,6 +582,8 @@ public PlanOptimizers( .addAll(columnPruningRules) .add(new InlineProjections(plannerContext, typeAnalyzer)) .addAll(new PushFilterThroughCountAggregation(plannerContext).rules()) // must run after PredicatePushDown and after TransformFilteringSemiJoinToInnerJoin + .addAll(new SimplifyFalseConditions(plannerContext).rules()) + .add(new RemoveTrivialFilters()) .build())); // Perform redirection before CBO rules to ensure stats from destination connector are used diff --git a/core/trino-main/src/main/java/io/trino/sql/planner/iterative/rule/PushPredicateIntoTableScan.java b/core/trino-main/src/main/java/io/trino/sql/planner/iterative/rule/PushPredicateIntoTableScan.java index cb2cbe6c41dc..2844e7449712 100644 --- a/core/trino-main/src/main/java/io/trino/sql/planner/iterative/rule/PushPredicateIntoTableScan.java +++ b/core/trino-main/src/main/java/io/trino/sql/planner/iterative/rule/PushPredicateIntoTableScan.java @@ -238,9 +238,7 @@ public static Optional pushFilterIntoTableScan( } if (newDomain.isNone()) { - // TODO: DomainTranslator.fromPredicate can infer that the expression is "false" in some cases (TupleDomain.none()). - // This should move to another rule that simplifies the filter using that logic and then rely on RemoveTrivialFilters - // to turn the subtree into a Values node + // This is just for extra safety, SimplifyFalseConditions is responsible for eliminating such filters return Optional.of(new ValuesNode(node.getId(), node.getOutputSymbols(), ImmutableList.of())); } diff --git a/core/trino-main/src/main/java/io/trino/sql/planner/iterative/rule/RemoveEmptyMergeWriterRuleSet.java b/core/trino-main/src/main/java/io/trino/sql/planner/iterative/rule/RemoveEmptyMergeWriterRuleSet.java index e5caec519e63..90d7e2a1272b 100644 --- a/core/trino-main/src/main/java/io/trino/sql/planner/iterative/rule/RemoveEmptyMergeWriterRuleSet.java +++ b/core/trino-main/src/main/java/io/trino/sql/planner/iterative/rule/RemoveEmptyMergeWriterRuleSet.java @@ -45,7 +45,7 @@ * - Exchange (optional) * - MergeWriter * - Exchange - * - Project + * - Project (optional) * - empty Values * * into @@ -61,10 +61,20 @@ public static Set> rules() { return ImmutableSet.of( removeEmptyMergeWriterRule(), - removeEmptyMergeWriterWithExchangeRule()); + removeEmptyMergeWriterWithProjectRule(), + removeEmptyMergeWriterWithExchangeRule(), + removeEmptyMergeWriterWithProjectAndExchangeRule()); } static Rule removeEmptyMergeWriterRule() + { + return new RemoveEmptyMergeWriter(tableFinish() + .with(source().matching(mergeWriter() + .with(source().matching(exchange() + .with(source().matching(emptyValues()))))))); + } + + static Rule removeEmptyMergeWriterWithProjectRule() { return new RemoveEmptyMergeWriter(tableFinish() .with(source().matching(mergeWriter() @@ -74,6 +84,15 @@ static Rule removeEmptyMergeWriterRule() } static Rule removeEmptyMergeWriterWithExchangeRule() + { + return new RemoveEmptyMergeWriter(tableFinish() + .with(source().matching(exchange() + .with(source().matching(mergeWriter() + .with(source().matching(exchange() + .with(source().matching(emptyValues()))))))))); + } + + static Rule removeEmptyMergeWriterWithProjectAndExchangeRule() { return new RemoveEmptyMergeWriter(tableFinish() .with(source().matching(exchange() diff --git a/core/trino-main/src/main/java/io/trino/sql/planner/iterative/rule/RemoveRedundantPredicateAboveTableScan.java b/core/trino-main/src/main/java/io/trino/sql/planner/iterative/rule/RemoveRedundantPredicateAboveTableScan.java index f43a8dbd6a65..60467aef5318 100644 --- a/core/trino-main/src/main/java/io/trino/sql/planner/iterative/rule/RemoveRedundantPredicateAboveTableScan.java +++ b/core/trino-main/src/main/java/io/trino/sql/planner/iterative/rule/RemoveRedundantPredicateAboveTableScan.java @@ -107,9 +107,7 @@ public Result apply(FilterNode filterNode, Captures captures, Context context) .transformKeys(node.getAssignments()::get); if (predicateDomain.isNone()) { - // TODO: DomainTranslator.fromPredicate can infer that the expression is "false" in some cases (TupleDomain.none()). - // This should move to another rule that simplifies the filter using that logic and then rely on RemoveTrivialFilters - // to turn the subtree into a Values node + // This is just for extra safety, SimplifyFalseConditions is responsible for eliminating such filters return Result.ofPlanNode(new ValuesNode(node.getId(), node.getOutputSymbols(), ImmutableList.of())); } diff --git a/core/trino-main/src/main/java/io/trino/sql/planner/iterative/rule/SimplifyFalseConditions.java b/core/trino-main/src/main/java/io/trino/sql/planner/iterative/rule/SimplifyFalseConditions.java new file mode 100644 index 000000000000..0495aabd0fa2 --- /dev/null +++ b/core/trino-main/src/main/java/io/trino/sql/planner/iterative/rule/SimplifyFalseConditions.java @@ -0,0 +1,89 @@ +/* + * 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.iterative.rule; + +import com.google.common.collect.ImmutableSet; +import io.trino.Session; +import io.trino.metadata.Metadata; +import io.trino.sql.PlannerContext; +import io.trino.sql.planner.DomainTranslator; +import io.trino.sql.planner.TypeProvider; +import io.trino.sql.planner.iterative.Rule; +import io.trino.sql.tree.Expression; + +import java.util.ArrayList; +import java.util.List; +import java.util.Set; + +import static io.trino.sql.ExpressionUtils.combineConjuncts; +import static io.trino.sql.ExpressionUtils.extractConjuncts; +import static io.trino.sql.planner.DeterminismEvaluator.isDeterministic; +import static io.trino.sql.tree.BooleanLiteral.FALSE_LITERAL; +import static java.util.Objects.requireNonNull; + +/** + * Uses DomainTranslator#getExtractionResult to infer that the expression is "false" in some cases (TupleDomain.none()). + */ +public class SimplifyFalseConditions + extends ExpressionRewriteRuleSet +{ + public SimplifyFalseConditions(PlannerContext plannerContext) + { + super(createRewrite(plannerContext)); + } + + @Override + public Set> rules() + { + // Can only apply on expressions that are used for filtering, otherwise TupleDomain.none() might represent a NULL value + return ImmutableSet.of( + filterExpressionRewrite(), + joinExpressionRewrite()); + } + + private static ExpressionRewriter createRewrite(PlannerContext plannerContext) + { + requireNonNull(plannerContext, "plannerContext is null"); + + return (expression, context) -> rewrite(context.getSession(), plannerContext, context.getSymbolAllocator().getTypes(), expression); + } + + public static Expression rewrite(Session session, PlannerContext plannerContext, TypeProvider types, Expression expression) + { + Metadata metadata = plannerContext.getMetadata(); + List deterministicPredicates = new ArrayList<>(); + for (Expression conjunct : extractConjuncts(expression)) { + try { + if (isDeterministic(conjunct, metadata)) { + deterministicPredicates.add(conjunct); + } + } + catch (IllegalArgumentException ignored) { + // Might fail in case of an unresolved function + } + } + + DomainTranslator.ExtractionResult decomposedPredicate = DomainTranslator.getExtractionResult( + plannerContext, + session, + combineConjuncts(metadata, deterministicPredicates), + types); + + if (decomposedPredicate.getTupleDomain().isNone()) { + return FALSE_LITERAL; + } + + return expression; + } +} diff --git a/core/trino-main/src/test/java/io/trino/sql/planner/iterative/rule/TestRemoveEmptyMergeWriterRuleSet.java b/core/trino-main/src/test/java/io/trino/sql/planner/iterative/rule/TestRemoveEmptyMergeWriterRuleSet.java index e2eb7ed0ff1c..26d4bbb01567 100644 --- a/core/trino-main/src/test/java/io/trino/sql/planner/iterative/rule/TestRemoveEmptyMergeWriterRuleSet.java +++ b/core/trino-main/src/test/java/io/trino/sql/planner/iterative/rule/TestRemoveEmptyMergeWriterRuleSet.java @@ -24,6 +24,7 @@ import io.trino.sql.planner.plan.Assignments; import io.trino.sql.planner.plan.ExchangeNode; import io.trino.sql.planner.plan.PlanNode; +import io.trino.sql.planner.plan.ProjectNode; import io.trino.sql.planner.plan.TableFinishNode; import org.testng.annotations.BeforeClass; import org.testng.annotations.Test; @@ -34,6 +35,8 @@ import static io.trino.sql.planner.assertions.PlanMatchPattern.values; import static io.trino.sql.planner.iterative.rule.RemoveEmptyMergeWriterRuleSet.removeEmptyMergeWriterRule; import static io.trino.sql.planner.iterative.rule.RemoveEmptyMergeWriterRuleSet.removeEmptyMergeWriterWithExchangeRule; +import static io.trino.sql.planner.iterative.rule.RemoveEmptyMergeWriterRuleSet.removeEmptyMergeWriterWithProjectAndExchangeRule; +import static io.trino.sql.planner.iterative.rule.RemoveEmptyMergeWriterRuleSet.removeEmptyMergeWriterWithProjectRule; public class TestRemoveEmptyMergeWriterRuleSet extends BaseRuleTest @@ -51,16 +54,28 @@ public void setup() @Test public void testRemoveEmptyMergeRewrite() { - testRemoveEmptyMergeRewrite(removeEmptyMergeWriterRule(), false); + testRemoveEmptyMergeRewrite(removeEmptyMergeWriterRule(), false, false); + } + + @Test + public void testRemoveEmptyMergeRewriteWithProject() + { + testRemoveEmptyMergeRewrite(removeEmptyMergeWriterWithProjectRule(), true, false); } @Test public void testRemoveEmptyMergeRewriteWithExchange() { - testRemoveEmptyMergeRewrite(removeEmptyMergeWriterWithExchangeRule(), true); + testRemoveEmptyMergeRewrite(removeEmptyMergeWriterWithExchangeRule(), false, true); } - private void testRemoveEmptyMergeRewrite(Rule rule, boolean planWithExchange) + @Test + public void testRemoveEmptyMergeRewriteWithProjectAndExchange() + { + testRemoveEmptyMergeRewrite(removeEmptyMergeWriterWithProjectAndExchangeRule(), true, true); + } + + private void testRemoveEmptyMergeRewrite(Rule rule, boolean planWithProject, boolean planWithExchange) { tester().assertThat(rule) .on(p -> { @@ -68,17 +83,11 @@ private void testRemoveEmptyMergeRewrite(Rule rule, boolean pla Symbol rowId = p.symbol("row_id"); Symbol rowCount = p.symbol("row_count"); + PlanNode values = p.values(mergeRow, rowId, rowCount); PlanNode merge = p.merge( schemaTableName, p.exchange(e -> e - .addSource( - p.project( - Assignments.builder() - .putIdentity(mergeRow) - .putIdentity(rowId) - .putIdentity(rowCount) - .build(), - p.values(mergeRow, rowId, rowCount))) + .addSource(planWithProject ? withProject(p, values) : values) .addInputsSet(mergeRow, rowId, rowCount) .partitioningScheme( new PartitioningScheme( @@ -95,6 +104,13 @@ private void testRemoveEmptyMergeRewrite(Rule rule, boolean pla .matches(values("A")); } + private ProjectNode withProject(PlanBuilder planBuilder, PlanNode values) + { + Assignments.Builder assignments = Assignments.builder(); + values.getOutputSymbols().forEach(assignments::putIdentity); + return planBuilder.project(assignments.build(), values); + } + private ExchangeNode withExchange(PlanBuilder planBuilder, PlanNode source, Symbol symbol) { return planBuilder.exchange(e -> e diff --git a/core/trino-main/src/test/java/io/trino/sql/planner/iterative/rule/TestSimplifyFalseConditions.java b/core/trino-main/src/test/java/io/trino/sql/planner/iterative/rule/TestSimplifyFalseConditions.java new file mode 100644 index 000000000000..10e2d0b6c13f --- /dev/null +++ b/core/trino-main/src/test/java/io/trino/sql/planner/iterative/rule/TestSimplifyFalseConditions.java @@ -0,0 +1,83 @@ +/* + * 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.iterative.rule; + +import io.trino.sql.planner.assertions.BasePlanTest; +import org.testng.annotations.Test; + +import static io.trino.sql.planner.assertions.PlanMatchPattern.output; +import static io.trino.sql.planner.assertions.PlanMatchPattern.values; +import static java.lang.String.format; + +public class TestSimplifyFalseConditions + extends BasePlanTest +{ + @Test + public void testPredicateIsFalse() + { + testRewrite("FALSE"); + testRewrite("NOT (TRUE)"); + } + + @Test + public void testCastNullToBoolean() + { + testRewriteWithAndWithoutNegation("CAST(null AS boolean)"); + } + + @Test + public void testLogicalExpression() + { + testRewrite("TRUE AND CAST(null AS boolean)"); + testRewrite("col = BIGINT '44' AND CAST(null AS boolean)"); + testRewrite("col = BIGINT '44' AND FALSE"); + } + + @Test + public void testComparison() + { + testRewriteWithAndWithoutNegation("col > CAST(null AS BIGINT)"); + testRewriteWithAndWithoutNegation("col >= CAST(null AS BIGINT)"); + testRewriteWithAndWithoutNegation("col < CAST(null AS BIGINT)"); + testRewriteWithAndWithoutNegation("col <= CAST(null AS BIGINT)"); + testRewriteWithAndWithoutNegation("col = CAST(null AS BIGINT)"); + testRewriteWithAndWithoutNegation("col != CAST(null AS BIGINT)"); + } + + @Test + public void testIn() + { + testRewriteWithAndWithoutNegation("col IN (CAST(null AS BIGINT))"); + } + + @Test + public void testBetween() + { + testRewrite("col BETWEEN (CAST(null AS BIGINT)) AND BIGINT '44'"); + testRewrite("col BETWEEN BIGINT '44' AND (CAST(null AS BIGINT))"); + } + + private void testRewriteWithAndWithoutNegation(String inputPredicate) + { + testRewrite(inputPredicate); + testRewrite(format("NOT (%s)", inputPredicate)); + } + + private void testRewrite(String inputPredicate) + { + assertPlan(String.format("SELECT * FROM (VALUES BIGINT '1') t(col) WHERE %s", inputPredicate), + output( + values("a"))); + } +}