[CALCITE-935] Improve how ReduceExpressionsRule handles duplicate constraints (Pengcheng Xiong)
Add a test case making sure that non-equi constraints and identical constraints do not prevent constant reduction. Some fix up (Julian Hyde) Project: http://git-wip-us.apache.org/repos/asf/calcite/repo Commit: http://git-wip-us.apache.org/repos/asf/calcite/commit/690faa55 Tree: http://git-wip-us.apache.org/repos/asf/calcite/tree/690faa55 Diff: http://git-wip-us.apache.org/repos/asf/calcite/diff/690faa55 Branch: refs/heads/branch-release Commit: 690faa55f553fdaa86aa91a3f2c731d6c16007ca Parents: c5f2599 Author: Pengcheng Xiong <[email protected]> Authored: Sat Oct 24 08:53:57 2015 -0700 Committer: Julian Hyde <[email protected]> Committed: Mon Oct 26 10:20:09 2015 -0700 ---------------------------------------------------------------------- .../rel/rules/ReduceExpressionsRule.java | 92 ++++++++++++++++++-- .../apache/calcite/test/RelOptRulesTest.java | 17 ++++ .../org/apache/calcite/test/RelOptRulesTest.xml | 26 +++++- 3 files changed, 125 insertions(+), 10 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/calcite/blob/690faa55/core/src/main/java/org/apache/calcite/rel/rules/ReduceExpressionsRule.java ---------------------------------------------------------------------- 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 4e2d890..75943b8 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 @@ -62,12 +62,14 @@ import org.apache.calcite.util.Util; import com.google.common.collect.ImmutableMap; import com.google.common.collect.Lists; -import com.google.common.collect.Maps; import java.util.ArrayList; import java.util.Collections; +import java.util.HashMap; +import java.util.HashSet; import java.util.List; import java.util.Map; +import java.util.Set; import java.util.regex.Pattern; /** @@ -510,17 +512,91 @@ public abstract class ReduceExpressionsRule extends RelOptRule { // We cannot use an ImmutableMap.Builder here. If there are multiple entries // with the same key (e.g. "WHERE deptno = 1 AND deptno = 2"), it doesn't // matter which we take, so the latter will replace the former. - final Map<RexNode, RexLiteral> builder = Maps.newHashMap(); + // The basic idea is to find all the pairs of RexNode = RexLiteral + // (1) If 'predicates' contain a non-EQUALS, we bail out. + // (2) It is OK if a RexNode is equal to the same RexLiteral several times, + // (e.g. "WHERE deptno = 1 AND deptno = 1") + // (3) It will return false if there are inconsistent constraints (e.g. + // "WHERE deptno = 1 AND deptno = 2") + final Map<RexNode, RexLiteral> map = new HashMap<>(); + final Set<RexNode> excludeSet = new HashSet<>(); for (RexNode predicate : predicates.pulledUpPredicates) { - switch (predicate.getKind()) { - case EQUALS: - final List<RexNode> operands = ((RexCall) predicate).getOperands(); - if (operands.get(1) instanceof RexLiteral) { - builder.put(operands.get(0), (RexLiteral) operands.get(1)); + gatherConstraints(map, predicate, excludeSet); + } + final ImmutableMap.Builder<RexNode, RexLiteral> builder = + ImmutableMap.builder(); + for (Map.Entry<RexNode, RexLiteral> entry : map.entrySet()) { + RexNode rexNode = entry.getKey(); + if (!overlap(rexNode, excludeSet)) { + builder.put(rexNode, entry.getValue()); + } + } + return builder.build(); + } + + private static boolean overlap(RexNode rexNode, Set<RexNode> set) { + if (rexNode instanceof RexCall) { + for (RexNode r : ((RexCall) rexNode).getOperands()) { + if (overlap(r, set)) { + return true; + } + } + return false; + } else { + return set.contains(rexNode); + } + } + + /** Tries to decompose the RexNode which is a RexCall into non-literal + * RexNodes. */ + private static void decompose(Set<RexNode> set, RexNode rexNode) { + if (rexNode instanceof RexCall) { + for (RexNode r : ((RexCall) rexNode).getOperands()) { + decompose(set, r); + } + } else if (!(rexNode instanceof RexLiteral)) { + set.add(rexNode); + } + } + + private static void gatherConstraints(Map<RexNode, RexLiteral> map, + RexNode predicate, Set<RexNode> excludeSet) { + if (predicate.getKind() != SqlKind.EQUALS) { + decompose(excludeSet, predicate); + return; + } + final List<RexNode> operands = ((RexCall) predicate).getOperands(); + if (operands.size() != 2) { + decompose(excludeSet, predicate); + return; + } + // if it reaches here, we have rexNode equals rexNode + final RexNode left = operands.get(0); + final RexNode right = operands.get(1); + // note that literals are immutable too and they can only be compared through + // values. + if (right instanceof RexLiteral && !excludeSet.contains(left)) { + RexLiteral existedValue = map.get(left); + if (existedValue == null) { + map.put(left, (RexLiteral) right); + } else { + if (!existedValue.getValue().equals(((RexLiteral) right).getValue())) { + // we found conflict values. + map.remove(left); + excludeSet.add(left); + } + } + } else if (left instanceof RexLiteral && !excludeSet.contains(right)) { + RexLiteral existedValue = map.get(right); + if (existedValue == null) { + map.put(right, (RexLiteral) left); + } else { + if (!existedValue.getValue().equals(((RexLiteral) left).getValue())) { + map.remove(right); + excludeSet.add(right); } } } - return ImmutableMap.copyOf(builder); } /** Pushes predicates into a CASE. http://git-wip-us.apache.org/repos/asf/calcite/blob/690faa55/core/src/test/java/org/apache/calcite/test/RelOptRulesTest.java ---------------------------------------------------------------------- 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 cce90b8..c237ece 100644 --- a/core/src/test/java/org/apache/calcite/test/RelOptRulesTest.java +++ b/core/src/test/java/org/apache/calcite/test/RelOptRulesTest.java @@ -817,6 +817,23 @@ public class RelOptRulesTest extends RelOptTestBase { + " where d.deptno=7 and d.deptno=8"); } + /** Test case for + * <a href="https://issues.apache.org/jira/browse/CALCITE-935";>[CALCITE-935] + * Improve how ReduceExpressionsRule handles duplicate constraints</a>. */ + @Test public void testReduceConstantsDup2() throws Exception { + HepProgram program = new HepProgramBuilder() + .addRuleInstance(ReduceExpressionsRule.PROJECT_INSTANCE) + .addRuleInstance(ReduceExpressionsRule.FILTER_INSTANCE) + .addRuleInstance(ReduceExpressionsRule.JOIN_INSTANCE) + .build(); + + final String sql = "select *\n" + + "from emp\n" + + "where deptno=7 and deptno=8\n" + + "and empno = 10 and mgr is null and empno = 10"; + checkPlanning(program, sql); + } + @Test public void testReduceConstants2() throws Exception { HepProgram program = new HepProgramBuilder() .addRuleInstance(ReduceExpressionsRule.PROJECT_INSTANCE) http://git-wip-us.apache.org/repos/asf/calcite/blob/690faa55/core/src/test/resources/org/apache/calcite/test/RelOptRulesTest.xml ---------------------------------------------------------------------- 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 ddfefbb..f51e4e9 100644 --- a/core/src/test/resources/org/apache/calcite/test/RelOptRulesTest.xml +++ b/core/src/test/resources/org/apache/calcite/test/RelOptRulesTest.xml @@ -646,7 +646,7 @@ LogicalProject(DEPTNO=[$0]) </Resource> <Resource name="planAfter"> <![CDATA[ -LogicalProject(DEPTNO=[8]) +LogicalProject(DEPTNO=[$0]) LogicalFilter(condition=[AND(=($0, 7), =($0, 8))]) LogicalTableScan(table=[[CATALOG, SALES, DEPT]]) ]]> @@ -2909,7 +2909,7 @@ LogicalProject(EMPNO=[$0], ENAME=[$1], JOB=[$2], MGR=[$3], HIREDATE=[$4], SAL=[$ <Resource name="planAfter"> <![CDATA[ LogicalProject(EMPNO=[$0], ENAME=[$1], JOB=[$2], MGR=[$3], HIREDATE=[$4], SAL=[$5], COMM=[$6], DEPTNO=[$7], SLACKER=[$8], DEPTNO0=[$9], NAME=[$10]) - LogicalProject(EMPNO=[10], ENAME=[$1], JOB=[$2], MGR=[$3], HIREDATE=[$4], SAL=[$5], COMM=[$6], DEPTNO=[$7], SLACKER=[$8], DEPTNO0=[$10], NAME=[$11]) + LogicalProject(EMPNO=[10], ENAME=[$1], JOB=[$2], MGR=[$3], HIREDATE=[$4], SAL=[$5], COMM=[$6], DEPTNO=[$7], SLACKER=[$8], DEPTNO0=[10], NAME=[$11]) LogicalJoin(condition=[AND(=(10, $10), =($9, $12))], joinType=[inner]) LogicalProject(EMPNO=[$0], ENAME=[$1], JOB=[$2], MGR=[$3], HIREDATE=[$4], SAL=[$5], COMM=[$6], DEPTNO=[$7], SLACKER=[$8], $f9=[+($7, $0)]) LogicalProject(EMPNO=[10], ENAME=[$1], JOB=[$2], MGR=[$3], HIREDATE=[$4], SAL=[$5], COMM=[$6], DEPTNO=[$7], SLACKER=[$8]) @@ -4213,4 +4213,26 @@ LogicalSort(sort0=[$0], dir0=[ASC], fetch=[10]) ]]> </Resource> </TestCase> + <TestCase name="testReduceConstantsDup2"> + <Resource name="sql"> + <![CDATA[select * +from emp +where deptno=7 and deptno=8 +and empno = 10 and mgr is null and empno = 10]]> + </Resource> + <Resource name="planBefore"> + <![CDATA[ +LogicalProject(EMPNO=[$0], ENAME=[$1], JOB=[$2], MGR=[$3], HIREDATE=[$4], SAL=[$5], COMM=[$6], DEPTNO=[$7], SLACKER=[$8]) + LogicalFilter(condition=[AND(=($7, 7), =($7, 8), =($0, 10), IS NULL($3), =($0, 10))]) + LogicalTableScan(table=[[CATALOG, SALES, EMP]]) +]]> + </Resource> + <Resource name="planAfter"> + <![CDATA[ +LogicalProject(EMPNO=[10], ENAME=[$1], JOB=[$2], MGR=[$3], HIREDATE=[$4], SAL=[$5], COMM=[$6], DEPTNO=[$7], SLACKER=[$8]) + LogicalFilter(condition=[AND(=($7, 7), =($7, 8), =($0, 10), IS NULL($3), =($0, 10))]) + LogicalTableScan(table=[[CATALOG, SALES, EMP]]) +]]> + </Resource> + </TestCase> </Root>
