Merge pull request #3015 from drrtuy/chore/MCOL-5568-math-with-domain…

…-range-check chore(datatypes): refactoring math ops results domain check functionality
mariadb-corporation · Oct 31, 2023 · 2f0bbed · 2f0bbed
2 parents 7aa6e4c + f704545
commit 2f0bbed
Show file tree

Hide file tree

Showing 4 changed files with 87 additions and 55 deletions.
diff --git a/datatypes/mcs_int128.h b/datatypes/mcs_int128.h
@@ -21,8 +21,10 @@
 #include <cfloat>
 #include <cstdint>
 #include <limits>
+#include <optional>
 #include <type_traits>
-#include <string>
+
+#include "mcs_datatype_basic.h"
 #include "mcs_float128.h"
 
 // Inline asm has three argument lists: output, input and clobber list
@@ -189,6 +191,11 @@ class TSInt128
     return s128Value == static_cast<int128_t>(x);
   }
 
+  inline operator bool() const
+  {
+    return s128Value != 0;
+  }
+
   inline operator double() const
   {
     return toDouble();
@@ -249,6 +256,18 @@ class TSInt128
     return static_cast<uint64_t>(s128Value);
   }
 
+  // This can be replaced with a template based on SQL data type
+  std::optional<uint64_t> toUBIGINTWithDomainCheck() const
+  {
+    if (s128Value > static_cast<int128_t>(datatypes::ranges_limits<SystemCatalog::UBIGINT>::max()) ||
+        s128Value < datatypes::ranges_limits<SystemCatalog::UBIGINT>::min())
+    {
+      return std::nullopt;
+    }
+
+    return static_cast<uint64_t>(s128Value);
+  }
+
   inline operator TFloat128() const
   {
     return toTFloat128();
@@ -271,6 +290,7 @@ class TSInt128
     return TSInt128(s128Value % rhs);
   }
 
+  // These math operators don't do out-of-range checks.
   inline TSInt128 operator*(const TSInt128& rhs) const
   {
     return TSInt128(s128Value * rhs.s128Value);
@@ -281,6 +301,16 @@ class TSInt128
     return TSInt128(s128Value + rhs.s128Value);
   }
 
+  inline TSInt128 operator-(const TSInt128& rhs) const
+  {
+    return TSInt128(s128Value - rhs.s128Value);
+  }
+
+  inline TSInt128 operator/(const TSInt128& rhs) const
+  {
+    return TSInt128(s128Value / rhs.s128Value);
+  }
+
   inline bool operator>(const TSInt128& rhs) const
   {
     return s128Value > rhs.s128Value;
@@ -321,4 +351,3 @@ class TSInt128
 };  // end of class
 
 }  // end of namespace datatypes
-
diff --git a/dbcon/execplan/arithmeticoperator.h b/dbcon/execplan/arithmeticoperator.h
@@ -29,9 +29,9 @@
 #include <cmath>
 #include <sstream>
 
+#include "mcs_int128.h"
 #include "operator.h"
 #include "parsetree.h"
-#include "mcs_datatype.h"
 
 namespace messageqcpp
 {
@@ -222,11 +222,33 @@ class ArithmeticOperator : public Operator
   template <typename result_t>
   inline result_t execute(result_t op1, result_t op2, bool& isNull);
   inline void execute(IDB_Decimal& result, IDB_Decimal op1, IDB_Decimal op2, bool& isNull);
+
   long fTimeZone;
   bool fDecimalOverflowCheck;
 };
 
-#include "parsetree.h"
+// Can be easily replaced with a template over T if MDB changes the result return type.
+inline uint64_t rangesCheck(const datatypes::TSInt128 x, const OpType op, const bool isNull)
+{
+  auto result = x.toUBIGINTWithDomainCheck();
+  if (!isNull && !result)
+  {
+    logging::Message::Args args;
+    static const std::string sqlType{"BIGINT UNSIGNED"};
+    args.add(sqlType);
+    switch (op)
+    {
+      case OP_ADD: args.add("\"+\""); break;
+      case OP_SUB: args.add("\"-\""); break;
+      case OP_MUL: args.add("\"*\""); break;
+      case OP_DIV: args.add("\"/\""); break;
+      default: args.add("<unknown>"); break;
+    }
+    const auto errcode = logging::ERR_MATH_PRODUCES_OUT_OF_RANGE_RESULT;
+    throw logging::IDBExcept(logging::IDBErrorInfo::instance()->errorMsg(errcode, args), errcode);
+  }
+  return result.value();  // if isNull returns some value
+}
 
 inline void ArithmeticOperator::evaluate(rowgroup::Row& row, bool& isNull, ParseTree* lop, ParseTree* rop)
 {
@@ -246,45 +268,13 @@ inline void ArithmeticOperator::evaluate(rowgroup::Row& row, bool& isNull, Parse
       // XXX: this is bandaid solution for specific customer case (MCOL-5568).
       // Despite that I tried to implement a proper solution: to have operations
       // performed using int128_t amd then check the result.
-      int128_t x, y;
       bool signedLeft = lop->data()->resultType().isSignedInteger();
       bool signedRight = rop->data()->resultType().isSignedInteger();
-      if (signedLeft)
-      {
-        x = static_cast<int128_t>(lop->getIntVal(row, isNull));
-      }
-      else
-      {
-        x = static_cast<int128_t>(lop->getUintVal(row, isNull));
-      }
-      if (signedRight)
-      {
-        y = static_cast<int128_t>(rop->getIntVal(row, isNull));
-      }
-      else
-      {
-        y = static_cast<int128_t>(rop->getUintVal(row, isNull));
-      }
-      int128_t result = execute(x, y, isNull);
-      if (!isNull && (result > MAX_UBIGINT || result < 0))
-      {
-        logging::Message::Args args;
-        std::string func = "<unknown>";
-        switch (fOp)
-        {
-          case OP_ADD: func = "\"+\""; break;
-          case OP_SUB: func = "\"-\""; break;
-          case OP_MUL: func = "\"*\""; break;
-          case OP_DIV: func = "\"/\""; break;
-          default: break;
-        }
-        args.add(func);
-        args.add(static_cast<double>(x));
-        args.add(static_cast<double>(y));
-        unsigned errcode = logging::ERR_FUNC_OUT_OF_RANGE_RESULT;
-        throw logging::IDBExcept(logging::IDBErrorInfo::instance()->errorMsg(errcode, args), errcode);
-      }
-      fResult.uintVal = static_cast<uint64_t>(result);
+      const datatypes::TSInt128 x((signedLeft) ? static_cast<int128_t>(lop->getIntVal(row, isNull))
+                                               : lop->getUintVal(row, isNull));
+      const datatypes::TSInt128 y((signedRight) ? static_cast<int128_t>(rop->getIntVal(row, isNull))
+                                                : rop->getUintVal(row, isNull));
+      fResult.uintVal = rangesCheck(execute(x, y, isNull), fOp, isNull);  // throws
     }
     break;
     case execplan::CalpontSystemCatalog::UINT:
@@ -320,8 +310,8 @@ inline void ArithmeticOperator::evaluate(rowgroup::Row& row, bool& isNull, Parse
   }
 }
 
-template <typename result_t>
-inline result_t ArithmeticOperator::execute(result_t op1, result_t op2, bool& isNull)
+template <typename T>
+inline T ArithmeticOperator::execute(T op1, T op2, bool& isNull)
 {
   switch (fOp)
   {
@@ -333,11 +323,22 @@ inline result_t ArithmeticOperator::execute(result_t op1, result_t op2, bool& is
 
     case OP_DIV:
       if (op2)
+      {
         return op1 / op2;
+      }
       else
+      {
         isNull = true;
+      }
 
-      return 0;
+      if constexpr (std::is_same<T, datatypes::TSInt128>::value)
+      {
+        return datatypes::TSInt128();  // returns 0
+      }
+      else
+      {
+        return T{0};
+      }
 
     default:
     {

diff --git a/mysql-test/columnstore/basic/r/MCOL-5568-out-of-range.result b/mysql-test/columnstore/basic/r/MCOL-5568-out-of-range.result
@@ -7,9 +7,9 @@ n_clms TINYINT(3) UNSIGNED
 ) ENGINE=COLUMNSTORE;
 INSERT INTO test_mult (indemnity_paid, n_clms) VALUES (-10, 1);
 SELECT indemnity_paid, n_clms, indemnity_paid * n_clms FROM test_mult;
-ERROR HY000: Internal error: MCS-2053: The result is out of range for function "*" using value(s): -10.000000 1.000000
+ERROR HY000: Internal error: MCS-2061: BIGINT UNSIGNED value is out of range in '`unk`.`unk`.`unk` "*" `unk`.`unk`.`unk`'
 SELECT indemnity_paid, n_clms, indemnity_paid + n_clms FROM test_mult;
-ERROR HY000: Internal error: MCS-2053: The result is out of range for function "+" using value(s): -10.000000 1.000000
+ERROR HY000: Internal error: MCS-2061: BIGINT UNSIGNED value is out of range in '`unk`.`unk`.`unk` "+" `unk`.`unk`.`unk`'
 SELECT indemnity_paid, n_clms, (indemnity_paid + 9) + n_clms FROM test_mult;
 indemnity_paid	n_clms	(indemnity_paid + 9) + n_clms
 -10	1	0
@@ -19,9 +19,9 @@ n_clms TINYINT UNSIGNED
 ) ENGINE=COLUMNSTORE;
 INSERT INTO test_mult2 (indemnity_paid, n_clms) VALUES (-10, 1);
 SELECT indemnity_paid, n_clms, indemnity_paid * n_clms FROM test_mult2;
-ERROR HY000: Internal error: MCS-2053: The result is out of range for function "*" using value(s): -10.000000 1.000000
+ERROR HY000: Internal error: MCS-2061: BIGINT UNSIGNED value is out of range in '`unk`.`unk`.`unk` "*" `unk`.`unk`.`unk`'
 SELECT indemnity_paid, n_clms, indemnity_paid + n_clms FROM test_mult2;
-ERROR HY000: Internal error: MCS-2053: The result is out of range for function "+" using value(s): -10.000000 1.000000
+ERROR HY000: Internal error: MCS-2061: BIGINT UNSIGNED value is out of range in '`unk`.`unk`.`unk` "+" `unk`.`unk`.`unk`'
 SELECT indemnity_paid, n_clms, (indemnity_paid + 9) + n_clms FROM test_mult2;
 indemnity_paid	n_clms	(indemnity_paid + 9) + n_clms
 -10	1	0
@@ -31,9 +31,9 @@ n_clms TINYINT UNSIGNED
 ) ENGINE=COLUMNSTORE;
 INSERT INTO test_mult3 (indemnity_paid, n_clms) VALUES (-10, 1);
 SELECT indemnity_paid, n_clms, indemnity_paid * n_clms FROM test_mult3;
-ERROR HY000: Internal error: MCS-2053: The result is out of range for function "*" using value(s): -10.000000 1.000000
+ERROR HY000: Internal error: MCS-2061: BIGINT UNSIGNED value is out of range in '`unk`.`unk`.`unk` "*" `unk`.`unk`.`unk`'
 SELECT indemnity_paid, n_clms, indemnity_paid + n_clms FROM test_mult3;
-ERROR HY000: Internal error: MCS-2053: The result is out of range for function "+" using value(s): -10.000000 1.000000
+ERROR HY000: Internal error: MCS-2061: BIGINT UNSIGNED value is out of range in '`unk`.`unk`.`unk` "+" `unk`.`unk`.`unk`'
 SELECT indemnity_paid, n_clms, (indemnity_paid + 9) + n_clms FROM test_mult3;
 indemnity_paid	n_clms	(indemnity_paid + 9) + n_clms
 -10	1	0
@@ -43,19 +43,19 @@ n_clms TINYINT UNSIGNED
 ) ENGINE=COLUMNSTORE;
 INSERT INTO test_mult4 (indemnity_paid, n_clms) VALUES (-10, 1);
 SELECT indemnity_paid, n_clms, indemnity_paid * n_clms FROM test_mult4;
-ERROR HY000: Internal error: MCS-2053: The result is out of range for function "*" using value(s): -10.000000 1.000000
+ERROR HY000: Internal error: MCS-2061: BIGINT UNSIGNED value is out of range in '`unk`.`unk`.`unk` "*" `unk`.`unk`.`unk`'
 SELECT indemnity_paid, n_clms, indemnity_paid + n_clms FROM test_mult4;
-ERROR HY000: Internal error: MCS-2053: The result is out of range for function "+" using value(s): -10.000000 1.000000
+ERROR HY000: Internal error: MCS-2061: BIGINT UNSIGNED value is out of range in '`unk`.`unk`.`unk` "+" `unk`.`unk`.`unk`'
 SELECT indemnity_paid, n_clms, (indemnity_paid + 9) + n_clms FROM test_mult4;
 indemnity_paid	n_clms	(indemnity_paid + 9) + n_clms
 -10	1	0
 SELECT indemnity_paid, n_clms, n_clms * indemnity_paid FROM test_mult4;
-ERROR HY000: Internal error: MCS-2053: The result is out of range for function "*" using value(s): 1.000000 -10.000000
+ERROR HY000: Internal error: MCS-2061: BIGINT UNSIGNED value is out of range in '`unk`.`unk`.`unk` "*" `unk`.`unk`.`unk`'
 SELECT indemnity_paid, n_clms, n_clms + indemnity_paid FROM test_mult4;
-ERROR HY000: Internal error: MCS-2053: The result is out of range for function "+" using value(s): 1.000000 -10.000000
+ERROR HY000: Internal error: MCS-2061: BIGINT UNSIGNED value is out of range in '`unk`.`unk`.`unk` "+" `unk`.`unk`.`unk`'
 SELECT indemnity_paid, n_clms, n_clms + (indemnity_paid + 9) FROM test_mult4;
 indemnity_paid	n_clms	n_clms + (indemnity_paid + 9)
 -10	1	0
 SELECT indemnity_paid, n_clms, n_clms - 2 FROM test_mult4;
-ERROR HY000: Internal error: MCS-2053: The result is out of range for function "-" using value(s): 1.000000 2.000000
+ERROR HY000: Internal error: MCS-2061: BIGINT UNSIGNED value is out of range in '`unk`.`unk`.`unk` "-" `unk`.`unk`.`unk`'
 DROP DATABASE MCOL5568;
diff --git a/utils/loggingcpp/ErrorMessage.txt b/utils/loggingcpp/ErrorMessage.txt
@@ -108,6 +108,8 @@
 
 2060	ERR_UNION_DECIMAL_OVERFLOW	Union operation exceeds maximum DECIMAL precision of 38.
 
+2061	ERR_MATH_PRODUCES_OUT_OF_RANGE_RESULT	%1% value is out of range in '`unk`.`unk`.`unk` %2% `unk`.`unk`.`unk`'
+
 # Sub-query errors
 3001	ERR_NON_SUPPORT_SUB_QUERY_TYPE	This subquery type is not supported yet.
 3002	ERR_MORE_THAN_1_ROW	Subquery returns more than 1 row.