From 619f3ae832f71a0964aafed8b5b7bb6884d6ff93 Mon Sep 17 00:00:00 2001 From: Shaun M Reed Date: Sat, 2 Mar 2024 05:41:22 -0500 Subject: [PATCH] Update crop_range to clamp to domain range. (#4781) While working on #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. --- tiledb/sm/array_schema/dimension.cc | 26 -------------------- tiledb/sm/array_schema/dimension.h | 24 +----------------- tiledb/sm/array_schema/domain.cc | 16 ++++++++++-- tiledb/type/range/range.h | 5 ++-- tiledb/type/range/test/unit_crop_range.cc | 30 +++++++++++++++++++++++ 5 files changed, 48 insertions(+), 53 deletions(-) diff --git a/tiledb/sm/array_schema/dimension.cc b/tiledb/sm/array_schema/dimension.cc index 947aaaca247..ea54b069e14 100644 --- 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 -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 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) { - crop_range_func_ = crop_range; - } - }; - apply_with_type(g, type_); -} - void Dimension::set_domain_range_func() { auto g = [&](auto T) { if constexpr (tiledb::type::TileDBFundamental) { diff --git a/tiledb/sm/array_schema/dimension.h b/tiledb/sm/array_schema/dimension.h index 0e2a0014e37..6ca7f85f033 100644 --- 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 - 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 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 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 index 139cca4ecac..05b85cb385e 100644 --- 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 @@ -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) { + tiledb::type::crop_range( + 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::deserialize( diff --git a/tiledb/type/range/range.h b/tiledb/type/range/range.h index 7009283094a..c3011983a76 100644 --- 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 #include #include #include @@ -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 index 46dfcf055b5..8234041d29c 100644 --- 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::max()}; test_crop_range(bounds, range, bounds); } + SECTION("Test crop outside lower bound") { + TestType range[2]{0, 0}; + TestType result[2]{1, 1}; + test_crop_range(bounds, range, result); + } + SECTION("Test crop outside upper bound") { + TestType range[2]{5, 6}; + TestType result[2]{4, 4}; + test_crop_range(bounds, range, result); + } } TEMPLATE_TEST_CASE( @@ -126,6 +136,16 @@ TEMPLATE_TEST_CASE( std::numeric_limits::max()}; test_crop_range(bounds, range, bounds); } + SECTION("Test crop outside lower bound") { + TestType range[2]{-6, -4}; + TestType result[2]{-2, -2}; + test_crop_range(bounds, range, result); + } + SECTION("Test crop outside upper bound") { + TestType range[2]{5, 6}; + TestType result[2]{2, 2}; + test_crop_range(bounds, range, result); + } } TEMPLATE_TEST_CASE( @@ -164,4 +184,14 @@ TEMPLATE_TEST_CASE( std::numeric_limits::infinity()}; test_crop_range(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(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(bounds, range, result); + } }