This is an automated email from the ASF dual-hosted git repository. jhyde pushed a commit to branch master in repository https://gitbox.apache.org/repos/asf/calcite.git
commit 82dd78a14f6aef2eeec2f9c94978d04b4acc5359 Author: yingyuwang <[email protected]> AuthorDate: Mon Aug 23 17:03:00 2021 -0400 [CALCITE-2736] ReduceExpressionsRule never reduces dynamic expressions but this should be configurable (Yingyu Wang) 1. Add new config option treatDynamicCallsAsConstant. 2. Update ReducibleExprLocator.analyzeCall() method to also rely on treatDynamicCallsAsnConstant option when determining whether to reduce a dynamic function. Fixup following "[CALCITE-4830] Immutables" (Julian Hyde). Close apache/calcite#2502 --- .../calcite/rel/rules/ReduceExpressionsRule.java | 54 +++++++++++++++------- .../apache/calcite/rel/rules/ValuesReduceRule.java | 2 +- .../org/apache/calcite/test/RelOptRulesTest.java | 46 ++++++++++++++++++ .../org/apache/calcite/test/RelOptRulesTest.xml | 28 +++++++++++ 4 files changed, 112 insertions(+), 18 deletions(-) diff --git a/core/src/main/java/org/apache/calcite/rel/rules/ReduceExpressionsRule.java b/core/src/main/java/org/apache/calcite/rel/rules/ReduceExpressionsRule.java index cbd877b..1177e16 100644 --- a/core/src/main/java/org/apache/calcite/rel/rules/ReduceExpressionsRule.java +++ b/core/src/main/java/org/apache/calcite/rel/rules/ReduceExpressionsRule.java @@ -153,7 +153,7 @@ public abstract class ReduceExpressionsRule<C extends ReduceExpressionsRule.Conf final RelOptPredicateList predicates = mq.getPulledUpPredicates(filter.getInput()); if (reduceExpressions(filter, expList, predicates, true, - config.matchNullability())) { + config.matchNullability(), config.treatDynamicCallsAsConstant())) { assert expList.size() == 1; newConditionExp = expList.get(0); reduced = true; @@ -313,7 +313,7 @@ public abstract class ReduceExpressionsRule<C extends ReduceExpressionsRule.Conf final List<RexNode> expList = Lists.newArrayList(project.getProjects()); if (reduceExpressions(project, expList, predicates, false, - config.matchNullability())) { + config.matchNullability(), config.treatDynamicCallsAsConstant())) { assert !project.getProjects().equals(expList) : "Reduced expressions should be different from original expressions"; call.transformTo( @@ -386,7 +386,7 @@ public abstract class ReduceExpressionsRule<C extends ReduceExpressionsRule.Conf leftPredicates.union(rexBuilder, rightPredicates.shift(rexBuilder, fieldCount)); if (!reduceExpressions(join, expList, predicates, true, - config.matchNullability())) { + config.matchNullability(), config.treatDynamicCallsAsConstant())) { return; } call.transformTo( @@ -471,7 +471,7 @@ public abstract class ReduceExpressionsRule<C extends ReduceExpressionsRule.Conf } final RelOptPredicateList predicates = RelOptPredicateList.EMPTY; if (reduceExpressions(calc, expandedExprList, predicates, false, - config.matchNullability())) { + config.matchNullability(), config.treatDynamicCallsAsConstant())) { final RexProgramBuilder builder = new RexProgramBuilder( calc.getInput().getRowType(), @@ -669,13 +669,13 @@ public abstract class ReduceExpressionsRule<C extends ReduceExpressionsRule.Conf */ protected static boolean reduceExpressions(RelNode rel, List<RexNode> expList, RelOptPredicateList predicates) { - return reduceExpressions(rel, expList, predicates, false, true); + return reduceExpressions(rel, expList, predicates, false, true, false); } @Deprecated // to be removed before 2.0 protected static boolean reduceExpressions(RelNode rel, List<RexNode> expList, RelOptPredicateList predicates, boolean unknownAsFalse) { - return reduceExpressions(rel, expList, predicates, unknownAsFalse, true); + return reduceExpressions(rel, expList, predicates, unknownAsFalse, true, false); } /** @@ -705,12 +705,14 @@ public abstract class ReduceExpressionsRule<C extends ReduceExpressionsRule.Conf * resulting from simplification and expression if the * expression had nullable type and the literal is * NOT NULL + * @param treatDynamicCallsAsConstant Whether to treat dynamic functions as + * constants * * @return whether reduction found something to change, and succeeded */ protected static boolean reduceExpressions(RelNode rel, List<RexNode> expList, RelOptPredicateList predicates, boolean unknownAsFalse, - boolean matchNullability) { + boolean matchNullability, boolean treatDynamicCallsAsConstant) { final RelOptCluster cluster = rel.getCluster(); final RexBuilder rexBuilder = cluster.getRexBuilder(); final List<RexNode> originExpList = Lists.newArrayList(expList); @@ -722,7 +724,7 @@ public abstract class ReduceExpressionsRule<C extends ReduceExpressionsRule.Conf // Simplify predicates in place final RexUnknownAs unknownAs = RexUnknownAs.falseIf(unknownAsFalse); final boolean reduced = reduceExpressionsInternal(rel, simplify, unknownAs, - expList, predicates); + expList, predicates, treatDynamicCallsAsConstant); boolean simplified = false; for (int i = 0; i < expList.size(); i++) { @@ -744,7 +746,7 @@ public abstract class ReduceExpressionsRule<C extends ReduceExpressionsRule.Conf protected static boolean reduceExpressionsInternal(RelNode rel, RexSimplify simplify, RexUnknownAs unknownAs, List<RexNode> expList, - RelOptPredicateList predicates) { + RelOptPredicateList predicates, boolean treatDynamicCallsAsConstant) { // Replace predicates on CASE to CASE on predicates. boolean changed = new CaseShuttle().mutate(expList); @@ -752,7 +754,7 @@ public abstract class ReduceExpressionsRule<C extends ReduceExpressionsRule.Conf final List<RexNode> constExps = new ArrayList<>(); List<Boolean> addCasts = new ArrayList<>(); findReducibleExps(rel.getCluster().getTypeFactory(), expList, - predicates.constantMap, constExps, addCasts); + predicates.constantMap, constExps, addCasts, treatDynamicCallsAsConstant); if (constExps.isEmpty()) { return changed; } @@ -818,13 +820,15 @@ public abstract class ReduceExpressionsRule<C extends ReduceExpressionsRule.Conf * @param addCasts indicator for each expression that can be constant * reduced, whether a cast of the resulting reduced * expression is potentially necessary + * @param treatDynamicCallsAsConstant Whether to treat dynamic functions as + * constants */ protected static void findReducibleExps(RelDataTypeFactory typeFactory, List<RexNode> exps, ImmutableMap<RexNode, RexNode> constants, - List<RexNode> constExps, List<Boolean> addCasts) { + List<RexNode> constExps, List<Boolean> addCasts, boolean treatDynamicCallsAsConstant) { ReducibleExprLocator gardener = new ReducibleExprLocator(typeFactory, constants, constExps, - addCasts); + addCasts, treatDynamicCallsAsConstant); for (RexNode exp : exps) { gardener.analyze(exp); } @@ -988,6 +992,8 @@ public abstract class ReduceExpressionsRule<C extends ReduceExpressionsRule.Conf NON_CONSTANT, REDUCIBLE_CONSTANT, IRREDUCIBLE_CONSTANT } + private final boolean treatDynamicCallsAsConstant; + private final List<Constancy> stack = new ArrayList<>(); private final ImmutableMap<RexNode, RexNode> constants; @@ -1000,12 +1006,13 @@ public abstract class ReduceExpressionsRule<C extends ReduceExpressionsRule.Conf ReducibleExprLocator(RelDataTypeFactory typeFactory, ImmutableMap<RexNode, RexNode> constants, List<RexNode> constExprs, - List<Boolean> addCasts) { + List<Boolean> addCasts, boolean treatDynamicCallsAsConstant) { // go deep super(true); this.constants = constants; this.constExprs = constExprs; this.addCasts = addCasts; + this.treatDynamicCallsAsConstant = treatDynamicCallsAsConstant; } public void analyze(RexNode exp) { @@ -1120,10 +1127,10 @@ public abstract class ReduceExpressionsRule<C extends ReduceExpressionsRule.Conf // be non-deterministic. if (!call.getOperator().isDeterministic()) { callConstancy = Constancy.NON_CONSTANT; - } else if (call.getOperator().isDynamicFunction()) { - // We can reduce the call to a constant, but we can't - // cache the plan if the function is dynamic. - // For now, treat it same as non-deterministic. + } else if (!treatDynamicCallsAsConstant + && call.getOperator().isDynamicFunction()) { + // In some circumstances, we should avoid caching the plan if we have dynamic functions. + // If desired, treat this situation the same as a non-deterministic function. callConstancy = Constancy.NON_CONSTANT; } @@ -1198,6 +1205,19 @@ public abstract class ReduceExpressionsRule<C extends ReduceExpressionsRule.Conf /** Sets {@link #matchNullability()}. */ Config withMatchNullability(boolean matchNullability); + /** Whether to treat + * {@link SqlOperator#isDynamicFunction() dynamic functions} as constants. + * + * <p>When false (the default), calls to dynamic functions (e.g. + * {@code USER}) are not reduced. When true, calls to dynamic functions + * are treated as a constant, and reduced. */ + @Value.Default default boolean treatDynamicCallsAsConstant() { + return false; + } + + /** Sets {@link #treatDynamicCallsAsConstant()}. */ + Config withTreatDynamicCallsAsConstant(boolean treatDynamicCallsAsConstant); + /** Defines an operand tree for the given classes. */ default Config withOperandFor(Class<? extends RelNode> relClass) { return withOperandSupplier(b -> b.operand(relClass).anyInputs()) diff --git a/core/src/main/java/org/apache/calcite/rel/rules/ValuesReduceRule.java b/core/src/main/java/org/apache/calcite/rel/rules/ValuesReduceRule.java index 3cde4f4..0d5789e 100644 --- a/core/src/main/java/org/apache/calcite/rel/rules/ValuesReduceRule.java +++ b/core/src/main/java/org/apache/calcite/rel/rules/ValuesReduceRule.java @@ -174,7 +174,7 @@ public class ValuesReduceRule // Compute the values they reduce to. final RelOptPredicateList predicates = RelOptPredicateList.EMPTY; ReduceExpressionsRule.reduceExpressions(values, reducibleExps, predicates, - false, true); + false, true, false); int changeCount = 0; final ImmutableList.Builder<ImmutableList<RexLiteral>> tuplesBuilder = diff --git a/core/src/test/java/org/apache/calcite/test/RelOptRulesTest.java b/core/src/test/java/org/apache/calcite/test/RelOptRulesTest.java index df4cd85..001f643 100644 --- a/core/src/test/java/org/apache/calcite/test/RelOptRulesTest.java +++ b/core/src/test/java/org/apache/calcite/test/RelOptRulesTest.java @@ -16,6 +16,8 @@ */ package org.apache.calcite.test; +import org.apache.calcite.DataContext; +import org.apache.calcite.DataContexts; import org.apache.calcite.adapter.enumerable.EnumerableConvention; import org.apache.calcite.adapter.enumerable.EnumerableLimit; import org.apache.calcite.adapter.enumerable.EnumerableLimitSort; @@ -26,6 +28,7 @@ import org.apache.calcite.plan.Context; import org.apache.calcite.plan.Contexts; import org.apache.calcite.plan.ConventionTraitDef; import org.apache.calcite.plan.RelOptCluster; +import org.apache.calcite.plan.RelOptPlanner; import org.apache.calcite.plan.RelOptRule; import org.apache.calcite.plan.RelOptRuleCall; import org.apache.calcite.plan.RelOptUtil; @@ -92,6 +95,7 @@ import org.apache.calcite.rel.type.RelDataTypeFactory; import org.apache.calcite.rel.type.RelDataTypeSystemImpl; import org.apache.calcite.rex.RexBuilder; import org.apache.calcite.rex.RexCall; +import org.apache.calcite.rex.RexExecutorImpl; import org.apache.calcite.rex.RexInputRef; import org.apache.calcite.rex.RexLiteral; import org.apache.calcite.rex.RexNode; @@ -143,6 +147,7 @@ import java.util.function.Supplier; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertNotNull; +import static org.junit.jupiter.api.Assertions.fail; /** * Unit test for rules in {@code org.apache.calcite.rel} and subpackages. @@ -2837,6 +2842,47 @@ class RelOptRulesTest extends RelOptTestBase { checkReduceNullableToNotNull(rule); } + /** Test case for + * <a href="https://issues.apache.org/jira/browse/CALCITE-2736";>[CALCITE-2736] + * ReduceExpressionsRule never reduces dynamic expressions but this should be + * configurable</a>. Tests that a dynamic function (USER) is reduced if and + * only if {@link ReduceExpressionsRule.Config#treatDynamicCallsAsConstant()} + * is true. */ + @Test public void testReduceDynamic() { + checkDynamicFunctions(true).check(); + } + + /** As {@link #testReduceDynamic()}. */ + @Test public void testNoReduceDynamic() { + checkDynamicFunctions(false).checkUnchanged(); + } + + private Sql checkDynamicFunctions(boolean treatDynamicCallsAsConstant) { + // Create a customized executor with given context operator that reduces + // "USER" to "happyCalciteUser" + final RexExecutorImpl executor = + new RexExecutorImpl( + DataContexts.of(name -> + name.equals(DataContext.Variable.USER.camelName) + ? "happyCalciteUser" + : fail("unknown: " + name))); + + RelOptPlanner planner = new MockRelOptPlanner(Contexts.empty()); + planner.setExecutor(executor); + + final ReduceExpressionsRule<?> rule = + CoreRules.PROJECT_REDUCE_EXPRESSIONS.config + .withOperandFor(LogicalProject.class) + .withTreatDynamicCallsAsConstant(treatDynamicCallsAsConstant) + .as(ProjectReduceExpressionsRule.Config.class) + .toRule(); + + final String sql = "select USER from emp"; + return sql(sql) + .withTester(t -> ((TesterImpl) tester).withPlannerFactory(context -> planner)) + .withRule(rule); + } + @Test void testReduceConstantsIsNull() { final String sql = "select empno from emp where empno=10 and empno is null"; sql(sql) diff --git a/core/src/test/resources/org/apache/calcite/test/RelOptRulesTest.xml b/core/src/test/resources/org/apache/calcite/test/RelOptRulesTest.xml index ec91207..12e944c 100644 --- a/core/src/test/resources/org/apache/calcite/test/RelOptRulesTest.xml +++ b/core/src/test/resources/org/apache/calcite/test/RelOptRulesTest.xml @@ -5473,6 +5473,17 @@ LogicalAggregate(group=[{0}], EXPR$1=[MAX($0)], EXPR$2=[AVG($1)], EXPR$3=[MIN($0 ]]> </Resource> </TestCase> + <TestCase name="testNoReduceDynamic"> + <Resource name="sql"> + <![CDATA[select USER from emp]]> + </Resource> + <Resource name="planBefore"> + <![CDATA[ +LogicalProject(USER=[USER]) + LogicalTableScan(table=[[CATALOG, SALES, EMP]]) +]]> + </Resource> + </TestCase> <TestCase name="testNoReduceSum"> <Resource name="sql"> <![CDATA[select name, sum(deptno) from sales.dept group by name]]> @@ -10579,6 +10590,23 @@ LogicalProject(ENAME=[$1]) ]]> </Resource> </TestCase> + <TestCase name="testReduceDynamic"> + <Resource name="sql"> + <![CDATA[select USER from emp]]> + </Resource> + <Resource name="planBefore"> + <![CDATA[ +LogicalProject(USER=[USER]) + LogicalTableScan(table=[[CATALOG, SALES, EMP]]) +]]> + </Resource> + <Resource name="planAfter"> + <![CDATA[ +LogicalProject(USER=['happyCalciteUser':VARCHAR(2000)]) + LogicalTableScan(table=[[CATALOG, SALES, EMP]]) +]]> + </Resource> + </TestCase> <TestCase name="testReduceExpressionsNot"> <Resource name="sql"> <![CDATA[select * from (values (false),(true)) as q (col1) where not(col1)]]>
