From 5ba6737965ad3573f816d71c24c2015ce2587e96 Mon Sep 17 00:00:00 2001 From: Roman Nozdrin Date: Sun, 22 Nov 2020 17:55:22 +0000 Subject: [PATCH] Fixes for Decimal multiplication overflow check and RowGroup UTs --- datatypes/mcs_decimal.cpp | 2 -- datatypes/mcs_decimal.h | 21 +++++++++---------- tests/mcs_decimal-tests.cpp | 21 ++++++++++++++++++- tests/rowgroup-tests.cpp | 41 ++++++++++++++++++------------------- 4 files changed, 50 insertions(+), 35 deletions(-) diff --git a/datatypes/mcs_decimal.cpp b/datatypes/mcs_decimal.cpp index b2563251f..8eccc4e9d 100644 --- a/datatypes/mcs_decimal.cpp +++ b/datatypes/mcs_decimal.cpp @@ -735,6 +735,4 @@ namespace datatypes os << dec.toString(); return os; } - - } // end of namespace diff --git a/datatypes/mcs_decimal.h b/datatypes/mcs_decimal.h index 387c4f662..5e53e9750 100644 --- a/datatypes/mcs_decimal.h +++ b/datatypes/mcs_decimal.h @@ -415,28 +415,26 @@ struct DivisionOverflowCheck { } }; -/** - @brief The structure contains an overflow check for int128 - and int64_t multiplication. -*/ +// +// @brief The structure contains an overflow check for int128 +// and int64_t multiplication. +// struct MultiplicationOverflowCheck { void operator()(const int128_t& x, const int128_t& y) { - if (x * y / y != x) - { - throw logging::OperationOverflowExcept( - "Decimal::multiplication or scale multiplication \ -produces an overflow."); - } + int128_t tempR = 0; + this->operator()(x, y, tempR); } bool operator()(const int128_t& x, const int128_t& y, int128_t& r) { - if ((r = x * y) / y != x) + volatile int128_t z = x * y; + if (z / y != x) { throw logging::OperationOverflowExcept( "Decimal::multiplication or scale multiplication \ produces an overflow."); } + r = z; return true; } void operator()(const int64_t x, const int64_t y) @@ -838,6 +836,7 @@ class VDecimal: public TSInt128 // STRICTLY for unit tests!!! void setTSInt64Value(const int64_t x) { value = x; } void setTSInt128Value(const int128_t& x) { s128Value = x; } + void setScale(const uint8_t x) { scale = x; } private: uint8_t writeIntPart(const int128_t& x, diff --git a/tests/mcs_decimal-tests.cpp b/tests/mcs_decimal-tests.cpp index 5d5b29189..cb2c26eb8 100644 --- a/tests/mcs_decimal-tests.cpp +++ b/tests/mcs_decimal-tests.cpp @@ -508,7 +508,6 @@ TEST(Decimal, additionWithOverflowCheck) result.scale = 1; result.precision = 38; result.s128Value = 0; - EXPECT_THROW(doAdd(l, r, result), logging::OperationOverflowExcept); // Normal execution w/o overflow @@ -880,6 +879,26 @@ TEST(Decimal, multiplicationWithOverflowCheck) EXPECT_NO_THROW(doMultiply(l, r, result)); EXPECT_EQ("21267647932558653966460912964485513216", result.toString()); + + l.setTSInt128Value((int128_t)1 << 122); + l.setScale(0); + r.setTSInt128Value(100); + r.setScale(0); + EXPECT_THROW(doMultiply(l, r, result), logging::OperationOverflowExcept); + + l.setTSInt128Value((int128_t)1 << 65); + l.setScale(0); + r.setTSInt128Value((int128_t)1 << 64); + r.setScale(0); + EXPECT_THROW(doMultiply(l, r, result), logging::OperationOverflowExcept); + + l.setTSInt128Value((int128_t)1 << 122); + l.setScale(0); + r.setTSInt128Value(2); + r.setScale(0); + EXPECT_NO_THROW(doMultiply(l, r, result)); + EXPECT_EQ("10633823966279326983230456482242756608", result.toString()); + } TEST(Decimal, DecimalToStringCheckScale0) diff --git a/tests/rowgroup-tests.cpp b/tests/rowgroup-tests.cpp index cc031606b..d386bfb31 100644 --- a/tests/rowgroup-tests.cpp +++ b/tests/rowgroup-tests.cpp @@ -104,12 +104,11 @@ protected: sValueVector.push_back(0); sValueVector.push_back(nullValue-1); - uValueVector.push_back(nullValue); - uValueVector.push_back(42); - uValueVector.push_back(bigValue); - uValueVector.push_back(0); - uValueVector.push_back(nullValue-1); - + anotherValueVector.push_back(nullValue); + anotherValueVector.push_back(42); + anotherValueVector.push_back(bigValue); + anotherValueVector.push_back(0); + anotherValueVector.push_back(nullValue-1); s8ValueVector.push_back(joblist::TINYINTNULL); s8ValueVector.push_back(-0x79); s8ValueVector.push_back(0); @@ -138,8 +137,8 @@ protected: { r.setBinaryField_offset(&sValueVector[i], sizeof(sValueVector[0]), offsets[0]); - r.setBinaryField_offset(&uValueVector[i], - sizeof(uValueVector[0]), offsets[1]); + r.setBinaryField_offset(&anotherValueVector[i], + sizeof(anotherValueVector[0]), offsets[1]); r.setIntField(s64ValueVector[i], 2); r.setIntField(s32ValueVector[i], 3); r.setIntField(s16ValueVector[i], 4); @@ -159,7 +158,7 @@ protected: uint32_t rowSize; size_t rowCount; std::vector sValueVector; - std::vector uValueVector; + std::vector anotherValueVector; std::vector s8ValueVector; std::vector s16ValueVector; std::vector s32ValueVector; @@ -199,15 +198,15 @@ TEST_F(RowDecimalTest, InitToNullAndIsNullValueCheck) TEST_F(RowDecimalTest, GetBinaryFieldCheck) { rg.getRow(0, &r); - uint128_t* u128Value; + int128_t* a128Value; int128_t* s128Value; for (size_t i = 0; i < sValueVector.size(); i++) { s128Value = r.getBinaryField(0); EXPECT_EQ(sValueVector[i], *s128Value); - u128Value = r.getBinaryField(1); - EXPECT_EQ(uValueVector[i], *u128Value); + a128Value = r.getBinaryField(1); + EXPECT_EQ(anotherValueVector[i], *a128Value); //EXPECT_EQ(s64ValueVector[i], r.getIntField(2)); //EXPECT_EQ(s32ValueVector[i],r.getIntField(3)); //EXPECT_EQ(r.getIntField(4),s16ValueVector[i]); @@ -246,27 +245,27 @@ TEST_F(RowDecimalTest, ApplyMappingCheck) rg.getRow(1, &r); rg.getRow(2, &rOutMappingCheck); int128_t* s128Value = rOutMappingCheck.getBinaryField(0); - uint128_t* u128Value = rOutMappingCheck.getBinaryField(1); + int128_t* a128Value = rOutMappingCheck.getBinaryField(1); EXPECT_NE(sValueVector[1], *s128Value); - EXPECT_NE(uValueVector[1], *u128Value); + EXPECT_NE(anotherValueVector[1], *a128Value); applyMapping(mapping, r, &rOutMappingCheck); s128Value = rOutMappingCheck.getBinaryField(0); - u128Value = rOutMappingCheck.getBinaryField(1); + a128Value = rOutMappingCheck.getBinaryField(1); EXPECT_EQ(sValueVector[1], *s128Value); - EXPECT_EQ(uValueVector[1], *u128Value); + EXPECT_EQ(anotherValueVector[1], *a128Value); } TEST_F(RowDecimalTest, CopyBinaryFieldCheck) { int128_t constVal = 1; int128_t *col1Out, *col1In; - uint128_t *col2Out, *col2In; + int128_t *col2Out, *col2In; rgOut.getRow(0, &rOut); for (size_t i = 0; i < sValueVector.size(); i++) { rOut.setBinaryField_offset(&constVal, 16, offsets[0]); - rOut.setBinaryField_offset((uint128_t*)&constVal, 16, offsets[1]); + rOut.setBinaryField_offset(&constVal, 16, offsets[1]); rOut.nextRow(rowSize); } @@ -278,14 +277,14 @@ TEST_F(RowDecimalTest, CopyBinaryFieldCheck) { col1In = r.getBinaryField(0); col1Out = rOut.getBinaryField(0); - col2In = r.getBinaryField(1); - col2Out = rOut.getBinaryField(1); + col2In = r.getBinaryField(1); + col2Out = rOut.getBinaryField(1); EXPECT_NE(*col1In, *col1Out); EXPECT_NE(*col2In, *col2Out); r.copyBinaryField(rOut, 0, 0); r.copyBinaryField(rOut, 1, 1); col1Out = rOut.getBinaryField(0); - col2Out = rOut.getBinaryField(1); + col2Out = rOut.getBinaryField(1); EXPECT_EQ(*col1In, *col1Out); EXPECT_EQ(*col2In, *col2Out); r.nextRow(rowSize);