You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@impala.apache.org by he...@apache.org on 2017/02/14 21:53:10 UTC
[2/4] incubator-impala git commit: IMPALA-4370: Divide and modulo
result types for DECIMAL version V2
IMPALA-4370: Divide and modulo result types for DECIMAL version V2
Implement the new DECIMAL return type rules for divide and modulo
expressions, active when query option DECIMAL_V2=1. See the comment
in the code for more details. A couple of examples that show why new
return type rules for divide are desirable.
For modulo, the return types are actually equivalent, though the
rules are expressed differently to have consistency with how
precision fixups are handled for each version.
DECIMAL Version 1:
+-------------------------------------------------------+
| cast(1 as decimal(20,0)) / cast(3 as decimal(20,0)) |
+-----------------------------------------------------+
| 0 |
+-------------------------------------------------------+
DECIMAL Version 2:
+-------------------------------------------------------+
| cast(1 as decimal(20,0)) / cast(3 as decimal(20,0)) |
+-----------------------------------------------------+
| 0.333333333333333333 |
+-------------------------------------------------------+
DECIMAL Version 1:
+-------------------------------------------------------+
| cast(1 as decimal(6,0)) / cast(0.1 as decimal(38,38)) |
+-------------------------------------------------------+
| NULL |
+-------------------------------------------------------+
WARNINGS: UDF WARNING: Expression overflowed, returning NULL
DECIMAL Version 2:
+-------------------------------------------------------+
| cast(1 as decimal(6,0)) / cast(0.1 as decimal(38,38)) |
+-------------------------------------------------------+
| 10.000000 |
+-------------------------------------------------------+
Change-Id: I83e7f7787edfa4b4bddc25945090542a0e90881b
Reviewed-on: http://gerrit.cloudera.org:8080/5952
Reviewed-by: Dan Hecht <dh...@cloudera.com>
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/a53eeb20
Tree: http://git-wip-us.apache.org/repos/asf/incubator-impala/tree/a53eeb20
Diff: http://git-wip-us.apache.org/repos/asf/incubator-impala/diff/a53eeb20
Branch: refs/heads/master
Commit: a53eeb2068970ff7575f4f3fdf3c383861a75f2d
Parents: 065bbda
Author: Dan Hecht <dh...@cloudera.com>
Authored: Tue Feb 7 12:08:54 2017 -0800
Committer: Impala Public Jenkins <im...@gerrit.cloudera.org>
Committed: Tue Feb 14 18:40:54 2017 +0000
----------------------------------------------------------------------
be/src/exprs/expr-test.cc | 203 +++++++++++++------
be/src/runtime/decimal-value.inline.h | 8 +-
be/src/testutil/impalad-query-executor.h | 6 +-
.../apache/impala/analysis/ArithmeticExpr.java | 3 +-
.../impala/analysis/FunctionCallExpr.java | 2 +-
.../org/apache/impala/analysis/TypesUtil.java | 93 +++++++--
.../org/apache/impala/catalog/ScalarType.java | 54 ++++-
.../java/org/apache/impala/catalog/Type.java | 7 +-
.../queries/QueryTest/decimal-exprs.test | 73 +++++++
.../queries/QueryTest/decimal.test | 44 ----
tests/query_test/test_decimal_queries.py | 29 ++-
11 files changed, 374 insertions(+), 148 deletions(-)
----------------------------------------------------------------------
http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/a53eeb20/be/src/exprs/expr-test.cc
----------------------------------------------------------------------
diff --git a/be/src/exprs/expr-test.cc b/be/src/exprs/expr-test.cc
index 1b2dc51..52e74c5 100644
--- a/be/src/exprs/expr-test.cc
+++ b/be/src/exprs/expr-test.cc
@@ -1287,89 +1287,164 @@ struct DecimalExpectedResult {
};
struct DecimalTestCase {
+
+ // Return the expected result for the given DECIMAL version. When the expected V1 and
+ // V2 results are the same, allows the V2 expected result to be unspecified in the
+ // test case table. When V2 expected results aren't specified, V2 entries have both
+ // 'null' set to false and 'precision' set to 0, which is an illegal combination and
+ // signals that V1 results should be used instead.
+ const DecimalExpectedResult& Expected(bool v2) const {
+ if (v2 && (expected[1].null || expected[1].precision != 0)) return expected[1];
+ // Either testing version 1, or the results for version 2 were not specified.
+ return expected[0];
+ }
+
+ // Expression to execute.
string expr;
- DecimalExpectedResult expected[2]; // Version 1 and Version 2
+
+ // Expected results for version 1 and Version 2. Version 2 results can be left unset
+ // when the expected result is the same between versions.
+ DecimalExpectedResult expected[2];
};
// Format is:
// { Test Expression (as a string),
-// { Decimal V1 expected results, Decimal V2 expected results } }
+// { expected null, scaled_val, precision, scale for V1
+// expected null, scaled_val, precision, scale for V2 }}
DecimalTestCase decimal_cases[] = {
- { "1.23 + cast(1 as decimal(4,3))",
- {{ false, 2230, 5, 3 }, { false, 2230, 5, 3 }} },
- { "1.23 - cast(0.23 as decimal(10,3))",
- {{ false, 1000, 11, 3 }, { false, 1000, 11, 3 }} },
- { "1.23 * cast(1 as decimal(20,3))",
- {{ false, 123000, 23, 5 }, { false, 123000, 23, 5 }} },
- { "cast(1.23 as decimal(8,2)) / cast(1 as decimal(4,3))",
- {{ false, 12300000, 16, 7}, { false, 12300000, 16, 7}} },
- { "cast(1.23 as decimal(8,2)) % cast(1 as decimal(10,3))",
- {{ false, 230, 9, 3 }, { false, 230, 9, 3 }} },
- { "cast(1.23 as decimal(8,2)) + cast(1 as decimal(20,3))",
- {{ false, 2230, 21, 3 }, { false, 2230, 21, 3 }} },
- { "cast(1.23 as decimal(30,2)) - cast(1 as decimal(4,3))",
- {{ false, 230, 32, 3 }, { false, 230, 32, 3 }} },
- { "cast(1.23 as decimal(30,2)) * cast(1 as decimal(10,3))",
- {{ false, 123000, 38, 5 }, { false, 123000, 38, 5 }} },
+ // Test add/subtract operators
+ { "1.23 + cast(1 as decimal(4,3))", {{ false, 2230, 5, 3 }}},
+ { "1.23 - cast(0.23 as decimal(10,3))", {{ false, 1000, 11, 3 }}},
+ { "cast(1.23 as decimal(8,2)) + cast(1 as decimal(20,3))", {{ false, 2230, 21, 3 }}},
+ { "cast(1.23 as decimal(30,2)) - cast(1 as decimal(4,3))", {{ false, 230, 32, 3 }}},
+ { "cast(1 as decimal(38,0)) + cast(.2 as decimal(38,1))", {{ false, 12, 38, 1 }}},
+ // Test multiply operator
+ { "cast(1.23 as decimal(30,2)) * cast(1 as decimal(10,3))", {{ false, 123000, 38, 5 }}},
+ { "1.23 * cast(1 as decimal(20,3))", {{ false, 123000, 23, 5 }}},
+ // Test divide operator
+ { "cast(1.23 as decimal(8,2)) / cast(1 as decimal(4,3))", {{ false, 12300000, 16, 7}}},
{ "cast(1.23 as decimal(30,2)) / cast(1 as decimal(20,3))",
- {{ false, 1230, 38, 3 }, { false, 1230, 38, 3 }} },
- { "cast(1 as decimal(38,0)) + cast(.2 as decimal(38,1))",
- {{ false, 12, 38, 1 }, { false, 12, 38, 1 }} },
+ {{ false, 1230, 38, 3 },
+ { false, 12300000, 38, 7 }}},
{ "cast(1 as decimal(38,0)) / cast(.2 as decimal(38,1))",
- {{ false, 50, 38, 1 }, { false, 50, 38, 1 }} },
- { "mod(cast('1' as decimal(2,0)), cast('10' as decimal(2,0)))",
- {{ false, 1, 2, 0 }, { false, 1, 2, 0 }} },
- { "mod(cast('1.1' as decimal(2,1)), cast('1.0' as decimal(2,1)))",
- {{ false, 1, 2,1 }, { false, 1, 2,1 }} },
+ {{ false, 50, 38, 1 },
+ { false, 5000000, 38, 6 }}},
+ { "cast(1 as decimal(38,0)) / cast(3 as decimal(38,0))",
+ {{ false, 0, 38, 0 },
+ { false, 333333, 38, 6 }}},
+ { "cast(99999999999999999999999999999999999999 as decimal(38,0)) / "
+ "cast(99999999999999999999999999999999999999 as decimal(38,0))",
+ {{ false, 1, 38, 0 },
+ { false, 1000000, 38, 6 }}},
+ { "cast(99999999999999999999999999999999999999 as decimal(38,0)) / "
+ "cast(0.00000000000000000000000000000000000001 as decimal(38,38))",
+ {{ true, 0, 38, 38 },
+ { true, 0, 38, 6 }}},
+ { "cast(0.00000000000000000000000000000000000001 as decimal(38,38)) / "
+ "cast(99999999999999999999999999999999999999 as decimal(38,0))",
+ {{ false, 0, 38, 38 }}},
+ { "cast(0.00000000000000000000000000000000000001 as decimal(38,38)) / "
+ "cast(0.00000000000000000000000000000000000001 as decimal(38,38))",
+ {{ true, 0, 38, 38 },
+ { false, 1000000, 38, 6 }}},
+ { "cast(9999999999999999999.9999999999999999999 as decimal(38,19)) / "
+ "cast(99999999999999999999999999999.999999999 as decimal(38,9))",
+ {{ false, 1000000000, 38, 19 },
+ { false, 1, 38, 10 }}},
+ { "cast(999999999999999999999999999999999999.99 as decimal(38,2)) / "
+ "cast(99999999999.999999999999999999999999999 as decimal(38,27))",
+ {{ true, 0, 38, 27 },
+ { false, static_cast<int128_t>(10) * 10000000000ll *
+ 10000000000ll * 10000000000ll, 38, 6 }}},
+ { "cast(-2.12 as decimal(17,2)) / cast(12515.95 as decimal(17,2))",
+ {{ false, -16938386618674571, 37, 20 }}},
+ { "cast(-2.12 as decimal(18,2)) / cast(12515.95 as decimal(18,2))",
+ {{ false, 0, 38, 2 },
+ { false, -16938386618674571, 38, 20 }}},
+ { "cast(737373 as decimal(6,0)) / cast(.52525252 as decimal(38,38))",
+ {{ true, 0, 38, 38 },
+ { false, 1403844764038, 38, 6 }}},
+ { "cast(0.000001 as decimal(6,6)) / "
+ "cast(0.0000000000000000000000000000000000001 as decimal(38,38))",
+ {{ true, 0, 38, 38 },
+ { false, static_cast<int128_t>(10000000ll) *
+ 10000000000ll * 10000000000ll * 10000000000ll, 38, 6 }}},
+ { "cast(98765432109876543210 as decimal(20,0)) / "
+ "cast(98765432109876543211 as decimal(20,0))",
+ {{ false, 0, 38, 0 },
+ { false, 999999999999999999, 38, 18 }}},
+ { "cast(111111.1111 as decimal(20, 10)) / cast(.7777 as decimal(38, 38))",
+ {{ true, 0, 38, 38 },
+ { false, 142871429985, 38, 6 }}},
+ // Test modulo operator
+ { "cast(1.23 as decimal(8,2)) % cast(1 as decimal(10,3))", {{ false, 230, 9, 3 }}},
+ { "cast(1 as decimal(38,0)) % cast(.2 as decimal(38,1))", {{ false, 0, 38, 1 }}},
+ { "cast(1 as decimal(38,0)) % cast(3 as decimal(38,0))", {{ false, 1, 38, 0 }}},
+ { "cast(-2.12 as decimal(17,2)) % cast(12515.95 as decimal(17,2))",
+ {{ false, -212, 17, 2 }}},
+ { "cast(-2.12 as decimal(18,2)) % cast(12515.95 as decimal(18,2))",
+ {{ false, -212, 18, 2 }}},
+ { "cast(99999999999999999999999999999999999999 as decimal(38,0)) % "
+ "cast(99999999999999999999999999999999999999 as decimal(38,0))",
+ {{ false, 0, 38, 0 }}},
+ { "cast(998 as decimal(38,0)) % cast(0.999 as decimal(38,38))", {{ false, 0, 38, 38 }}},
+ { "cast(0.998 as decimal(38,38)) % cast(999 as decimal(38,0))", {{ false, 0, 38, 38 }}},
+ { "cast(0.00000000000000000000000000000000000001 as decimal(38,38)) % "
+ "cast(0.0000000000000000000000000000000000001 as decimal(38,38))",
+ {{ false, 1, 38, 38 }}},
+ // Test MOD builtin
+ { "mod(cast('1' as decimal(2,0)), cast('10' as decimal(2,0)))", {{ false, 1, 2, 0 }}},
+ { "mod(cast('1.1' as decimal(2,1)), cast('1.0' as decimal(2,1)))", {{ false, 1, 2, 1 }}},
{ "mod(cast('-1.23' as decimal(5,2)), cast('1.0' as decimal(5,2)))",
- {{ false, -23, 5,2 }, { false, -23, 5,2 }} },
- { "mod(cast('1' as decimal(12,0)), cast('10' as decimal(12,0)))",
- {{ false, 1, 12, 0 }, { false, 1, 12, 0 }} },
+ {{ false, -23, 5, 2 }}},
+ { "mod(cast('1' as decimal(12,0)), cast('10' as decimal(12,0)))", {{ false, 1, 12, 0 }}},
{ "mod(cast('1.1' as decimal(12,1)), cast('1.0' as decimal(12,1)))",
- {{ false, 1, 12, 1 }, { false, 1, 12, 1 }} },
+ {{ false, 1, 12, 1 }}},
{ "mod(cast('-1.23' as decimal(12,2)), cast('1.0' as decimal(12,2)))",
- {{ false, -23, 12, 2 }, { false, -23, 12, 2 }} },
+ {{ false, -23, 12, 2 }}},
{ "mod(cast('1' as decimal(32,0)), cast('10' as decimal(32,0)))",
- {{ false, 1, 32, 0 }, { false, 1, 32, 0 }} },
+ {{ false, 1, 32, 0 }}},
{ "mod(cast('1.1' as decimal(32,1)), cast('1.0' as decimal(32,1)))",
- {{ false, 1, 32, 1 }, { false, 1, 32, 1 }} },
+ {{ false, 1, 32, 1 }}},
{ "mod(cast('-1.23' as decimal(32,2)), cast('1.0' as decimal(32,2)))",
- {{ false, -23, 32, 2 }, { false, -23, 32, 2 }} },
+ {{ false, -23, 32, 2 }}},
+ { "mod(cast(NULL as decimal(2,0)), cast('10' as decimal(2,0)))", {{ true, 0, 2, 0 }}},
+ { "mod(cast('10' as decimal(2,0)), cast(NULL as decimal(2,0)))", {{ true, 0, 2, 0 }}},
+ { "mod(cast('10' as decimal(2,0)), cast('0' as decimal(2,0)))", {{ true, 0, 2, 0 }}},
+ { "mod(cast('10' as decimal(2,0)), cast('0' as decimal(2,0)))", {{ true, 0, 2, 0 }}},
+ { "mod(cast(NULL as decimal(2,0)), NULL)", {{ true, 0, 2, 0 }}},
+ // Test CAST DECIMAL -> DECIMAL
{ "cast(cast(0.12344 as decimal(6,5)) as decimal(6,4))",
- {{ false, 1234, 6, 4 }, { false, 1234, 6, 4 }} },
+ {{ false, 1234, 6, 4 }}},
{ "cast(cast(0.12345 as decimal(6,5)) as decimal(6,4))",
- {{ false, 1234, 6, 4 }, { false, 1235, 6, 4 }} },
+ {{ false, 1234, 6, 4 },
+ { false, 1235, 6, 4 }}},
{ "cast(cast('0.999' as decimal(4,3)) as decimal(1,0))",
- {{ false, 0, 1, 0 }, { false, 1, 1, 0 }} },
+ {{ false, 0, 1, 0 },
+ { false, 1, 1, 0 }}},
{ "cast(cast(999999999.99 as DECIMAL(11,2)) as DECIMAL(9,0))",
- {{ false, 999999999, 9, 0 }, { true, 0, 9, 0 }} },
+ {{ false, 999999999, 9, 0 },
+ { true, 0, 9, 0 }}},
{ "cast(cast(-999999999.99 as DECIMAL(11,2)) as DECIMAL(9,0))",
- {{ false, -999999999, 9, 0 }, { true, 0, 9, 0 }} },
- { "mod(cast(NULL as decimal(2,0)), cast('10' as decimal(2,0)))",
- {{ true, 0, 2, 0 }, { true, 0, 2, 0 }} },
- { "mod(cast('10' as decimal(2,0)), cast(NULL as decimal(2,0)))",
- {{ true, 0, 2, 0 }, { true, 0, 2, 0 }} },
- { "mod(cast('10' as decimal(2,0)), cast('0' as decimal(2,0)))",
- {{ true, 0, 2, 0 }, { true, 0, 2, 0 }} },
- { "mod(cast('10' as decimal(2,0)), cast('0' as decimal(2,0)))",
- {{ true, 0, 2, 0 }, { true, 0, 2, 0 }} },
- { "mod(cast(NULL as decimal(2,0)), NULL)",
- {{ true, 0, 2, 0 }, { true, 0, 2, 0 }} },
+ {{ false, -999999999, 9, 0 },
+ { true, 0, 9, 0 }}},
// IMPALA-2233: Test that implicit casts do not lose precision.
// The overload greatest(decimal(*,*)) is available and should be used.
{ "greatest(0, cast('99999.1111' as decimal(30,10)))",
- {{ false, 999991111000000, 30, 10 }, { false, 999991111000000, 30, 10 }} }
+ {{ false, 999991111000000, 30, 10 },
+ { false, 999991111000000, 30, 10 }}}
};
TEST_F(ExprTest, DecimalArithmeticExprs) {
// Test with both decimal_v2={false, true}
- for (int v2 = 0; v2 <= 1; ++v2) {
+ for (int v2: { 0, 1 }) {
string opt = "DECIMAL_V2=" + lexical_cast<string>(v2);
- executor_->pushExecOption(opt);
+ executor_->PushExecOption(opt);
for (const DecimalTestCase& c : decimal_cases) {
- const DecimalExpectedResult& r = c.expected[v2];
+ const DecimalExpectedResult& r = c.Expected(v2);
const ColumnType& type = ColumnType::CreateDecimalType(r.precision, r.scale);
if (r.null) {
+ TestDecimalResultType(c.expr, type);
TestIsNull(c.expr, type);
} else {
switch (type.GetByteSize()) {
@@ -1387,7 +1462,7 @@ TEST_F(ExprTest, DecimalArithmeticExprs) {
}
}
}
- executor_->popExecOption();
+ executor_->PopExecOption();
}
}
@@ -6252,19 +6327,19 @@ int main(int argc, char **argv) {
int ret;
disable_codegen_ = true;
enable_expr_rewrites_ = false;
- executor_->clearExecOptions();
- executor_->pushExecOption("ENABLE_EXPR_REWRITES=0");
- executor_->pushExecOption("DISABLE_CODEGEN=1");
+ executor_->ClearExecOptions();
+ executor_->PushExecOption("ENABLE_EXPR_REWRITES=0");
+ executor_->PushExecOption("DISABLE_CODEGEN=1");
cout << "Running without codegen" << endl;
ret = RUN_ALL_TESTS();
if (ret != 0) return ret;
disable_codegen_ = false;
enable_expr_rewrites_ = false;
- executor_->clearExecOptions();
- executor_->pushExecOption("ENABLE_EXPR_REWRITES=0");
- executor_->pushExecOption("DISABLE_CODEGEN=0");
- executor_->pushExecOption("EXEC_SINGLE_NODE_ROWS_THRESHOLD=0");
+ executor_->ClearExecOptions();
+ executor_->PushExecOption("ENABLE_EXPR_REWRITES=0");
+ executor_->PushExecOption("DISABLE_CODEGEN=0");
+ executor_->PushExecOption("EXEC_SINGLE_NODE_ROWS_THRESHOLD=0");
cout << endl << "Running with codegen" << endl;
ret = RUN_ALL_TESTS();
if (ret != 0) return ret;
@@ -6272,9 +6347,9 @@ int main(int argc, char **argv) {
// Enable FE expr rewrites to get test for constant folding over all exprs.
disable_codegen_ = true;
enable_expr_rewrites_ = true;
- executor_->clearExecOptions();
- executor_->pushExecOption("ENABLE_EXPR_REWRITES=1");
- executor_->pushExecOption("DISABLE_CODEGEN=1");
+ executor_->ClearExecOptions();
+ executor_->PushExecOption("ENABLE_EXPR_REWRITES=1");
+ executor_->PushExecOption("DISABLE_CODEGEN=1");
cout << endl << "Running without codegen and expr rewrites" << endl;
return RUN_ALL_TESTS();
}
http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/a53eeb20/be/src/runtime/decimal-value.inline.h
----------------------------------------------------------------------
diff --git a/be/src/runtime/decimal-value.inline.h b/be/src/runtime/decimal-value.inline.h
index 3a65a16..e7e89aa 100644
--- a/be/src/runtime/decimal-value.inline.h
+++ b/be/src/runtime/decimal-value.inline.h
@@ -228,15 +228,15 @@ template<typename RESULT_T>
inline DecimalValue<RESULT_T> DecimalValue<T>::Divide(int this_scale,
const DecimalValue& other, int other_scale, int result_precision, int result_scale,
bool* is_nan, bool* overflow) const {
- DCHECK_GE(result_scale, this_scale);
+ DCHECK_GE(result_scale + other_scale, this_scale);
if (other.value() == 0) {
// Divide by 0.
*is_nan = true;
return DecimalValue<RESULT_T>();
}
- // We need to scale x up by the result precision and then do an integer divide.
- // This truncates the result to the output precision.
- // TODO: confirm with standard that truncate is okay.
+ // We need to scale x up by the result scale and then do an integer divide.
+ // This truncates the result to the output scale.
+ // TODO: implement rounding when decimal_v2=true.
int scale_by = result_scale + other_scale - this_scale;
// Use higher precision ints for intermediates to avoid overflows. Divides lead to
// large numbers very quickly (and get eliminated by the int divide).
http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/a53eeb20/be/src/testutil/impalad-query-executor.h
----------------------------------------------------------------------
diff --git a/be/src/testutil/impalad-query-executor.h b/be/src/testutil/impalad-query-executor.h
index 89bed7e..c50ac33 100644
--- a/be/src/testutil/impalad-query-executor.h
+++ b/be/src/testutil/impalad-query-executor.h
@@ -84,15 +84,15 @@ class ImpaladQueryExecutor {
bool eos() { return eos_; }
/// Add a query option, preserving the existing set of query options.
- void pushExecOption(const std::string& exec_option) {
+ void PushExecOption(const std::string& exec_option) {
exec_options_.push_back(exec_option);
}
/// Remove the last query option that was added by pushExecOption().
- void popExecOption() { exec_options_.pop_back(); }
+ void PopExecOption() { exec_options_.pop_back(); }
/// Remove all query options previously added by pushExecOption().
- void clearExecOptions() { exec_options_.clear(); }
+ void ClearExecOptions() { exec_options_.clear(); }
private:
/// fe service-related
http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/a53eeb20/fe/src/main/java/org/apache/impala/analysis/ArithmeticExpr.java
----------------------------------------------------------------------
diff --git a/fe/src/main/java/org/apache/impala/analysis/ArithmeticExpr.java b/fe/src/main/java/org/apache/impala/analysis/ArithmeticExpr.java
index 3e574b2..763a38d 100644
--- a/fe/src/main/java/org/apache/impala/analysis/ArithmeticExpr.java
+++ b/fe/src/main/java/org/apache/impala/analysis/ArithmeticExpr.java
@@ -199,7 +199,8 @@ public class ArithmeticExpr extends Expr {
case DIVIDE:
case MULTIPLY:
case MOD:
- type_ = TypesUtil.getArithmeticResultType(t0, t1, op_);
+ type_ = TypesUtil.getArithmeticResultType(t0, t1, op_,
+ analyzer.getQueryOptions().isDecimal_v2());
// If both of the children are null, we'll default to the DOUBLE version of the
// operator. This prevents the BE from seeing NULL_TYPE.
if (type_.isNull()) type_ = Type.DOUBLE;
http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/a53eeb20/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 4d7dca8..482a31e 100644
--- a/fe/src/main/java/org/apache/impala/analysis/FunctionCallExpr.java
+++ b/fe/src/main/java/org/apache/impala/analysis/FunctionCallExpr.java
@@ -386,7 +386,7 @@ public class FunctionCallExpr extends Expr {
}
}
Preconditions.checkState(returnType.isDecimal() && !returnType.isWildcardDecimal());
- return ScalarType.createDecimalTypeInternal(digitsBefore + digitsAfter, digitsAfter);
+ return ScalarType.createClippedDecimalType(digitsBefore + digitsAfter, digitsAfter);
}
@Override
http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/a53eeb20/fe/src/main/java/org/apache/impala/analysis/TypesUtil.java
----------------------------------------------------------------------
diff --git a/fe/src/main/java/org/apache/impala/analysis/TypesUtil.java b/fe/src/main/java/org/apache/impala/analysis/TypesUtil.java
index 4a61f38..b29fc05 100644
--- a/fe/src/main/java/org/apache/impala/analysis/TypesUtil.java
+++ b/fe/src/main/java/org/apache/impala/analysis/TypesUtil.java
@@ -87,8 +87,7 @@ public class TypesUtil {
int p2 = t2.decimalPrecision();
int digitsBefore = Math.max(p1 - s1, p2 - s2);
int digitsAfter = Math.max(s1, s2);
- return ScalarType.createDecimalTypeInternal(
- digitsBefore + digitsAfter, digitsAfter);
+ return ScalarType.createClippedDecimalType(digitsBefore + digitsAfter, digitsAfter);
}
/**
@@ -96,7 +95,7 @@ public class TypesUtil {
* if the operation does not make sense for the types.
*/
public static Type getArithmeticResultType(Type t1, Type t2,
- ArithmeticExpr.Operator op) throws AnalysisException {
+ ArithmeticExpr.Operator op, boolean decimal_v2) throws AnalysisException {
Preconditions.checkState(t1.isNumericType() || t1.isNull());
Preconditions.checkState(t2.isNumericType() || t2.isNull());
@@ -117,7 +116,7 @@ public class TypesUtil {
t2 = ((ScalarType) t2).getMinResolutionDecimal();
Preconditions.checkState(t1.isDecimal());
Preconditions.checkState(t2.isDecimal());
- return getDecimalArithmeticResultType(t1, t2, op);
+ return getDecimalArithmeticResultType(t1, t2, op, decimal_v2);
}
Type type = null;
@@ -147,15 +146,36 @@ public class TypesUtil {
}
/**
- * Returns the resulting typical type from (t1 op t2)
- * These rules are mostly taken from the hive/sql server rules with some changes.
+ * Returns the result type for (t1 op t2) where t1 and t2 are both DECIMAL, depending
+ * on whether DECIMAL version 1 or DECIMAL version 2 is enabled.
+ *
+ * TODO: IMPALA-4924: remove DECIMAL V1 code.
+ */
+ private static ScalarType getDecimalArithmeticResultType(Type t1, Type t2,
+ ArithmeticExpr.Operator op, boolean decimal_v2) throws AnalysisException {
+ if (decimal_v2) return getDecimalArithmeticResultTypeV2(t1, t2, op);
+ return getDecimalArithmeticResultTypeV1(t1, t2, op);
+ }
+
+ /**
+ * Returns the result type for (t1 op t2) where t1 and t2 are both DECIMAL, used when
+ * DECIMAL version 1 is enabled.
+ *
+ * These rules were mostly taken from the (pre Dec 2016) Hive / sql server rules with
+ * some changes.
* http://blogs.msdn.com/b/sqlprogrammability/archive/2006/03/29/564110.aspx
+ * https://msdn.microsoft.com/en-us/library/ms190476.aspx
*
* Changes:
* - Multiply does not need +1 for the result precision.
- * - Divide scale truncation is different.
+ * - Divide scale truncation is different. When the maximum precision is exceeded,
+ * unlike SQL server, we do not try to preserve at least a scale of 6.
+ * - For other operators, when maximum precision is exceeded, we clip the precision
+ * and scale at maximum precision rather tha reducing scale. Therefore, we
+ * sacrifice digits to the left of the decimal point before sacrificing digits to
+ * the right.
*/
- public static ScalarType getDecimalArithmeticResultType(Type t1, Type t2,
+ private static ScalarType getDecimalArithmeticResultTypeV1(Type t1, Type t2,
ArithmeticExpr.Operator op) throws AnalysisException {
Preconditions.checkState(t1.isFullySpecifiedDecimal());
Preconditions.checkState(t2.isFullySpecifiedDecimal());
@@ -170,10 +190,10 @@ public class TypesUtil {
switch (op) {
case ADD:
case SUBTRACT:
- return ScalarType.createDecimalTypeInternal(
+ return ScalarType.createClippedDecimalType(
sMax + Math.max(p1 - s1, p2 - s2) + 1, sMax);
case MULTIPLY:
- return ScalarType.createDecimalTypeInternal(p1 + p2, s1 + s2);
+ return ScalarType.createClippedDecimalType(p1 + p2, s1 + s2);
case DIVIDE:
int resultScale = Math.max(DECIMAL_DIVISION_SCALE_INCREMENT, s1 + p2 + 1);
int resultPrecision = p1 - s1 + s2 + resultScale;
@@ -187,9 +207,9 @@ public class TypesUtil {
resultScale = Math.max(s1, s2);
resultPrecision = ScalarType.MAX_PRECISION;
}
- return ScalarType.createDecimalTypeInternal(resultPrecision, resultScale);
+ return ScalarType.createClippedDecimalType(resultPrecision, resultScale);
case MOD:
- return ScalarType.createDecimalTypeInternal(
+ return ScalarType.createClippedDecimalType(
Math.min(p1 - s1, p2 - s2) + sMax, sMax);
default:
throw new AnalysisException(
@@ -198,6 +218,55 @@ public class TypesUtil {
}
/**
+ * Returns the result type for (t1 op t2) where t1 and t2 are both DECIMAL, used when
+ * DECIMAL version 2 is enabled.
+ *
+ * These rules are similar to (post Dec 2016) Hive / sql server rules.
+ * http://blogs.msdn.com/b/sqlprogrammability/archive/2006/03/29/564110.aspx
+ * https://msdn.microsoft.com/en-us/library/ms190476.aspx
+ *
+ * TODO: implement V2 rules for ADD/SUB/MULTIPLY.
+ *
+ * Changes:
+ * - There are slight difference with how precision/scale reduction occurs compared
+ * to SQL server when the desired precision is more than the maximum supported
+ * precision. But an algorithm of reducing scale to a minimum of 6 is used.
+ */
+ private static ScalarType getDecimalArithmeticResultTypeV2(Type t1, Type t2,
+ ArithmeticExpr.Operator op) throws AnalysisException {
+ Preconditions.checkState(t1.isFullySpecifiedDecimal());
+ Preconditions.checkState(t2.isFullySpecifiedDecimal());
+ ScalarType st1 = (ScalarType) t1;
+ ScalarType st2 = (ScalarType) t2;
+ int s1 = st1.decimalScale();
+ int s2 = st2.decimalScale();
+ int p1 = st1.decimalPrecision();
+ int p2 = st2.decimalPrecision();
+ int resultScale;
+ int resultPrecision;
+
+ switch (op) {
+ case DIVIDE:
+ // Divide result always gets at least MIN_ADJUSTED_SCALE decimal places.
+ resultScale = Math.max(ScalarType.MIN_ADJUSTED_SCALE, s1 + p2 + 1);
+ resultPrecision = p1 - s1 + s2 + resultScale;
+ break;
+ case MOD:
+ resultScale = Math.max(s1, s2);
+ resultPrecision = Math.min(p1 - s1, p2 - s2) + resultScale;
+ break;
+ case ADD:
+ case SUBTRACT:
+ case MULTIPLY:
+ default:
+ // Not yet implemented - fall back to V1 rules.
+ return getDecimalArithmeticResultTypeV1(t1, t2, op);
+ }
+ // Use the scale reduction technique when resultPrecision is too large.
+ return ScalarType.createAdjustedDecimalType(resultPrecision, resultScale);
+ }
+
+ /**
* Computes the ColumnType that can represent 'v' with no loss of resolution.
* The scale/precision in BigDecimal is not compatible with SQL decimal semantics
* (much more like significant figures and exponent).
http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/a53eeb20/fe/src/main/java/org/apache/impala/catalog/ScalarType.java
----------------------------------------------------------------------
diff --git a/fe/src/main/java/org/apache/impala/catalog/ScalarType.java b/fe/src/main/java/org/apache/impala/catalog/ScalarType.java
index 052fd24..297f90e 100644
--- a/fe/src/main/java/org/apache/impala/catalog/ScalarType.java
+++ b/fe/src/main/java/org/apache/impala/catalog/ScalarType.java
@@ -66,6 +66,7 @@ public class ScalarType extends Type {
// Hive, mysql, sql server standard.
public static final int MAX_PRECISION = 38;
public static final int MAX_SCALE = MAX_PRECISION;
+ public static final int MIN_ADJUSTED_SCALE = 6;
protected ScalarType(PrimitiveType type) {
type_ = type;
@@ -107,6 +108,9 @@ public class ScalarType extends Type {
return createDecimalType(precision, DEFAULT_SCALE);
}
+ /**
+ * Returns a DECIMAL type with the specified precision and scale.
+ */
public static ScalarType createDecimalType(int precision, int scale) {
Preconditions.checkState(precision >= 0); // Enforced by parser
Preconditions.checkState(scale >= 0); // Enforced by parser.
@@ -116,16 +120,50 @@ public class ScalarType extends Type {
return type;
}
- // Identical to createDecimalType except that higher precisions are truncated
- // to the max storable precision. The BE will report overflow in these cases
- // (think of this as adding ints to BIGINT but BIGINT can still overflow).
- public static ScalarType createDecimalTypeInternal(int precision, int scale) {
+ /**
+ * Returns a DECIMAL wildcard type (i.e. precision and scale hasn't yet been resolved).
+ */
+ public static ScalarType createWildCardDecimalType() {
+ ScalarType type = new ScalarType(PrimitiveType.DECIMAL);
+ type.precision_ = -1;
+ type.scale_ = -1;
+ return type;
+ }
+
+ /**
+ * Returns a DECIMAL type with the specified precision and scale, but truncating the
+ * precision to the max storable precision (i.e. removes digits from before the
+ * decimal point).
+ */
+ public static ScalarType createClippedDecimalType(int precision, int scale) {
+ Preconditions.checkState(precision >= 0);
+ Preconditions.checkState(scale >= 0);
ScalarType type = new ScalarType(PrimitiveType.DECIMAL);
type.precision_ = Math.min(precision, MAX_PRECISION);
type.scale_ = Math.min(type.precision_, scale);
return type;
}
+ /**
+ * Returns a DECIMAL type with the specified precision and scale. When the given
+ * precision exceeds the max storable precision, reduce both precision and scale but
+ * preserve at least MIN_ADJUSTED_SCALE for scale (unless the desired scale was less).
+ */
+ public static ScalarType createAdjustedDecimalType(int precision, int scale) {
+ Preconditions.checkState(precision >= 0);
+ Preconditions.checkState(scale >= 0);
+ if (precision > MAX_PRECISION) {
+ int minScale = Math.min(scale, MIN_ADJUSTED_SCALE);
+ int delta = precision - MAX_PRECISION;
+ precision = MAX_PRECISION;
+ scale = Math.max(scale - delta, minScale);
+ }
+ ScalarType type = new ScalarType(PrimitiveType.DECIMAL);
+ type.precision_ = precision;
+ type.scale_ = scale;
+ return type;
+ }
+
public static ScalarType createVarcharType(int len) {
// length checked in analysis
ScalarType type = new ScalarType(PrimitiveType.VARCHAR);
@@ -336,7 +374,7 @@ public class ScalarType extends Type {
} else if (isNull()) {
return ScalarType.NULL;
} else if (isDecimal()) {
- return createDecimalTypeInternal(MAX_PRECISION, scale_);
+ return createClippedDecimalType(MAX_PRECISION, scale_);
} else {
return ScalarType.INVALID;
}
@@ -347,7 +385,7 @@ public class ScalarType extends Type {
if (type_ == PrimitiveType.DOUBLE || type_ == PrimitiveType.BIGINT || isNull()) {
return this;
} else if (type_ == PrimitiveType.DECIMAL) {
- return createDecimalTypeInternal(MAX_PRECISION, scale_);
+ return createClippedDecimalType(MAX_PRECISION, scale_);
}
return createType(PrimitiveType.values()[type_.ordinal() + 1]);
}
@@ -364,8 +402,8 @@ public class ScalarType extends Type {
case SMALLINT: return createDecimalType(5);
case INT: return createDecimalType(10);
case BIGINT: return createDecimalType(19);
- case FLOAT: return createDecimalTypeInternal(MAX_PRECISION, 9);
- case DOUBLE: return createDecimalTypeInternal(MAX_PRECISION, 17);
+ case FLOAT: return createDecimalType(MAX_PRECISION, 9);
+ case DOUBLE: return createDecimalType(MAX_PRECISION, 17);
default: return ScalarType.INVALID;
}
}
http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/a53eeb20/fe/src/main/java/org/apache/impala/catalog/Type.java
----------------------------------------------------------------------
diff --git a/fe/src/main/java/org/apache/impala/catalog/Type.java b/fe/src/main/java/org/apache/impala/catalog/Type.java
index 05c71c7..45848c5 100644
--- a/fe/src/main/java/org/apache/impala/catalog/Type.java
+++ b/fe/src/main/java/org/apache/impala/catalog/Type.java
@@ -62,14 +62,13 @@ public abstract class Type {
public static final ScalarType TIMESTAMP = new ScalarType(PrimitiveType.TIMESTAMP);
public static final ScalarType DATE = new ScalarType(PrimitiveType.DATE);
public static final ScalarType DATETIME = new ScalarType(PrimitiveType.DATETIME);
- public static final ScalarType DEFAULT_DECIMAL = (ScalarType)
+ public static final ScalarType DEFAULT_DECIMAL =
ScalarType.createDecimalType(ScalarType.DEFAULT_PRECISION,
ScalarType.DEFAULT_SCALE);
- public static final ScalarType DECIMAL =
- (ScalarType) ScalarType.createDecimalTypeInternal(-1, -1);
+ public static final ScalarType DECIMAL = ScalarType.createWildCardDecimalType();
public static final ScalarType DEFAULT_VARCHAR = ScalarType.createVarcharType(-1);
public static final ScalarType VARCHAR = ScalarType.createVarcharType(-1);
- public static final ScalarType CHAR = (ScalarType) ScalarType.createCharType(-1);
+ public static final ScalarType CHAR = ScalarType.createCharType(-1);
private static ArrayList<ScalarType> integerTypes;
private static ArrayList<ScalarType> numericTypes;
http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/a53eeb20/testdata/workloads/functional-query/queries/QueryTest/decimal-exprs.test
----------------------------------------------------------------------
diff --git a/testdata/workloads/functional-query/queries/QueryTest/decimal-exprs.test b/testdata/workloads/functional-query/queries/QueryTest/decimal-exprs.test
new file mode 100644
index 0000000..89385bd
--- /dev/null
+++ b/testdata/workloads/functional-query/queries/QueryTest/decimal-exprs.test
@@ -0,0 +1,73 @@
+====
+---- QUERY
+# Test DECIMAL V1 divide result type
+set decimal_v2=false;
+select d1 / d2, d2 / d1, d3 / d4, d5 / d3, d3 / d5 from decimal_tbl;
+---- RESULTS
+0.55535553555,1.8006482982,NULL,10000.0891810008,0.000099999108197945064
+21.12612612612,0.0473347547,NULL,0.2544210023,3.930493123209169054441
+37.07207207207,0.0269744835,NULL,0.0000810000,12345.678900000000000000000
+37.07207207207,0.0269744835,NULL,0.0908820008,11.003278877005347593582
+398.92492492492,0.0025067373,NULL,0.0000630900,15850.349728459731155875669
+---- TYPES
+DECIMAL, DECIMAL, DECIMAL, DECIMAL, DECIMAL
+====
+---- QUERY
+# Verify DECIMAL V2. Differences with V1:
+# * d3/d4 does not overflow
+# * d5/d3 has more scale
+set decimal_v2=true;
+select d1 / d2, d2 / d1, d3 / d4, d5 / d3, d3 / d5 from decimal_tbl;
+---- RESULTS
+0.55535553555,1.8006482982,10.000000,10000.08918100081154710738507,0.000099999108197945064
+21.12612612612,0.0473347547,100.000000,0.25442100231523112106860,3.930493123209169054441
+37.07207207207,0.0269744835,1000.000000,0.09088200082702620752593,11.003278877005347593582
+37.07207207207,0.0269744835,10000.000000,0.00008100000073710000670,12345.678900000000000000000
+398.92492492492,0.0025067373,100000.000000,0.00006309009057411982422,15850.349728459731155875669
+---- TYPES
+DECIMAL, DECIMAL, DECIMAL, DECIMAL, DECIMAL
+====
+---- QUERY
+# Test casting behavior without decimal_v2 query option set.
+set decimal_v2=false;
+select cast(d3 as decimal(20, 3)) from functional.decimal_tbl;
+---- RESULTS
+1.234
+12.345
+123.456
+1234.567
+12345.678
+---- TYPES
+DECIMAL
+====
+---- QUERY
+# Test casting behavior with decimal_v2 query option set.
+set decimal_v2=true;
+select cast(d3 as decimal(20, 3)) from functional.decimal_tbl;
+---- RESULTS
+1.235
+12.346
+123.457
+1234.568
+12345.679
+---- TYPES
+DECIMAL
+====
+---- QUERY
+# Test casting behavior without decimal_v2 query option set.
+set decimal_v2=false;
+select sum(cast(d3 as DECIMAL(20,2)) + cast(d5 as DECIMAL(20,4))) from functional.decimal_tbl;
+---- RESULTS
+26078.2788
+---- TYPES
+DECIMAL
+====
+---- QUERY
+# Test casting behavior with decimal_v2 query option set.
+set decimal_v2=true;
+select sum(cast(d3 as DECIMAL(20,2)) + cast(d5 as DECIMAL(20,4))) from functional.decimal_tbl;
+---- RESULTS
+26078.3189
+---- TYPES
+DECIMAL
+====
http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/a53eeb20/testdata/workloads/functional-query/queries/QueryTest/decimal.test
----------------------------------------------------------------------
diff --git a/testdata/workloads/functional-query/queries/QueryTest/decimal.test b/testdata/workloads/functional-query/queries/QueryTest/decimal.test
index 4a1316b..6e7cbae 100644
--- a/testdata/workloads/functional-query/queries/QueryTest/decimal.test
+++ b/testdata/workloads/functional-query/queries/QueryTest/decimal.test
@@ -449,47 +449,3 @@ NULL,0.0000
---- TYPES
DECIMAL, DECIMAL
====
----- QUERY
-# Test casting behavior without decimal_v2 query option set.
-set decimal_v2=false;
-select cast(d3 as decimal(20, 3)) from functional.decimal_tbl;
----- RESULTS
-1.234
-12.345
-123.456
-1234.567
-12345.678
----- TYPES
-DECIMAL
-====
----- QUERY
-# Test casting behavior with decimal_v2 query option set.
-set decimal_v2=true;
-select cast(d3 as decimal(20, 3)) from functional.decimal_tbl;
----- RESULTS
-1.235
-12.346
-123.457
-1234.568
-12345.679
----- TYPES
-DECIMAL
-====
----- QUERY
-# Test casting behavior without decimal_v2 query option set.
-set decimal_v2=false;
-select sum(cast(d3 as DECIMAL(20,2)) + cast(d5 as DECIMAL(20,4))) from functional.decimal_tbl;
----- RESULTS
-26078.2788
----- TYPES
-DECIMAL
-====
----- QUERY
-# Test casting behavior with decimal_v2 query option set.
-set decimal_v2=true;
-select sum(cast(d3 as DECIMAL(20,2)) + cast(d5 as DECIMAL(20,4))) from functional.decimal_tbl;
----- RESULTS
-26078.3189
----- TYPES
-DECIMAL
-====
http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/a53eeb20/tests/query_test/test_decimal_queries.py
----------------------------------------------------------------------
diff --git a/tests/query_test/test_decimal_queries.py b/tests/query_test/test_decimal_queries.py
index 80caf56..036d879 100644
--- a/tests/query_test/test_decimal_queries.py
+++ b/tests/query_test/test_decimal_queries.py
@@ -20,11 +20,10 @@
from copy import copy
from tests.common.impala_test_suite import ImpalaTestSuite
+from tests.common.test_dimensions import create_exec_option_dimension_from_dict
from tests.common.test_vector import ImpalaTestDimension
class TestDecimalQueries(ImpalaTestSuite):
- BATCH_SIZES = [0, 1]
-
@classmethod
def get_workload(cls):
return 'functional-query'
@@ -33,8 +32,9 @@ class TestDecimalQueries(ImpalaTestSuite):
def add_test_dimensions(cls):
super(TestDecimalQueries, cls).add_test_dimensions()
cls.ImpalaTestMatrix.add_dimension(
- ImpalaTestDimension('batch_size', *TestDecimalQueries.BATCH_SIZES))
-
+ create_exec_option_dimension_from_dict({
+ 'decimal_v2' : ['false', 'true'],
+ 'batch_size' : [0, 1]}))
# Hive < 0.11 does not support decimal so we can't run these tests against the other
# file formats.
# TODO: Enable them on Hive >= 0.11.
@@ -44,9 +44,24 @@ class TestDecimalQueries(ImpalaTestSuite):
v.get_value('table_format').file_format == 'parquet')
def test_queries(self, vector):
- new_vector = copy(vector)
- new_vector.get_value('exec_option')['batch_size'] = vector.get_value('batch_size')
- self.run_test_case('QueryTest/decimal', new_vector)
+ self.run_test_case('QueryTest/decimal', vector)
+
+# Tests involving DECIMAL typed expressions. The results depend on whether DECIMAL
+# version 1 or version 2 are enabled, so the .test file itself toggles the DECIMAL_V2
+# query option.
+class TestDecimalExprs(ImpalaTestSuite):
+ @classmethod
+ def get_workload(cls):
+ return 'functional-query'
+
+ @classmethod
+ def add_test_dimensions(cls):
+ super(TestDecimalExprs, cls).add_test_dimensions()
+ cls.ImpalaTestMatrix.add_constraint(lambda v:
+ (v.get_value('table_format').file_format == 'parquet'))
+
+ def test_exprs(self, vector):
+ self.run_test_case('QueryTest/decimal-exprs', vector)
# TODO: when we have a good way to produce Avro decimal data (e.g. upgrade Hive), we can
# run Avro through the same tests as above instead of using avro_decimal_tbl.