Changes from review.

TileDB-Inc · Aug 19, 2024 · a588737 · a588737
1 parent 1829491
commit a588737
Show file tree

Hide file tree

Showing 8 changed files with 69 additions and 29 deletions.
diff --git a/tiledb/sm/array_schema/array_schema_operations.cc b/tiledb/sm/array_schema/array_schema_operations.cc
@@ -243,7 +243,7 @@ shared_ptr<ArraySchema> load_array_schema(
     auto&& [st, array_schema_response] =
         rest_client.get_array_schema_from_rest(uri);
     throw_if_not_ok(st);
-    return std::move(array_schema_response.value());
+    return std::move(array_schema_response).value();
   } else {
     // Create key
     tiledb::sm::EncryptionKey key;

diff --git a/tiledb/sm/c_api/tiledb.cc b/tiledb/sm/c_api/tiledb.cc
@@ -460,7 +460,7 @@ int32_t tiledb_array_schema_load(
   ensure_output_pointer_is_valid(array_schema);
   *array_schema = new (std::nothrow) tiledb_array_schema_t;
   if (*array_schema == nullptr) {
-    throw CAPIStatusException("Failed to allocate TileDB array schema object");
+    throw std::bad_alloc();
   }
 
   try {
@@ -475,7 +475,7 @@ int32_t tiledb_array_schema_load(
   return TILEDB_OK;
 }
 
-int32_t tiledb_array_schema_load_with_options(
+int32_t tiledb_array_schema_load_with_config(
     tiledb_ctx_t* ctx,
     tiledb_config_t* config,
     const char* array_uri,
@@ -5000,12 +5000,12 @@ CAPI_INTERFACE(
 }
 
 CAPI_INTERFACE(
-    array_schema_load_with_options,
+    array_schema_load_with_config,
     tiledb_ctx_t* ctx,
     tiledb_config_t* config,
     const char* array_uri,
     tiledb_array_schema_t** array_schema) {
-  return api_entry<tiledb::api::tiledb_array_schema_load_with_options>(
+  return api_entry<tiledb::api::tiledb_array_schema_load_with_config>(
       ctx, config, array_uri, array_schema);
 }
 

diff --git a/tiledb/sm/c_api/tiledb.h b/tiledb/sm/c_api/tiledb.h
@@ -603,8 +603,8 @@ TILEDB_EXPORT int32_t tiledb_array_schema_check(
     tiledb_ctx_t* ctx, tiledb_array_schema_t* array_schema) TILEDB_NOEXCEPT;
 
 /**
- * Retrieves the schema of an array from the disk, creating an array schema
- * struct. The schema retrieved will always be the latest schema for the array.
+ * Retrieves the latest schema of an array from the disk, creating an array
+ * schema struct.
  *
  * **Example:**
  *
@@ -637,7 +637,7 @@ TILEDB_EXPORT int32_t tiledb_array_schema_load(
  *
  * @code{.c}
  * tiledb_array_schema_t* array_schema;
- * tiledb_array_schema_load_with_options(
+ * tiledb_array_schema_load_with_config(
  *     ctx,
  *     config,
  *     "s3://tiledb_bucket/my_array",
@@ -651,7 +651,7 @@ TILEDB_EXPORT int32_t tiledb_array_schema_load(
  * @param array_schema The array schema to be retrieved, or `NULL` upon error.
  * @return `TILEDB_OK` for success and `TILEDB_OOM` or `TILEDB_ERR` for error.
  */
-TILEDB_EXPORT int32_t tiledb_array_schema_load_with_options(
+TILEDB_EXPORT int32_t tiledb_array_schema_load_with_config(
     tiledb_ctx_t* ctx,
     tiledb_config_t* config,
     const char* array_uri,

diff --git a/tiledb/sm/serialization/array_schema.cc b/tiledb/sm/serialization/array_schema.cc
@@ -1850,6 +1850,9 @@ void load_array_schema_request_to_capnp(
     const LoadArraySchemaRequest& req) {
   auto config_builder = builder.initConfig();
   throw_if_not_ok(config_to_capnp(config, &config_builder));
+  // This boolean is only serialized to support clients using TileDB < 2.26.
+  // Future options should only be serialized within the Config object above.
+  builder.setIncludeEnumerations(req.include_enumerations());
 }
 
 void serialize_load_array_schema_request(
@@ -1907,6 +1910,8 @@ LoadArraySchemaRequest load_array_schema_request_from_capnp(
     capnp::LoadArraySchemaRequest::Reader& reader) {
   tdb_unique_ptr<Config> decoded_config = nullptr;
   throw_if_not_ok(config_from_capnp(reader.getConfig(), &decoded_config));
+  // We intentionally do not use the includeEnumerations field, as it is stored
+  // in the Config and set using the LoadArraySchemaRequest constructor.
   return LoadArraySchemaRequest(*decoded_config);
 }
 

diff --git a/tiledb/sm/serialization/array_schema.h b/tiledb/sm/serialization/array_schema.h
@@ -60,20 +60,16 @@ namespace serialization {
 class LoadArraySchemaRequest {
  public:
   explicit LoadArraySchemaRequest(const Config& config)
-      : config_(config) {
-  }
-
-  inline const Config& config() const {
-    return config_;
+      : include_enumerations_(config.get<bool>(
+            "rest.load_enumerations_on_array_open", Config::must_find)) {
   }
 
   inline bool include_enumerations() const {
-    return config_.get<bool>(
-        "rest.load_enumerations_on_array_open", config_.must_find);
+    return include_enumerations_;
   }
 
  private:
-  Config config_;
+  bool include_enumerations_;
 };
 
 #ifdef TILEDB_SERIALIZATION

diff --git a/tiledb/sm/serialization/tiledb-rest.capnp b/tiledb/sm/serialization/tiledb-rest.capnp
@@ -1264,6 +1264,11 @@ struct LoadEnumerationsResponse {
 struct LoadArraySchemaRequest {
   config @0 :Config;
   # Config
+
+  includeEnumerations @1 :Bool;
+  # When true, include all enumeration data in the returned ArraySchema
+  # This field is only serialized for backwards compatibility. Future options
+  # that modify array schema load behavior should be handled within the Config.
 }
 
 struct LoadArraySchemaResponse {

diff --git a/tiledb/sm/serialization/tiledb-rest.capnp.c++ b/tiledb/sm/serialization/tiledb-rest.capnp.c++
@@ -9731,17 +9731,17 @@ const ::capnp::_::RawSchema s_805c080c10c1e959 = {
   1, 1, i_805c080c10c1e959, nullptr, nullptr, { &s_805c080c10c1e959, nullptr, nullptr, 0, 0, nullptr }, false
 };
 #endif  // !CAPNP_LITE
-static const ::capnp::_::AlignedData<35> b_83f094010132ff21 = {
+static const ::capnp::_::AlignedData<52> b_83f094010132ff21 = {
   {   0,   0,   0,   0,   5,   0,   6,   0,
      33, 255,  50,   1,   1, 148, 240, 131,
-     18,   0,   0,   0,   1,   0,   0,   0,
+     18,   0,   0,   0,   1,   0,   1,   0,
     127, 216, 135, 181,  36, 146, 125, 181,
       1,   0,   7,   0,   0,   0,   0,   0,
       0,   0,   0,   0,   0,   0,   0,   0,
      21,   0,   0,   0,  74,   1,   0,   0,
      41,   0,   0,   0,   7,   0,   0,   0,
       0,   0,   0,   0,   0,   0,   0,   0,
-     37,   0,   0,   0,  63,   0,   0,   0,
+     37,   0,   0,   0, 119,   0,   0,   0,
       0,   0,   0,   0,   0,   0,   0,   0,
       0,   0,   0,   0,   0,   0,   0,   0,
     116, 105, 108, 101, 100,  98,  45, 114,
@@ -9751,20 +9751,37 @@ static const ::capnp::_::AlignedData<35> b_83f094010132ff21 = {
      97,  82, 101, 113, 117, 101, 115, 116,
       0,   0,   0,   0,   0,   0,   0,   0,
       0,   0,   0,   0,   1,   0,   1,   0,
-      4,   0,   0,   0,   3,   0,   4,   0,
+      8,   0,   0,   0,   3,   0,   4,   0,
       0,   0,   0,   0,   0,   0,   0,   0,
       0,   0,   1,   0,   0,   0,   0,   0,
       0,   0,   0,   0,   0,   0,   0,   0,
-     13,   0,   0,   0,  58,   0,   0,   0,
+     41,   0,   0,   0,  58,   0,   0,   0,
       0,   0,   0,   0,   0,   0,   0,   0,
-      8,   0,   0,   0,   3,   0,   1,   0,
-     20,   0,   0,   0,   2,   0,   1,   0,
+     36,   0,   0,   0,   3,   0,   1,   0,
+     48,   0,   0,   0,   2,   0,   1,   0,
+      1,   0,   0,   0,   0,   0,   0,   0,
+      0,   0,   1,   0,   1,   0,   0,   0,
+      0,   0,   0,   0,   0,   0,   0,   0,
+     45,   0,   0,   0, 162,   0,   0,   0,
+      0,   0,   0,   0,   0,   0,   0,   0,
+     48,   0,   0,   0,   3,   0,   1,   0,
+     60,   0,   0,   0,   2,   0,   1,   0,
      99, 111, 110, 102, 105, 103,   0,   0,
      16,   0,   0,   0,   0,   0,   0,   0,
      54, 173,  17, 129,  75,  91, 201, 182,
       0,   0,   0,   0,   0,   0,   0,   0,
       0,   0,   0,   0,   0,   0,   0,   0,
      16,   0,   0,   0,   0,   0,   0,   0,
+      0,   0,   0,   0,   0,   0,   0,   0,
+      0,   0,   0,   0,   0,   0,   0,   0,
+    105, 110,  99, 108, 117, 100, 101,  69,
+    110, 117, 109, 101, 114,  97, 116, 105,
+    111, 110, 115,   0,   0,   0,   0,   0,
+      1,   0,   0,   0,   0,   0,   0,   0,
+      0,   0,   0,   0,   0,   0,   0,   0,
+      0,   0,   0,   0,   0,   0,   0,   0,
+      0,   0,   0,   0,   0,   0,   0,   0,
+      1,   0,   0,   0,   0,   0,   0,   0,
       0,   0,   0,   0,   0,   0,   0,   0,
       0,   0,   0,   0,   0,   0,   0,   0, }
 };
@@ -9773,11 +9790,11 @@ static const ::capnp::_::AlignedData<35> b_83f094010132ff21 = {
 static const ::capnp::_::RawSchema* const d_83f094010132ff21[] = {
   &s_b6c95b4b8111ad36,
 };
-static const uint16_t m_83f094010132ff21[] = {0};
-static const uint16_t i_83f094010132ff21[] = {0};
+static const uint16_t m_83f094010132ff21[] = {0, 1};
+static const uint16_t i_83f094010132ff21[] = {0, 1};
 const ::capnp::_::RawSchema s_83f094010132ff21 = {
-  0x83f094010132ff21, b_83f094010132ff21.words, 35, d_83f094010132ff21, m_83f094010132ff21,
-  1, 1, i_83f094010132ff21, nullptr, nullptr, { &s_83f094010132ff21, nullptr, nullptr, 0, 0, nullptr }, false
+  0x83f094010132ff21, b_83f094010132ff21.words, 52, d_83f094010132ff21, m_83f094010132ff21,
+  1, 2, i_83f094010132ff21, nullptr, nullptr, { &s_83f094010132ff21, nullptr, nullptr, 0, 0, nullptr }, false
 };
 #endif  // !CAPNP_LITE
 static const ::capnp::_::AlignedData<69> b_ebe17f59ac9a1df1 = {

diff --git a/tiledb/sm/serialization/tiledb-rest.capnp.h b/tiledb/sm/serialization/tiledb-rest.capnp.h
@@ -1667,7 +1667,7 @@ struct LoadArraySchemaRequest {
   class Pipeline;
 
   struct _capnpPrivate {
-    CAPNP_DECLARE_STRUCT_HEADER(83f094010132ff21, 0, 1)
+    CAPNP_DECLARE_STRUCT_HEADER(83f094010132ff21, 1, 1)
 #if !CAPNP_LITE
     static constexpr ::capnp::_::RawBrandedSchema const* brand() {
       return &schema->defaultBrand;
@@ -14925,6 +14925,8 @@ class LoadArraySchemaRequest::Reader {
   inline bool hasConfig() const;
   inline ::tiledb::sm::serialization::capnp::Config::Reader getConfig() const;
 
+  inline bool getIncludeEnumerations() const;
+
  private:
   ::capnp::_::StructReader _reader;
   template <typename, ::capnp::Kind>
@@ -14974,6 +14976,9 @@ class LoadArraySchemaRequest::Builder {
   inline ::capnp::Orphan<::tiledb::sm::serialization::capnp::Config>
   disownConfig();
 
+  inline bool getIncludeEnumerations();
+  inline void setIncludeEnumerations(bool value);
+
  private:
   ::capnp::_::StructBuilder _builder;
   template <typename, ::capnp::Kind>
@@ -33148,6 +33153,18 @@ LoadArraySchemaRequest::Builder::disownConfig() {
           _builder.getPointerField(::capnp::bounded<0>() * ::capnp::POINTERS));
 }
 
+inline bool LoadArraySchemaRequest::Reader::getIncludeEnumerations() const {
+  return _reader.getDataField<bool>(::capnp::bounded<0>() * ::capnp::ELEMENTS);
+}
+
+inline bool LoadArraySchemaRequest::Builder::getIncludeEnumerations() {
+  return _builder.getDataField<bool>(::capnp::bounded<0>() * ::capnp::ELEMENTS);
+}
+inline void LoadArraySchemaRequest::Builder::setIncludeEnumerations(
+    bool value) {
+  _builder.setDataField<bool>(::capnp::bounded<0>() * ::capnp::ELEMENTS, value);
+}
+
 inline bool LoadArraySchemaResponse::Reader::hasSchema() const {
   return !_reader.getPointerField(::capnp::bounded<0>() * ::capnp::POINTERS)
               .isNull();