Fixed issues found during PR review

Enmk · Enmk · commit aa15fc059425 · 2021-03-24T12:51:50.000+02:00
Fragile implementation of ItemView::ValidateData
And several other minor issues.
diff --git a/clickhouse/columns/date.cpp b/clickhouse/columns/date.cpp
@@ -135,7 +135,7 @@ void ColumnDateTime64::Append(const Int64& value) {
 
 Int64 ColumnDateTime64::At(size_t n) const {
     // make sure to use Absl's Int128 conversion
-    return static_cast<long long>(data_->At(n));
+    return static_cast<Int64>(data_->At(n));
 }
 
 void ColumnDateTime64::Append(ColumnRef column) {
diff --git a/clickhouse/columns/decimal.cpp b/clickhouse/columns/decimal.cpp
@@ -5,7 +5,8 @@ namespace
 using namespace clickhouse;
 
 #ifdef ABSL_HAVE_INTRINSIC_INT128
-inline bool addOverflow(const Int128 & l, const Int128 & r, Int128 * result)
+template <typename T>
+inline bool addOverflow(const Int128 & l, const T & r, Int128 * result)
 {
     __int128 res;
     const auto ret_value = __builtin_add_overflow(static_cast<__int128>(l), static_cast<__int128>(r), &res);
@@ -14,7 +15,8 @@ inline bool addOverflow(const Int128 & l, const Int128 & r, Int128 * result)
     return ret_value;
 }
 
-inline bool mulOverflow(const Int128 & l, const Int128 & r, Int128 * result)
+template <typename T>
+inline bool mulOverflow(const Int128 & l, const T & r, Int128 * result)
 {
     __int128 res;
     const auto ret_value = __builtin_mul_overflow(static_cast<__int128>(l), static_cast<__int128>(r), &res);
@@ -24,7 +26,13 @@ inline bool mulOverflow(const Int128 & l, const Int128 & r, Int128 * result)
 }
 
 #else
-inline bool getSign(const Int128 & v)
+template <typename T>
+inline bool getSignBit(const T & v)
+{
+    return std::signbit(v);
+}
+
+inline bool getSignBit(const Int128 & v)
 {
 //    static constexpr Int128 zero {};
 //    return v < zero;
@@ -36,12 +44,12 @@ inline bool getSign(const Int128 & v)
 inline bool addOverflow(const Int128 & l, const Int128 & r, Int128 * result)
 {
     //    *result = l + r;
-    //    const auto result_sign = getSign(*result);
+    //    const auto result_sign = getSignBit(*result);
     //    return l_sign == r_sign && l_sign != result_sign;
 
     // Based on code from:
     // https://wiki.sei.cmu.edu/confluence/display/c/INT32-C.+Ensure+that+operations+on+signed+integers+do+not+result+in+overflow#INT32C.Ensurethatoperationsonsignedintegersdonotresultinoverflow-CompliantSolution
-    const auto r_positive = !getSign(r);
+    const auto r_positive = !getSignBit(r);
 
     if ((r_positive && (l > (std::numeric_limits<Int128>::max() - r))) ||
         (!r_positive && (l < (std::numeric_limits<Int128>::min() - r)))) {
@@ -52,30 +60,31 @@ inline bool addOverflow(const Int128 & l, const Int128 & r, Int128 * result)
     return false;
 }
 
-inline bool mulOverflow(const Int128 & l, const Int128 & r, Int128 * result)
+template <typename T>
+inline bool mulOverflow(const Int128 & l, const T & r, Int128 * result)
 {
     // Based on code from:
     // https://wiki.sei.cmu.edu/confluence/display/c/INT32-C.+Ensure+that+operations+on+signed+integers+do+not+result+in+overflow#INT32C.Ensurethatoperationsonsignedintegersdonotresultinoverflow-CompliantSolution.3
-    const auto l_positive = !getSign(l);
-    const auto r_positive = !getSign(r);
+    const auto l_positive = !getSignBit(l);
+    const auto r_positive = !getSignBit(r);
 
     if (l_positive) {
         if (r_positive) {
-            if (l > (std::numeric_limits<Int128>::max() / r)) {
+            if (r != 0 && l > (std::numeric_limits<Int128>::max() / r)) {
                 return true;
             }
         } else {
-            if (r < (std::numeric_limits<Int128>::min() / l)) {
+            if (l != 0 && r < (std::numeric_limits<Int128>::min() / l)) {
                 return true;
             }
         }
     } else {
         if (r_positive) {
-            if (l < (std::numeric_limits<Int128>::min() / r)) {
+            if (r != 0 && l < (std::numeric_limits<Int128>::min() / r)) {
                 return true;
             }
         } else {
-            if ( (l != 0) && (r < (std::numeric_limits<Int128>::max() / l))) {
+            if (l != 0 && (r < (std::numeric_limits<Int128>::max() / l))) {
                 return true;
             }
         }
@@ -145,8 +154,8 @@ void ColumnDecimal::Append(const std::string& value) {
 
             has_dot = true;
         } else if (*c >= '0' && *c <= '9') {
-            if (mulOverflow(int_value, Int128(10), &int_value) ||
-                addOverflow(int_value, Int128(*c - '0'), &int_value)) {
+            if (mulOverflow(int_value, 10, &int_value) ||
+                addOverflow(int_value, *c - '0', &int_value)) {
                 throw std::runtime_error("value is too big for 128-bit integer");
             }
         } else {
@@ -160,7 +169,7 @@ void ColumnDecimal::Append(const std::string& value) {
     }
 
     while (zeros) {
-        if (mulOverflow(int_value, Int128(10), &int_value)) {
+        if (mulOverflow(int_value, 10, &int_value)) {
             throw std::runtime_error("value is too big for 128-bit integer");
         }
         --zeros;
diff --git a/clickhouse/columns/itemview.cpp b/clickhouse/columns/itemview.cpp
@@ -3,52 +3,69 @@
 namespace clickhouse {
 
 void ItemView::ValidateData(Type::Code type, DataType data) {
-    static const int ANY = -1; // value can be of any size
-    static const int ERR = -2; // value is not allowed inside ItemView
-    static const int value_size_by_type[] = {
-        0,   /*Void*/
-        1,   /*Int8*/
-        2,   /*Int16*/
-        4,   /*Int32*/
-        8,   /*Int64*/
-        1,   /*UInt8*/
-        2,   /*UInt16*/
-        4,   /*UInt32*/
-        8,   /*UInt64*/
-        4,   /*Float32*/
-        8,   /*Float64*/
-        ANY, /*String*/
-        ANY, /*FixedString*/
-        4,   /*DateTime*/
-        2,   /*Date*/
-        ERR, /*Array*/
-        ERR, /*Nullable*/
-        ERR, /*Tuple*/
-        1,   /*Enum8*/
-        2,   /*Enum16*/
-        16,  /*UUID*/
-        4,   /*IPv4*/
-        8,   /*IPv6*/
-        16,  /*Int128*/
-        16,  /*Decimal*/
-        4,   /*Decimal32*/
-        8,   /*Decimal64*/
-        16,  /*Decimal128*/
-        ERR, /*LowCardinality*/
-        8,   /*DateTime64*/
-    };
-
-    if (type >= sizeof(value_size_by_type)/sizeof(value_size_by_type[0]) || type < 0) {
-        throw std::runtime_error("Unknon type code:" + std::to_string(static_cast<int>(type)));
-    } else {
-        const auto expected_size = value_size_by_type[type];
-        if (expected_size == ERR) {
+    int expected_size = 0;
+    switch (type) {
+        case Type::Code::Void:
+            expected_size = 0;
+            break;
+
+        case Type::Code::Int8:
+        case Type::Code::UInt8:
+        case Type::Code::Enum8:
+            expected_size = 1;
+            break;
+
+        case Type::Code::Int16:
+        case Type::Code::UInt16:
+        case Type::Code::Date:
+        case Type::Code::Enum16:
+            expected_size = 2;
+            break;
+
+        case Type::Code::Int32:
+        case Type::Code::UInt32:
+        case Type::Code::Float32:
+        case Type::Code::DateTime:
+        case Type::Code::IPv4:
+        case Type::Code::Decimal32:
+            expected_size = 4;
+            break;
+
+        case Type::Code::Int64:
+        case Type::Code::UInt64:
+        case Type::Code::Float64:
+        case Type::Code::DateTime64:
+        case Type::Code::IPv6:
+        case Type::Code::Decimal64:
+            expected_size = 8;
+            break;
+
+        case Type::Code::String:
+        case Type::Code::FixedString:
+            // value can be of any size
+            return;
+
+        case Type::Code::Array:
+        case Type::Code::Nullable:
+        case Type::Code::Tuple:
+        case Type::Code::LowCardinality:
             throw std::runtime_error("Unsupported type in ItemView: " + std::to_string(static_cast<int>(type)));
-        } else if (expected_size != ANY && expected_size != static_cast<int>(data.size())) {
-            throw std::runtime_error("Value size mismatch for type "
-                    + std::to_string(static_cast<int>(type)) + " expected: "
-                    + std::to_string(expected_size) + ", got: " + std::to_string(data.size()));
-        }
+
+        case Type::Code::UUID:
+        case Type::Code::Int128:
+        case Type::Code::Decimal:
+        case Type::Code::Decimal128:
+            expected_size = 16;
+            break;
+
+        default:
+            throw std::runtime_error("Unknon type code:" + std::to_string(static_cast<int>(type)));
+    }
+
+    if (expected_size != static_cast<int>(data.size())) {
+        throw std::runtime_error("Value size mismatch for type "
+                + std::to_string(static_cast<int>(type)) + " expected: "
+                + std::to_string(expected_size) + ", got: " + std::to_string(data.size()));
     }
 }
 
diff --git a/clickhouse/columns/itemview.h b/clickhouse/columns/itemview.h
@@ -26,13 +26,10 @@ struct ItemView {
     inline auto ConvertToStorageValue(const T& t) {
         if constexpr (std::is_same_v<std::string_view, T> || std::is_same_v<std::string, T>) {
             return std::string_view{t};
-        } else if constexpr (std::is_fundamental_v<T>) {
-            return std::string_view{reinterpret_cast<const char*>(&t), sizeof(T)};
-        }
-        else if constexpr (std::is_same_v<Int128, std::decay_t<T>>) {
+        } else if constexpr (std::is_fundamental_v<T> || std::is_same_v<Int128, std::decay_t<T>>) {
             return std::string_view{reinterpret_cast<const char*>(&t), sizeof(T)};
         } else {
-            // will caue error at compile-time
+            // will cause a compile-time error
             return;
         }
     }

Original file line number	Diff line number	Diff line change
`@@ -135,7 +135,7 @@ void ColumnDateTime64::Append(const Int64& value) {`
`135`	`135`
`136`	`136`	`Int64 ColumnDateTime64::At(size_t n) const {`
`137`	`137`	`// make sure to use Absl's Int128 conversion`
`138`		`- return static_cast<long long>(data_->At(n));`
	`138`	`+ return static_cast<Int64>(data_->At(n));`
`139`	`139`	`}`
`140`	`140`
`141`	`141`	`void ColumnDateTime64::Append(ColumnRef column) {`