Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 8 commits into
base: dev
Choose a base branch
from

Conversation

rroelke
Copy link
Contributor

@rroelke rroelke commented Oct 23, 2024

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 implements the above and adds unit tests which demonstrate that tiledb_array_schema_get_enumeration abides by the above:

  1. if there is no enumeration with the requested name attached to the schema, we throw an exception.
  2. if there is an enumeration with the requested name attached to the schema but it has not been loaded, we return NULL.
  3. if there is an enumeration with the requested name attached to the schema and it has been loaded in some way, we return the enumeration.

This pull request does not attempt to bridge the gap between 2 and 3, i.e. load the enumeration when it is requested.


TYPE: FEATURE | C_API | CPP_API
DESC: add tiledb_array_schema_get_enumeration

Copy link
Contributor

@shaunrd0 shaunrd0 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just left some small comments, looks great so far 👍

tiledb/sm/cpp_api/array.h Show resolved Hide resolved
test/src/unit-cppapi-enumerations.cc Outdated Show resolved Hide resolved
Copy link
Contributor

@davisp davisp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

tiledb/sm/cpp_api/array_schema_experimental.h Outdated Show resolved Hide resolved
@rroelke
Copy link
Contributor Author

rroelke commented Oct 25, 2024

+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.

Copy link
Contributor

@shaunrd0 shaunrd0 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants