Skip to content

Commit

Permalink
address review comments
Browse files Browse the repository at this point in the history
  • Loading branch information
robertbindar committed Jul 8, 2024
1 parent 5a39b09 commit 0218ee7
Show file tree
Hide file tree
Showing 7 changed files with 58 additions and 18 deletions.
33 changes: 27 additions & 6 deletions test/src/unit-current-domain-rest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,8 @@ using namespace tiledb::test;
struct RestCurrentDomainFx {
RestCurrentDomainFx();

void create_sparse_array(const std::string& array_name);
void create_sparse_array(
const std::string& array_name, bool empty_ndr = false);

VFSTestSetup vfs_test_setup_;

Expand All @@ -54,7 +55,8 @@ RestCurrentDomainFx::RestCurrentDomainFx()
: ctx_c_(vfs_test_setup_.ctx_c) {
}

void RestCurrentDomainFx::create_sparse_array(const std::string& array_name) {
void RestCurrentDomainFx::create_sparse_array(
const std::string& array_name, bool empty_ndr) {
uri_ = vfs_test_setup_.array_uri(array_name);

// Create dimensions
Expand Down Expand Up @@ -107,9 +109,11 @@ void RestCurrentDomainFx::create_sparse_array(const std::string& array_name) {
range.min_size = sizeof(uint64_t);
range.max = &max;
range.max_size = sizeof(uint64_t);
REQUIRE(
tiledb_ndrectangle_set_range_for_name(ctx_c_, ndr, "d1", &range) ==
TILEDB_OK);
if (!empty_ndr) {
REQUIRE(
tiledb_ndrectangle_set_range_for_name(ctx_c_, ndr, "d1", &range) ==
TILEDB_OK);
}
REQUIRE(tiledb_current_domain_set_ndrectangle(crd, ndr) == TILEDB_OK);
REQUIRE(
tiledb_array_schema_set_current_domain(ctx_c_, array_schema, crd) ==
Expand All @@ -121,7 +125,20 @@ void RestCurrentDomainFx::create_sparse_array(const std::string& array_name) {

// Create array
rc = tiledb_array_create(ctx_c_, uri_.c_str(), array_schema);
CHECK(rc == TILEDB_OK);
if (empty_ndr) {
CHECK(rc == TILEDB_ERR);
tiledb_error_t* err;
CHECK(tiledb_ctx_get_last_error(ctx_c_, &err) == TILEDB_OK);
const char* errmsg;
CHECK(tiledb_error_message(err, &errmsg) == TILEDB_OK);
CHECK_THAT(
errmsg,
Catch::Matchers::Equals("NDRectangle serialization failed. The "
"NDRectangle on the array current "
"domain has no ranges set"));
} else {
CHECK(rc == TILEDB_OK);
}

// Clean up
REQUIRE(tiledb_ndrectangle_free(&ndr) == TILEDB_OK);
Expand All @@ -136,6 +153,10 @@ TEST_CASE_METHOD(
RestCurrentDomainFx,
"C API: Current Domain basic behavior",
"[current_domain][rest]") {
// Check that an empty NDRectangle set on a current domain throws on the
// client
create_sparse_array("currentdomain_array", true);

create_sparse_array("currentdomain_array");

// Open array, read back current domain from schema and check
Expand Down
8 changes: 4 additions & 4 deletions tiledb/sm/array_schema/ndrectangle.cc
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,7 @@ shared_ptr<NDRectangle> NDRectangle::deserialize(

void NDRectangle::serialize(Serializer& serializer) const {
for (unsigned d = 0; d < range_data_.size(); ++d) {
auto dim{domain_->dimension_ptr(d)};
auto dim{domain()->dimension_ptr(d)};
const auto& r = range_data_[d];
if (!dim->var_size()) { // Fixed-sized
serializer.write(r.data(), r.size());
Expand All @@ -109,7 +109,7 @@ void NDRectangle::dump(FILE* out) const {
std::stringstream ss;
ss << " - NDRectangle ###" << std::endl;
for (uint32_t i = 0; i < range_data_.size(); ++i) {
auto dtype = domain_->dimension_ptr(i)->type();
auto dtype = domain()->dimension_ptr(i)->type();
ss << " - " << range_str(range_data_[i], dtype) << std::endl;
}

Expand All @@ -125,7 +125,7 @@ void NDRectangle::set_range(const Range& r, uint32_t idx) {
}

void NDRectangle::set_range_for_name(const Range& r, const std::string& name) {
auto idx = domain_->get_dimension_index(name);
auto idx = domain()->get_dimension_index(name);
set_range(r, idx);
}

Expand All @@ -138,7 +138,7 @@ const Range& NDRectangle::get_range(uint32_t idx) const {
}

const Range& NDRectangle::get_range_for_name(const std::string& name) const {
auto idx = domain_->get_dimension_index(name);
auto idx = domain()->get_dimension_index(name);
return get_range(idx);
}

Expand Down
12 changes: 11 additions & 1 deletion tiledb/sm/array_schema/ndrectangle.h
Original file line number Diff line number Diff line change
Expand Up @@ -135,6 +135,12 @@ class NDRectangle {

/** domain accessor */
shared_ptr<Domain> domain() const {
// Guards for a special cased behavior in REST array schema evolution, see
// domain_ declaration for a more detailed explanation
if (domain_ == nullptr) {
throw std::runtime_error(
"The Domain instance on this NDRectangle is nullptr");
}
return domain_;
}

Expand Down Expand Up @@ -187,7 +193,11 @@ class NDRectangle {
/** Per dimension ranges of the rectangle */
NDRange range_data_;

/** Array Schema domain */
/**
* Array Schema domain. This can be nullptr during
* array schema evolution on REST when we construct with a nullptr domain_
* and set it later during ArraySchema::expand_current_domain to avoid
* serializing the domain on the evolution object */
shared_ptr<Domain> domain_;
};

Expand Down
4 changes: 2 additions & 2 deletions tiledb/sm/enums/current_domain_type.h
Original file line number Diff line number Diff line change
Expand Up @@ -51,13 +51,13 @@ enum class CurrentDomainType : uint8_t {
};

/** Returns the string representation of the input current domain type. */
inline const std::string& current_domain_type_str(
inline const std::string current_domain_type_str(
CurrentDomainType current_domain_type) {
switch (current_domain_type) {
case CurrentDomainType::NDRECTANGLE:
return constants::current_domain_ndrectangle_str;
default:
return constants::empty_str;
return std::to_string(static_cast<uint8_t>(current_domain_type));
}
}

Expand Down
6 changes: 4 additions & 2 deletions tiledb/sm/serialization/array_schema.cc
Original file line number Diff line number Diff line change
Expand Up @@ -1153,11 +1153,13 @@ shared_ptr<ArraySchema> array_schema_from_capnp(
name = schema_reader.getName().cStr();
}

auto crd = make_shared<CurrentDomain>(
memory_tracker, constants::current_domain_version);
std::shared_ptr<CurrentDomain> crd;
if (schema_reader.hasCurrentDomain()) {
crd = current_domain_from_capnp(
schema_reader.getCurrentDomain(), domain, memory_tracker);
} else {
crd = make_shared<CurrentDomain>(
memory_tracker, constants::current_domain_version);
}

return make_shared<ArraySchema>(
Expand Down
8 changes: 5 additions & 3 deletions tiledb/sm/serialization/array_schema_evolution.cc
Original file line number Diff line number Diff line change
Expand Up @@ -155,9 +155,11 @@ Status array_schema_evolution_to_capnp(
timestamp_builder.set(1, timestamp_range.second);

auto crd = array_schema_evolution->current_domain_to_expand();
auto current_domain_builder =
array_schema_evolution_builder->initCurrentDomainToExpand();
current_domain_to_capnp(crd, &current_domain_builder);
if (crd != nullptr) {
auto current_domain_builder =
array_schema_evolution_builder->initCurrentDomainToExpand();
current_domain_to_capnp(crd, &current_domain_builder);
}

return Status::Ok();
}
Expand Down
5 changes: 5 additions & 0 deletions tiledb/sm/serialization/current_domain.cc
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,12 @@ void ndrectangle_to_capnp(
// only one range per dimension
range_buffers_to_capnp({ranges[i]}, range_builder);
}
return;
}

throw std::logic_error(
"NDRectangle serialization failed. The NDRectangle on the array current "
"domain has no ranges set");
}

shared_ptr<CurrentDomain> current_domain_from_capnp(
Expand Down

0 comments on commit 0218ee7

Please sign in to comment.