diff --git a/fe/src/main/java/com/cloudera/impala/analysis/CastExpr.java b/fe/src/main/java/com/cloudera/impala/analysis/CastExpr.java index d5906416a..c7ed3b004 100644 --- a/fe/src/main/java/com/cloudera/impala/analysis/CastExpr.java +++ b/fe/src/main/java/com/cloudera/impala/analysis/CastExpr.java @@ -126,6 +126,15 @@ public class CastExpr extends Expr { analyze(); } + @Override + public void unsetIsAnalyzed() { + // We need to reset noOp_ which is set in analyze(). This can be because + // this expr needs to be reanalyzed with the children replaced. + // TODO: this seems brittle. Revisit. + noOp_ = false; + super.unsetIsAnalyzed(); + } + private void analyze() throws AnalysisException { targetType_.analyze(); diff --git a/fe/src/main/java/com/cloudera/impala/analysis/DecimalLiteral.java b/fe/src/main/java/com/cloudera/impala/analysis/DecimalLiteral.java index 51daabbbf..6326a0af4 100644 --- a/fe/src/main/java/com/cloudera/impala/analysis/DecimalLiteral.java +++ b/fe/src/main/java/com/cloudera/impala/analysis/DecimalLiteral.java @@ -33,6 +33,15 @@ import com.google.common.base.Preconditions; * parsed to this class. (e.g. "1.0") */ public class DecimalLiteral extends LiteralExpr { + // Use the java BigDecimal (arbitrary scale/precision) to represent the value. + // This object has notions of precision and scale but they do *not* match what + // we need. BigDecima's precision is similar to significant figures and scale + // is the exponent. + // ".1" could be represented with an unscaled value = 1 and scale = 1 or + // unscaled value = 100 and scale = 3. Manipulating the value_ (e.g. multiplying + // it by 10) does not unnecessarily change the unscaled value. Special care + // needs to be taken when converting between the big decimals unscaled value + // and ours. (See getUnscaledValue()). private BigDecimal value_; // If true, this literal has been expicitly cast to a type and should not @@ -43,10 +52,6 @@ public class DecimalLiteral extends LiteralExpr { init(value); } - public DecimalLiteral(BigInteger value) { - init(new BigDecimal(value)); - } - public DecimalLiteral(String value, ColumnType t) throws AnalysisException, AuthorizationException { BigDecimal val = null; @@ -109,7 +114,7 @@ public class DecimalLiteral extends LiteralExpr { case DECIMAL: msg.node_type = TExprNodeType.DECIMAL_LITERAL; TDecimalLiteral literal = new TDecimalLiteral(); - literal.setValue(value_.unscaledValue().toByteArray()); + literal.setValue(getUnscaledValue().toByteArray()); msg.decimal_literal = literal; break; default: @@ -194,4 +199,16 @@ public class DecimalLiteral extends LiteralExpr { isAnalyzed_ = false; value_ = value; } + + // Returns the unscaled value of this literal. BigDecimal doesn't treat scale + // the way we do. We need to pad it out with zeros or truncate as necessary. + private BigInteger getUnscaledValue() { + Preconditions.checkState(type_.isDecimal()); + BigInteger result = value_.unscaledValue(); + int valueScale = value_.scale(); + // If valueScale is less than 0, it indicates the power of 10 to multiply the + // unscaled value. This path also handles this case by padding with zeros. + // e.g. unscaled value = 123, value scale = -2 means 12300. + return result.multiply(BigInteger.TEN.pow(type_.decimalScale() - valueScale)); + } } diff --git a/fe/src/main/java/com/cloudera/impala/analysis/IntLiteral.java b/fe/src/main/java/com/cloudera/impala/analysis/IntLiteral.java index 6034e30ac..a12b6d651 100644 --- a/fe/src/main/java/com/cloudera/impala/analysis/IntLiteral.java +++ b/fe/src/main/java/com/cloudera/impala/analysis/IntLiteral.java @@ -115,13 +115,7 @@ public class IntLiteral extends LiteralExpr { this.type_ = targetType; return this; } else if (targetType.isFloatingPointType() || targetType.isDecimal()) { - BigInteger val = value_; - if (targetType.isDecimal()) { - for (int i = 0; i < targetType.decimalScale(); ++i) { - val = val.multiply(BigInteger.TEN); - } - } - return new DecimalLiteral(val, targetType); + return new DecimalLiteral(value_, targetType); } Preconditions.checkState(false, "Unhandled case"); return this; diff --git a/fe/src/main/java/com/cloudera/impala/analysis/TypesUtil.java b/fe/src/main/java/com/cloudera/impala/analysis/TypesUtil.java index fda31d4ed..a1ab71123 100644 --- a/fe/src/main/java/com/cloudera/impala/analysis/TypesUtil.java +++ b/fe/src/main/java/com/cloudera/impala/analysis/TypesUtil.java @@ -198,8 +198,8 @@ public class TypesUtil { ++digitsBefore; } } - if (digitsAfter >= ColumnType.MAX_SCALE) return null; - if (digitsBefore + digitsAfter >= ColumnType.MAX_PRECISION) return null; + if (digitsAfter > ColumnType.MAX_SCALE) return null; + if (digitsBefore + digitsAfter > ColumnType.MAX_PRECISION) return null; if (digitsBefore == 0 && digitsAfter == 0) digitsBefore = 1; return ColumnType.createDecimalType(digitsBefore + digitsAfter, digitsAfter); } diff --git a/fe/src/main/java/com/cloudera/impala/service/FeSupport.java b/fe/src/main/java/com/cloudera/impala/service/FeSupport.java index cc78c0ba1..3eb8bdc30 100644 --- a/fe/src/main/java/com/cloudera/impala/service/FeSupport.java +++ b/fe/src/main/java/com/cloudera/impala/service/FeSupport.java @@ -33,7 +33,6 @@ import com.cloudera.impala.thrift.TCacheJarResult; import com.cloudera.impala.thrift.TCatalogObject; import com.cloudera.impala.thrift.TCatalogObjectType; import com.cloudera.impala.thrift.TColumnValue; -import com.cloudera.impala.thrift.TExpr; import com.cloudera.impala.thrift.TExprBatch; import com.cloudera.impala.thrift.TPrioritizeLoadRequest; import com.cloudera.impala.thrift.TPrioritizeLoadResponse; @@ -189,7 +188,6 @@ public class FeSupport { */ public static TResultRow EvalPredicateBatch(ArrayList exprs, TQueryContext queryCtxt) throws InternalException { - boolean[] results = new boolean[exprs.size()]; TSerializer serializer = new TSerializer(new TBinaryProtocol.Factory()); TExprBatch exprBatch = new TExprBatch(); for (Expr expr: exprs) { 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 4f07fddb0..2eeea8024 100644 --- a/fe/src/test/java/com/cloudera/impala/analysis/AnalyzeExprsTest.java +++ b/fe/src/test/java/com/cloudera/impala/analysis/AnalyzeExprsTest.java @@ -89,6 +89,20 @@ public class AnalyzeExprsTest extends AnalyzerTest { "Decimal literal '1.7976931348623157E+3081' exceeds maximum range of doubles."); AnalysisError(String.format("select %s1", Double.toString(Double.MIN_VALUE)), "Decimal literal '4.9E-3241' underflows minimum resolution of doubles."); + + testNumericLiteral("0.99999999999999999999999999999999999999", + ColumnType.createDecimalType(38,38)); + testNumericLiteral("99999999999999999999999999999999999999.", + ColumnType.createDecimalType(38,0)); + testNumericLiteral("-0.99999999999999999999999999999999999999", + ColumnType.createDecimalType(38,38)); + testNumericLiteral("-99999999999999999999999999999999999999.", + ColumnType.createDecimalType(38,0)); + testNumericLiteral("999999999999999999999.99999999999999999", + ColumnType.createDecimalType(38,17)); + testNumericLiteral("-999999999999999999.99999999999999999999", + ColumnType.createDecimalType(38,20)); + } /** @@ -97,7 +111,9 @@ public class AnalyzeExprsTest extends AnalyzerTest { */ private void testNumericLiteral(String literal, ColumnType expectedType) { SelectStmt selectStmt = (SelectStmt) AnalyzesOk("select " + literal); - Assert.assertTrue(expectedType.equals(selectStmt.resultExprs_.get(0).getType())); + ColumnType actualType = selectStmt.resultExprs_.get(0).getType(); + Assert.assertTrue("Expected Type: " + expectedType + " Actual type: " + actualType, + expectedType.equals(actualType)); } @Test diff --git a/fe/src/test/java/com/cloudera/impala/analysis/AnalyzeStmtsTest.java b/fe/src/test/java/com/cloudera/impala/analysis/AnalyzeStmtsTest.java index 383442c7f..dc39892fe 100644 --- a/fe/src/test/java/com/cloudera/impala/analysis/AnalyzeStmtsTest.java +++ b/fe/src/test/java/com/cloudera/impala/analysis/AnalyzeStmtsTest.java @@ -269,6 +269,14 @@ public class AnalyzeStmtsTest extends AnalyzerTest { "from functional.alltypes) t", createAnalyzerUsingHiveColLabels(), "couldn't resolve column reference: '_c2'"); + + // Regression test for IMPALA-984. + AnalyzesOk("SELECT 1 " + + "FROM functional.decimal_tbl AS t1 LEFT JOIN " + + "(SELECT SUM(t1.d2) - SUM(t1.d3) as double_col_3, " + + "SUM(t1.d2) IS NULL " + + "FROM functional.decimal_tbl AS t1) AS t3 " + + "ON t3.double_col_3 = t1.d3"); } @Test diff --git a/testdata/workloads/functional-query/queries/QueryTest/values.test b/testdata/workloads/functional-query/queries/QueryTest/values.test index 39b57f29c..17b9dc7c7 100644 --- a/testdata/workloads/functional-query/queries/QueryTest/values.test +++ b/testdata/workloads/functional-query/queries/QueryTest/values.test @@ -30,3 +30,43 @@ values((1+8, 2, 5.0, 'a'), (2, 3, 6.0, 'b'), (3, 4, 7.0, 'c')) order by 1 desc l ---- TYPES SMALLINT, TINYINT, DECIMAL, STRING ==== +# Test literal casts by inserting into a table that requires a float. +---- QUERY +drop table if exists values_test_float_tbl; +create table values_test_float_tbl(f float); +insert into values_test_float_tbl values +(1), (16), (1024), (65536), (1000000), (1.1), (98.6), (0.07), (33.333); +select * from values_test_float_tbl; +---- RESULTS +1 +16 +1024 +65536 +1000000 +1.100000023841858 +98.59999847412109 +0.07 +33.33300018310547 +---- TYPES +float +==== +# Test literal casts by inserting into a table that requires a decimal. +---- QUERY +drop table if exists values_test_decimal_tbl; +create table values_test_decimal_tbl(f decimal(20, 4)); +insert into values_test_decimal_tbl values +(1), (16), (1024), (65536), (1000000), (1.1), (98.6), (0.07), (33.333); +select * from values_test_decimal_tbl; +---- RESULTS +1.0000 +16.0000 +1024.0000 +65536.0000 +1000000.0000 +1.1000 +98.6000 +0.0700 +33.3330 +---- TYPES +decimal +====