Skip to content

Commit f8da3e9

Browse files
shaunrd0shaunrd0
committed
Make Subarray::add_range_unsafe private
1 parent e4fcc1c commit f8da3e9

File tree

4 files changed

+112
-39
lines changed

4 files changed

+112
-39
lines changed

‎test/src/unit-cppapi-subarray.cc‎

Lines changed: 89 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -463,50 +463,119 @@ TEST_CASE(
463463
Array array_r(ctx, array_name, TILEDB_READ);
464464
Subarray subarray(ctx, array_r);
465465

466+
// If read_range_oob_error is false, the range will be cropped with a
467+
// warning and the query will succeed.
468+
auto read_range_oob_error = GENERATE(true, false);
466469
auto expected = TILEDB_ERR;
470+
int fill_val = tiledb::sm::constants::empty_int32;
471+
std::vector<int> expected_data(16, fill_val);
472+
expected_data[0] = 1;
473+
expected_data[1] = 2;
474+
expected_data[4] = 3;
475+
expected_data[5] = 4;
467476
SECTION("- Upper bound OOB") {
468477
int range[] = {0, 100};
469478
auto r = Range(&range[0], &range[1], sizeof(int));
470-
CHECK(subarray.ptr().get()->subarray_->add_range_unsafe(0, r).ok());
479+
if (read_range_oob_error) {
480+
CHECK_FALSE(
481+
subarray.ptr()
482+
.get()
483+
->subarray_->add_range(0, std::move(r), read_range_oob_error)
484+
.ok());
485+
} else {
486+
CHECK(subarray.ptr()
487+
.get()
488+
->subarray_->add_range(0, std::move(r), read_range_oob_error)
489+
.ok());
490+
// The subarray will warn and crop to full domain ranges.
491+
expected = TILEDB_OK;
492+
}
471493
}
472494

473495
SECTION("- Lower bound OOB") {
474496
int range[] = {-1, 2};
475497
auto r = Range(&range[0], &range[1], sizeof(int));
476-
CHECK(subarray.ptr().get()->subarray_->add_range_unsafe(0, r).ok());
498+
if (read_range_oob_error) {
499+
CHECK_FALSE(
500+
subarray.ptr()
501+
.get()
502+
->subarray_->add_range(0, std::move(r), read_range_oob_error)
503+
.ok());
504+
} else {
505+
// Warn and crop dim0 to [0, 2] with [0, 3] implicitly set on dim1.
506+
CHECK(subarray.ptr()
507+
.get()
508+
->subarray_->add_range(0, std::move(r), read_range_oob_error)
509+
.ok());
510+
expected_data.resize(12);
511+
expected = TILEDB_OK;
512+
}
477513
}
478514

479515
SECTION("- Second range OOB") {
480516
int range[] = {1, 4};
481517
auto r = Range(&range[0], &range[1], sizeof(int));
482-
CHECK(subarray.ptr().get()->subarray_->add_range_unsafe(0, r).ok());
483518
int range2[] = {10, 20};
484519
auto r2 = Range(&range2[0], &range2[1], sizeof(int));
485-
CHECK(subarray.ptr().get()->subarray_->add_range_unsafe(1, r2).ok());
520+
if (read_range_oob_error) {
521+
CHECK_FALSE(
522+
subarray.ptr()
523+
.get()
524+
->subarray_->add_range(0, std::move(r), read_range_oob_error)
525+
.ok());
526+
CHECK_FALSE(
527+
subarray.ptr()
528+
.get()
529+
->subarray_->add_range(1, std::move(r2), read_range_oob_error)
530+
.ok());
531+
} else {
532+
// Warn and crop dim0 to [1, 3] and dim1 to [3, 3]
533+
CHECK(subarray.ptr()
534+
.get()
535+
->subarray_->add_range(0, std::move(r), read_range_oob_error)
536+
.ok());
537+
CHECK(subarray.ptr()
538+
.get()
539+
->subarray_->add_range(1, std::move(r2), read_range_oob_error)
540+
.ok());
541+
expected_data = {fill_val, fill_val, fill_val};
542+
expected = TILEDB_OK;
543+
}
486544
}
487545

488546
SECTION("- Valid ranges") {
489547
int range[] = {0, 1};
490548
auto r = Range(&range[0], &range[1], sizeof(int));
491-
CHECK(subarray.ptr().get()->subarray_->add_range_unsafe(0, r).ok());
492-
CHECK(subarray.ptr().get()->subarray_->add_range_unsafe(1, r).ok());
549+
CHECK(subarray.ptr()
550+
.get()
551+
->subarray_->add_range(0, std::move(r), read_range_oob_error)
552+
.ok());
553+
CHECK(subarray.ptr()
554+
.get()
555+
->subarray_->add_range(1, std::move(r), read_range_oob_error)
556+
.ok());
557+
expected_data = data_w;
493558
expected = TILEDB_OK;
494559
}
495560

496-
Query query(ctx, array_r);
497-
query.set_subarray(subarray);
498-
query.set_config(cfg);
499-
500-
std::vector<int> a(4);
501-
query.set_data_buffer("a", a);
502-
tiledb::test::ServerQueryBuffers buffers;
503-
CHECK(
504-
submit_query_wrapper(
505-
ctx, array_name, &query, buffers, true, query_v2, false) == expected);
506-
507-
if (expected == TILEDB_OK) {
508-
CHECK(query.query_status() == tiledb::Query::Status::COMPLETE);
509-
CHECK(a == std::vector<int>{1, 2, 3, 4});
561+
// If the Subarray threw an exception when adding OOB ranges it will be unset.
562+
if (!read_range_oob_error || expected == TILEDB_OK) {
563+
Query query(ctx, array_r);
564+
query.set_subarray(subarray);
565+
query.set_config(cfg);
566+
567+
std::vector<int> a(expected_data.size());
568+
query.set_data_buffer("a", a);
569+
tiledb::test::ServerQueryBuffers buffers;
570+
CHECK(
571+
submit_query_wrapper(
572+
ctx, array_name, &query, buffers, true, query_v2, false) ==
573+
expected);
574+
575+
if (expected == TILEDB_OK) {
576+
CHECK(query.query_status() == tiledb::Query::Status::COMPLETE);
577+
CHECK(a == expected_data);
578+
}
510579
}
511580

512581
if (vfs.is_dir(array_name)) {

‎tiledb/sm/subarray/subarray.cc‎

Lines changed: 11 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -358,17 +358,6 @@ Status Subarray::add_range(
358358
return Status::Ok();
359359
}
360360

361-
Status Subarray::add_range_unsafe(uint32_t dim_idx, const Range& range) {
362-
// Must reset the result size and tile overlap
363-
est_result_size_computed_ = false;
364-
tile_overlap_.clear();
365-
366-
// Add the range
367-
throw_if_not_ok(range_subset_[dim_idx].add_range_unrestricted(range));
368-
is_default_[dim_idx] = range_subset_[dim_idx].is_implicitly_initialized();
369-
return Status::Ok();
370-
}
371-
372361
Status Subarray::set_subarray(const void* subarray) {
373362
if (!array_->array_schema_latest().domain().all_dims_same_type())
374363
return LOG_STATUS(
@@ -2123,6 +2112,17 @@ void Subarray::add_default_ranges() {
21232112
add_default_label_ranges(dim_num);
21242113
}
21252114

2115+
Status Subarray::add_range_unsafe(uint32_t dim_idx, const Range& range) {
2116+
// Must reset the result size and tile overlap
2117+
est_result_size_computed_ = false;
2118+
tile_overlap_.clear();
2119+
2120+
// Add the range
2121+
throw_if_not_ok(range_subset_[dim_idx].add_range_unrestricted(range));
2122+
is_default_[dim_idx] = range_subset_[dim_idx].is_implicitly_initialized();
2123+
return Status::Ok();
2124+
}
2125+
21262126
void Subarray::add_default_label_ranges(dimension_size_type dim_num) {
21272127
label_range_subset_.clear();
21282128
label_range_subset_.resize(dim_num, nullopt);

‎tiledb/sm/subarray/subarray.h‎

Lines changed: 9 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -151,6 +151,9 @@ class ITileRange {
151151
*/
152152
class Subarray {
153153
public:
154+
/** Friend with Query to call `add_range_unsafe` */
155+
friend class Query;
156+
154157
/* ********************************* */
155158
/* PUBLIC DATA TYPES */
156159
/* ********************************* */
@@ -524,12 +527,6 @@ class Subarray {
524527
const void* end,
525528
uint64_t end_size);
526529

527-
/**
528-
* Adds a range along the dimension with the given index, without
529-
* performing any error checks.
530-
*/
531-
Status add_range_unsafe(uint32_t dim_idx, const Range& range);
532-
533530
/**
534531
* Adds a range to the (read/write) query on the input dimension by name,
535532
* in the form of (start, end, stride).
@@ -1554,6 +1551,12 @@ class Subarray {
15541551
*/
15551552
void add_default_ranges();
15561553

1554+
/**
1555+
* Adds a range along the dimension with the given index, without
1556+
* performing any error checks.
1557+
*/
1558+
Status add_range_unsafe(uint32_t dim_idx, const Range& range);
1559+
15571560
/** Computes the estimated result size for all attributes/dimensions. */
15581561
Status compute_est_result_size(const Config* config, ThreadPool* compute_tp);
15591562

‎tiledb/type/range/range.h‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -38,6 +38,7 @@
3838
#include "tiledb/common/tag.h"
3939
#include "tiledb/sm/enums/datatype.h"
4040

41+
#include <algorithm>
4142
#include <cmath>
4243
#include <cstring>
4344
#include <sstream>
@@ -461,8 +462,8 @@ template <
461462
void crop_range(const Range& bounds, Range& range) {
462463
auto bounds_data = (const T*)bounds.data();
463464
auto range_data = (T*)range.data();
464-
range_data[0] = std::max(bounds_data[0], range_data[0]);
465-
range_data[1] = std::min(bounds_data[1], range_data[1]);
465+
range_data[0] = std::clamp(range_data[0], bounds_data[0], bounds_data[1]);
466+
range_data[1] = std::clamp(range_data[1], bounds_data[0], bounds_data[1]);
466467
};
467468

468469
/**

0 commit comments

Comments
 (0)