From 2ae94e7ead090b3e80b7a75fee7026f2fe8d8ca9 Mon Sep 17 00:00:00 2001 From: Thomas Tauber-Marshall Date: Wed, 2 Aug 2017 10:51:57 -0700 Subject: [PATCH] IMPALA-5725: coalesce() with outer join incorrectly rewritten A recent change, IMPALA-5016, added an expr rewrite rule to simplfy coalesce(). This rule eliminates the coalesce() when its first parameter (that isn't constant null) is a SlotRef pointing to a SlotDescriptor that is non-nullable (for example because it is from a non-nullable Kudu column or because it is from an HDFS partition column with no null partitions), under the assumption that the SlotRef could never have a null value. This assumption is violated when the SlotRef is the output of an outer join, leading to incorrect results being returned. The problem is that the nullability of a SlotDescriptor (which determines whether there is a null indicator bit in the tuple for that slot) is a slightly different property than the nullability of a SlotRef pointing to that SlotDescriptor (since the SlotRef can still be NULL if the entire tuple is NULL). This patch removes the portion of the rewrite rule that considers the nullability of the SlotDescriptor. This means that we're missing out on some optimizations opportunities and we should revisit this in a way that works with outer joins (IMPALA-5753) Testing: - Updated FE tests. - Added regression tests to exprs.test Change-Id: I1ca6df949f9d416ab207016236dbcb5886295337 Reviewed-on: http://gerrit.cloudera.org:8080/7567 Reviewed-by: Matthew Jacobs Reviewed-by: Thomas Tauber-Marshall Tested-by: Impala Public Jenkins --- .../impala/analysis/SlotDescriptor.java | 2 ++ .../rewrite/SimplifyConditionalsRule.java | 30 +------------------ .../impala/analysis/ExprRewriteRulesTest.java | 17 ++--------- .../queries/QueryTest/exprs.test | 24 ++++++++++++++- 4 files changed, 29 insertions(+), 44 deletions(-) diff --git a/fe/src/main/java/org/apache/impala/analysis/SlotDescriptor.java b/fe/src/main/java/org/apache/impala/analysis/SlotDescriptor.java index f67ac8b53..e303a11ef 100644 --- a/fe/src/main/java/org/apache/impala/analysis/SlotDescriptor.java +++ b/fe/src/main/java/org/apache/impala/analysis/SlotDescriptor.java @@ -55,6 +55,8 @@ public class SlotDescriptor { private boolean isMaterialized_ = false; // if false, this slot cannot be NULL + // Note: it is still possible that a SlotRef pointing to this descriptor could have a + // NULL value if the entire tuple is NULL, for example as the result of an outer join. private boolean isNullable_ = true; // physical layout parameters diff --git a/fe/src/main/java/org/apache/impala/rewrite/SimplifyConditionalsRule.java b/fe/src/main/java/org/apache/impala/rewrite/SimplifyConditionalsRule.java index 287b4be1b..6d0b38c8f 100644 --- a/fe/src/main/java/org/apache/impala/rewrite/SimplifyConditionalsRule.java +++ b/fe/src/main/java/org/apache/impala/rewrite/SimplifyConditionalsRule.java @@ -103,8 +103,6 @@ public class SimplifyConditionalsRule implements ExprRewriteRule { * Simplify COALESCE by skipping leading nulls and applying the following transformations: * COALESCE(null, a, b) -> COALESCE(a, b); * COALESCE(, a, b) -> , when literal is not NullLiteral; - * COALESCE(, a, b) -> , - * when the partition column does not contain NULL. */ private Expr simplifyCoalesceFunctionCallExpr(FunctionCallExpr expr) { int numChildren = expr.getChildren().size(); @@ -113,7 +111,7 @@ public class SimplifyConditionalsRule implements ExprRewriteRule { Expr childExpr = expr.getChildren().get(i); // Skip leading nulls. if (childExpr.isNullLiteral()) continue; - if ((i == numChildren - 1) || canSimplifyCoalesceUsingChild(childExpr)) { + if ((i == numChildren - 1) || childExpr.isLiteral()) { result = childExpr; } else if (i == 0) { result = expr; @@ -126,32 +124,6 @@ public class SimplifyConditionalsRule implements ExprRewriteRule { return result; } - /** - * Checks if the given child expr is nullable. Returns true if one of the following holds: - * child is a non-NULL literal; - * child is a possibly cast SlotRef against a non-nullable slot; - * child is a possible cast SlotRef against a partition column that does not contain NULL. - */ - private boolean canSimplifyCoalesceUsingChild(Expr child) { - if (child.isLiteral() && !child.isNullLiteral()) return true; - - SlotRef slotRef = child.unwrapSlotRef(false); - if (slotRef == null) return false; - SlotDescriptor slotDesc = slotRef.getDesc(); - if (!slotDesc.getIsNullable()) return true; - // Check partition column using partition metadata. - if (slotDesc.getParent().getTable() instanceof HdfsTable - && slotDesc.getColumn() != null - && slotDesc.getParent().getTable().isClusteringColumn(slotDesc.getColumn())) { - HdfsTable table = (HdfsTable) slotDesc.getParent().getTable(); - // Return true if the partition column does not have a NULL value. - if (table.getNullPartitionIds(slotDesc.getColumn().getPosition()).isEmpty()) { - return true; - } - } - return false; - } - private Expr simplifyFunctionCallExpr(FunctionCallExpr expr) { FunctionName fnName = expr.getFnName(); 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 e49c6525f..4ff50a33b 100644 --- a/fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java +++ b/fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java @@ -398,20 +398,9 @@ public class ExprRewriteRulesTest extends FrontendTestBase { RewritesOk("coalesce(1 + 2, id, year)", rules, "3"); RewritesOk("coalesce(null is null, bool_col)", rules, "TRUE"); RewritesOk("coalesce(10 + null, id, year)", rules, "coalesce(id, year)"); - // If the leading parameter is partition column, try to rewrite with partition metadata. - RewritesOk("coalesce(year, id)", rule, "year"); - RewritesOk("coalesce(year, bigint_col)", rule, "year"); - RewritesOk("coalesce(cast(year as string), string_col)", rule, "CAST(year AS STRING)"); - RewritesOk("coalesce(id, year)", rule, null); - RewritesOk("coalesce(null, year, id)", rule, "year"); - // If the leading partition column has NULL value, do not rewrite. - RewritesOk("functional.alltypesagg", "coalesce(year, id)", rule, "year"); - RewritesOk("functional.alltypesagg", "coalesce(day, id)", rule, null); - // If the leading column is not nullable, rewrite to the column. - RewritesOk("functional_kudu.alltypessmall", "coalesce(id, year)", rule, "id"); - RewritesOk("functional_kudu.alltypessmall", "coalesce(cast(id as string), string_col)", rule, - "CAST(id AS STRING)"); - RewritesOk("functional_kudu.alltypessmall", "coalesce(null, id, year)", rule, "id"); + // Don't rewrite based on nullability of slots. TODO (IMPALA-5753). + RewritesOk("coalesce(year, id)", rule, null); + RewritesOk("functional_kudu.alltypessmall", "coalesce(id, year)", rule, null); } @Test diff --git a/testdata/workloads/functional-query/queries/QueryTest/exprs.test b/testdata/workloads/functional-query/queries/QueryTest/exprs.test index d618a2497..9b1a1a73d 100644 --- a/testdata/workloads/functional-query/queries/QueryTest/exprs.test +++ b/testdata/workloads/functional-query/queries/QueryTest/exprs.test @@ -2741,4 +2741,26 @@ select if (true, 0, sum(id)) from functional.alltypestiny 0 ---- TYPES BIGINT -==== \ No newline at end of file +==== +---- QUERY +# IMPALA-5725: test coalesce() is not rewritten when its first parameter is a non-nullable +# Kudu column, but can have NULL values due to an outer join. +select coalesce(b.id, a.id), b.id, a.id +from functional_kudu.alltypes a left join functional_kudu.alltypestiny b on a.id = b.id +where a.id = 100 +---- RESULTS +100,NULL,100 +---- TYPES +INT,INT,INT +==== +---- QUERY +# IMPALA-5725: test coalesce() is not rewritten when its first parameter is an HDFS +# partition col with no NULL partitions, but can have NULL values due to an outer join. +select coalesce(b.year, a.id), b.id, a.id +from functional.alltypes a left join functional.alltypestiny b on a.id = b.id +where a.id = 100 +---- RESULTS +100,NULL,100 +---- TYPES +INT,INT,INT +====