Repository: calcite Updated Branches: refs/heads/branch-1.18 7070f53e3 -> 527b500a4 (forced update)
[CALCITE-2730] RelBuilder incorrectly simplifies a filter with duplicate conjunction to empty (Stamatis Zampetakis) 1. Modify RexSimplify#processRange method to replace at most one term at a time. 2. Add unit test reproducing the problem. 3. Add missing filter in the expected plan of RelOptRulesTest#testMergeJoinFilter. 4. Add utility method RexUtil#replaceFirst. Move RexUtil.replaceFirst into RexSimplify; change it to replaceLast, so that it simplifies (x, y, x) to (x, y) rather than the less intuitive (y, x). (Julian Hyde) Close apache/calcite#959 Project: http://git-wip-us.apache.org/repos/asf/calcite/repo Commit: http://git-wip-us.apache.org/repos/asf/calcite/commit/8c7dc786 Tree: http://git-wip-us.apache.org/repos/asf/calcite/tree/8c7dc786 Diff: http://git-wip-us.apache.org/repos/asf/calcite/diff/8c7dc786 Branch: refs/heads/branch-1.18 Commit: 8c7dc786312d0dc0dc3f075c81ff71ac1ea871c5 Parents: f3655e1 Author: Stamatis Zampetakis <zabe...@gmail.com> Authored: Fri Dec 7 17:16:57 2018 +0100 Committer: Julian Hyde <jh...@apache.org> Committed: Wed Dec 12 22:36:50 2018 -0800 ---------------------------------------------------------------------- .../org/apache/calcite/rex/RexSimplify.java | 43 +++++++++++--------- .../org/apache/calcite/test/RelBuilderTest.java | 36 ++++++++++++++++ .../org/apache/calcite/test/RelOptRulesTest.xml | 3 +- 3 files changed, 62 insertions(+), 20 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/calcite/blob/8c7dc786/core/src/main/java/org/apache/calcite/rex/RexSimplify.java ---------------------------------------------------------------------- diff --git a/core/src/main/java/org/apache/calcite/rex/RexSimplify.java b/core/src/main/java/org/apache/calcite/rex/RexSimplify.java index c7b6dac..52e2ea1 100644 --- a/core/src/main/java/org/apache/calcite/rex/RexSimplify.java +++ b/core/src/main/java/org/apache/calcite/rex/RexSimplify.java @@ -1687,6 +1687,7 @@ public class RexSimplify { boolean removeUpperBound = false; boolean removeLowerBound = false; Range<C> r = p.left; + final RexLiteral trueLiteral = rexBuilder.makeLiteral(true); switch (comparison) { case EQUALS: if (!r.contains(v0)) { @@ -1698,7 +1699,7 @@ public class RexSimplify { (List<RexNode>) ImmutableList.of(term))); // remove for (RexNode e : p.right) { - Collections.replaceAll(terms, e, rexBuilder.makeLiteral(true)); + replaceLast(terms, e, trueLiteral); } break; case LESS_THAN: { @@ -1732,10 +1733,7 @@ public class RexSimplify { removeUpperBound = true; } else { // Remove this term as it is contained in current upper bound - final int index = terms.indexOf(term); - if (index >= 0) { - terms.set(index, rexBuilder.makeLiteral(true)); - } + replaceLast(terms, term, trueLiteral); } break; } @@ -1769,10 +1767,7 @@ public class RexSimplify { removeUpperBound = true; } else { // Remove this term as it is contained in current upper bound - final int index = terms.indexOf(term); - if (index >= 0) { - terms.set(index, rexBuilder.makeLiteral(true)); - } + replaceLast(terms, term, trueLiteral); } break; } @@ -1807,10 +1802,7 @@ public class RexSimplify { removeLowerBound = true; } else { // Remove this term as it is contained in current lower bound - final int index = terms.indexOf(term); - if (index >= 0) { - terms.set(index, rexBuilder.makeLiteral(true)); - } + replaceLast(terms, term, trueLiteral); } break; } @@ -1844,10 +1836,7 @@ public class RexSimplify { removeLowerBound = true; } else { // Remove this term as it is contained in current lower bound - final int index = terms.indexOf(term); - if (index >= 0) { - terms.set(index, rexBuilder.makeLiteral(true)); - } + replaceLast(terms, term, trueLiteral); } break; } @@ -1858,7 +1847,7 @@ public class RexSimplify { ImmutableList.Builder<RexNode> newBounds = ImmutableList.builder(); for (RexNode e : p.right) { if (isUpperBound(e)) { - Collections.replaceAll(terms, e, rexBuilder.makeLiteral(true)); + replaceLast(terms, e, trueLiteral); } else { newBounds.add(e); } @@ -1870,7 +1859,7 @@ public class RexSimplify { ImmutableList.Builder<RexNode> newBounds = ImmutableList.builder(); for (RexNode e : p.right) { if (isLowerBound(e)) { - Collections.replaceAll(terms, e, rexBuilder.makeLiteral(true)); + replaceLast(terms, e, trueLiteral); } else { newBounds.add(e); } @@ -2044,6 +2033,22 @@ public class RexSimplify { return removeNullabilityCast(simplifiedAnds); } + /** + * Replaces the last occurrence of one specified value in a list with + * another. + * + * <p>Does not change the size of the list. + * + * <p>Returns whether the value was found. + */ + private static <E> boolean replaceLast(List<E> list, E oldVal, E newVal) { + final int index = list.lastIndexOf(oldVal); + if (index < 0) { + return false; + } + list.set(index, newVal); + return true; + } } // End RexSimplify.java http://git-wip-us.apache.org/repos/asf/calcite/blob/8c7dc786/core/src/test/java/org/apache/calcite/test/RelBuilderTest.java ---------------------------------------------------------------------- diff --git a/core/src/test/java/org/apache/calcite/test/RelBuilderTest.java b/core/src/test/java/org/apache/calcite/test/RelBuilderTest.java index 77a91f7..d345241 100644 --- a/core/src/test/java/org/apache/calcite/test/RelBuilderTest.java +++ b/core/src/test/java/org/apache/calcite/test/RelBuilderTest.java @@ -367,6 +367,42 @@ public class RelBuilderTest { assertThat(root, hasTree(expected)); } + /** Test case for + * <a href="https://issues.apache.org/jira/browse/CALCITE-2730";>[CALCITE-2730] + * RelBuilder incorrectly simplifies a filter with duplicate conjunction to + * empty</a>. */ + @Test public void testScanFilterDuplicateAnd() { + // Equivalent SQL: + // SELECT * + // FROM emp + // WHERE deptno > 20 AND deptno > 20 AND deptno > 20 + final RelBuilder builder = RelBuilder.create(config().build()); + builder.scan("EMP"); + final RexNode condition = builder.call(SqlStdOperatorTable.GREATER_THAN, + builder.field("DEPTNO"), + builder.literal(20)); + final RexNode condition2 = builder.call(SqlStdOperatorTable.LESS_THAN, + builder.field("DEPTNO"), + builder.literal(30)); + final RelNode root = builder.filter(condition, condition, condition) + .build(); + final String expected = "LogicalFilter(condition=[>($7, 20)])\n" + + " LogicalTableScan(table=[[scott, EMP]])\n"; + assertThat(root, hasTree(expected)); + + // Equivalent SQL: + // SELECT * + // FROM emp + // WHERE deptno > 20 AND deptno < 30 AND deptno > 20 + final RelNode root2 = builder.scan("EMP") + .filter(condition, condition2, condition, condition) + .build(); + final String expected2 = "" + + "LogicalFilter(condition=[AND(>($7, 20), <($7, 30))])\n" + + " LogicalTableScan(table=[[scott, EMP]])\n"; + assertThat(root2, hasTree(expected2)); + } + @Test public void testBadFieldName() { final RelBuilder builder = RelBuilder.create(config().build()); try { http://git-wip-us.apache.org/repos/asf/calcite/blob/8c7dc786/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 192e573..23bf02a 100644 --- a/core/src/test/resources/org/apache/calcite/test/RelOptRulesTest.xml +++ b/core/src/test/resources/org/apache/calcite/test/RelOptRulesTest.xml @@ -5794,7 +5794,8 @@ LogicalProject(DEPTNO=[$0], ENAME=[$1]) LogicalProject(DEPTNO=[$9], ENAME=[$1]) LogicalJoin(condition=[=($7, $9)], joinType=[inner]) LogicalTableScan(table=[[CATALOG, SALES, EMP]]) - LogicalTableScan(table=[[CATALOG, SALES, DEPT]]) + LogicalFilter(condition=[=($0, 10)]) + LogicalTableScan(table=[[CATALOG, SALES, DEPT]]) ]]> </Resource> </TestCase>
