Repository: incubator-impala Updated Branches: refs/heads/master 495325440 -> 646920810
IMPALA-4513: Promote integer types for ABS() The internal representation of the most negative number in two's complement requires 1 more bit to represent the positive version. This means ABS() must promote integer types to the next highest width. Change-Id: I86cc880e78258d5f90471bd8af4caeb4305eed77 Reviewed-on: http://gerrit.cloudera.org:8080/8004 Reviewed-by: Tim Armstrong <[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/f53ce3b1 Tree: http://git-wip-us.apache.org/repos/asf/incubator-impala/tree/f53ce3b1 Diff: http://git-wip-us.apache.org/repos/asf/incubator-impala/diff/f53ce3b1 Branch: refs/heads/master Commit: f53ce3b16d476214167a686ecb513c6d866105b9 Parents: 4953254 Author: Zachary Amsden <[email protected]> Authored: Thu Sep 7 14:15:19 2017 -0700 Committer: Impala Public Jenkins <[email protected]> Committed: Sat Sep 23 02:41:32 2017 +0000 ---------------------------------------------------------------------- be/src/exprs/expr-test.cc | 17 +++++++++++++++- be/src/exprs/math-functions-ir.cc | 21 ++++++++++++++++---- be/src/exprs/math-functions.h | 6 +++--- common/function-registry/impala_functions.py | 6 +++--- .../queries/QueryTest/exprs.test | 4 ++-- 5 files changed, 41 insertions(+), 13 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/f53ce3b1/be/src/exprs/expr-test.cc ---------------------------------------------------------------------- diff --git a/be/src/exprs/expr-test.cc b/be/src/exprs/expr-test.cc index 33d77e0..e4d633d 100644 --- a/be/src/exprs/expr-test.cc +++ b/be/src/exprs/expr-test.cc @@ -4253,6 +4253,21 @@ TEST_F(ExprTest, MathFunctions) { TestValue("e()", TYPE_DOUBLE, M_E); TestValue("abs(cast(-1.0 as double))", TYPE_DOUBLE, 1.0); TestValue("abs(cast(1.0 as double))", TYPE_DOUBLE, 1.0); + TestValue("abs(-127)", TYPE_SMALLINT, 127); + TestValue("abs(-128)", TYPE_SMALLINT, 128); + TestValue("abs(127)", TYPE_SMALLINT, 127); + TestValue("abs(128)", TYPE_INT, 128); + TestValue("abs(-32767)", TYPE_INT, 32767); + TestValue("abs(-32768)", TYPE_INT, 32768); + TestValue("abs(32767)", TYPE_INT, 32767); + TestValue("abs(32768)", TYPE_BIGINT, 32768); + TestValue("abs(-1 * cast(pow(2, 31) as int))", TYPE_BIGINT, 2147483648); + TestValue("abs(cast(pow(2, 31) as int))", TYPE_BIGINT, 2147483648); + TestValue("abs(2147483647)", TYPE_BIGINT, 2147483647); + TestValue("abs(2147483647)", TYPE_BIGINT, 2147483647); + TestValue("abs(-9223372036854775807)", TYPE_BIGINT, 9223372036854775807); + TestValue("abs(9223372036854775807)", TYPE_BIGINT, 9223372036854775807); + TestIsNull("abs(-9223372036854775808)", TYPE_BIGINT); TestValue("sign(0.0)", TYPE_FLOAT, 0.0f); TestValue("sign(-0.0)", TYPE_FLOAT, 0.0f); TestValue("sign(+0.0)", TYPE_FLOAT, 0.0f); @@ -4521,7 +4536,7 @@ TEST_F(ExprTest, MathFunctions) { // NULL arguments. In some cases the NULL can match multiple overloads so the result // type depends on the order in which function overloads are considered. - TestIsNull("abs(NULL)", TYPE_TINYINT); + TestIsNull("abs(NULL)", TYPE_SMALLINT); TestIsNull("sign(NULL)", TYPE_FLOAT); TestIsNull("exp(NULL)", TYPE_DOUBLE); TestIsNull("ln(NULL)", TYPE_DOUBLE); http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/f53ce3b1/be/src/exprs/math-functions-ir.cc ---------------------------------------------------------------------- diff --git a/be/src/exprs/math-functions-ir.cc b/be/src/exprs/math-functions-ir.cc index 48b98a9..cc86d5b 100644 --- a/be/src/exprs/math-functions-ir.cc +++ b/be/src/exprs/math-functions-ir.cc @@ -59,12 +59,25 @@ DoubleVal MathFunctions::E(FunctionContext* ctx) { return RET_TYPE(FN(v1.val, v2.val)); \ } -ONE_ARG_MATH_FN(Abs, BigIntVal, BigIntVal, llabs); +// N.B. - for integer math, we have to promote ABS() to the next highest integer type +// because in two's complement arithmetic, the largest negative value for any bit width +// is not representable as a positive value within the same width. For the largest width, +// we simply overflow. In the unlikely event a workaround is needed, one can simply +// cast to a higher precision decimal type. +BigIntVal MathFunctions::Abs(FunctionContext* ctx, const BigIntVal& v) { + if (v.is_null) return BigIntVal::null(); + if (UNLIKELY(v.val == std::numeric_limits<BigIntVal::underlying_type_t>::min())) { + ctx->AddWarning("abs() overflowed, returning NULL"); + return BigIntVal::null(); + } + return BigIntVal(llabs(v.val)); +} + +ONE_ARG_MATH_FN(Abs, BigIntVal, IntVal, llabs); +ONE_ARG_MATH_FN(Abs, IntVal, SmallIntVal, abs); +ONE_ARG_MATH_FN(Abs, SmallIntVal, TinyIntVal, abs); ONE_ARG_MATH_FN(Abs, DoubleVal, DoubleVal, fabs); ONE_ARG_MATH_FN(Abs, FloatVal, FloatVal, fabs); -ONE_ARG_MATH_FN(Abs, IntVal, IntVal, abs); -ONE_ARG_MATH_FN(Abs, SmallIntVal, SmallIntVal, abs); -ONE_ARG_MATH_FN(Abs, TinyIntVal, TinyIntVal, abs); ONE_ARG_MATH_FN(Sin, DoubleVal, DoubleVal, sin); ONE_ARG_MATH_FN(Asin, DoubleVal, DoubleVal, asin); ONE_ARG_MATH_FN(Cos, DoubleVal, DoubleVal, cos); http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/f53ce3b1/be/src/exprs/math-functions.h ---------------------------------------------------------------------- diff --git a/be/src/exprs/math-functions.h b/be/src/exprs/math-functions.h index 867ca2b..7063907 100644 --- a/be/src/exprs/math-functions.h +++ b/be/src/exprs/math-functions.h @@ -48,9 +48,9 @@ class MathFunctions { static BigIntVal Abs(FunctionContext*, const BigIntVal&); static DoubleVal Abs(FunctionContext*, const DoubleVal&); static FloatVal Abs(FunctionContext*, const FloatVal&); - static IntVal Abs(FunctionContext*, const IntVal&); - static SmallIntVal Abs(FunctionContext*, const SmallIntVal&); - static TinyIntVal Abs(FunctionContext*, const TinyIntVal&); + static BigIntVal Abs(FunctionContext*, const IntVal&); + static IntVal Abs(FunctionContext*, const SmallIntVal&); + static SmallIntVal Abs(FunctionContext*, const TinyIntVal&); static DoubleVal Sin(FunctionContext*, const DoubleVal&); static DoubleVal Asin(FunctionContext*, const DoubleVal&); static DoubleVal Cos(FunctionContext*, const DoubleVal&); http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/f53ce3b1/common/function-registry/impala_functions.py ---------------------------------------------------------------------- diff --git a/common/function-registry/impala_functions.py b/common/function-registry/impala_functions.py index c004775..0e3a3b8 100644 --- a/common/function-registry/impala_functions.py +++ b/common/function-registry/impala_functions.py @@ -260,9 +260,9 @@ visible_functions = [ [['abs'], 'BIGINT', ['BIGINT'], 'impala::MathFunctions::Abs'], [['abs'], 'DOUBLE', ['DOUBLE'], 'impala::MathFunctions::Abs'], [['abs'], 'FLOAT', ['FLOAT'], 'impala::MathFunctions::Abs'], - [['abs'], 'INT', ['INT'], 'impala::MathFunctions::Abs'], - [['abs'], 'SMALLINT', ['SMALLINT'], 'impala::MathFunctions::Abs'], - [['abs'], 'TINYINT', ['TINYINT'], 'impala::MathFunctions::Abs'], + [['abs'], 'BIGINT', ['INT'], 'impala::MathFunctions::Abs'], + [['abs'], 'INT', ['SMALLINT'], 'impala::MathFunctions::Abs'], + [['abs'], 'SMALLINT', ['TINYINT'], 'impala::MathFunctions::Abs'], [['sign'], 'FLOAT', ['DOUBLE'], 'impala::MathFunctions::Sign'], [['sin'], 'DOUBLE', ['DOUBLE'], 'impala::MathFunctions::Sin'], [['asin'], 'DOUBLE', ['DOUBLE'], 'impala::MathFunctions::Asin'], http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/f53ce3b1/testdata/workloads/functional-query/queries/QueryTest/exprs.test ---------------------------------------------------------------------- diff --git a/testdata/workloads/functional-query/queries/QueryTest/exprs.test b/testdata/workloads/functional-query/queries/QueryTest/exprs.test index 9b1a1a7..92854b1 100644 --- a/testdata/workloads/functional-query/queries/QueryTest/exprs.test +++ b/testdata/workloads/functional-query/queries/QueryTest/exprs.test @@ -1866,7 +1866,7 @@ Infinity,NaN,-0,-Infinity,Nan double,double,double,double,double ==== ---- QUERY -# Test that abs() retains the type of the paramter IMPALA-1424 +# Test that abs() promotes the type of the paramter IMPALA-4513 select abs(cast(1 as int)), abs(cast(1 as smallint)), abs(cast(1 as tinyint)), abs(cast(8589934592 as bigint)), abs(cast(-1.3 as double)), abs(cast(-1.3 as float)), @@ -1874,7 +1874,7 @@ select abs(cast(1 as int)), abs(cast(1 as smallint)), ---- RESULTS 1,1,1,8589934592,1.3,1.299999952316284,1.322 ---- TYPES -int, smallint, tinyint, bigint, double, float, decimal +bigint, int, smallint, bigint, double, float, decimal ==== ---- QUERY # Regression test for IMPALA-1508
