Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove serialization non C.41 constructors from Subarray. #4685

Open
wants to merge 10 commits into
base: dev
Choose a base branch
from
+339 −212
Open

Remove serialization non C.41 constructors from Subarray. #4685

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
7464c41
Add C.41 Subarray constructor
shaunrd0 Jan 30, 2024
fc6c31f
Fix test
shaunrd0 Jan 31, 2024
1f98d52
Fix tests
shaunrd0 Feb 9, 2024
3ac3cf3
Remove set_subarray_unsafe
shaunrd0 Feb 9, 2024
cff8cdc
Clean code
shaunrd0 Feb 12, 2024
6c79915
Update Stats deserialization
shaunrd0 Feb 12, 2024
b6009a6
Feedback from review
shaunrd0 Feb 15, 2024
b86a687
Use reserve for label_range_subset
shaunrd0 Feb 16, 2024
712fa4b
Make Subarray::add_range_unsafe private
shaunrd0 Feb 16, 2024
c9e4726
Making methods public again.
KiterLuc Feb 29, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
109 changes: 89 additions & 20 deletions test/src/unit-cppapi-subarray.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment is too short to be easily comprehensible. It requires knowing what the badly-named-from-the-beginning parameter read_range_oob_error does, which is only changeable through a configuration variable. It's worth expanding on that here.

auto read_range_oob_error = GENERATE(true, false);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The structure of these out-of-bounds tests is overly complicated.

  • They're buried inside a serialization test, but the add_range tests could and should be tested on a plain Subarray object without using a query to do it. The test vectors (subarray definition, whether it's out of bounds, expected results, etc.) can be defined in tabular form outside the definition of any test case.
  • Common test vectors should run slightly differently. These distinctions follow the principle that (1) test setup code should throw and (2) tests themselves should use CHECK etc.
    • For testing Subarray, all the vectors get used. CHECK should be used on add_range.
    • For testing queries, only the test vectors that don't fail add_range should be run. Failures of add_range should throw. CHECK should only be used for the query operations.
  • The OOB sections have a whole lot of copypasta. Using a loop over test vectors with DYNAMIC_SECTION would be much clearer.
  • When add_range converts away from Status, it'll throw instead. It might be better to use throw_if_not_ok in these tests. The occurrences of CHECK_FALSE can be replaced with CHECK_THROWS if testing class Subarray directly

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));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's a Range constructor that allows more direct initialization from constants. It's relatively recent.

  Range r{Tag<int>, 0, 100};

This constructor allows Range objects to statically initialize member variables, as needed for tables of test vectors.

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.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Huh? There's no exception handling in the code as written.

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)) {
Expand Down
11 changes: 8 additions & 3 deletions test/support/src/serialization_wrappers.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@
*/

#include "test/support/src/helpers.h"
#include "tiledb/api/c_api/context/context_api_internal.h"
#include "tiledb/sm/c_api/tiledb.h"
#include "tiledb/sm/c_api/tiledb_serialization.h"
#include "tiledb/sm/c_api/tiledb_struct_def.h"
Expand Down Expand Up @@ -204,11 +205,15 @@ void tiledb_subarray_serialize(
.ok());
// Deserialize
tiledb_subarray_t* deserialized_subarray;
auto layout = (*subarray)->subarray_->layout();
auto stats = ctx->storage_manager()->stats()->create_child("Subarray");
shared_ptr<Logger> dummy_logger = make_shared<Logger>(HERE(), "");

tiledb::test::require_tiledb_ok(
ctx, tiledb_subarray_alloc(ctx, array, &deserialized_subarray));
REQUIRE(tiledb::sm::serialization::subarray_from_capnp(
builder, deserialized_subarray->subarray_)
.ok());
*deserialized_subarray->subarray_ =
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This pointer should be check against nullptr before dereferencing. It shouldn't be null, of course, but this is test code. If it's null, it's fine to throw a runtime error, since this is test setup code.

tiledb::sm::serialization::subarray_from_capnp(
builder, array->array_.get(), layout, stats, dummy_logger);
*subarray = deserialized_subarray;
#endif
}
9 changes: 0 additions & 9 deletions tiledb/sm/query/query.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1528,11 +1528,6 @@ const Subarray* Query::subarray() const {
return &subarray_;
}

Status Query::set_subarray_unsafe(const Subarray& subarray) {
subarray_ = subarray;
return Status::Ok();
}

void Query::set_subarray(const tiledb::sm::Subarray& subarray) {
// Perform checks related to the query type.
switch (type_) {
Expand Down Expand Up @@ -1583,10 +1578,6 @@ Status Query::set_subarray_unsafe(const NDRange& subarray) {
return Status::Ok();
}

void Query::set_subarray_unsafe(const void* subarray) {
subarray_.set_subarray_unsafe(subarray);
}

Status Query::submit() {
// Do not resubmit completed reads.
if (type_ == QueryType::READ && status_ == QueryStatus::COMPLETED) {
Expand Down
12 changes: 0 additions & 12 deletions tiledb/sm/query/query.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -617,21 +617,9 @@ class Query {
*/
void set_subarray(const tiledb::sm::Subarray& subarray);

/** Sets the query subarray, without performing any checks. */
Status set_subarray_unsafe(const Subarray& subarray);

/** Sets the query subarray, without performing any checks. */
Status set_subarray_unsafe(const NDRange& subarray);

/**
* Sets the query subarray without performing any checks.
*
* Used for deserialize dense writes.
*
* @param subarray The subarray to be set.
*/
void set_subarray_unsafe(const void* subarray);

/** Submits the query to the storage manager. */
Status submit();

Expand Down
Loading
Loading