feat: [sc-58279] [core] add `tiledb_array_schema_get_enumeration` API #5359

rroelke · 2024-10-23T20:46:08Z

Story details: https://app.shortcut.com/tiledb-inc/story/58279

In tiledb-rs we have have "property-based testing" strategies which generate arbitrary tiledb schema. We would like to extend these strategies to generate arbitrary enumerations to put in those schemata.

As a step towards this we must add an API tiledb_array_schema_get_enumeration so that we can load the values in an enumeration without having a pointer to the open array in the same function.

It is not required for this API to actually load the enumeration, just to return its contents if it is already loaded. There should be a graceful signal to distinguish between an enumeration not being found and an enumeration not being loaded.

This pull request goes all the way and adds functions tiledb_array_schema_get_enumeration_from_name and tiledb_array_schema_get_enumeration_from_attribute_name which load the contents of an enumeration and return a handle to the user.

See this comment for further discussion.

TYPE: FEATURE | C_API | CPP_API
DESC: add tiledb_array_schema_get_enumeration

tiledb/api/c_api/array_schema/array_schema_api_experimental.h

test/src/unit-cppapi-enumerations.cc

shaunrd0

Just left some small comments, looks great so far 👍

tiledb/sm/cpp_api/array.h

tiledb/api/c_api/array_schema/test/unit_capi_array_schema.cc

test/src/unit-cppapi-enumerations.cc

davisp

+1 from me. @ypatia already suggested the REST test which is all I had thought of.

tiledb/sm/cpp_api/array_schema_experimental.h

…ation_if_needed

rroelke · 2024-10-25T01:43:02Z

+1 from me. @ypatia already suggested the REST test which is all I had thought of.

@davisp I mis-clicked when I requested a re-review, but feel free :)

All the changes are strictly in response to the review comments.

shaunrd0

Nice! 👍

…r attribute name, and also use context to do the I/O

rroelke · 2024-10-29T02:51:54Z

After using this branch in my tiledb-rs branch I was not very satisfied, so instead of debugging the REST failure today I instead spent time making the schema lookup actually load the enumeration (instead of returning data only if the enumeration was already loaded).

While doing so I also noticed that Array::get_enumeration uses the attribute name as the lookup key, not the enumeration name. This does not match what I had done here.

So the most recent commit does the following:

Removes tiledb_array_schema_get_enumeration_if_loaded
Adds tiledb_array_schema_get_enumeration_from_name
Adds tiledb_array_schema_get_enumeration_from_attribute_name
Adds the internal method ArraySchema::load_enumeration which does the I/O and updates the ArraySchema internal pointer, and calls this method from the C API handler

I reckon this merits a re-review since it's a pretty big change.

rroelke · 2024-10-29T02:52:55Z

tiledb/api/c_api/array_schema/CMakeLists.txt

@@ -36,6 +36,7 @@ commence(object_library capi_array_schema_stub)
  this_target_sources(${SOURCES})
  this_target_link_libraries(export)
  this_target_object_libraries(array_schema)
+  this_target_object_libraries(array)


This is because tiledb/sm/array_schema/array_schema.cc calls upon tiledb/sm/array/array_directory.cc. So now the unit test needs to link with that.

…b_array_schema_get_attribute_from_name

…ay-schema-get-enumeration

rroelke · 2024-10-30T22:30:24Z

@shaunrd0 @davisp @ypatia

I think this is all set, CI passes and I don't intend to make any more changes. I'd recommend re-reviewing as this has drfited a bit since it was approved.

Since the last review I determined I was unsatisfied with the "return the enumeration only if loaded" approach and have updated it to load the enumeration as part of the API request.

The code was easy enough but the troubles come with avoiding circular dependencies. array/array_directory.cc is used to do the I/O, but the array "object library" depends on array_schema - so we can't have array_schema circularly depend on array. The solution to this is to have the C API handler call a separate function to load the enumeration.

The other change since the last review is tweaking the API to be tiledb_array_schema_get_enumeration_from_name and tiledb_array_schema_get_enumeration_from_attribute_name. I noticed that tiledb_array_get_enumeration used the attribute name, not the enumeration name, as the lookup key, and I wanted to disambiguate for these versions. The implementation of tiledb_array_schema_get_enumeration_from_attribute_name calls tiledb_attribute_get_enumeration which required a few more changes to some "object libraries" but I thought this would make sense so that any errors would be presented the same way.

The good news is that my downstream tiledb-rs is very happy with this code and does exactly what I want it to. Hooray!

shaunrd0

Very nice! Most comments were nits on updating docs since things have changed a bit since the first pass. I don't think there should be too many changes from my comments but if you want me to take another look let me know. Just as a heads up we should probably update the PR description before merging.

tiledb/api/c_api/array_schema/array_schema_api_experimental.h

shaunrd0 · 2024-10-31T14:58:07Z

tiledb/sm/array_schema/array_schema_operations.cc

+      auto tracker = ctx.resources().ephemeral_memory_tracker();
+      // Pass an empty list of enumeration names. REST will use timestamps to
+      // load all enumerations on all schemas for the array within that range.
+      auto ret = rest_client.post_enumerations_from_rest(


It would have been great to have avoided the second REST request here, but probably this is the best we can do without changing REST.. I'm ok with this for the time being, #5181 is a WIP and once that's finished this can definitely happen in a single request. Also the rest.load_enumerations_on_array_open option defaults to false so until that work is finished we won't make this second request unless the client asks for it.

Yeah, I didn't love this implementation, but... it gets the job done without having any obvious problems

@shaunrd0 please add a TODO in your #5181 or its ticket to address this. I don't know of any other place where we do 2 REST requests so we should really treat that as temporary and fix it asap.

tiledb/sm/rest/rest_client_remote.h

shaunrd0 · 2024-10-31T17:32:02Z

tiledb/sm/cpp_api/array.h

@@ -717,6 +717,32 @@ class Array {
    return ArraySchema(ctx, schema);
  }

+  /**
+   * Loads the array schema from an array.
+   * Options to load additional features are read from the optionally-provided


~~optionally-provided~~

tiledb/api/c_api/array_schema/array_schema_api_experimental.h

shaunrd0 · 2024-10-31T17:45:36Z

tiledb/api/c_api/array_schema/array_schema_api.cc

+  }
+
+  std::string enumeration_name(enumeration_name_inner->view());
+  return api_entry_with_context<


Could we do return tiledb_array_schema_get_enumeration_from_name(ctx, array_schema, enumeration_name.c_str(), enumeration); to shorten this a bit?

Surprisingly I think the answer is "no" without making changes elsewhere.

tiledb_string_t has no c_str(), just view()

and I was initially surprised to see that string_view also has no c_str(), though in retrospect it makes sense.

shaunrd0 · 2024-10-31T18:10:20Z

tiledb/sm/array/array.h

+ * @param enmr_name the requested enumeration
+ * @param schema the target schema
+ */
+void load_enumeration_into_schema(


Could this maybe go into array_schema_operations.cc, or did you also run into problems there with dependencies? At a glance it seems like it could work

I believe it won't work there either. I am pretty new to cmake but my impression is that all the array_schema/*.cc files are compiled into the same "object library", hence putting this code in array_schema_operations.cc would also have the circular dependency between array and array_schema

shaunrd0 · 2024-10-31T18:12:12Z

test/support/src/array_schema_helpers.cc

+
+namespace tiledb::test {
+
+bool is_equivalent_enumeration(


Thanks for putting this here, I'm likely going to use it when you merge

Co-authored-by: Shaun M Reed <[email protected]>

ypatia

I was sick Thursday and this went in before I could review it. I am sorry for the delayed comments, but the ones around the new C-API body would need to be addressed.

tiledb/api/c_api/array_schema/array_schema_api_experimental.h

ypatia · 2024-10-31T10:23:16Z

tiledb/sm/rest/rest_client_remote.cc

+  auto array_schema = serialization::array_schema_deserialize(
+      serialization_type_, returned_data, memory_tracker_);
+
+  array_schema->set_array_uri(uri);


Some background on why this was needed? Some test failing?

IIRC this change came about when testing against the REST server. The REST server does not serialize the array URI (and I think it may also be different on the backend).

I will remove this line and run the unit tests to see what happened - probably will get to that later this week

Without this we get an error on the client side in tiledb::sm::load_enumeration_into_schema.

if (array_schema.array_uri().is_tiledb()) { /* it's remote, talk to REST server */ } else { /* it's local, read filesystem */ }

If we don't set the array URI here then it is blank. As a result is_tiledb is false and the schema tries to read the local filesystem, which results in error when it tries to open the empty path.

tiledb/api/c_api/array_schema/array_schema_api.cc

tiledb/sm/array/array.cc

ypatia · 2024-11-04T12:24:58Z

tiledb/sm/array_schema/array_schema_operations.cc

+      auto tracker = ctx.resources().ephemeral_memory_tracker();
+      // Pass an empty list of enumeration names. REST will use timestamps to
+      // load all enumerations on all schemas for the array within that range.
+      auto ret = rest_client.post_enumerations_from_rest(


@shaunrd0 please add a TODO in your #5181 or its ticket to address this. I don't know of any other place where we do 2 REST requests so we should really treat that as temporary and fix it asap.

#5359 was merged and then there were some additional changes requested. This pull request implements those changes. - [fix a comment typo](#5359 (comment)) - [avoid nested C API calls and fix memory leak](#5359 (comment)) - [add a test with schema evolution](#5359 (comment)) --- TYPE: BUG DESC: Follow up on #5359

Add tiledb_array_schema_get_enumeration API

b484eb6

rroelke requested review from davisp and shaunrd0 October 23, 2024 20:46

ypatia reviewed Oct 24, 2024

View reviewed changes

tiledb/api/c_api/array_schema/array_schema_api_experimental.h Outdated Show resolved Hide resolved

test/src/unit-cppapi-enumerations.cc Outdated Show resolved Hide resolved

shaunrd0 reviewed Oct 24, 2024

View reviewed changes

tiledb/sm/cpp_api/array.h Show resolved Hide resolved

tiledb/api/c_api/array_schema/test/unit_capi_array_schema.cc Outdated Show resolved Hide resolved

test/src/unit-cppapi-enumerations.cc Outdated Show resolved Hide resolved

davisp approved these changes Oct 24, 2024

View reviewed changes

tiledb/sm/cpp_api/array_schema_experimental.h Outdated Show resolved Hide resolved

rroelke added 7 commits October 24, 2024 12:04

Fix comment typo

c1efcf0

Move is_equivalent_enumeration to array_schema_helpers.cc

0a77551

Fix missing return code

dc2b5ae

Add missing rest tag

5d5c3ba

Fix formatting

d18cb94

tiledb_array_schema_get_enumeration => tiledb_array_schema_get_enumer…

0bf188d

…ation_if_needed

Undo default argument, add ArraySchema::load_schema_with_config

754f239

rroelke requested review from ypatia, davisp and shaunrd0 October 25, 2024 01:42

shaunrd0 approved these changes Oct 25, 2024

View reviewed changes

Split ArraySchema::get_enumeration to look up from enumeration name o…

2c2de31

…r attribute name, and also use context to do the I/O

rroelke requested a review from shaunrd0 October 29, 2024 02:51

rroelke commented Oct 29, 2024

View reviewed changes

rroelke added 8 commits October 29, 2024 09:58

ArraySchema::load_enumeration branch for remote array case

adfcfea

ArraySchema enumeration tests pass with REST server

efc7cea

Move schema enumeration loading code to different compilation unit

2eeda89

Fix linking issues

9454c21

Revert array_operations changes

1f09625

Revert no-longer-used includes

d42551e

Remove C4459 errors

b9a5b46

Fix duplicate definitions in unit_capi_array_schema

4008d80

rroelke added 3 commits October 30, 2024 12:01

Fix unit_capi_array linker errors

f13411d

Fix platform-dependent SEGV from passing NULL attribute name to tiled…

7b42669

…b_array_schema_get_attribute_from_name

Merge remote-tracking branch 'origin/dev' into rr/sc-58279-tiledb-arr…

7ba3f5e

…ay-schema-get-enumeration

ihnorton requested review from davisp and removed request for davisp October 31, 2024 18:35

shaunrd0 approved these changes Oct 31, 2024

View reviewed changes

rroelke and others added 4 commits October 31, 2024 15:16

Update tiledb/api/c_api/array_schema/array_schema_api_experimental.h

f0727b9

Co-authored-by: Shaun M Reed <[email protected]>

Update tiledb/api/c_api/array_schema/array_schema_api_experimental.h

85ffae9

Co-authored-by: Shaun M Reed <[email protected]>

Update tiledb/sm/rest/rest_client_remote.h

373c4c2

Co-authored-by: Shaun M Reed <[email protected]>

Update tiledb/api/c_api/array_schema/array_schema_api_experimental.h

2967b0d

Co-authored-by: Shaun M Reed <[email protected]>

rroelke merged commit aad089c into dev Nov 1, 2024
64 checks passed

rroelke deleted the rr/sc-58279-tiledb-array-schema-get-enumeration branch November 1, 2024 00:27

ypatia reviewed Nov 4, 2024

View reviewed changes

rroelke mentioned this pull request Nov 12, 2024

Follow up on requested changes from #5359 after it was merged #5368

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: [sc-58279] [core] add `tiledb_array_schema_get_enumeration` API #5359

feat: [sc-58279] [core] add `tiledb_array_schema_get_enumeration` API #5359

rroelke commented Oct 23, 2024 •

edited

Loading

shaunrd0 left a comment

davisp left a comment

rroelke commented Oct 25, 2024

shaunrd0 left a comment

rroelke commented Oct 29, 2024

rroelke Oct 29, 2024

rroelke commented Oct 30, 2024 •

edited

Loading

shaunrd0 left a comment

shaunrd0 Oct 31, 2024

rroelke Oct 31, 2024

ypatia Nov 4, 2024

shaunrd0 Oct 31, 2024

shaunrd0 Oct 31, 2024

rroelke Oct 31, 2024

shaunrd0 Oct 31, 2024

rroelke Oct 31, 2024

shaunrd0 Oct 31, 2024

ypatia left a comment

ypatia Oct 31, 2024

rroelke Nov 5, 2024

rroelke Nov 12, 2024

ypatia Nov 4, 2024


		namespace tiledb::test {

		bool is_equivalent_enumeration(

feat: [sc-58279] [core] add tiledb_array_schema_get_enumeration API #5359

feat: [sc-58279] [core] add tiledb_array_schema_get_enumeration API #5359

Conversation

rroelke commented Oct 23, 2024 • edited Loading

shaunrd0 left a comment

Choose a reason for hiding this comment

davisp left a comment

Choose a reason for hiding this comment

rroelke commented Oct 25, 2024

shaunrd0 left a comment

Choose a reason for hiding this comment

rroelke commented Oct 29, 2024

Choose a reason for hiding this comment

rroelke commented Oct 30, 2024 • edited Loading

shaunrd0 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

ypatia 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

feat: [sc-58279] [core] add `tiledb_array_schema_get_enumeration` API #5359

feat: [sc-58279] [core] add `tiledb_array_schema_get_enumeration` API #5359

rroelke commented Oct 23, 2024 •

edited

Loading

rroelke commented Oct 30, 2024 •

edited

Loading