Final fixes.

- Small comment on FixedStr16 that clarifies that effectively up to 15 characters can be stored, as the 16th character must always be a null character in order for all kernels to work correctly. - CastObj-kernel: forward the DaphneContext to the CastSca-kernel calls. - CastSca-kernel: consideration of signed/unsigned integers (don't parse all integers using stoll(), use stoull() for unsigned integers). - Removed remaining occurrences of platform-dependent int/long types from test cases, replaced them by [u]intX_t types. - And some more minor things.
daphne-eu · Oct 17, 2024 · d399e2e · d399e2e
1 parent e04978f
commit d399e2e
Show file tree

Hide file tree

Showing 8 changed files with 67 additions and 52 deletions.
diff --git a/src/runtime/local/datastructures/FixedSizeStringValueType.h b/src/runtime/local/datastructures/FixedSizeStringValueType.h
@@ -1,5 +1,5 @@
 /*
- * Copyright 2021 The DAPHNE Consortium
+ * Copyright 2024 The DAPHNE Consortium
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -16,17 +16,21 @@
 
 #pragma once
 
+#include <algorithm>
 #include <iostream>
 #include <stdexcept>
+#include <string>
+#include <vector>
 
-#include <algorithm>
 #include <cstddef>
 #include <cstring>
 
-#include <string.h>
-#include <string>
-#include <vector>
-
+/**
+ * @brief A string value type with a maximum length of 15 characters.
+ *
+ * Each instance is backed by a 16-character buffer, whereby at least the last character must always be a null
+ * character. The null-termination is required for some operations to work correctly (e.g., casting to a number).
+ */
 struct FixedStr16 {
     static const std::size_t N = 16;
     char buffer[N];
@@ -37,7 +41,7 @@ struct FixedStr16 {
     // Constructor from a C-style string
     FixedStr16(const char *str) {
         size_t len = std::strlen(str);
-        if (len > N) {
+        if (len >= N) {
             throw std::length_error("string exceeds fixed buffer size");
         }
         std::copy(str, str + len, buffer);
@@ -50,10 +54,10 @@ struct FixedStr16 {
     // Constructor from a std::string
     FixedStr16(const std::string &other) {
         size_t len = other.size();
-        if (len > N) {
+        if (len >= N) {
             throw std::length_error("string exceeds fixed buffer size");
         }
-        std::copy(other.begin(), other.end() + N, buffer);
+        std::copy(other.begin(), other.end(), buffer);
         std::fill(buffer + len, buffer + N, '\0');
     }
 
@@ -103,7 +107,7 @@ struct FixedStr16 {
     // Method to set the string
     void set(const char *str) {
         size_t len = std::strlen(str);
-        if (len > N) {
+        if (len >= N) {
             throw std::length_error("string exceeds fixed buffer size");
         }
         std::transform(str, str + len, buffer, [](char c) { return c; });

diff --git a/src/runtime/local/kernels/CastObj.h b/src/runtime/local/kernels/CastObj.h
@@ -188,12 +188,12 @@ template <typename VTRes, typename VTArg> class CastObj<DenseMatrix<VTRes>, Dens
             // a single dense array of values, we can simply
             // perform cast in one loop over that array.
             for (size_t idx = 0; idx < numCols * numRows; idx++)
-                resVals[idx] = castSca<VTRes, VTArg>(argVals[idx], nullptr);
+                resVals[idx] = castSca<VTRes, VTArg>(argVals[idx], ctx);
         else
             // res and arg might be views into a larger DenseMatrix.
             for (size_t r = 0; r < numRows; r++) {
                 for (size_t c = 0; c < numCols; c++)
-                    resVals[c] = castSca<VTRes, VTArg>(argVals[c], nullptr);
+                    resVals[c] = castSca<VTRes, VTArg>(argVals[c], ctx);
                 resVals += res->getRowSkip();
                 argVals += arg->getRowSkip();
             }

diff --git a/src/runtime/local/kernels/CastSca.h b/src/runtime/local/kernels/CastSca.h
@@ -74,44 +74,47 @@ template <typename VTArg> struct CastSca<const char *, VTArg> {
 
 template <typename VTRes> struct CastSca<VTRes, std::string> {
     static VTRes apply(std::string arg, DCTX(ctx)) {
-        if constexpr (std::is_integral<VTRes>::value)
-            return static_cast<VTRes>(std::stoll(arg));
-
-        else if constexpr (std::is_same<VTRes, double>::value)
+        if constexpr (std::is_integral<VTRes>::value) {
+            if constexpr (std::is_unsigned<VTRes>::value)
+                return static_cast<VTRes>(std::stoull(arg));
+            else
+                return static_cast<VTRes>(std::stoll(arg));
+        } else if constexpr (std::is_same<VTRes, double>::value)
             return static_cast<VTRes>(std::stold(arg));
 
         else if constexpr (std::is_same<VTRes, float>::value)
             return static_cast<VTRes>(std::stof(arg));
         else {
-            // Trigger a compiler warning using deprecated attribute
-            return unsupported_type(arg);
+            // Trigger a compiler warning using deprecated attribute.
+            return throwUnsupportedType(arg);
         }
     }
 
-    [[deprecated("CastSca: Warning! Unsupported result type in casting string values.")]] static VTRes
-    unsupported_type(std::string arg) {
+    [[deprecated("CastSca: Warning! Unsupported result type in casting string values.")]]
+    static VTRes throwUnsupportedType(std::string arg) {
         throw std::runtime_error("CastSca: Unsupported result type in casting string values");
     }
 };
 
 template <typename VTRes> struct CastSca<VTRes, FixedStr16> {
     static VTRes apply(FixedStr16 arg, DCTX(ctx)) {
-        if constexpr (std::is_integral<VTRes>::value)
-            return static_cast<VTRes>(std::stoll(arg.buffer));
-
-        else if constexpr (std::is_same<VTRes, double>::value)
+        if constexpr (std::is_integral<VTRes>::value) {
+            if constexpr (std::is_unsigned<VTRes>::value)
+                return static_cast<VTRes>(std::stoull(arg.buffer));
+            else
+                return static_cast<VTRes>(std::stoll(arg.buffer));
+        } else if constexpr (std::is_same<VTRes, double>::value)
             return static_cast<VTRes>(std::stold(arg.buffer));
-
         else if constexpr (std::is_same<VTRes, float>::value)
             return static_cast<VTRes>(std::stof(arg.buffer));
         else {
-            // Trigger a compiler warning using deprecated attribute
-            return unsupported_type(arg);
+            // Trigger a compiler warning using deprecated attribute.
+            return throwUnsupportedType(arg);
         }
     }
 
-    [[deprecated("CastSca: Warning! Unsupported result type in casting string values.")]] static VTRes
-    unsupported_type(std::string arg) {
+    [[deprecated("CastSca: Warning! Unsupported result type in casting string values.")]]
+    static VTRes throwUnsupportedType(std::string arg) {
         throw std::runtime_error("CastSca: Unsupported result type in casting string values");
     }
 };

diff --git a/test/runtime/local/kernels/CastObjTest.cpp b/test/runtime/local/kernels/CastObjTest.cpp
@@ -243,8 +243,8 @@ TEMPLATE_PRODUCT_TEST_CASE("castObj, DenseMatrix<string> to DenseMatrix<number>,
     DataObjectFactory::destroy(arg_stdSTR, arg_FixedStr16);
 }
 
-TEMPLATE_PRODUCT_TEST_CASE("castObj, DenseMatrix<string> to DenseMatrix<long>, long int", TAG_KERNELS, (DenseMatrix),
-                           (long)) {
+TEMPLATE_PRODUCT_TEST_CASE("castObj, DenseMatrix<string> to DenseMatrix<int64_t>, int64_t", TAG_KERNELS, (DenseMatrix),
+                           (int64_t)) {
     using DTRes = TestType;
     using VTRes = typename DTRes::VT;
 
@@ -271,12 +271,12 @@ TEMPLATE_PRODUCT_TEST_CASE("castObj, DenseMatrix<string> to DenseMatrix<long>, l
 
     SECTION("FixedStr16") {
         auto arg_FixedStr16 = genGivenVals<DenseMatrix<FixedStr16>>(
-            numRows, {FixedStr16("9223372036854774"), FixedStr16("9223372036854773"), FixedStr16("9223372036854772"),
-                      FixedStr16("9223372036854771"), FixedStr16("9223372036854776"), FixedStr16("9223372036854775")});
+            numRows, {FixedStr16("123456789012345"), FixedStr16("123456789012344"), FixedStr16("123456789012343"),
+                      FixedStr16("123456789012342"), FixedStr16("123456789012341"), FixedStr16("123456789012340")});
         DTRes *res_FixedStr16 = nullptr;
         auto check_FixedStr16 =
-            genGivenVals<DenseMatrix<VTRes>>(numRows, {9223372036854774, 9223372036854773, 9223372036854772,
-                                                       9223372036854771, 9223372036854776, 9223372036854775});
+            genGivenVals<DenseMatrix<VTRes>>(numRows, {123456789012345, 123456789012344, 123456789012343,
+                                                       123456789012342, 123456789012341, 123456789012340});
 
         castObj<DenseMatrix<VTRes>, DenseMatrix<FixedStr16>>(res_FixedStr16, arg_FixedStr16, nullptr);
 

diff --git a/test/runtime/local/kernels/CastScaTest.cpp b/test/runtime/local/kernels/CastScaTest.cpp
@@ -20,6 +20,8 @@
 
 #include <catch.hpp>
 
+#include <limits>
+
 #include <cstdint>
 
 TEST_CASE("castSca, no-op casts", TAG_KERNELS) {
@@ -47,6 +49,7 @@ TEST_CASE("castSca, actual casts", TAG_KERNELS) {
     CHECK(castSca<bool, double>(-123.4, nullptr) == true);
     CHECK(castSca<bool, double>(0.0, nullptr) == false);
 }
+
 TEST_CASE("castSca, actual casts strings to numbers", TAG_KERNELS) {
 
     CHECK(castSca<int64_t, std::string>("123", nullptr) == 123);
@@ -55,15 +58,19 @@ TEST_CASE("castSca, actual casts strings to numbers", TAG_KERNELS) {
     CHECK(castSca<double, std::string>("123.4", nullptr) == 123.4);
     CHECK(castSca<double, std::string>("-123.4", nullptr) == -123.4);
     CHECK(castSca<double, std::string>("0.0", nullptr) == 0.0);
-    CHECK(castSca<long, std::string>("9223372036854775807", nullptr) == 9223372036854775807);
-    CHECK(castSca<long, std::string>("-9223372036854775807", nullptr) == -9223372036854775807);
+    CHECK(castSca<int64_t, std::string>("9223372036854775807", nullptr) == std::numeric_limits<int64_t>::max());
+    CHECK(castSca<int64_t, std::string>("-9223372036854775808", nullptr) == std::numeric_limits<int64_t>::min());
+    CHECK(castSca<uint64_t, std::string>("18446744073709551615", nullptr) == std::numeric_limits<uint64_t>::max());
+    CHECK(castSca<uint64_t, std::string>("0", nullptr) == std::numeric_limits<uint64_t>::min());
 
     CHECK(castSca<int64_t, FixedStr16>("123", nullptr) == 123);
     CHECK(castSca<int64_t, FixedStr16>("-123", nullptr) == -123);
     CHECK(castSca<int64_t, FixedStr16>("0", nullptr) == 0);
     CHECK(castSca<double, FixedStr16>("123.4", nullptr) == 123.4);
     CHECK(castSca<double, FixedStr16>("-123.4", nullptr) == -123.4);
     CHECK(castSca<double, FixedStr16>("0.0", nullptr) == 0.0);
-    CHECK(castSca<long, FixedStr16>("922337203685477", nullptr) == 922337203685477);
-    CHECK(castSca<long, FixedStr16>("-92233720368547", nullptr) == -92233720368547);
+    CHECK(castSca<int64_t, FixedStr16>("123456789012345", nullptr) == 123456789012345ll);
+    CHECK(castSca<int64_t, FixedStr16>("-12345678901234", nullptr) == -12345678901234ll);
+    CHECK(castSca<uint64_t, FixedStr16>("123456789012345", nullptr) == 123456789012345ull);
+    CHECK(castSca<uint64_t, FixedStr16>("0", nullptr) == std::numeric_limits<uint64_t>::min());
 }
diff --git a/test/runtime/local/kernels/EwBinaryMatTest.cpp b/test/runtime/local/kernels/EwBinaryMatTest.cpp
@@ -35,10 +35,10 @@
 // CSRMatrix currently only supports ADD and MUL opCodes
 #define DATA_TYPES_NO_CSR DenseMatrix, Matrix
 
-template <class DTarg, class DTres>
-void checkEwBinaryMat(BinaryOpCode opCode, const DTarg *lhs, const DTarg *rhs, const DTres *exp) {
-    DTres *res = nullptr;
-    ewBinaryMat<DTres, DTarg, DTarg>(opCode, res, lhs, rhs, nullptr);
+template <class DTArg, class DTRes>
+void checkEwBinaryMat(BinaryOpCode opCode, const DTArg *lhs, const DTArg *rhs, const DTRes *exp) {
+    DTRes *res = nullptr;
+    ewBinaryMat<DTRes, DTArg, DTArg>(opCode, res, lhs, rhs, nullptr);
     CHECK(*res == *exp);
 }
 

diff --git a/test/runtime/local/kernels/EwBinaryScaTest.cpp b/test/runtime/local/kernels/EwBinaryScaTest.cpp
@@ -30,12 +30,12 @@ template <BinaryOpCode opCode, typename VT> void checkEwBinarySca(VT lhs, VT rhs
     CHECK(ewBinarySca<VT, VT, VT>(opCode, lhs, rhs, nullptr) == exp);
 }
 
-template <BinaryOpCode opCode> void checkEwBinarySca(std::string lhs, std::string rhs, int exp) {
+template <BinaryOpCode opCode> void checkEwBinarySca(std::string lhs, std::string rhs, int64_t exp) {
     CHECK(EwBinarySca<opCode, int64_t, std::string, std::string>::apply(lhs, rhs, nullptr) == exp);
     CHECK(ewBinarySca<int64_t, std::string, std::string>(opCode, lhs, rhs, nullptr) == exp);
 }
 
-template <BinaryOpCode opCode> void checkEwBinarySca(FixedStr16 lhs, FixedStr16 rhs, int exp) {
+template <BinaryOpCode opCode> void checkEwBinarySca(FixedStr16 lhs, FixedStr16 rhs, int64_t exp) {
     CHECK(EwBinarySca<opCode, int64_t, FixedStr16, FixedStr16>::apply(lhs, rhs, nullptr) == exp);
     CHECK(ewBinarySca<int64_t, FixedStr16, FixedStr16>(opCode, lhs, rhs, nullptr) == exp);
 }
@@ -222,8 +222,9 @@ TEMPLATE_TEST_CASE(TEST_NAME("or"), TAG_KERNELS, VALUE_TYPES) {
 }
 
 // ****************************************************************************
-// String op
+// String ops
 // ****************************************************************************
+
 TEMPLATE_TEST_CASE(TEST_NAME("concat"), TAG_KERNELS, ALL_STRING_VALUE_TYPES) {
     using VT = TestType;
     checkEwBinarySca<VT>(VT("abcd"), VT("abcd"), std::string("abcdabcd"));

diff --git a/test/runtime/local/kernels/EwUnaryMatTest.cpp b/test/runtime/local/kernels/EwUnaryMatTest.cpp
@@ -612,11 +612,11 @@ TEMPLATE_PRODUCT_TEST_CASE(TEST_NAME("Upper, string data"), TAG_KERNELS, (DenseM
 
     auto arg = genGivenVals<DT>(2, {VT(""), VT("Ab"), VT("123 abc"), VT("ABc"), VT("ab"), VT("12")});
 
-    auto dense_exp = genGivenVals<DenseMatrix<VT>>(2, {VT(""), VT("AB"), VT("123 ABC"), VT("ABC"), VT("AB"), VT("12")});
+    auto exp = genGivenVals<DenseMatrix<VT>>(2, {VT(""), VT("AB"), VT("123 ABC"), VT("ABC"), VT("AB"), VT("12")});
 
-    checkEwUnaryMat(UnaryOpCode::UPPER, arg, dense_exp);
+    checkEwUnaryMat(UnaryOpCode::UPPER, arg, exp);
 
-    DataObjectFactory::destroy(arg, dense_exp);
+    DataObjectFactory::destroy(arg, exp);
 }
 
 TEMPLATE_PRODUCT_TEST_CASE(TEST_NAME("Lower, string data"), TAG_KERNELS, (DenseMatrix), (ALL_STRING_VALUE_TYPES)) {
@@ -625,11 +625,11 @@ TEMPLATE_PRODUCT_TEST_CASE(TEST_NAME("Lower, string data"), TAG_KERNELS, (DenseM
 
     auto arg = genGivenVals<DT>(2, {VT(""), VT("Ab"), VT("123 ABC"), VT("ABc"), VT("ab"), VT("14")});
 
-    auto dense_exp = genGivenVals<DenseMatrix<VT>>(2, {VT(""), VT("ab"), VT("123 abc"), VT("abc"), VT("ab"), VT("14")});
+    auto exp = genGivenVals<DenseMatrix<VT>>(2, {VT(""), VT("ab"), VT("123 abc"), VT("abc"), VT("ab"), VT("14")});
 
-    checkEwUnaryMat(UnaryOpCode::LOWER, arg, dense_exp);
+    checkEwUnaryMat(UnaryOpCode::LOWER, arg, exp);
 
-    DataObjectFactory::destroy(arg, dense_exp);
+    DataObjectFactory::destroy(arg, exp);
 }
 
 // ****************************************************************************