From 0c8022b9bc683cfe4e31f20aa2fca0149d0def87 Mon Sep 17 00:00:00 2001 From: Matthew Jacobs Date: Mon, 2 Mar 2015 18:44:16 -0800 Subject: [PATCH] IMPALA-1559: FIRST_VALUE rewrite fn type might not match slot type In some cases, the lookup for the first_value_rewrite function could return a different fn with the wrong (yet technically compatible) type. first_value_rewrite takes 2 parameters, the first is the parameter to first_value, and the second is an integer explicitly added by the FE during analysis. first_value_rewrite has signatures where the first parameter can be of any type and the second is always a BIGINT. When the FE inserts the second parameter, it may not actually be a BIGINT (e.g. it could be a TINYINT, SMALLINT, etc.). In this case, the function arguments will not match one of the registered signatures exactly and some compatable function will be returned. For example, FIRST_VALUE(1.1) has a DECIMAL parameter and if a TINYINT/SMALLINT/INT parameter is added for the rewrite, then the first_value_rewrite fn lookup happened to match the fn taking a FLOAT and BIGINT. (Ideally DECIMAL would not be implicitly castable to a FLOAT/DOUBLE, but NumericLiterals do allow this casting.) As a result, the agg fn actually returned a FLOAT while the AnalyticExpr was of the type DECIMAL, and the analytic tuple contained a DECIMAL slot which would throw a DCHECK in the BE (or perhaps crash in retail builds). This fixes the issue by setting the NumericLiterals to be explicitlyCast, i.e. the type will not change if re-analyzed. Then the correct fn signature is found. Change-Id: I1cefa3e29734ae647bd690263bb63f08f10ea8b9 Reviewed-on: http://gerrit.cloudera.org:8080/136 Reviewed-by: Alex Behm Tested-by: Internal Jenkins --- .../impala/analysis/AnalyticExpr.java | 5 ++++- .../impala/analysis/AnalyticWindow.java | 1 + .../impala/analysis/NumericLiteral.java | 5 ++++- .../queries/QueryTest/analytic-fns.test | 22 +++++++++++++++++++ 4 files changed, 31 insertions(+), 2 deletions(-) diff --git a/fe/src/main/java/com/cloudera/impala/analysis/AnalyticExpr.java b/fe/src/main/java/com/cloudera/impala/analysis/AnalyticExpr.java index 53ceeb678..65e8f9977 100644 --- a/fe/src/main/java/com/cloudera/impala/analysis/AnalyticExpr.java +++ b/fe/src/main/java/com/cloudera/impala/analysis/AnalyticExpr.java @@ -474,13 +474,15 @@ public class AnalyticExpr extends Expr { if (window_.getRightBoundary().getType() == BoundaryType.PRECEDING) { // The number of rows preceding for the end bound determines the number of // rows at the beginning of each partition that should have a NULL value. - paramExprs.add(window_.getRightBoundary().getExpr()); + paramExprs.add(new NumericLiteral(window_.getRightBoundary().getOffsetValue(), + Type.BIGINT)); } else { // -1 indicates that no NULL values are inserted even though we set the end // bound to the start bound (which is PRECEDING) below; this is different from // the default behavior of windows with an end bound PRECEDING. paramExprs.add(new NumericLiteral(BigInteger.valueOf(-1), Type.BIGINT)); } + window_ = new AnalyticWindow(window_.getType(), new Boundary(BoundaryType.UNBOUNDED_PRECEDING, null), window_.getLeftBoundary()); @@ -490,6 +492,7 @@ public class AnalyticExpr extends Expr { } fnCall_.setIsAnalyticFnCall(true); fnCall_.analyzeNoThrow(analyzer); + type_ = fnCall_.getReturnType(); analyticFnName = getFnCall().getFnName(); } diff --git a/fe/src/main/java/com/cloudera/impala/analysis/AnalyticWindow.java b/fe/src/main/java/com/cloudera/impala/analysis/AnalyticWindow.java index d7463bbb0..af578cca6 100644 --- a/fe/src/main/java/com/cloudera/impala/analysis/AnalyticWindow.java +++ b/fe/src/main/java/com/cloudera/impala/analysis/AnalyticWindow.java @@ -121,6 +121,7 @@ public class AnalyticWindow { public BoundaryType getType() { return type_; } public Expr getExpr() { return expr_; } + public BigDecimal getOffsetValue() { return offsetValue_; } public Boundary(BoundaryType type, Expr e) { this(type, e, null); diff --git a/fe/src/main/java/com/cloudera/impala/analysis/NumericLiteral.java b/fe/src/main/java/com/cloudera/impala/analysis/NumericLiteral.java index 771a06dfe..296668c1d 100644 --- a/fe/src/main/java/com/cloudera/impala/analysis/NumericLiteral.java +++ b/fe/src/main/java/com/cloudera/impala/analysis/NumericLiteral.java @@ -78,18 +78,21 @@ public class NumericLiteral extends LiteralExpr { /** * The versions of the ctor that take types assume the type is correct - * and the NumericLiteral is created as analyzed with that type. + * and the NumericLiteral is created as analyzed with that type. The specified + * type is preserved across substitutions and re-analysis. */ public NumericLiteral(BigInteger value, Type type) { isAnalyzed_ = true; value_ = new BigDecimal(value); type_ = type; + explicitlyCast_ = true; } public NumericLiteral(BigDecimal value, Type type) { isAnalyzed_ = true; value_ = value; type_ = type; + explicitlyCast_ = true; } /** diff --git a/testdata/workloads/functional-query/queries/QueryTest/analytic-fns.test b/testdata/workloads/functional-query/queries/QueryTest/analytic-fns.test index 8f6d9c147..971a19be7 100644 --- a/testdata/workloads/functional-query/queries/QueryTest/analytic-fns.test +++ b/testdata/workloads/functional-query/queries/QueryTest/analytic-fns.test @@ -1079,3 +1079,25 @@ select max(t3.c1) from ---- TYPES STRING ==== +---- QUERY +# IMPALA-1559: FIRST_VALUE rewrite function intermediate type not matching slot type +select +first_value(c3) over (order by c3 rows between 92 preceding and current row), +first_value(c2) over (order by c3 rows between 92 preceding and 1 preceding), +first_value(-32.9) over (order by c3 rows between 92 preceding and unbounded following), +first_value(1.1) over (order by c3 rows between 92 preceding and 1 preceding) +from decimal_tiny where c3 = 0.0 +---- RESULTS +0.0,NULL,-32.9,NULL +0.0,100.00000,-32.9,1.1 +0.0,100.00000,-32.9,1.1 +0.0,100.00000,-32.9,1.1 +0.0,100.00000,-32.9,1.1 +0.0,100.00000,-32.9,1.1 +0.0,100.00000,-32.9,1.1 +0.0,100.00000,-32.9,1.1 +0.0,100.00000,-32.9,1.1 +0.0,100.00000,-32.9,1.1 +---- TYPES +DECIMAL, DECIMAL, DECIMAL, DECIMAL +====