Update crop_range to clamp to domain range. (TileDB-Inc#4781)

While working on TileDB-Inc#4685 we found it was possible for a cropped range to fall outside of the given domain range. This updates crop_range to call `std::clamp` instead of min/max to ensure the resulting cropped range is within the domain. --- TYPE: BUG DESC: Update crop_range to clamp to domain range.
davisp · Mar 3, 2024 · e39b319 · e39b319
1 parent 4f99444
commit e39b319
Show file tree

Hide file tree

Showing 5 changed files with 48 additions and 53 deletions.
diff --git a/tiledb/sm/array_schema/dimension.cc b/tiledb/sm/array_schema/dimension.cc
@@ -71,7 +71,6 @@ Dimension::Dimension(const std::string& name, Datatype type)
   set_ceil_to_tile_func();
   set_coincides_with_tiles_func();
   set_compute_mbr_func();
-  set_crop_range_func();
   set_domain_range_func();
   set_expand_range_func();
   set_expand_range_v_func();
@@ -107,7 +106,6 @@ Dimension::Dimension(
   set_ceil_to_tile_func();
   set_coincides_with_tiles_func();
   set_compute_mbr_func();
-  set_crop_range_func();
   set_domain_range_func();
   set_expand_range_func();
   set_expand_range_v_func();
@@ -355,21 +353,6 @@ Range Dimension::compute_mbr_var(
   return compute_mbr_var_func_(tile_off, tile_val);
 }
 
-template <class T>
-void Dimension::crop_range(const Dimension* dim, Range* range) {
-  assert(dim != nullptr);
-  assert(!range->empty());
-  auto dim_dom = (const T*)dim->domain().data();
-  auto r = (const T*)range->data();
-  T res[2] = {std::max(r[0], dim_dom[0]), std::min(r[1], dim_dom[1])};
-  range->set_range(res, sizeof(res));
-}
-
-void Dimension::crop_range(Range* range) const {
-  assert(crop_range_func_ != nullptr);
-  crop_range_func_(this, range);
-}
-
 template <class T>
 uint64_t Dimension::domain_range(const Range& range) {
   assert(!range.empty());
@@ -1591,15 +1574,6 @@ std::string Dimension::tile_extent_str() const {
   return apply_with_type(g, type_);
 }
 
-void Dimension::set_crop_range_func() {
-  auto g = [&](auto T) {
-    if constexpr (tiledb::type::TileDBNumeric<decltype(T)>) {
-      crop_range_func_ = crop_range<decltype(T)>;
-    }
-  };
-  apply_with_type(g, type_);
-}
-
 void Dimension::set_domain_range_func() {
   auto g = [&](auto T) {
     if constexpr (tiledb::type::TileDBFundamental<decltype(T)>) {

diff --git a/tiledb/sm/array_schema/dimension.h b/tiledb/sm/array_schema/dimension.h
@@ -487,19 +487,6 @@ class Dimension {
   static Range compute_mbr_var(
       const WriterTile& tile_off, const WriterTile& tile_val);
 
-  /**
-   * Crops the input 1D range such that it does not exceed the
-   * dimension domain.
-   */
-  void crop_range(Range* range) const;
-
-  /**
-   * Crops the input 1D range such that it does not exceed the
-   * dimension domain.
-   */
-  template <class T>
-  static void crop_range(const Dimension* dim, Range* range);
-
   /**
    * Returns the domain range (high - low + 1) of the input
    * 1D range. It returns 0 in case the dimension datatype
@@ -818,13 +805,7 @@ class Dimension {
       compute_mbr_var_func_;
 
   /**
-   * Stores the appropriate templated crop_range() function based on the
-   * dimension datatype.
-   */
-  std::function<void(const Dimension* dim, Range*)> crop_range_func_;
-
-  /**
-   * Stores the appropriate templated crop_range() function based on the
+   * Stores the appropriate templated domain_range() function based on the
    * dimension datatype.
    */
   std::function<uint64_t(const Range&)> domain_range_func_;
@@ -1032,9 +1013,6 @@ class Dimension {
   /** Sets the templated compute_mbr() function. */
   void set_compute_mbr_func();
 
-  /** Sets the templated crop_range() function. */
-  void set_crop_range_func();
-
   /** Sets the templated domain_range() function. */
   void set_domain_range_func();
 

diff --git a/tiledb/sm/array_schema/domain.cc b/tiledb/sm/array_schema/domain.cc
@@ -41,6 +41,7 @@
 #include "tiledb/sm/enums/layout.h"
 #include "tiledb/sm/misc/tdb_math.h"
 #include "tiledb/sm/misc/utils.h"
+#include "tiledb/type/apply_with_type.h"
 #include "tiledb/type/range/range.h"
 
 #include <cassert>
@@ -312,8 +313,19 @@ int Domain::cell_order_cmp(
 }
 
 void Domain::crop_ndrange(NDRange* ndrange) const {
-  for (unsigned d = 0; d < dim_num_; ++d)
-    dimension_ptrs_[d]->crop_range(&(*ndrange)[d]);
+  for (unsigned d = 0; d < dim_num_; ++d) {
+    auto type = dimension_ptrs_[d]->type();
+    auto g = [&](auto T) {
+      if constexpr (tiledb::type::TileDBIntegral<decltype(T)>) {
+        tiledb::type::crop_range<decltype(T)>(
+            dimension_ptrs_[d]->domain(), (*ndrange)[d]);
+      } else {
+        throw std::invalid_argument(
+            "Unsupported dimension datatype " + datatype_str(type));
+      }
+    };
+    apply_with_type(g, type);
+  }
 }
 
 shared_ptr<Domain> Domain::deserialize(

diff --git a/tiledb/type/range/range.h b/tiledb/type/range/range.h
@@ -38,6 +38,7 @@
 #include "tiledb/common/tag.h"
 #include "tiledb/sm/enums/datatype.h"
 
+#include <algorithm>
 #include <cmath>
 #include <cstring>
 #include <sstream>
@@ -461,8 +462,8 @@ template <
 void crop_range(const Range& bounds, Range& range) {
   auto bounds_data = (const T*)bounds.data();
   auto range_data = (T*)range.data();
-  range_data[0] = std::max(bounds_data[0], range_data[0]);
-  range_data[1] = std::min(bounds_data[1], range_data[1]);
+  range_data[0] = std::clamp(range_data[0], bounds_data[0], bounds_data[1]);
+  range_data[1] = std::clamp(range_data[1], bounds_data[0], bounds_data[1]);
 };
 
 /**

diff --git a/tiledb/type/range/test/unit_crop_range.cc b/tiledb/type/range/test/unit_crop_range.cc
@@ -89,6 +89,16 @@ TEMPLATE_TEST_CASE(
         std::numeric_limits<TestType>::max()};
     test_crop_range<TestType>(bounds, range, bounds);
   }
+  SECTION("Test crop outside lower bound") {
+    TestType range[2]{0, 0};
+    TestType result[2]{1, 1};
+    test_crop_range<TestType>(bounds, range, result);
+  }
+  SECTION("Test crop outside upper bound") {
+    TestType range[2]{5, 6};
+    TestType result[2]{4, 4};
+    test_crop_range<TestType>(bounds, range, result);
+  }
 }
 
 TEMPLATE_TEST_CASE(
@@ -126,6 +136,16 @@ TEMPLATE_TEST_CASE(
         std::numeric_limits<TestType>::max()};
     test_crop_range<TestType>(bounds, range, bounds);
   }
+  SECTION("Test crop outside lower bound") {
+    TestType range[2]{-6, -4};
+    TestType result[2]{-2, -2};
+    test_crop_range<TestType>(bounds, range, result);
+  }
+  SECTION("Test crop outside upper bound") {
+    TestType range[2]{5, 6};
+    TestType result[2]{2, 2};
+    test_crop_range<TestType>(bounds, range, result);
+  }
 }
 
 TEMPLATE_TEST_CASE(
@@ -164,4 +184,14 @@ TEMPLATE_TEST_CASE(
         std::numeric_limits<TestType>::infinity()};
     test_crop_range<TestType>(bounds, range, bounds);
   }
+  SECTION("Test crop outside lower bound") {
+    TestType range[2]{-60.1f, -40.3f};
+    TestType result[2]{-10.5f, -10.5f};
+    test_crop_range<TestType>(bounds, range, result);
+  }
+  SECTION("Test crop outside upper bound") {
+    TestType range[2]{5.1f, 6.5f};
+    TestType result[2]{3.33f, 3.33f};
+    test_crop_range<TestType>(bounds, range, result);
+  }
 }