Load enumerations for all array schemas in a single request. #5349

shaunrd0 · 2024-10-15T18:07:39Z

This fixes a performance regression added in #5291 while loading enumerations for all array schemas. The regression impacts arrays with multiple evolved schemas. Previously #5291 introduced a loop over REST requests for-each schema, this PR loads all enumerations for all schemas in a single request.

TYPE: C_API
DESC: Introduce tiledb_array_load_enumerations_all_schemas

TYPE: CPP_API
DESC: Introduce ArrayExperimental::load_enumerations_all_schemas

shaunrd0 · 2024-10-15T19:50:37Z

tiledb/sm/array/array.cc

+    }
+  }
+
+  return ret;
 }

 std::vector<shared_ptr<const Enumeration>> Array::get_enumerations(


Reverted changes to this function from #5237.

shaunrd0 · 2024-10-15T19:50:40Z

tiledb/api/c_api/array/array_api_internal.h

+  get_enumerations_all_schemas() {
+    return array_->get_enumerations_all_schemas();
+  }
+
  std::vector<shared_ptr<const tiledb::sm::Enumeration>> get_enumerations(


Reverted changes to this function from #5237.

shaunrd0 · 2024-10-15T19:52:14Z

tiledb/api/c_api/array/array_api.cc

@@ -663,6 +663,13 @@ capi_return_t tiledb_array_load_all_enumerations(const tiledb_array_t* array) {
  return TILEDB_OK;
 }

+capi_return_t tiledb_array_load_enumerations_all_schemas(


Adding this API instead of changing parameters of tiledb_array_load_all_enumerations allows this PR to fix the regression without changes to REST or other repositories. We will have a new binding to wrap but that's just for completeness, this will only be used by the client to submit the request to REST. Had we changed the parameters like I did initially REST would fail to link against this branch until we updated the modified binding for the APIs.

I mean, we can't really change a C API after we have released, can we? Or that's some exception for experimentals? What we can do if it's incorrect though is mark it for deprecation. But rather than incorrect, is tiledb_array_load_all_enumerations just fetching the enumerations for the latest schema only?
Then IMO it's good choice to add another API anyway since it's serving a different purpose and has different performance impact.

IIUC for experimental APIs we can change them since they are experimental, but it's a breaking change and I was trying to avoid doing that. Since tiledb_array_load_all_enumerations only loads for the latest schema I thought a new API would not be out of the question.

Yeah I think the name tiledb_array_load_all_enumerations is a little misleading as you probably noticed but I updated those docs to clarify and if we want to rename or restructure those methods I think we could do that separately and go through a deprecation cycle.

shaunrd0 · 2024-10-15T19:52:47Z

tiledb/sm/array/array.cc

+
+    // For now, reopen the array if it's found to be using array open v1.
+    // Once we update to use post_array_schema_from_rest we will always have
+    // array_schemas_all initialized and this could go away.


Just want to point these comments out. As I mentioned in the comment I could serialize allArraySchemas with the enumerations intact and that would be fine, but that would only be required until post_array_schema_from_rest is finished. Array open v3 is now the default so I thought this was better than locking us into serializing full schemas here.

If we want to just serialize all the schemas instead I can do that.

Let's not do array operations under the hood. I have a more radical idea. How about returning a message at the C API level that this operation is unsupported for array v1 opened arrays and redirect the user to use v2 instead?

@ihnorton Are we ok to introduce new C APIs assuming array v2/query v3 now that it's default, and just be backwards compatible forever for the pre-existing ones?

Done, I added exceptions in both the C API and in tiledb::sm::Array::load_all_enumerations in case it is called internally like we do in unit-enumerations.cc. I modified the existing tests to check for both exceptions if we are not using array open v2. The exception could be removed and tests updated as part of #5181.

shaunrd0 · 2024-10-15T19:53:08Z

Just marked as a draft until 59c529a is reverted, this is ready for review. There is a passing run with Query / Array open v2 here: https://gitlab.com/TileDB-Inc/TileDB-Internal/-/jobs/8093304584

ypatia

LGTM, a few comments and we're good. Thanks for that complex fix!

ypatia · 2024-10-16T13:00:07Z

tiledb/sm/array/array.cc

+    throw ArrayException("Unable to load enumerations; Array is not open.");
+  }
+
+  std::unordered_map<std::string, std::vector<shared_ptr<const Enumeration>>>


NIT: a general comment, those complex types are easier to read if modeled as structs so that we avoid .second type of names that are hard to remember what they refer to. No need to change it here, just stating my preference :)

Good call! I did not change it here but I'll keep it in mind for the future.. I have been confused by .first and .second in the past so I agree, a structure would have been better.

ypatia · 2024-10-16T13:16:20Z

tiledb/api/c_api/array/array_api.cc

@@ -663,6 +663,13 @@ capi_return_t tiledb_array_load_all_enumerations(const tiledb_array_t* array) {
  return TILEDB_OK;
 }

+capi_return_t tiledb_array_load_enumerations_all_schemas(


I mean, we can't really change a C API after we have released, can we? Or that's some exception for experimentals? What we can do if it's incorrect though is mark it for deprecation. But rather than incorrect, is tiledb_array_load_all_enumerations just fetching the enumerations for the latest schema only?
Then IMO it's good choice to add another API anyway since it's serving a different purpose and has different performance impact.

ypatia · 2024-10-16T13:26:26Z

tiledb/sm/cpp_api/array_experimental.h

+   * @param ctx The context to use.
+   * @param array The array to load enumerations for.
+   */
+  static void load_enumerations_all_schemas(


should this be experimental for some reason?

I put this in experimental because the other enumerations APIs were still experimental and it felt weird to add a new API that was so similar outside of experimental

ypatia · 2024-10-16T13:32:59Z

tiledb/sm/array/array.cc

+
+    // For now, reopen the array if it's found to be using array open v1.
+    // Once we update to use post_array_schema_from_rest we will always have
+    // array_schemas_all initialized and this could go away.


Let's not do array operations under the hood. I have a more radical idea. How about returning a message at the C API level that this operation is unsupported for array v1 opened arrays and redirect the user to use v2 instead?

@ihnorton Are we ok to introduce new C APIs assuming array v2/query v3 now that it's default, and just be backwards compatible forever for the pre-existing ones?

ypatia

Thanks for fixing this!

tiledb/api/c_api/array/array_api_experimental.h

+ Should fix TileDB-Py nightly failure.

Co-authored-by: Ypatia Tsavliri <[email protected]>

This reverts commit b0e2da4.

shaunrd0 requested a review from ypatia October 15, 2024 18:07

shaunrd0 force-pushed the smr/sc-55029/load-enmrs-all-schemas branch from 227549e to 5285bb7 Compare October 15, 2024 18:52

shaunrd0 commented Oct 15, 2024

View reviewed changes

ypatia reviewed Oct 16, 2024

View reviewed changes

shaunrd0 force-pushed the smr/sc-55029/load-enmrs-all-schemas branch from 5dcc0cd to 3e1787d Compare October 16, 2024 21:03

ypatia approved these changes Oct 17, 2024

View reviewed changes

tiledb/api/c_api/array/array_api_experimental.h Outdated Show resolved Hide resolved

shaunrd0 and others added 12 commits October 17, 2024 09:31

Load enumerations for all schemas in one request.

7b78909

Test REST CI.

5f83015

Reopen instead of Close,Open.

f7cadf3

Add tiledb_array_load_enumerations_all_schemas.

3662216

Plumb latest schema name through deserialization.

336b3fa

Update comments and docs.

483b3a1

Fix CI.

a4fd5da

Revert changes to error message.

9b25263

+ Should fix TileDB-Py nightly failure.

Load enmrs for all schemas during serialization.

3898359

Throw exception if not using array open v3.

65c7362

Update tiledb/api/c_api/array/array_api_experimental.h

d99f8e9

Co-authored-by: Ypatia Tsavliri <[email protected]>

Revert "Test REST CI."

8ec2827

This reverts commit b0e2da4.

shaunrd0 force-pushed the smr/sc-55029/load-enmrs-all-schemas branch from 093c2d7 to 8ec2827 Compare October 17, 2024 13:32

shaunrd0 marked this pull request as ready for review October 17, 2024 13:33

shaunrd0 merged commit b1dd2b4 into dev Oct 17, 2024
63 checks passed

shaunrd0 deleted the smr/sc-55029/load-enmrs-all-schemas branch October 17, 2024 14:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Load enumerations for all array schemas in a single request. #5349

Load enumerations for all array schemas in a single request. #5349

shaunrd0 commented Oct 15, 2024

shaunrd0 Oct 15, 2024 •

edited

Loading

shaunrd0 Oct 15, 2024 •

edited

Loading

shaunrd0 Oct 15, 2024

ypatia Oct 16, 2024

shaunrd0 Oct 16, 2024

shaunrd0 Oct 15, 2024

ypatia Oct 16, 2024

shaunrd0 Oct 16, 2024

shaunrd0 commented Oct 15, 2024

ypatia left a comment

ypatia Oct 16, 2024

shaunrd0 Oct 16, 2024

ypatia Oct 16, 2024

ypatia Oct 16, 2024

shaunrd0 Oct 16, 2024

ypatia Oct 16, 2024

ypatia left a comment

Load enumerations for all array schemas in a single request. #5349

Load enumerations for all array schemas in a single request. #5349

Conversation

shaunrd0 commented Oct 15, 2024

shaunrd0 Oct 15, 2024 • edited Loading

Choose a reason for hiding this comment

shaunrd0 Oct 15, 2024 • 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

shaunrd0 commented Oct 15, 2024

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

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

shaunrd0 Oct 15, 2024 •

edited

Loading

shaunrd0 Oct 15, 2024 •

edited

Loading