chore: Attribute::get_enumeration_name returns a reference rather than a copy in order to add Rust binding (#5567)

Differences in copy and move semantics between Rust and C++ mean that
they are generally not very good at passing values across the boundary -
most of the time it is necessary to pass references instead.

#5566 intends to call `Attribute::get_enumeration_name` from Rust. Prior
to these changes, that function returns `std::optional<std::string>`,
which cannot be passed across the boundary (neither `std::optional` nor
`std::string` can be). We can either pass a pointer to a string, or pass
a string contained within a smart pointer.

To avoid the additional memory allocations required for both copying the
`std::string` and placing it in a `unique_ptr`, we choose the former.

However, `std::optional` does not naturally support references, so we
cannot do `std::optional<std::string&>`. Instead we change the return
type to `std::optional<std::reference_wrapper<std::string>>`.

In addition to enabling passing the result of this function to Rust
without allocating additional memory, this also avoids making additional
copies of the string. This is not likely to matter from a performance
perspective, but is still nice!

---
TYPE: NO_HISTORY
DESC: Rust binding for `Attribute::get_enumeration_name`

Author: rroelke

cmake/oxidize.cmake

@@ -306,10 +306,9 @@ if (TILEDB_RUST)
       set(bridge_h_src "${bridge_generated_dir}/src/${bridge}.h")
       set(bridge_cc_src "${bridge_generated_dir}/src/${bridge}.cc")
 
+      string(REPLACE "-" "_" bridge_sanitize_relpath ${bridge_sanitize_relpath})
       set(bridge_h_dst "${OXIDIZE_INCLUDE_DIR}/tiledb/oxidize/${bridge_sanitize_relpath}.h")
       set(bridge_cc_dst "${OXIDIZE_INCLUDE_DIR}/tiledb/oxidize/${bridge_sanitize_relpath}.cc")
-      string(REPLACE "-" "_" bridge_h_dst ${bridge_h_dst})
-      string(REPLACE "-" "_" bridge_cc_dst ${bridge_cc_dst})
 
       cmake_path(GET bridge_h_dst PARENT_PATH bridge_sanitize_dirname)
 

test/src/unit-enumerations.cc

@@ -1244,7 +1244,7 @@ TEST_CASE_METHOD(
   auto enmr_name = schema->attribute("attr1")->get_enumeration_name();
   REQUIRE(enmr_name.has_value());
 
-  auto enmr_path = schema->get_enumeration_path_name(enmr_name.value());
+  auto enmr_path = schema->get_enumeration_path_name(enmr_name.value().get());
 
   auto loaded =
       ad->load_enumerations_from_paths({enmr_path}, enc_key_, memory_tracker_);
@@ -1292,7 +1292,7 @@ TEST_CASE_METHOD(
   auto ad = get_array_directory();
 
   auto enmr_name = schema->attribute("attr1")->get_enumeration_name();
-  auto enmr_path = schema->get_enumeration_path_name(enmr_name.value());
+  auto enmr_path = schema->get_enumeration_path_name(enmr_name.value().get());
 
   memory_tracker_->set_budget(memory_tracker_->get_memory_usage() + 1);
 

test/support/src/helpers.cc

@@ -1636,7 +1636,13 @@ void schema_equiv(
     CHECK(a->name() == b->name());
     CHECK(a->type() == b->type());
     CHECK(a->nullable() == b->nullable());
-    CHECK(a->get_enumeration_name() == b->get_enumeration_name());
+
+    const auto a_enmr = a->get_enumeration_name(),
+               b_enmr = b->get_enumeration_name();
+    CHECK(a_enmr.has_value() == b_enmr.has_value());
+    if (a_enmr.has_value() && b_enmr.has_value()) {
+      CHECK(a_enmr.value().get() == b_enmr.value().get());
+    }
   }
   CHECK(schema1.capacity() == schema2.capacity());
   CHECK(schema1.cell_order() == schema2.cell_order());

tiledb/api/c_api/array_schema/array_schema_api.cc

@@ -250,9 +250,9 @@ capi_return_t tiledb_array_schema_get_enumeration_from_attribute_name(
     return TILEDB_OK;
   }
 
-  array_schema->load_enumeration(ctx, enumeration_name->c_str());
+  array_schema->load_enumeration(ctx, enumeration_name->get().c_str());
 
-  auto ptr = array_schema->get_enumeration(enumeration_name->c_str());
+  auto ptr = array_schema->get_enumeration(enumeration_name->get().c_str());
   *enumeration = tiledb_enumeration_handle_t::make_handle(ptr);
 
   return TILEDB_OK;

tiledb/api/c_api/attribute/attribute_api_internal.h

@@ -187,7 +187,12 @@ struct tiledb_attribute_handle_t
    * Facade for `Attribute` function
    */
   [[nodiscard]] std::optional<std::string> get_enumeration_name() const {
-    return attr_->get_enumeration_name();
+    const auto eref = attr_->get_enumeration_name();
+    if (eref.has_value()) {
+      return eref.value().get();
+    } else {
+      return std::nullopt;
+    }
   };
   /**
    * Facade for `Attribute` function

tiledb/oxidize/cxx-interface/cc/array_schema.cc

@@ -1,9 +1,25 @@
 #include "tiledb/oxidize/cxx-interface/cc/array_schema.h"
 
-namespace tiledb::oxidize {
-
 using namespace tiledb::sm;
 
+namespace tiledb::oxidize::sm {
+
+namespace attribute {
+
+const std::string* enumeration_name_cxx(const Attribute& attribute) {
+  std::optional<std::reference_wrapper<const std::string>> e =
+      attribute.get_enumeration_name();
+  if (e.has_value()) {
+    return &e.value().get();
+  } else {
+    return nullptr;
+  }
+}
+
+}  // namespace attribute
+
+namespace dimension {
+
 void set_domain(Dimension& dimension, rust::Slice<const uint8_t> domain) {
   dimension.set_domain(static_cast<const void*>(domain.data()));
 }
@@ -12,4 +28,6 @@ void set_tile_extent(Dimension& dimension, rust::Slice<const uint8_t> domain) {
   dimension.set_tile_extent(static_cast<const void*>(domain.data()));
 }
 
-}  // namespace tiledb::oxidize
+}  // namespace dimension
+
+}  // namespace tiledb::oxidize::sm

tiledb/oxidize/cxx-interface/cc/array_schema.h

@@ -4,14 +4,27 @@
 #include "tiledb/sm/array_schema/dimension.h"
 #include "tiledb/sm/array_schema/domain.h"
 
-namespace tiledb::oxidize {
+namespace tiledb::oxidize::sm {
 
 using namespace tiledb::sm;
 
+namespace attribute {
+
 using ConstAttribute = const Attribute;
-using ConstDimension = const Dimension;
+
+const std::string* enumeration_name_cxx(const Attribute& attribute);
+
+}  // namespace attribute
+
+namespace dimension {
+
+using namespace tiledb::sm;
+
+using ConstDimension = const tiledb::sm::Dimension;
 
 void set_domain(Dimension& dimension, rust::Slice<const uint8_t> domain);
 void set_tile_extent(Dimension& dimension, rust::Slice<const uint8_t> domain);
 
-}  // namespace tiledb::oxidize
+}  // namespace dimension
+
+}  // namespace tiledb::oxidize::sm

tiledb/oxidize/cxx-interface/src/sm/array_schema/mod.rs

@@ -6,7 +6,7 @@ mod ffi {
         type Layout = crate::sm::enums::Layout;
     }
 
-    #[namespace = "tiledb::oxidize"]
+    #[namespace = "tiledb::oxidize::sm::attribute"]
     unsafe extern "C++" {
         include!("tiledb/oxidize/cxx-interface/cc/array_schema.h");
         type ConstAttribute;
@@ -26,6 +26,9 @@ mod ffi {
         #[cxx_name = "type"]
         fn datatype(&self) -> Datatype;
 
+        #[namespace = "tiledb::oxidize::sm::attribute"]
+        fn enumeration_name_cxx(attr: &Attribute) -> *const CxxString;
+
         fn set_cell_val_num(self: Pin<&mut Attribute>, cell_val_num: u32);
     }
 
@@ -42,10 +45,10 @@ mod ffi {
         #[cxx_name = "type"]
         fn datatype(&self) -> Datatype;
 
-        #[namespace = "tiledb::oxidize"]
+        #[namespace = "tiledb::oxidize::sm::dimension"]
         fn set_domain(dimension: Pin<&mut Dimension>, domain: &[u8]) -> Result<()>;
 
-        #[namespace = "tiledb::oxidize"]
+        #[namespace = "tiledb::oxidize::sm::dimension"]
         fn set_tile_extent(dimension: Pin<&mut Dimension>, extent: &[u8]) -> Result<()>;
     }
 
@@ -63,6 +66,19 @@ mod ffi {
         fn add_dimension(self: Pin<&mut Domain>, dim: SharedPtr<Dimension>);
     }
 
+    #[namespace = "tiledb::sm"]
+    unsafe extern "C++" {
+        include!("tiledb/sm/array_schema/enumeration.h");
+
+        type Enumeration;
+
+        #[cxx_name = "cell_val_num"]
+        fn cell_val_num_cxx(&self) -> u32;
+
+        #[cxx_name = "type"]
+        fn datatype(&self) -> Datatype;
+    }
+
     #[namespace = "tiledb::sm"]
     unsafe extern "C++" {
         include!("tiledb/sm/array_schema/array_schema.h");
@@ -117,7 +133,7 @@ use std::str::Utf8Error;
 
 use num_traits::ToBytes;
 
-pub use ffi::{ArraySchema, Attribute, ConstAttribute, Datatype, Dimension, Domain};
+pub use ffi::{ArraySchema, Attribute, ConstAttribute, Datatype, Dimension, Domain, Enumeration};
 
 #[derive(Debug)]
 pub enum CellValNum {
@@ -222,6 +238,19 @@ impl Attribute {
         // SAFETY: non-zero would have been validated by the ArraySchema
         CellValNum::from_cxx(cxx).unwrap()
     }
+
+    pub fn enumeration_name_cxx(&self) -> *const cxx::CxxString {
+        ffi::enumeration_name_cxx(self)
+    }
+
+    pub fn enumeration_name(&self) -> Option<Result<&str, Utf8Error>> {
+        let ptr = self.enumeration_name_cxx();
+        if ptr.is_null() {
+            return None;
+        }
+        let cxx = unsafe { &*ptr };
+        Some(cxx.to_str())
+    }
 }
 
 impl Debug for Attribute {
@@ -274,6 +303,22 @@ impl Field<'_> {
             Self::Dimension(_) => false,
         }
     }
+
+    pub fn enumeration_name(&self) -> Option<Result<&str, Utf8Error>> {
+        match self {
+            Self::Attribute(a) => a.enumeration_name(),
+            Self::Dimension(_) => None,
+        }
+    }
+}
+
+impl Enumeration {
+    pub fn cell_val_num(&self) -> CellValNum {
+        let cxx = self.cell_val_num_cxx();
+
+        // SAFETY: non-zero would have been validated by the ArraySchema
+        CellValNum::from_cxx(cxx).unwrap()
+    }
 }
 
 impl ArraySchema {

tiledb/oxidize/test-support/cxx-interface/src/lib.rs

@@ -32,7 +32,7 @@ mod ffi {
         type ResultTile = tiledb_cxx_interface::sm::query::readers::ResultTile;
     }
 
-    #[namespace = "tiledb::oxidize"]
+    #[namespace = "tiledb::oxidize::sm::attribute"]
     extern "C++" {
         type ConstAttribute = tiledb_cxx_interface::sm::array_schema::ConstAttribute;
     }

tiledb/sm/array_schema/array_schema.cc

@@ -806,13 +806,13 @@ void ArraySchema::add_attribute(
 
   auto enmr_name = attr->get_enumeration_name();
   if (enmr_name.has_value()) {
-    // The referenced enumeration must exist when the attribut is added
-    auto iter = enumeration_map_.find(enmr_name.value());
+    // The referenced enumeration must exist when the attribute is added
+    auto iter = enumeration_map_.find(enmr_name.value().get());
     if (iter == enumeration_map_.end()) {
       std::string msg =
           "Cannot add attribute; Attribute refers to an "
           "unknown enumeration named '" +
-          enmr_name.value() + "'.";
+          enmr_name.value().get() + "'.";
       throw ArraySchemaException(msg);
     }
 
@@ -835,14 +835,15 @@ void ArraySchema::add_attribute(
     auto enmr = get_enumeration(enmr_name.value());
     if (enmr == nullptr) {
       throw ArraySchemaException(
-          "Cannot add attribute referencing enumeration '" + enmr_name.value() +
+          "Cannot add attribute referencing enumeration '" +
+          enmr_name.value().get() +
           "' as the enumeration has not been loaded.");
     }
 
     // The +1 here is because of 0 being a valid index into the enumeration.
     if (datatype_max_integral_value(attr->type()) <= enmr->elem_count()) {
       throw ArraySchemaException(
-          "Unable to use enumeration '" + enmr_name.value() +
+          "Unable to use enumeration '" + enmr_name.value().get() +
           "' for attribute '" + attr->name() +
           "' because the attribute's type is not large enough to represent "
           "all enumeration values.");
@@ -1147,7 +1148,7 @@ void ArraySchema::drop_enumeration(const std::string& enmr_name) {
     if (!attr_enmr_name.has_value()) {
       continue;
     }
-    if (attr_enmr_name.value() == enmr_name) {
+    if (attr_enmr_name.value().get() == enmr_name) {
       throw ArraySchemaException(
           "Unable to drop enumeration '" + enmr_name + "' as it is used by" +
           " attribute '" + attr->name() + "'.");