Perform validity checks for ranges to add in an NDRectangle. (#5144)

[SC-49157](https://app.shortcut.com/tiledb-inc/story/49157/check-and-test-out-of-order-ranges-for-currentdomain)

This PR adds validation that the ranges added in an `NDRectangle` are in
the correct order. We add a `check_range_is_valid` function that accepts
a range and a datatype, and call it on `NDRectangle::set_range` with the
dimension's data type. The generic `check_range_is_valid` was also
updated to handle `std::string_view`.

---
TYPE: NO_HISTORY

Author: teo-tsirpanis

test/src/test-cppapi-ndrectangle.cc

@@ -39,12 +39,9 @@
 
 using namespace tiledb::test;
 
-TEST_CASE_METHOD(
-    TemporaryDirectoryFixture,
-    "NDRectangle - Basic",
-    "[cppapi][ArraySchema][NDRectangle]") {
+TEST_CASE("NDRectangle - Basic", "[cppapi][ArraySchema][NDRectangle]") {
   // Create the C++ context.
-  tiledb::Context ctx_{ctx, false};
+  tiledb::Context ctx_;
   tiledb::Domain domain(ctx_);
   auto d1 = tiledb::Dimension::create<int32_t>(ctx_, "x", {{0, 100}}, 10);
   auto d2 = tiledb::Dimension::create<int32_t>(ctx_, "y", {{0, 100}}, 10);
@@ -59,9 +56,6 @@ TEST_CASE_METHOD(
   int range_two[] = {30, 40};
   ndrect.set_range(1, range_two[0], range_two[1]);
 
-  // Check setting range in non-existent dim
-  CHECK_THROWS(ndrect.set_range(2, range_two[0], range_two[1]));
-
   // Get range
   auto range = ndrect.range<int>(0);
   CHECK(range[0] == 10);
@@ -77,3 +71,30 @@ TEST_CASE_METHOD(
   CHECK(range[0] == 30);
   CHECK(range[1] == 40);
 }
+
+TEST_CASE("NDRectangle - Errors", "[cppapi][ArraySchema][NDRectangle]") {
+  tiledb::Context ctx;
+
+  // Create a domain
+  tiledb::Domain domain(ctx);
+  auto d1 = tiledb::Dimension::create<int32_t>(ctx, "d1", {{1, 10}}, 5);
+  auto d2 = tiledb::Dimension::create(
+      ctx, "d2", TILEDB_STRING_ASCII, nullptr, nullptr);
+  domain.add_dimension(d1);
+  domain.add_dimension(d2);
+
+  // Create an NDRectangle
+  tiledb::NDRectangle ndrect(ctx, domain);
+
+  // Set range with non-existent dimension
+  CHECK_THROWS(ndrect.set_range(2, 1, 2));
+  CHECK_THROWS(ndrect.set_range("d3", 1, 2));
+
+  // Set too small range type
+  CHECK_THROWS(ndrect.set_range<uint8_t>(0, 1, 2));
+  CHECK_THROWS(ndrect.set_range<uint8_t>("d1", 1, 2));
+
+  // Set range out of order
+  CHECK_THROWS(ndrect.set_range(0, 2, 1));
+  CHECK_THROWS(ndrect.set_range("d2", "bbb", "aaa"));
+}

tiledb/sm/array_schema/ndrectangle.cc

@@ -121,6 +121,7 @@ void NDRectangle::set_range(const Range& r, uint32_t idx) {
     throw std::logic_error(
         "Trying to set a range for an index out of bounds is not possible.");
   }
+  check_range_is_valid(r, domain_->dimension_ptr(idx)->type());
   range_data_[idx] = r;
 }
 

tiledb/sm/cpp_api/ndrectangle.h

@@ -101,13 +101,18 @@ class NDRectangle {
    * ndrect.set_range(0, start, end);
    * @endcode
    *
-   * @tparam T The dimension datatype.
+   * @tparam T The dimension datatype. Must be an integer or a floating point
+   * number.
    * @param dim_name The name of the dimension to add the range to.
    * @param start The range start to add.
    * @param end The range end to add.
    * @return Reference to this NDRectangle.
    */
-  template <class T>
+  template <
+      class T,
+      std::enable_if_t<
+          std::is_integral_v<T> || std::is_floating_point_v<T>,
+          bool> = true>
   NDRectangle& set_range(const std::string& dim_name, T start, T end) {
     auto& ctx = ctx_.get();
 
@@ -138,13 +143,18 @@ class NDRectangle {
    * ndrect.set_range(0, start, end);
    * @endcode
    *
-   * @tparam T The dimension datatype.
+   * @tparam T The dimension datatype. Must be an integer or a floating point
+   * number.
    * @param dim_idx The index of the dimension to add the range to.
    * @param start The range start to add.
    * @param end The range end to add.
    * @return Reference to this NDRectangle.
    */
-  template <class T>
+  template <
+      class T,
+      std::enable_if_t<
+          std::is_integral_v<T> || std::is_floating_point_v<T>,
+          bool> = true>
   NDRectangle& set_range(uint32_t dim_idx, T start, T end) {
     auto& ctx = ctx_.get();
     // Create the tiledb_range_t struct and fill it

tiledb/type/range/range.cc

@@ -29,6 +29,7 @@
 #include "tiledb/type/range/range.h"
 #include "tiledb/sm/enums/datatype.h"
 #include "tiledb/sm/misc/constants.h"
+#include "tiledb/type/apply_with_type.h"
 
 using namespace tiledb::common;
 
@@ -126,4 +127,21 @@ std::string range_str(const Range& range, const tiledb::sm::Datatype type) {
   return ss.str();
 }
 
+void check_range_is_valid(const Range& range, const tiledb::sm::Datatype type) {
+  if (range.empty())
+    throw std::invalid_argument("Range is empty");
+
+  if (range.var_size()) {
+    if (type != sm::Datatype::STRING_ASCII)
+      throw std::invalid_argument(
+          "Validating a variable range is only supported for type " +
+          datatype_str(sm::Datatype::STRING_ASCII) + ".");
+    check_range_is_valid<std::string_view>(range);
+    return;
+  }
+
+  apply_with_type(
+      [&](const auto T) { check_range_is_valid<decltype(T)>(range); }, type);
+}
+
 }  // namespace tiledb::type

tiledb/type/range/range.h

@@ -453,22 +453,40 @@ Status check_range_is_subset(const Range& superset, const Range& range) {
  */
 template <
     typename T,
-    typename std::enable_if<std::is_arithmetic<T>::value>::type* = nullptr>
+    typename std::enable_if_t<
+        std::is_arithmetic_v<T> || std::is_same_v<T, std::string_view>>* =
+        nullptr>
 void check_range_is_valid(const Range& range) {
   // Check has data.
   if (range.empty())
     throw std::invalid_argument("Range is empty");
-  auto r = (const T*)range.data();
-  // Check for NaN
-  if constexpr (std::is_floating_point_v<T>) {
-    if (std::isnan(r[0]) || std::isnan(r[1]))
-      throw std::invalid_argument("Range contains NaN");
-  }
-  // Check range bounds
-  if (r[0] > r[1])
-    throw std::invalid_argument(
-        "Lower range bound " + std::to_string(r[0]) +
-        " cannot be larger than the higher bound " + std::to_string(r[1]));
+
+  // Compare string views
+  if constexpr (std::is_same_v<T, std::string_view>) {
+    auto start = range.start_str();
+    auto end = range.end_str();
+    // Check range bounds
+    if (start > end)
+      throw std::invalid_argument(
+          "Lower range bound " + std::string(start) +
+          " cannot be larger than the higher bound " + std::string(end));
+  } else {
+    if (range.size() != 2 * sizeof(T))
+      throw std::invalid_argument(
+          "Range size " + std::to_string(range.size()) +
+          " does not match the expected size " + std::to_string(2 * sizeof(T)));
+    auto r = (const T*)range.data();
+    // Check for NaN
+    if constexpr (std::is_floating_point_v<T>) {
+      if (std::isnan(r[0]) || std::isnan(r[1]))
+        throw std::invalid_argument("Range contains NaN");
+    }
+    // Check range bounds
+    if (r[0] > r[1])
+      throw std::invalid_argument(
+          "Lower range bound " + std::to_string(r[0]) +
+          " cannot be larger than the higher bound " + std::to_string(r[1]));
+  }
 };
 
 /**
@@ -495,6 +513,17 @@ void crop_range(const Range& bounds, Range& range) {
  */
 std::string range_str(const Range& range, const tiledb::sm::Datatype type);
 
+/**
+ * Validates that the range's elements are in the correct order.
+ *
+ * @param range The range to validate.
+ * @param type The datatype to view the range's elements as.
+ *
+ * @throws std::invalid_argument If the range's elements are not in the correct
+ * order.
+ */
+void check_range_is_valid(const Range& range, const tiledb::sm::Datatype type);
+
 }  // namespace tiledb::type
 
 #endif