Make Subarray::add_range_unsafe private

TileDB-Inc · Feb 16, 2024 · a70f757 · a70f757
1 parent e4fcc1c
commit a70f757
Show file tree

Hide file tree

Showing 4 changed files with 111 additions and 39 deletions.
diff --git 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<int> 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<int> 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<int>{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<int> 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
@@ -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
@@ -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
@@ -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]);
 };
 
 /**