This is an automated email from the ASF dual-hosted git repository. stigahuang pushed a commit to branch branch-4.4.1 in repository https://gitbox.apache.org/repos/asf/impala.git
commit 9eb43fba05947119b3e7bf33810ad9e677b41ff5 Author: Eyizoha <[email protected]> AuthorDate: Tue Jul 9 11:23:00 2024 +0800 IMPALA-13203: Rewrite 'id = 0 OR false' as expected Currently, ExprRewriter cannot rewrite 'id = 0 OR false' to 'id = 0' as expected. More precisely, it fails to rewrite any cases where a boolean literal follows 'AND/OR' as expected. The issue is that the CompoundPredicate generated by NormalizeExprsRule is not analyzed, causing SimplifyConditionalsRule to skip the rewrite. This patch fixes the issue by adding analysis of the rewritten CompoundPredicate in NormalizeExprsRule. Testing: - Modified and passed FE test case ExprRewriteRulesTest#testCompoundPredicate - Modified and passed related test case Change-Id: I9d9fffdd1cc644cc2b48f08c2509f22a72362d22 Reviewed-on: http://gerrit.cloudera.org:8080/21568 Reviewed-by: Csaba Ringhofer <[email protected]> Tested-by: Impala Public Jenkins <[email protected]> --- .../org/apache/impala/rewrite/NormalizeExprsRule.java | 9 ++++++--- .../org/apache/impala/analysis/ExprCardinalityTest.java | 3 ++- .../org/apache/impala/analysis/ExprRewriteRulesTest.java | 15 +++++++++------ .../queries/PlannerTest/constant-propagation.test | 2 +- .../functional-planner/queries/PlannerTest/empty.test | 14 +++++--------- .../functional-planner/queries/PlannerTest/hdfs.test | 2 +- 6 files changed, 24 insertions(+), 21 deletions(-) diff --git a/fe/src/main/java/org/apache/impala/rewrite/NormalizeExprsRule.java b/fe/src/main/java/org/apache/impala/rewrite/NormalizeExprsRule.java index 5706b65d5..962fea251 100644 --- a/fe/src/main/java/org/apache/impala/rewrite/NormalizeExprsRule.java +++ b/fe/src/main/java/org/apache/impala/rewrite/NormalizeExprsRule.java @@ -38,17 +38,20 @@ public class NormalizeExprsRule implements ExprRewriteRule { // TODO: add normalization for other expr types. if (expr instanceof CompoundPredicate) { - return normalizeCompoundPredicate((CompoundPredicate) expr); + return normalizeCompoundPredicate((CompoundPredicate) expr, analyzer); } return expr; } - private Expr normalizeCompoundPredicate(CompoundPredicate expr) { + private Expr normalizeCompoundPredicate(CompoundPredicate expr, Analyzer analyzer) { if (expr.getOp() == CompoundPredicate.Operator.NOT) return expr; if (!(expr.getChild(0) instanceof BoolLiteral) && expr.getChild(1) instanceof BoolLiteral) { - return new CompoundPredicate(expr.getOp(), expr.getChild(1), expr.getChild(0)); + CompoundPredicate newExpr = new CompoundPredicate(expr.getOp(), expr.getChild(1), + expr.getChild(0)); + newExpr.analyzeNoThrow(analyzer); + return newExpr; } return expr; } diff --git a/fe/src/test/java/org/apache/impala/analysis/ExprCardinalityTest.java b/fe/src/test/java/org/apache/impala/analysis/ExprCardinalityTest.java index 3bd408a73..ef92c08bc 100644 --- a/fe/src/test/java/org/apache/impala/analysis/ExprCardinalityTest.java +++ b/fe/src/test/java/org/apache/impala/analysis/ExprCardinalityTest.java @@ -527,7 +527,8 @@ public class ExprCardinalityTest { // Chain of OR rewritten to IN verifySelectExpr("alltypes", "int_col = 10 or int_col = 20", 3, 2.0/10); // Or with literals - verifySelectExpr("alltypes", "int_col = 10 or true", 3, 1.0); + // 'int_col = 10 or true' rewritten to 'true', expected NDV = 1 + verifySelectExpr("alltypes", "int_col = 10 or true", 1, 1.0); verifySelectExpr("alltypes", "int_col = 10 or false", 3, 0.1); verifySelectExpr("alltypes", "int_col = 10 or null", 3, 0.1); } diff --git a/fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java b/fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java index 2fe863434..65fccab41 100644 --- a/fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java +++ b/fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java @@ -440,12 +440,15 @@ public class ExprRewriteRulesTest extends FrontendTestBase { @Test public void testCompoundPredicate() throws ImpalaException { - ExprRewriteRule rule = SimplifyConditionalsRule.INSTANCE; - - RewritesOk("false OR id = 0", rule, "id = 0"); - RewritesOk("true OR id = 0", rule, "TRUE"); - RewritesOk("false && id = 0", rule, "FALSE"); - RewritesOk("true && id = 0", rule, "id = 0"); + List<ExprRewriteRule> rules = Lists.newArrayList(NormalizeExprsRule.INSTANCE, + SimplifyConditionalsRule.INSTANCE); + + RewritesOk("id = 0 OR false", rules, "id = 0"); + RewritesOk("id = 0 OR true", rules, "TRUE"); + RewritesOk("id = 0 && false", rules, "FALSE"); + RewritesOk("id = 0 && true", rules, "id = 0"); + RewritesOk("false OR id = 0 AND true", rules, "id = 0"); + RewritesOk("true AND id = 0 OR false", rules, "id = 0"); } @Test diff --git a/testdata/workloads/functional-planner/queries/PlannerTest/constant-propagation.test b/testdata/workloads/functional-planner/queries/PlannerTest/constant-propagation.test index 3455e227c..32e711731 100644 --- a/testdata/workloads/functional-planner/queries/PlannerTest/constant-propagation.test +++ b/testdata/workloads/functional-planner/queries/PlannerTest/constant-propagation.test @@ -273,7 +273,7 @@ PLAN-ROOT SINK | 00:SCAN HDFS [functional.alltypes] HDFS partitions=24/24 files=24 size=478.45KB - predicates: int_col = 10, TRUE OR 10 + random() * functional.alltypes.tinyint_col = 100 + predicates: int_col = 10 row-size=5B cardinality=231 ==== # Collection predicates within HDFS scan nodes get optimized diff --git a/testdata/workloads/functional-planner/queries/PlannerTest/empty.test b/testdata/workloads/functional-planner/queries/PlannerTest/empty.test index b03ab5a17..9855f3d39 100644 --- a/testdata/workloads/functional-planner/queries/PlannerTest/empty.test +++ b/testdata/workloads/functional-planner/queries/PlannerTest/empty.test @@ -157,9 +157,8 @@ on (a.id = b.id and 1 + 1 > 10) ---- PLAN PLAN-ROOT SINK | -02:HASH JOIN [LEFT OUTER JOIN] -| hash predicates: a.id = b.id -| other join predicates: FALSE +02:NESTED LOOP JOIN [LEFT OUTER JOIN] +| join predicates: FALSE | row-size=178B cardinality=100 | |--01:SCAN HDFS [functional.alltypestiny b] @@ -179,11 +178,9 @@ on (a.id = b.id and !true) ---- PLAN PLAN-ROOT SINK | -02:HASH JOIN [RIGHT OUTER JOIN] -| hash predicates: a.id = b.id -| other join predicates: FALSE -| runtime filters: RF000 <- b.id -| row-size=178B cardinality=9 +02:NESTED LOOP JOIN [RIGHT OUTER JOIN] +| join predicates: FALSE +| row-size=178B cardinality=100 | |--01:SCAN HDFS [functional.alltypestiny b] | HDFS partitions=4/4 files=4 size=460B @@ -191,7 +188,6 @@ PLAN-ROOT SINK | 00:SCAN HDFS [functional.alltypessmall a] HDFS partitions=4/4 files=4 size=6.32KB - runtime filters: RF000 -> a.id row-size=89B cardinality=100 ==== # Constant conjunct in the ON-clause of an outer join is diff --git a/testdata/workloads/functional-planner/queries/PlannerTest/hdfs.test b/testdata/workloads/functional-planner/queries/PlannerTest/hdfs.test index a3d92f2ff..57bb460b5 100644 --- a/testdata/workloads/functional-planner/queries/PlannerTest/hdfs.test +++ b/testdata/workloads/functional-planner/queries/PlannerTest/hdfs.test @@ -1276,6 +1276,6 @@ PLAN-ROOT SINK | 00:SCAN HDFS [functional.alltypes] partitions=24/24 files=24 size=478.45KB - predicates: functional.alltypes.int_col = 1, FALSE OR 1 + random() * 1 = 100 + predicates: functional.alltypes.int_col = 1, 1 + random() * 1 = 100 row-size=4B cardinality=231 ====
