IMPALA-3439: Only convert decimal literals in convertNumericLiteralsFromDecimal().
Numeric literals with a decimal point are typed as DECIMAL, if possible. Since decimals have a higher processing cost than FLOAT/DOUBLE we have special casting rules to convert DECIMAL operations to DOUBLE in certain reasonable circumstances. The bug was that in this conversion process we used to blindly replace all numeric literals with DOUBLE. The blind conversion caused expressions to fail analysis after the substitution if they did not have a matching DOUBLE signature. For example, round() requies the second argument to be an integer, and does not have a signature with DOUBLE as the second argument. The fix is to only replace DECIMAL literals with DOUBLE. Based on the comments in the relevant parts of the code, this new behavior matches the original intent more cleanly/directly. Testing: I did a private core/hdfs run and local testing. Change-Id: I33f319b00a8fef3a81418d6c7ac030b002c5d0d1 Reviewed-on: http://gerrit.cloudera.org:8080/3138 Reviewed-by: Alex Behm <[email protected]> Tested-by: Internal 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/bd6b16f1 Tree: http://git-wip-us.apache.org/repos/asf/incubator-impala/tree/bd6b16f1 Diff: http://git-wip-us.apache.org/repos/asf/incubator-impala/diff/bd6b16f1 Branch: refs/heads/master Commit: bd6b16f132cd2e236c8339a7aa7d1a5aaa864881 Parents: afdc32e Author: Alex Behm <[email protected]> Authored: Wed May 18 23:43:08 2016 -0700 Committer: Tim Armstrong <[email protected]> Committed: Mon May 23 08:40:20 2016 -0700 ---------------------------------------------------------------------- fe/src/main/java/com/cloudera/impala/analysis/Expr.java | 11 +++++------ .../com/cloudera/impala/analysis/AnalyzeExprsTest.java | 5 +++++ 2 files changed, 10 insertions(+), 6 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/bd6b16f1/fe/src/main/java/com/cloudera/impala/analysis/Expr.java ---------------------------------------------------------------------- diff --git a/fe/src/main/java/com/cloudera/impala/analysis/Expr.java b/fe/src/main/java/com/cloudera/impala/analysis/Expr.java index b931e85..9a21d71 100644 --- a/fe/src/main/java/com/cloudera/impala/analysis/Expr.java +++ b/fe/src/main/java/com/cloudera/impala/analysis/Expr.java @@ -16,8 +16,6 @@ package com.cloudera.impala.analysis; import java.lang.reflect.Method; import java.util.ArrayList; -import java.util.Collections; -import java.util.Comparator; import java.util.Iterator; import java.util.List; import java.util.ListIterator; @@ -410,10 +408,10 @@ abstract public class Expr extends TreeNode<Expr> implements ParseNode, Cloneabl } /** - * Returns a clone of child with all NumericLiterals in it explicitly + * Returns a clone of child with all decimal-typed NumericLiterals in it explicitly * cast to targetType. */ - private Expr convertNumericLiteralsToFloat(Analyzer analyzer, Expr child, + private Expr convertDecimalLiteralsToFloat(Analyzer analyzer, Expr child, Type targetType) throws AnalysisException { if (!targetType.isFloatingPointType() && !targetType.isIntegerType()) return child; if (targetType.isIntegerType()) targetType = Type.DOUBLE; @@ -421,6 +419,7 @@ abstract public class Expr extends TreeNode<Expr> implements ParseNode, Cloneabl child.collectAll(Predicates.instanceOf(NumericLiteral.class), literals); ExprSubstitutionMap smap = new ExprSubstitutionMap(); for (NumericLiteral l: literals) { + if (!l.getType().isDecimal()) continue; NumericLiteral castLiteral = (NumericLiteral) l.clone(); castLiteral.explicitlyCastToFloat(targetType); smap.put(l, castLiteral); @@ -463,11 +462,11 @@ abstract public class Expr extends TreeNode<Expr> implements ParseNode, Cloneabl // Only child(0) or child(1) is a const decimal. See if we can cast it to // the type of the other child. if (c0IsConstantDecimal && !isExplicitCastToDecimal(getChild(0))) { - Expr c0 = convertNumericLiteralsToFloat(analyzer, getChild(0), t1); + Expr c0 = convertDecimalLiteralsToFloat(analyzer, getChild(0), t1); setChild(0, c0); } if (c1IsConstantDecimal && !isExplicitCastToDecimal(getChild(1))) { - Expr c1 = convertNumericLiteralsToFloat(analyzer, getChild(1), t0); + Expr c1 = convertDecimalLiteralsToFloat(analyzer, getChild(1), t0); setChild(1, c1); } } http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/bd6b16f1/fe/src/test/java/com/cloudera/impala/analysis/AnalyzeExprsTest.java ---------------------------------------------------------------------- diff --git a/fe/src/test/java/com/cloudera/impala/analysis/AnalyzeExprsTest.java b/fe/src/test/java/com/cloudera/impala/analysis/AnalyzeExprsTest.java index eb56c02..7305a68 100644 --- a/fe/src/test/java/com/cloudera/impala/analysis/AnalyzeExprsTest.java +++ b/fe/src/test/java/com/cloudera/impala/analysis/AnalyzeExprsTest.java @@ -1280,6 +1280,11 @@ public class AnalyzeExprsTest extends AnalyzerTest { checkReturnType("select int_col + 1.1 from functional.alltypestiny", Type.DOUBLE); + // IMPALA-3439: Non-trivial constant expression with decimal + double = double. + // Tests that only the decimal literals in the constant decimal expr are cast + // to double. The second argument of round() must be an integer. + checkReturnType("select round(1.2345, 2) * pow(10, 10)", Type.DOUBLE); + // Explicitly casting the literal to a decimal will override the behavior checkReturnType("select int_col + cast(1.1 as decimal(2,1)) from " + " functional.alltypestiny", ScalarType.createDecimalType(12,1));
