From 1acc631a0407684b409a3a4536ce8e7abb42e942 Mon Sep 17 00:00:00 2001 From: Alexander Barkov Date: Mon, 29 Mar 2021 17:30:41 +0400 Subject: [PATCH] MCOL-4600 CAST(decimal AS SIGNED/UNSIGNED) returns a wrong result The "SIGNED" part of the problem was previously fixed by MCOL-4640. Fixing the "UNSIGNED" part. - Adding TDecimal64::toUInt64Round() and Decimal::decimal64ToUInt64Round() - Renaming Decimal::narrowRound() to decimal64ToSInt64Round(), for a more self-descriptive name, and for symmetry with decimal64ToUInt64Round() - Reusing TDecimal64::toSInt64Round() inside decimal64ToSInt64Round(). This change was forgotten in MCOL-4640 :( - Removing the old code in Func_cast_unsigned::getUintVal with pow(). It caused precision loss, hence the bug. Adding a call for Decimal::decimal64ToUInt64Round() instead. - Adding tests for both SIGNED and UNSIGNED casts. Additional change: - Moving the wide-decimal-to-uint64_t rounding code from Func_cast_unsigned::getUintVal() to TDecimal128::toUInt64Round() (with refactoring). Adding TDecimal::toUInt64Round() for symmetry with TDecimal::toSInt64Round(). It will be easier to reuse the code with way. --- datatypes/mcs_decimal.h | 40 +++++++++++++++++++++-------- mtr/basic/r/type_decimal.result | 11 ++++++++ mtr/basic/t/type_decimal.test | 12 +++++++++ utils/funcexp/func_bitwise.cpp | 2 +- utils/funcexp/func_cast.cpp | 45 +-------------------------------- 5 files changed, 55 insertions(+), 55 deletions(-) diff --git a/datatypes/mcs_decimal.h b/datatypes/mcs_decimal.h index 94df57364..818c08963 100644 --- a/datatypes/mcs_decimal.h +++ b/datatypes/mcs_decimal.h @@ -213,6 +213,12 @@ public: return intg - 1; return intg; } + uint64_t toUInt64Round(uint32_t scale) const + { + return value < 0 ? + 0 : + static_cast(toSInt64Round(scale)); + } }; @@ -255,6 +261,18 @@ public: explicit TDecimal128(const int128_t* valPtr) :TSInt128(valPtr) { } + uint64_t toUInt64Round(uint32_t scale) const + { + if (s128Value <= 0) + return 0; + auto divisor = scaleDivisor(scale); + uint128_t intg = s128Value / divisor; + uint128_t frac2 = 2 * (s128Value % divisor); + if (frac2 >= divisor) + intg++; + return intg > numeric_limits::max() ? numeric_limits::max() : + static_cast(intg); + } }; @@ -519,17 +537,13 @@ class Decimal: public TDecimal128, public TDecimal64 return TSInt128(roundedValue); } - int64_t narrowRound() const + int64_t decimal64ToSInt64Round() const { - int64_t scaleDivisor; - getScaleDivisor(scaleDivisor, scale); - int64_t intg = value / scaleDivisor; - int64_t frac2 = 2 * (value % scaleDivisor); - if (frac2 >= scaleDivisor) - return intg + 1; - if (frac2 <= -scaleDivisor) - return intg - 1; - return intg; + return TDecimal64::toSInt64Round((uint32_t) scale); + } + uint64_t decimal64ToUInt64Round() const + { + return TDecimal64::toUInt64Round((uint32_t) scale); } int64_t toSInt64Round() const @@ -538,6 +552,12 @@ class Decimal: public TDecimal128, public TDecimal64 static_cast(getPosNegRoundedIntegralPart(4)) : TDecimal64::toSInt64Round((uint32_t) scale); } + uint64_t toUInt64Round() const + { + return isWideDecimalTypeByPrecision(precision) ? + TDecimal128::toUInt64Round((uint32_t) scale) : + TDecimal64::toUInt64Round((uint32_t) scale); + } // MOD operator for an integer divisor to be used // for integer rhs diff --git a/mtr/basic/r/type_decimal.result b/mtr/basic/r/type_decimal.result index f99ed6d37..c223ef3a9 100644 --- a/mtr/basic/r/type_decimal.result +++ b/mtr/basic/r/type_decimal.result @@ -3571,3 +3571,14 @@ SELECT CAST(a AS SIGNED), HEX(CHAR(a USING latin1)) FROM t1; CAST(a AS SIGNED) HEX(CHAR(a USING latin1)) 8999999999999999 CAFA7FFF DROP TABLE t1; +# +# MCOL-4600 CAST(decimal AS SIGNED/UNSIGNED) returns a wrong result +# +CREATE TABLE t1 (a DECIMAL(18,17)); +INSERT INTO t1 VALUES (-9.49999999999999999); +INSERT INTO t1 VALUES (9.49999999999999999); +SELECT a, CAST(a AS UNSIGNED), CAST(a AS SIGNED) FROM t1; +a CAST(a AS UNSIGNED) CAST(a AS SIGNED) +-9.49999999999999999 0 -9 +9.49999999999999999 9 9 +DROP TABLE t1; diff --git a/mtr/basic/t/type_decimal.test b/mtr/basic/t/type_decimal.test index ba5c5aadb..44f273a6c 100644 --- a/mtr/basic/t/type_decimal.test +++ b/mtr/basic/t/type_decimal.test @@ -217,3 +217,15 @@ CREATE TABLE t1 (a DECIMAL(17,1)) ENGINE=ColumnStore; INSERT INTO t1 VALUES (8999999999999999.0); SELECT CAST(a AS SIGNED), HEX(CHAR(a USING latin1)) FROM t1; DROP TABLE t1; + +--echo # +--echo # MCOL-4600 CAST(decimal AS SIGNED/UNSIGNED) returns a wrong result +--echo # + +CREATE TABLE t1 (a DECIMAL(18,17)); +INSERT INTO t1 VALUES (-9.49999999999999999); +INSERT INTO t1 VALUES (9.49999999999999999); +--disable_warnings +SELECT a, CAST(a AS UNSIGNED), CAST(a AS SIGNED) FROM t1; +--enable_warnings +DROP TABLE t1; diff --git a/utils/funcexp/func_bitwise.cpp b/utils/funcexp/func_bitwise.cpp index b8d2f7a93..ce0f365ba 100644 --- a/utils/funcexp/func_bitwise.cpp +++ b/utils/funcexp/func_bitwise.cpp @@ -100,7 +100,7 @@ datatypes::TUInt64Null DecimalToBitOperand(Row& row, return ConvertToBitOperand(val); } - return datatypes::TUInt64Null((uint64_t) d.narrowRound()); + return datatypes::TUInt64Null((uint64_t) d.decimal64ToSInt64Round()); } diff --git a/utils/funcexp/func_cast.cpp b/utils/funcexp/func_cast.cpp index c314a6ca6..b6fcd73ca 100644 --- a/utils/funcexp/func_cast.cpp +++ b/utils/funcexp/func_cast.cpp @@ -331,50 +331,7 @@ uint64_t Func_cast_unsigned::getUintVal(Row& row, case execplan::CalpontSystemCatalog::UDECIMAL: { IDB_Decimal d = parm[0]->data()->getDecimalVal(row, isNull); - - if (parm[0]->data()->resultType().colWidth == datatypes::MAXDECIMALWIDTH) - { - if (d.s128Value < 0) - { - return 0; - } - - int128_t scaleDivisor, scaleDivisor2; - - datatypes::getScaleDivisor(scaleDivisor, d.scale); - - scaleDivisor2 = (scaleDivisor <= 10) ? 1 : (scaleDivisor / 10); - - uint128_t tmpval = d.s128Value / scaleDivisor; - int128_t lefto = (d.s128Value - tmpval * scaleDivisor) / scaleDivisor2; - - if (utils::is_nonnegative(tmpval) && lefto > 4) - tmpval++; - - if (tmpval > static_cast(UINT64_MAX)) - tmpval = UINT64_MAX; - - return static_cast(tmpval); - } - else - { - if (d.value < 0) - { - return 0; - } - - double dscale = d.scale; - - uint64_t value = d.value / pow(10.0, dscale); - int lefto = (d.value - value * pow(10.0, dscale)) / pow(10.0, dscale - 1); - - if ( utils::is_nonnegative(value) && lefto > 4 ) - { - value++; - } - - return value; - } + return d.toUInt64Round(); } break;