Add serialization and API changes for `post_array_schema_from_rest`. #5237

shaunrd0 · 2024-08-13T20:23:34Z

This factors out serialization and API changes in #5181 that are required for the HandleGetArraySchema route. These changes will need to be available on REST before we can enable the new route for loading the array schema.

There is a quick summary of the changes required in SC-52877.

TYPE: IMPROVEMENT
DESC: Add serialization and API changes for post_array_schema_from_rest.

nickvigilante · 2024-08-13T21:34:40Z

tiledb/api/c_api/config/config_api_external.h

Can you also apply this change to https://github.com/TileDB-Inc/TileDB/blob/dev/tiledb/sm/cpp_api/config.h?

Done, thanks!

+ Fix test failure in CI.

robertbindar

Left an inline comment which we'll need to triple check before merging.

robertbindar · 2024-08-18T10:12:47Z

tiledb/sm/serialization/tiledb-rest.capnp

@@ -1264,14 +1264,14 @@ struct LoadEnumerationsResponse {
 struct LoadArraySchemaRequest {
  config @0 :Config;
  # Config
-
-  includeEnumerations @1 :Bool;


Unfortunately I think you might risk crashing older clients if you do this. Please check

TileDB/tiledb/sm/serialization/array_schema.cc

Line 1909 in f84be6c

return LoadArraySchemaRequest(reader.getIncludeEnumerations());

which is read whithout checking if the field exists on the message :(

Yeah bummer, it was exposed in #4399. I added it back and used it for this option, I documented that future options like this should be stored in the serialized Config.

As a general note, whenever we see a field accessed without an if (has) wrapped around it, we should at the minimum ask a question about it, these things bite hard and lock us into supporting fields forever.

Thanks! I added the hasConfig check during deserialization. I reran tests this morning in #5181 with these changes and everything is passing, this is ready for review when you have some time.

teo-tsirpanis · 2024-08-19T08:14:27Z

tiledb/sm/c_api/tiledb.h

+ * @param array_schema The array schema to be retrieved, or `NULL` upon error.
+ * @return `TILEDB_OK` for success and `TILEDB_OOM` or `TILEDB_ERR` for error.
+ */
+TILEDB_EXPORT int32_t tiledb_array_schema_load_with_options(


tiledb_array_schema_load_with_config or load_v2 sounds like a better name. The word options does not appear in other APIs.

teo-tsirpanis · 2024-08-19T08:15:15Z

tiledb/sm/c_api/tiledb.h

 * Retrieves the schema of an array from the disk, creating an array schema
- * struct.
+ * struct. The schema retrieved will always be the latest schema for the array.


This could be Retrieves the latest schema of an array from the disk..

teo-tsirpanis · 2024-08-19T08:17:57Z

test/src/unit-capi-config.cc

+  ss << "rest.load_enumerations_on_array_open false\n";
  ss << "rest.load_metadata_on_array_open true\n";
  ss << "rest.load_non_empty_domain_on_array_open true\n";


Why are these options restricted to REST only? Especially the first two have use cases outside of it.

Yeah also crossed my mind while working on this.. I didn't introduce any new config options in this PR but maybe sm.load_X_on_array_open or sm.array_open.load_X would be more fitting for these? I think they were each introduced to tune REST behavior and that's probably where the names originated.

I opened SC-53109 to consider renaming them, probably best to handle the renaming separately.

teo-tsirpanis · 2024-08-19T08:19:04Z

tiledb/sm/serialization/array_schema.h

+    return config_.get<bool>(
+        "rest.load_enumerations_on_array_open", config_.must_find);


How about we read this value on construction? There is no reason to copy a whole config.

teo-tsirpanis · 2024-08-19T08:20:21Z

tiledb/sm/c_api/tiledb.cc

-    LOG_STATUS_NO_RETURN_VALUE(st);
-    save_error(ctx, st);
-    return TILEDB_OOM;
+    throw CAPIStatusException("Failed to allocate TileDB array schema object");


You can throw bad_alloc, which will map to TILEDB_OOM in the C API.

teo-tsirpanis · 2024-08-19T08:38:52Z

tiledb/sm/array_schema/array_schema_operations.cc

+    auto&& [st, array_schema_response] =
+        rest_client.get_array_schema_from_rest(uri);
+    throw_if_not_ok(st);
+    return std::move(array_schema_response.value());


Suggested change

return std::move(array_schema_response.value());

return std::move(array_schema_response).value();

Nit: This will have the same behavior and use the rvalue reference-qualified overload of value().

Done, nice!

teo-tsirpanis · 2024-08-19T08:42:56Z

tiledb/sm/array/array_directory.cc

@@ -123,7 +123,7 @@ shared_ptr<ArraySchema> ArrayDirectory::load_array_schema_latest(
  auto&& array_schema = load_array_schema_from_uri(
      resources_.get(), schema_uri, encryption_key, memory_tracker);

-  array_schema->set_array_uri(uri_);
+  array_schema->set_array_uri(uri_.remove_trailing_slash());

  return std::move(array_schema);


I had written a return std::move(x); in the past and CI had failed with a warning-turned-error. I'm surprised it does not fail here.

…tion-update-load-schema

KiterLuc · 2024-08-26T15:44:35Z

tiledb/api/c_api/array/array_api.cc

 #include "tiledb/sm/enums/encryption_type.h"
 #include "tiledb/sm/rest/rest_client.h"

 namespace tiledb::api {

-capi_return_t tiledb_array_schema_load(
+int32_t tiledb_array_schema_load(


Suggested change

int32_t tiledb_array_schema_load(

capi_return_t tiledb_array_schema_load(

KiterLuc · 2024-08-26T15:44:52Z

tiledb/api/c_api/array/array_api.cc

+  return TILEDB_OK;
+}
+
+int32_t tiledb_array_schema_load_with_config(


Suggested change

int32_t tiledb_array_schema_load_with_config(

capi_return_t tiledb_array_schema_load_with_config(

KiterLuc · 2024-08-26T15:49:32Z

tiledb/sm/array_schema/array_schema_operations.h

+ * @param ctx TileDB Context.
+ * @param uri The URI of the array to load schema.
+ * @param config TileDB Config. If null, the context config will be used.
+ */


@bekadavis9 do you think this belongs here? I'm not sure we want to add the dependency to array directory if we don't have to.

Yes, this is fine. This file is intended to separate out the I/O-based ArraySchema operations and will undoubtedly rely on ArrayDirectory at some point; it just hasn't needed to yet. The array CAPI Handle will already need to link against capi_array_schema_stub to make use of its internals, so there's no additional overhead here that I can think of. It could be argued that it's a potentially unnecessary change, and the body could stay as-is in the C API, but I have no real qualms about the change.

teo-tsirpanis

Thanks!

…tion-update-load-schema

robertbindar

LGTM, great work @shaunrd0!

robertbindar · 2024-08-27T09:32:29Z

tiledb/sm/serialization/array_schema.cc

@@ -1850,6 +1850,8 @@ void load_array_schema_request_to_capnp(
    const LoadArraySchemaRequest& req) {
  auto config_builder = builder.initConfig();
  throw_if_not_ok(config_to_capnp(config, &config_builder));
+  // This boolean is only serialized to support clients using TileDB < 2.26.
+  // Future options should only be serialized within the Config object above.
  builder.setIncludeEnumerations(req.include_enumerations());


This is excellent, thank you.

KiterLuc · 2024-08-27T19:32:42Z

/backport to release-2.26

github-actions · 2024-08-27T19:32:55Z

Started backporting to release-2.26: https://github.com/TileDB-Inc/TileDB/actions/runs/10584654319

github-actions · 2024-08-27T19:33:04Z

@KiterLuc backporting to release-2.26 failed, the patch most likely resulted in conflicts:

$ git am --3way --ignore-whitespace --keep-non-patch changes.patch

Applying: Serialization and C API handler changes.
Applying: Changes to loading enumerations.
Applying: Array schema load changes.
Applying: Add config docs to cpp_api/config.h
Applying: Fix CI failures.
Applying: Add test for loading enumerations for all schemas.
Applying: Changes from review.
Applying: Check hasConfig during deserialization.
Applying: Fix C API return types.
error: sha1 information is lacking or useless (tiledb/api/c_api/array/array_api.cc).
error: could not build fake ancestor
hint: Use 'git am --show-current-patch=diff' to see the failed patch
hint: When you have resolved this problem, run "git am --continue".
hint: If you prefer to skip this patch, run "git am --skip" instead.
hint: To restore the original branch and stop patching, run "git am --abort".
hint: Disable this message with "git config advice.mergeConflict false"
Patch failed at 0009 Fix C API return types.
Error: The process '/usr/bin/git' failed with exit code 128

Please backport manually!

github-actions · 2024-08-27T19:33:04Z

@KiterLuc an error occurred while backporting to release-2.26, please check the run log for details!

Error: git am failed, most likely due to a merge conflict.

…ray_schema_from_rest. (#5237) (#5261) Backport of #5237 to release-2.26 --- TYPE: IMPROVEMENT DESC: Add serialization and API changes for post_array_schema_from_rest.

… post_array_schema_from_rest. (#5237) (#5261)" This reverts commit 6460a8b.

shaunrd0 requested a review from KiterLuc August 13, 2024 20:23

shaunrd0 requested a review from a team as a code owner August 13, 2024 20:23

nickvigilante reviewed Aug 13, 2024

View reviewed changes

shaunrd0 added 6 commits August 14, 2024 13:13

Serialization and C API handler changes.

1ce8e9d

Changes to loading enumerations.

2a6978b

Array schema load changes.

cf7f4f7

Add config docs to cpp_api/config.h

001d53a

Fix CI failures.

2bfaa39

Add test for loading enumerations for all schemas.

1829491

+ Fix test failure in CI.

shaunrd0 force-pushed the smr/sc-52877/serialization-update-load-schema branch from 30d4a81 to 1829491 Compare August 14, 2024 17:14

robertbindar self-requested a review August 15, 2024 10:16

teo-tsirpanis self-requested a review August 16, 2024 15:52

robertbindar reviewed Aug 18, 2024

View reviewed changes

teo-tsirpanis reviewed Aug 19, 2024

View reviewed changes

Changes from review.

a588737

shaunrd0 force-pushed the smr/sc-52877/serialization-update-load-schema branch from 279b18f to a588737 Compare August 19, 2024 20:10

shaunrd0 added 2 commits August 20, 2024 11:32

Merge remote-tracking branch 'origin/dev' into smr/sc-52877/serializa…

97e6c14

…tion-update-load-schema

Check hasConfig during deserialization.

3b206ae

shaunrd0 force-pushed the smr/sc-52877/serialization-update-load-schema branch from 748eaea to 3b206ae Compare August 20, 2024 16:42

shaunrd0 requested review from robertbindar, teo-tsirpanis and nickvigilante August 22, 2024 14:22

KiterLuc reviewed Aug 26, 2024

View reviewed changes

teo-tsirpanis approved these changes Aug 26, 2024

View reviewed changes

shaunrd0 added 2 commits August 26, 2024 12:34

Fix C API return types.

02cb1be

Merge remote-tracking branch 'origin/dev' into smr/sc-52877/serializa…

643a88d

…tion-update-load-schema

robertbindar approved these changes Aug 27, 2024

View reviewed changes

KiterLuc changed the title ~~Serialization and API changes for post_array_schema_from_rest.~~ Add serialization and API changes for post_array_schema_from_rest. Aug 27, 2024

KiterLuc added the backport release-2.26 label Aug 27, 2024

KiterLuc merged commit 9b4e5ea into dev Aug 27, 2024
68 checks passed

KiterLuc deleted the smr/sc-52877/serialization-update-load-schema branch August 27, 2024 19:32

shaunrd0 mentioned this pull request Aug 27, 2024

[Backport release-2.26] Add serialization and API changes for post_array_schema_from_rest. (#5237) #5261

Merged

KiterLuc added a commit that referenced this pull request Sep 10, 2024

Revert "[Backport release-2.26] Add serialization and API changes for…

ba7266c

… post_array_schema_from_rest. (#5237) (#5261)" This reverts commit 6460a8b.

shaunrd0 mentioned this pull request Oct 16, 2024

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

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add serialization and API changes for `post_array_schema_from_rest`. #5237

Add serialization and API changes for `post_array_schema_from_rest`. #5237

shaunrd0 commented Aug 13, 2024 •

edited by KiterLuc

Loading

nickvigilante Aug 13, 2024

shaunrd0 Aug 19, 2024

robertbindar left a comment

robertbindar Aug 18, 2024 •

edited

Loading

shaunrd0 Aug 19, 2024

robertbindar Aug 20, 2024

shaunrd0 Aug 20, 2024

teo-tsirpanis Aug 19, 2024

shaunrd0 Aug 19, 2024

teo-tsirpanis Aug 19, 2024

shaunrd0 Aug 19, 2024

teo-tsirpanis Aug 19, 2024

shaunrd0 Aug 19, 2024

teo-tsirpanis Aug 19, 2024

shaunrd0 Aug 19, 2024

teo-tsirpanis Aug 19, 2024

shaunrd0 Aug 19, 2024

teo-tsirpanis Aug 19, 2024

shaunrd0 Aug 19, 2024

teo-tsirpanis Aug 19, 2024 •

edited

Loading

KiterLuc Aug 26, 2024

shaunrd0 Aug 26, 2024

KiterLuc Aug 26, 2024

shaunrd0 Aug 26, 2024

KiterLuc Aug 26, 2024

bekadavis9 Aug 26, 2024

teo-tsirpanis left a comment

robertbindar left a comment

robertbindar Aug 27, 2024

KiterLuc commented Aug 27, 2024

github-actions bot commented Aug 27, 2024

github-actions bot commented Aug 27, 2024

github-actions bot commented Aug 27, 2024

		return config_.get<bool>(
		"rest.load_enumerations_on_array_open", config_.must_find);

	return std::move(array_schema_response.value());
	return std::move(array_schema_response).value();

	int32_t tiledb_array_schema_load(
	capi_return_t tiledb_array_schema_load(

	int32_t tiledb_array_schema_load_with_config(
	capi_return_t tiledb_array_schema_load_with_config(

Add serialization and API changes for post_array_schema_from_rest. #5237

Add serialization and API changes for post_array_schema_from_rest. #5237

Conversation

shaunrd0 commented Aug 13, 2024 • edited by KiterLuc Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robertbindar left a comment

Choose a reason for hiding this comment

robertbindar Aug 18, 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

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

teo-tsirpanis Aug 19, 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

teo-tsirpanis left a comment

Choose a reason for hiding this comment

robertbindar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KiterLuc commented Aug 27, 2024

github-actions bot commented Aug 27, 2024

github-actions bot commented Aug 27, 2024

github-actions bot commented Aug 27, 2024

Add serialization and API changes for `post_array_schema_from_rest`. #5237

Add serialization and API changes for `post_array_schema_from_rest`. #5237

shaunrd0 commented Aug 13, 2024 •

edited by KiterLuc

Loading

robertbindar Aug 18, 2024 •

edited

Loading

teo-tsirpanis Aug 19, 2024 •

edited

Loading