From 0218ee7f3d6e5fc592f7d95fee1b9e988b64f775 Mon Sep 17 00:00:00 2001 From: Robert Bindar Date: Mon, 8 Jul 2024 13:36:44 +0300 Subject: [PATCH] address review comments --- test/src/unit-current-domain-rest.cc | 33 +++++++++++++++---- tiledb/sm/array_schema/ndrectangle.cc | 8 ++--- tiledb/sm/array_schema/ndrectangle.h | 12 ++++++- tiledb/sm/enums/current_domain_type.h | 4 +-- tiledb/sm/serialization/array_schema.cc | 6 ++-- .../serialization/array_schema_evolution.cc | 8 +++-- tiledb/sm/serialization/current_domain.cc | 5 +++ 7 files changed, 58 insertions(+), 18 deletions(-) diff --git a/test/src/unit-current-domain-rest.cc b/test/src/unit-current-domain-rest.cc index 5628fd63ab3b..7d8acef86768 100644 --- a/test/src/unit-current-domain-rest.cc +++ b/test/src/unit-current-domain-rest.cc @@ -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_; @@ -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 @@ -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) == @@ -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); @@ -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 diff --git a/tiledb/sm/array_schema/ndrectangle.cc b/tiledb/sm/array_schema/ndrectangle.cc index bc0df55855ce..aabe4d4297e4 100644 --- a/tiledb/sm/array_schema/ndrectangle.cc +++ b/tiledb/sm/array_schema/ndrectangle.cc @@ -87,7 +87,7 @@ shared_ptr 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()); @@ -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; } @@ -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); } @@ -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); } diff --git a/tiledb/sm/array_schema/ndrectangle.h b/tiledb/sm/array_schema/ndrectangle.h index 461ca83076ca..654212c66a4f 100644 --- a/tiledb/sm/array_schema/ndrectangle.h +++ b/tiledb/sm/array_schema/ndrectangle.h @@ -135,6 +135,12 @@ class NDRectangle { /** domain accessor */ shared_ptr 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_; } @@ -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_; }; diff --git a/tiledb/sm/enums/current_domain_type.h b/tiledb/sm/enums/current_domain_type.h index 7bbd5ed3b8a0..10e3800e3c51 100644 --- a/tiledb/sm/enums/current_domain_type.h +++ b/tiledb/sm/enums/current_domain_type.h @@ -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(current_domain_type)); } } diff --git a/tiledb/sm/serialization/array_schema.cc b/tiledb/sm/serialization/array_schema.cc index c77620ffe35a..74a7df1d7caf 100644 --- a/tiledb/sm/serialization/array_schema.cc +++ b/tiledb/sm/serialization/array_schema.cc @@ -1153,11 +1153,13 @@ shared_ptr array_schema_from_capnp( name = schema_reader.getName().cStr(); } - auto crd = make_shared( - memory_tracker, constants::current_domain_version); + std::shared_ptr crd; if (schema_reader.hasCurrentDomain()) { crd = current_domain_from_capnp( schema_reader.getCurrentDomain(), domain, memory_tracker); + } else { + crd = make_shared( + memory_tracker, constants::current_domain_version); } return make_shared( diff --git a/tiledb/sm/serialization/array_schema_evolution.cc b/tiledb/sm/serialization/array_schema_evolution.cc index 792b9be12f48..6d443df499e5 100644 --- a/tiledb/sm/serialization/array_schema_evolution.cc +++ b/tiledb/sm/serialization/array_schema_evolution.cc @@ -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, ¤t_domain_builder); + if (crd != nullptr) { + auto current_domain_builder = + array_schema_evolution_builder->initCurrentDomainToExpand(); + current_domain_to_capnp(crd, ¤t_domain_builder); + } return Status::Ok(); } diff --git a/tiledb/sm/serialization/current_domain.cc b/tiledb/sm/serialization/current_domain.cc index a8097949fa5b..b569f47496ef 100644 --- a/tiledb/sm/serialization/current_domain.cc +++ b/tiledb/sm/serialization/current_domain.cc @@ -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 current_domain_from_capnp(