C API attribute handle #3982

eric-hughes-tiledb · 2023-03-20T20:51:31Z

Change C API attribute handle to new class tiledb_attribute_handle_t. Move all declarations and definitions of attribute-section API functions into tiledb/api/c_api/attribute/. Handle class is a complete facade around Attribute; C API functions do not access such objects directly. Add exhaustive argument checking in a new attribute handle unit test runner.

Rewrite remaining functions within tiledb.cc that use attributes. Supporting this, reworked a few things in serialization and schema evolution involving attribute arguments to use shared_ptr rather than *.

Removed sanity_check for attributes, replacing it with ensure_attribute_is_valid.

Change class Attribute in some small but textually-significant ways: removed some dead code (including a default constructor) and inlined a number of simple functions. More significantly, remove a non-standard Attribute constructor and replace it with one in standard form. Reworked a number of tests that were relying on this constructor.

Add support in class ArraySchema to return shared_ptr to its internal attributes rather than plain pointers. This simplifies the C API function that return the attributes of a schema.

Create testsupport_capi_datatype.h to hold the INVALID_DATATYPE() function for argument testing.

Fix a defect where tiledb_attribute_alloc was not checking for valid datatype and was able to create invalid attributes. Add null argument checks in most attribute-section functions. All but a few were sources of unrecognized defects; those few were checking for nullptr themselves already.

TYPE: IMPROVEMENT
DESC: C API attribute handle

eric-hughes-tiledb · 2023-03-20T20:57:01Z

Note: This branch is based on eh/capi-handle-dimension rather than dev. #3947 should merge before this one.

eric-hughes-tiledb · 2023-03-22T14:26:48Z

Rebased to current dev

eric-hughes-tiledb · 2023-03-29T17:42:55Z

The new functions ArraySchema::shared_attribute (in two signatures) are tested in a subsequent PR #4004. We've had inadequate support for inside-the-API testing, and I chose to leave the tests out of this PR and to combine them with some new support classes.

bekadavis9

New Handle looks great.
Just a couple clarification questions, mostly documentation nits

bekadavis9 · 2023-03-31T19:39:41Z

tiledb/sm/array_schema/attribute.cc

-  fill_value_validity_ = attr->fill_value_validity_;
-  order_ = attr->order_;
+Attribute::Attribute(const Attribute& attr) {
+  name_ = attr.name();


Why don't / can't you use an initializer list here?

Certainly could. I didn't change it in this PR just to minimize the number of changes and avoid analyzing what the old code had been doing.

The result is that we don't need anything but the default copy constructor.

bekadavis9 · 2023-03-31T19:46:09Z

tiledb/sm/array_schema/array_schema.h

  /**
   * Returns a constant pointer to the selected attribute (nullptr if it
   * does not exist).
   */
  const Attribute* attribute(const std::string& name) const;

+  /**
+   * Returns a constant pointer to the selected attribute if found. Returns an


nit: shared pointer

bekadavis9 · 2023-03-31T19:47:57Z

tiledb/sm/array_schema/array_schema.h

-  /** It maps each attribute name to the corresponding attribute object.
-   * Lifespan is maintained by the shared_ptr in attributes_. */
-  std::unordered_map<std::string, const Attribute*> attribute_map_;
+  struct attribute_reference {


nit: Add Doxygen comment?

I added one. I was on the fence about whether it was really needed.

bekadavis9 · 2023-03-31T19:56:51Z

tiledb/sm/array_schema/array_schema.h

+   * Invariant: The number of entries in `attribute_map_` is the same as the
+   *   number of entries in `attributes_`
+   */
+  std::unordered_map<std::string, attribute_reference> attribute_map_;


At face value, I'm not sure I understand the need for this change and/or the addition of attribute_reference

This change supports the addition of shared_attribute by name, which in turns supports the C API function that returns attributes from an array schema. The attribute returned through the API uses a shared_ptr in its handle. Using shared_attribute means that we can copy the pointer rather than copying the attribute object.

We need to retrieve attributes both by name and by index. Using the new structure for the range of the attribute map means we can have one unordered_map structure rather than two of them.

bekadavis9 · 2023-03-31T20:07:35Z

tiledb/api/c_api/attribute/attribute_api_internal.h

+ *
+ * @section DESCRIPTION
+ *
+ * This file declares the configuration section of the C API for TileDB.


nit: attribute

bekadavis9 · 2023-03-31T20:13:08Z

tiledb/api/c_api/attribute/attribute_api_external.h

+    tiledb_datatype_t* type) TILEDB_NOEXCEPT;
+
+/**
+ * Sets the nullability of an attribute.


nit: Retrieves

Done.

TBH I didn't read the copy-paste from tiledb.h that appears here.

bekadavis9 · 2023-03-31T20:17:15Z

tiledb/api/c_api/attribute/attribute_api.cc

+ *
+ * @section DESCRIPTION
+ *
+ * This file defines C API functions for the config section.


nit: attribute

bekadavis9 · 2023-03-31T20:21:48Z

tiledb/api/c_api/attribute/CMakeLists.txt

+)
+gather_sources(${SOURCES})
+
+commence(object_library capi_attribute_stub)


Does this need to be a stub because of its dependency on capi_filter_list_stub (or in general, a dependency on a stub)?

because of its dependency on capi_filter_list_stub

This. I put the work in to create the StorageManager stub because it was a cyclic dependency at the bottom of a dependency tree. (context depends on it.) One of the consequences of it is that most of the API object modules must be stub modules. When the stub goes away eventually, we'll unwind all of this and make non-stub versions of everything.

bekadavis9 · 2023-03-31T20:23:17Z

tiledb/api/c_api_test_support/testsupport_capi_datatype.h

@@ -0,0 +1,49 @@
+/**
+ * @file tiledb/api/c_api_test_support/context/testsupport_capi_context.h


nit: testsupport_capi_datatype

bekadavis9 · 2023-03-31T20:23:26Z

tiledb/api/c_api_test_support/testsupport_capi_datatype.h

+ *
+ * @section DESCRIPTION
+ *
+ * This file defines test support classes for the context section of the C API.


nit: datatype

eric-hughes-tiledb

Thanks for the close reading.

eric-hughes-tiledb · 2023-04-05T13:29:15Z

tiledb/api/c_api/attribute/CMakeLists.txt

+)
+gather_sources(${SOURCES})
+
+commence(object_library capi_attribute_stub)


because of its dependency on capi_filter_list_stub

This. I put the work in to create the StorageManager stub because it was a cyclic dependency at the bottom of a dependency tree. (context depends on it.) One of the consequences of it is that most of the API object modules must be stub modules. When the stub goes away eventually, we'll unwind all of this and make non-stub versions of everything.

eric-hughes-tiledb · 2023-04-05T13:32:07Z

tiledb/api/c_api/attribute/attribute_api.cc

+ *
+ * @section DESCRIPTION
+ *
+ * This file defines C API functions for the config section.


eric-hughes-tiledb · 2023-04-05T13:33:05Z

tiledb/api/c_api/attribute/attribute_api_external.h

+ *
+ * @section DESCRIPTION
+ *
+ * This file declares the configuration section of the C API for TileDB. It


eric-hughes-tiledb · 2023-04-05T13:37:35Z

tiledb/api/c_api/attribute/attribute_api_external.h

+ * @param attr The TileDB attribute to be created.
+ * @return `TILEDB_OK` for success and `TILEDB_OOM` or `TILEDB_ERR` for error.
+ *
+ * @note The default number of values per cell is 1.


When a caller is explicitly setting a value, it doesn't matter what the default is, although the code author might not know.

The default is certainly relevant here, where the API object is first created.

eric-hughes-tiledb · 2023-04-05T13:39:52Z

tiledb/api/c_api/attribute/attribute_api_external.h

+    tiledb_filter_list_t* filter_list) TILEDB_NOEXCEPT;
+
+/**
+ * Sets the number of values per cell for an attribute. If this is not


It's OK to repeat relevant facts in function documentation. Otherwise it moves toward the kind of documentation where programmers have to read everything to get anything done.

eric-hughes-tiledb · 2023-04-05T13:43:09Z

tiledb/api/c_api_test_support/testsupport_capi_datatype.h

@@ -0,0 +1,49 @@
+/**
+ * @file tiledb/api/c_api_test_support/context/testsupport_capi_context.h


eric-hughes-tiledb · 2023-04-05T13:43:51Z

tiledb/sm/array_schema/array_schema.h

  /**
   * Returns a constant pointer to the selected attribute (nullptr if it
   * does not exist).
   */
  const Attribute* attribute(const std::string& name) const;

+  /**
+   * Returns a constant pointer to the selected attribute if found. Returns an


eric-hughes-tiledb · 2023-04-05T13:48:39Z

tiledb/sm/array_schema/array_schema.h

-  /** It maps each attribute name to the corresponding attribute object.
-   * Lifespan is maintained by the shared_ptr in attributes_. */
-  std::unordered_map<std::string, const Attribute*> attribute_map_;
+  struct attribute_reference {


I added one. I was on the fence about whether it was really needed.

eric-hughes-tiledb · 2023-04-05T13:51:08Z

tiledb/sm/array_schema/array_schema.h

+   * Invariant: The number of entries in `attribute_map_` is the same as the
+   *   number of entries in `attributes_`
+   */
+  std::unordered_map<std::string, attribute_reference> attribute_map_;


This change supports the addition of shared_attribute by name, which in turns supports the C API function that returns attributes from an array schema. The attribute returned through the API uses a shared_ptr in its handle. Using shared_attribute means that we can copy the pointer rather than copying the attribute object.

We need to retrieve attributes both by name and by index. Using the new structure for the range of the attribute map means we can have one unordered_map structure rather than two of them.

eric-hughes-tiledb · 2023-04-05T14:02:13Z

tiledb/sm/array_schema/attribute.cc

-  fill_value_validity_ = attr->fill_value_validity_;
-  order_ = attr->order_;
+Attribute::Attribute(const Attribute& attr) {
+  name_ = attr.name();


Certainly could. I didn't change it in this PR just to minimize the number of changes and avoid analyzing what the old code had been doing.

The result is that we don't need anything but the default copy constructor.

bekadavis9

LGTM

Add new experimental attribute header to C API header list.

Change C API attribute handle to new `class tiledb_attribute_handle_t`. Move all declarations and definitions of attribute-section API functions into `tiledb/api/c_api/attribute/`. Handle class is a complete facade around `Attribute`; C API functions do not access such objects directly. Add exhaustive argument checking in a new attribute handle unit test runner. Rewrite remaining functions within `tiledb.cc` that use attributes. Supporting this, reworked a few things in serialization and schema evolution involving attribute arguments to use `shared_ptr` rather than `*`. Removed `sanity_check` for attributes, replacing it with `ensure_attribute_is_valid`. Change `class Attribute` in some small but textually-significant ways: removed some dead code (including a default constructor) and inlined a number of simple functions. More significantly, remove a non-standard `Attribute` constructor and replace it with one in standard form. Reworked a number of tests that were relying on this constructor. Add support in `class ArraySchema` to return `shared_ptr` to its internal attributes rather than plain pointers. This simplifies the C API function that return the attributes of a schema. Create `testsupport_capi_datatype.h` to hold the `INVALID_DATATYPE()` function for argument testing. Fix a defect where `tiledb_attribute_alloc` was not checking for valid datatype and was able to create invalid attributes. Add null argument checks in most attribute-section functions. All but a few were sources of unrecognized defects; those few were checking for `nullptr` themselves already. --- TYPE: IMPROVEMENT DESC: C API attribute handle

eric-hughes-tiledb marked this pull request as draft March 21, 2023 19:04

eric-hughes-tiledb force-pushed the eh/capi-handle-attribute branch 2 times, most recently from b8686f5 to 6144329 Compare March 22, 2023 14:23

eric-hughes-tiledb force-pushed the eh/capi-handle-attribute branch from 6144329 to 5389f05 Compare March 23, 2023 00:38

eric-hughes-tiledb marked this pull request as ready for review March 24, 2023 13:17

ihnorton requested a review from bekadavis9 March 24, 2023 13:23

eric-hughes-tiledb mentioned this pull request Mar 29, 2023

Array schema: C.41 changes, test support #4004

Merged

bekadavis9 requested changes Mar 31, 2023

View reviewed changes

eric-hughes-tiledb commented Apr 5, 2023

View reviewed changes

eric-hughes-tiledb requested a review from bekadavis9 April 5, 2023 14:08

bekadavis9 approved these changes Apr 6, 2023

View reviewed changes

ihnorton added the pending-2.17 label Apr 7, 2023

Rebase and update to intervening changes.

76e352a

eric-hughes-tiledb force-pushed the eh/capi-handle-attribute branch from 81cbd63 to 76e352a Compare July 24, 2023 16:37

Last attempt to fix a doomed branch

eae889d

eric-hughes-tiledb force-pushed the eh/capi-handle-attribute branch from 76e352a to 81ccdf7 Compare July 25, 2023 11:20

Fix Attribute copy constructor

7ac0d66

eric-hughes-tiledb force-pushed the eh/capi-handle-attribute branch from 81ccdf7 to 7ac0d66 Compare July 31, 2023 16:31

Add guard for definition of tiledb_attribute_t.

c31120a

Add new experimental attribute header to C API header list.

ihnorton merged commit b146b1d into dev Aug 1, 2023

ihnorton deleted the eh/capi-handle-attribute branch August 1, 2023 03:32

		@@ -0,0 +1,49 @@
		/**
		* @file tiledb/api/c_api_test_support/context/testsupport_capi_context.h

C API attribute handle #3982

C API attribute handle #3982

Conversation

eric-hughes-tiledb commented Mar 20, 2023

eric-hughes-tiledb commented Mar 20, 2023

eric-hughes-tiledb commented Mar 22, 2023

eric-hughes-tiledb commented Mar 29, 2023

bekadavis9 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eric-hughes-tiledb Apr 5, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eric-hughes-tiledb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eric-hughes-tiledb Apr 5, 2023 • edited Loading

Choose a reason for hiding this comment

bekadavis9 left a comment

Choose a reason for hiding this comment

eric-hughes-tiledb Apr 5, 2023 •

edited

Loading

eric-hughes-tiledb Apr 5, 2023 •

edited

Loading