diff --git a/test/src/unit-cppapi-subarray.cc b/test/src/unit-cppapi-subarray.cc index 96455c04ff7c..ad9b9a899bd8 100644 --- a/test/src/unit-cppapi-subarray.cc +++ b/test/src/unit-cppapi-subarray.cc @@ -463,50 +463,119 @@ TEST_CASE( Array array_r(ctx, array_name, TILEDB_READ); Subarray subarray(ctx, array_r); + // If read_range_oob_error is false, the range will be cropped with a + // warning and the query will succeed. + auto read_range_oob_error = GENERATE(true, false); auto expected = TILEDB_ERR; + int fill_val = tiledb::sm::constants::empty_int32; + std::vector expected_data(16, fill_val); + expected_data[0] = 1; + expected_data[1] = 2; + expected_data[4] = 3; + expected_data[5] = 4; SECTION("- Upper bound OOB") { int range[] = {0, 100}; auto r = Range(&range[0], &range[1], sizeof(int)); - CHECK(subarray.ptr().get()->subarray_->add_range_unsafe(0, r).ok()); + if (read_range_oob_error) { + CHECK_FALSE( + subarray.ptr() + .get() + ->subarray_->add_range(0, std::move(r), read_range_oob_error) + .ok()); + } else { + CHECK(subarray.ptr() + .get() + ->subarray_->add_range(0, std::move(r), read_range_oob_error) + .ok()); + // The subarray will warn and crop to full domain ranges. + expected = TILEDB_OK; + } } SECTION("- Lower bound OOB") { int range[] = {-1, 2}; auto r = Range(&range[0], &range[1], sizeof(int)); - CHECK(subarray.ptr().get()->subarray_->add_range_unsafe(0, r).ok()); + if (read_range_oob_error) { + CHECK_FALSE( + subarray.ptr() + .get() + ->subarray_->add_range(0, std::move(r), read_range_oob_error) + .ok()); + } else { + // Warn and crop dim0 to [0, 2] with [0, 3] implicitly set on dim1. + CHECK(subarray.ptr() + .get() + ->subarray_->add_range(0, std::move(r), read_range_oob_error) + .ok()); + expected_data.resize(12); + expected = TILEDB_OK; + } } SECTION("- Second range OOB") { int range[] = {1, 4}; auto r = Range(&range[0], &range[1], sizeof(int)); - CHECK(subarray.ptr().get()->subarray_->add_range_unsafe(0, r).ok()); int range2[] = {10, 20}; auto r2 = Range(&range2[0], &range2[1], sizeof(int)); - CHECK(subarray.ptr().get()->subarray_->add_range_unsafe(1, r2).ok()); + if (read_range_oob_error) { + CHECK_FALSE( + subarray.ptr() + .get() + ->subarray_->add_range(0, std::move(r), read_range_oob_error) + .ok()); + CHECK_FALSE( + subarray.ptr() + .get() + ->subarray_->add_range(1, std::move(r2), read_range_oob_error) + .ok()); + } else { + // Warn and crop dim0 to [1, 3] and dim1 to [3, 3] + CHECK(subarray.ptr() + .get() + ->subarray_->add_range(0, std::move(r), read_range_oob_error) + .ok()); + CHECK(subarray.ptr() + .get() + ->subarray_->add_range(1, std::move(r2), read_range_oob_error) + .ok()); + expected_data = {fill_val, fill_val, fill_val}; + expected = TILEDB_OK; + } } SECTION("- Valid ranges") { int range[] = {0, 1}; auto r = Range(&range[0], &range[1], sizeof(int)); - CHECK(subarray.ptr().get()->subarray_->add_range_unsafe(0, r).ok()); - CHECK(subarray.ptr().get()->subarray_->add_range_unsafe(1, r).ok()); + CHECK(subarray.ptr() + .get() + ->subarray_->add_range(0, std::move(r), read_range_oob_error) + .ok()); + CHECK(subarray.ptr() + .get() + ->subarray_->add_range(1, std::move(r), read_range_oob_error) + .ok()); + expected_data = data_w; expected = TILEDB_OK; } - Query query(ctx, array_r); - query.set_subarray(subarray); - query.set_config(cfg); - - std::vector a(4); - query.set_data_buffer("a", a); - tiledb::test::ServerQueryBuffers buffers; - CHECK( - submit_query_wrapper( - ctx, array_name, &query, buffers, true, query_v2, false) == expected); - - if (expected == TILEDB_OK) { - CHECK(query.query_status() == tiledb::Query::Status::COMPLETE); - CHECK(a == std::vector{1, 2, 3, 4}); + // If the Subarray threw an exception when adding OOB ranges it will be unset. + if (!read_range_oob_error || expected == TILEDB_OK) { + Query query(ctx, array_r); + query.set_subarray(subarray); + query.set_config(cfg); + + std::vector a(expected_data.size()); + query.set_data_buffer("a", a); + tiledb::test::ServerQueryBuffers buffers; + CHECK( + submit_query_wrapper( + ctx, array_name, &query, buffers, true, query_v2, false) == + expected); + + if (expected == TILEDB_OK) { + CHECK(query.query_status() == tiledb::Query::Status::COMPLETE); + CHECK(a == expected_data); + } } if (vfs.is_dir(array_name)) { diff --git a/tiledb/sm/subarray/subarray.cc b/tiledb/sm/subarray/subarray.cc index 5a964b4d6dc7..4df514cbe3ae 100644 --- a/tiledb/sm/subarray/subarray.cc +++ b/tiledb/sm/subarray/subarray.cc @@ -358,17 +358,6 @@ Status Subarray::add_range( return Status::Ok(); } -Status Subarray::add_range_unsafe(uint32_t dim_idx, const Range& range) { - // Must reset the result size and tile overlap - est_result_size_computed_ = false; - tile_overlap_.clear(); - - // Add the range - throw_if_not_ok(range_subset_[dim_idx].add_range_unrestricted(range)); - is_default_[dim_idx] = range_subset_[dim_idx].is_implicitly_initialized(); - return Status::Ok(); -} - Status Subarray::set_subarray(const void* subarray) { if (!array_->array_schema_latest().domain().all_dims_same_type()) return LOG_STATUS( @@ -2123,6 +2112,17 @@ void Subarray::add_default_ranges() { add_default_label_ranges(dim_num); } +Status Subarray::add_range_unsafe(uint32_t dim_idx, const Range& range) { + // Must reset the result size and tile overlap + est_result_size_computed_ = false; + tile_overlap_.clear(); + + // Add the range + throw_if_not_ok(range_subset_[dim_idx].add_range_unrestricted(range)); + is_default_[dim_idx] = range_subset_[dim_idx].is_implicitly_initialized(); + return Status::Ok(); +} + void Subarray::add_default_label_ranges(dimension_size_type dim_num) { label_range_subset_.clear(); label_range_subset_.resize(dim_num, nullopt); diff --git a/tiledb/sm/subarray/subarray.h b/tiledb/sm/subarray/subarray.h index 4ca07ef90f68..cc51be6172aa 100644 --- a/tiledb/sm/subarray/subarray.h +++ b/tiledb/sm/subarray/subarray.h @@ -151,6 +151,9 @@ class ITileRange { */ class Subarray { public: + /** Friend with Query to call `add_range_unsafe` */ + friend class Query; + /* ********************************* */ /* PUBLIC DATA TYPES */ /* ********************************* */ @@ -524,12 +527,6 @@ class Subarray { const void* end, uint64_t end_size); - /** - * Adds a range along the dimension with the given index, without - * performing any error checks. - */ - Status add_range_unsafe(uint32_t dim_idx, const Range& range); - /** * Adds a range to the (read/write) query on the input dimension by name, * in the form of (start, end, stride). @@ -1554,6 +1551,12 @@ class Subarray { */ void add_default_ranges(); + /** + * Adds a range along the dimension with the given index, without + * performing any error checks. + */ + Status add_range_unsafe(uint32_t dim_idx, const Range& range); + /** Computes the estimated result size for all attributes/dimensions. */ Status compute_est_result_size(const Config* config, ThreadPool* compute_tp); diff --git a/tiledb/type/range/range.h b/tiledb/type/range/range.h index 7009283094a4..d46478ef58a5 100644 --- a/tiledb/type/range/range.h +++ b/tiledb/type/range/range.h @@ -461,8 +461,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]); }; /**