Repository: incubator-impala Updated Branches: refs/heads/master 34d9a79c5 -> 9d1e4449c
IMPALA-5657: Fix a couple of bugs with FunctionCallExpr and IGNORE NULLS Bugs: - FunctionCallExpr's toSql() doesn't include IGNORE NULLS if present causing view definitions to break and leading to incorrect results. - FunctionCallExpr's clone() implementation doesn't carry forward IGNORE NULLS option if present. One case that breaks with this is querying views containing analytic exprs causing wrong plans. Fixed both the bugs and added a test that can reliably reproduce this. Change-Id: I723897886c95763c3f29d6f24c4d9e7d43898ade Reviewed-on: http://gerrit.cloudera.org:8080/7416 Reviewed-by: Bharath Vissapragada <[email protected]> Tested-by: Impala Public Jenkins Project: http://git-wip-us.apache.org/repos/asf/incubator-impala/repo Commit: http://git-wip-us.apache.org/repos/asf/incubator-impala/commit/9f2f0655 Tree: http://git-wip-us.apache.org/repos/asf/incubator-impala/tree/9f2f0655 Diff: http://git-wip-us.apache.org/repos/asf/incubator-impala/diff/9f2f0655 Branch: refs/heads/master Commit: 9f2f06556fe3bfe569831da99ed80f3d282dc146 Parents: 34d9a79 Author: Bharath Vissapragada <[email protected]> Authored: Thu Jul 13 12:00:20 2017 -0700 Committer: Impala Public Jenkins <[email protected]> Committed: Thu Jul 20 19:57:53 2017 +0000 ---------------------------------------------------------------------- .../impala/analysis/FunctionCallExpr.java | 9 ++++++-- .../org/apache/impala/analysis/ToSqlTest.java | 5 +++++ .../queries/QueryTest/analytic-fns.test | 22 ++++++++++++++++++++ 3 files changed, 34 insertions(+), 2 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/9f2f0655/fe/src/main/java/org/apache/impala/analysis/FunctionCallExpr.java ---------------------------------------------------------------------- diff --git a/fe/src/main/java/org/apache/impala/analysis/FunctionCallExpr.java b/fe/src/main/java/org/apache/impala/analysis/FunctionCallExpr.java index f263cd5..20add13 100644 --- a/fe/src/main/java/org/apache/impala/analysis/FunctionCallExpr.java +++ b/fe/src/main/java/org/apache/impala/analysis/FunctionCallExpr.java @@ -150,7 +150,8 @@ public class FunctionCallExpr extends Expr { Preconditions.checkState(children_.isEmpty()); params_ = FunctionParams.createStarParam(); } else { - params_ = new FunctionParams(other.params_.isDistinct(), children_); + params_ = new FunctionParams(other.params_.isDistinct(), + other.params_.isIgnoreNulls(), children_); } label_ = other.label_; } @@ -173,6 +174,7 @@ public class FunctionCallExpr extends Expr { FunctionCallExpr o = (FunctionCallExpr)obj; return fnName_.equals(o.fnName_) && params_.isDistinct() == o.params_.isDistinct() && + params_.isIgnoreNulls() == o.params_.isIgnoreNulls() && params_.isStar() == o.params_.isStar(); } @@ -185,7 +187,9 @@ public class FunctionCallExpr extends Expr { sb.append(fnName_).append("("); if (params_.isStar()) sb.append("*"); if (params_.isDistinct()) sb.append("DISTINCT "); - sb.append(Joiner.on(", ").join(childrenToSql())).append(")"); + sb.append(Joiner.on(", ").join(childrenToSql())); + if (params_.isIgnoreNulls()) sb.append(" IGNORE NULLS"); + sb.append(")"); return sb.toString(); } @@ -195,6 +199,7 @@ public class FunctionCallExpr extends Expr { .add("name", fnName_) .add("isStar", params_.isStar()) .add("isDistinct", params_.isDistinct()) + .add("isIgnoreNulls", params_.isIgnoreNulls()) .addValue(super.debugString()) .toString(); } http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/9f2f0655/fe/src/test/java/org/apache/impala/analysis/ToSqlTest.java ---------------------------------------------------------------------- diff --git a/fe/src/test/java/org/apache/impala/analysis/ToSqlTest.java b/fe/src/test/java/org/apache/impala/analysis/ToSqlTest.java index 8741e22..6ef4b04 100644 --- a/fe/src/test/java/org/apache/impala/analysis/ToSqlTest.java +++ b/fe/src/test/java/org/apache/impala/analysis/ToSqlTest.java @@ -1166,6 +1166,11 @@ public class ToSqlTest extends FrontendTestBase { + "rows between unbounded preceding and current row) from functional.alltypes", "SELECT sum(int_col) OVER (PARTITION BY id ORDER BY tinyint_col ASC ROWS " + "BETWEEN UNBOUNDED PRECEDING AND CURRENT ROW) FROM functional.alltypes"); + testToSql( + "select last_value(tinyint_col ignore nulls) over (order by tinyint_col) " + + "from functional.alltypesagg", + "SELECT last_value(tinyint_col IGNORE NULLS) OVER (ORDER BY tinyint_col ASC) " + + "FROM functional.alltypesagg"); } /** http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/9f2f0655/testdata/workloads/functional-query/queries/QueryTest/analytic-fns.test ---------------------------------------------------------------------- diff --git a/testdata/workloads/functional-query/queries/QueryTest/analytic-fns.test b/testdata/workloads/functional-query/queries/QueryTest/analytic-fns.test index 34b5196..0e2f6b0 100644 --- a/testdata/workloads/functional-query/queries/QueryTest/analytic-fns.test +++ b/testdata/workloads/functional-query/queries/QueryTest/analytic-fns.test @@ -1933,3 +1933,25 @@ STRING, STRING 'a\x00b','' 'a\x00b','NULL' ==== +---- QUERY +# Test for IMPALA-5657: Ignore nulls should be applied correctly on views. +# Without the fix for IMPALA-5657, the select * query on the view does not +# apply ignore nulls and returns a few NULLs in the query result. +create view if not exists imp5657_view as +select + last_value(tinyint_col ignore nulls) over (order by tinyint_col) +from functional.alltypesagg +where id < 5; +==== +---- QUERY +select * from imp5657_view; +---- TYPES +TINYINT +---- RESULTS +1 +2 +3 +4 +4 +4 +====
