From ea4bdb32d87b497093de663dcc14a24c88926c82 Mon Sep 17 00:00:00 2001 From: Theodore Tsirpanis Date: Thu, 7 Nov 2024 21:51:38 +0200 Subject: [PATCH 1/5] Add API to dump groups that returns a string handle. --- examples/c_api/groups.c | 11 ++++++---- tiledb/api/c_api/group/group_api.cc | 23 +++++++++++++++++++++ tiledb/api/c_api/group/group_api_external.h | 21 +++++++++++++++++-- tiledb/sm/cpp_api/group.h | 8 +++---- 4 files changed, 52 insertions(+), 11 deletions(-) diff --git a/examples/c_api/groups.c b/examples/c_api/groups.c index 0e7310caf02..a365b6fc879 100644 --- a/examples/c_api/groups.c +++ b/examples/c_api/groups.c @@ -148,12 +148,15 @@ void print_group() { tiledb_group_alloc(ctx, "my_group", &my_group); tiledb_group_open(ctx, my_group, TILEDB_READ); - char* str; - tiledb_group_dump_str(ctx, my_group, &str, 1); + tiledb_string_t* str; + const char* str_ptr; + size_t str_len; + tiledb_group_dump_str_v2(ctx, my_group, &str, 1); - printf("%s\n", str); - free(str); + tiledb_string_view(str, &str_ptr, &str_len); + printf("%s\n", str_ptr); + tiledb_string_free(&str); tiledb_group_close(ctx, my_group); tiledb_group_free(&my_group); } diff --git a/tiledb/api/c_api/group/group_api.cc b/tiledb/api/c_api/group/group_api.cc index b8b55b67bd8..ac6c86ba36c 100644 --- a/tiledb/api/c_api/group/group_api.cc +++ b/tiledb/api/c_api/group/group_api.cc @@ -434,6 +434,19 @@ capi_return_t tiledb_group_dump_str( return TILEDB_OK; } +capi_return_t tiledb_group_dump_str_v2( + tiledb_group_handle_t* group, + tiledb_string_handle_t** dump_ascii, + const uint8_t recursive) { + ensure_group_is_valid(group); + ensure_output_pointer_is_valid(dump_ascii); + + *dump_ascii = + tiledb_string_handle_t::make_handle(group->group().dump(2, 0, recursive)); + + return TILEDB_OK; +} + capi_return_t tiledb_serialize_group( tiledb_ctx_handle_t* ctx, const tiledb_group_handle_t* group, @@ -787,6 +800,16 @@ CAPI_INTERFACE( ctx, group, dump_ascii, recursive); } +CAPI_INTERFACE( + group_dump_str_v2, + tiledb_ctx_t* ctx, + tiledb_group_t* group, + tiledb_string_handle_t** dump_ascii, + const uint8_t recursive) { + return api_entry_context( + ctx, group, dump_ascii, recursive); +} + CAPI_INTERFACE( serialize_group, tiledb_ctx_t* ctx, diff --git a/tiledb/api/c_api/group/group_api_external.h b/tiledb/api/c_api/group/group_api_external.h index 9bd03659fff..14ff01aac16 100644 --- a/tiledb/api/c_api/group/group_api_external.h +++ b/tiledb/api/c_api/group/group_api_external.h @@ -621,7 +621,9 @@ TILEDB_EXPORT capi_return_t tiledb_group_get_query_type( tiledb_query_type_t* query_type) TILEDB_NOEXCEPT; /** - * Dump a string representation of a group + * Dump a string representation of a group. + * + * Deprecated, use tiledb_group_dump_str instead. * * @param ctx The TileDB context. * @param group The group. @@ -630,12 +632,27 @@ TILEDB_EXPORT capi_return_t tiledb_group_get_query_type( * @param recursive should we recurse into sub-groups * @return `TILEDB_OK` for success and `TILEDB_ERR` for error. */ -TILEDB_EXPORT capi_return_t tiledb_group_dump_str( +TILEDB_DEPRECATED_EXPORT capi_return_t tiledb_group_dump_str( tiledb_ctx_t* ctx, tiledb_group_t* group, char** dump_ascii, const uint8_t recursive) TILEDB_NOEXCEPT; +/** + * Dump a string representation of a group. + * + * @param ctx The TileDB context. + * @param group The group. + * @param dump_ascii The output string handle. + * @param recursive should we recurse into sub-groups + * @return `TILEDB_OK` for success and `TILEDB_ERR` for error. + */ +TILEDB_EXPORT capi_return_t tiledb_group_dump_str_v2( + tiledb_ctx_t* ctx, + tiledb_group_t* group, + tiledb_string_t** dump_ascii, + const uint8_t recursive) TILEDB_NOEXCEPT; + /** * Consolidates the group metadata into a single group metadata file. * diff --git a/tiledb/sm/cpp_api/group.h b/tiledb/sm/cpp_api/group.h index fc8021f45d7..4be836bc13b 100644 --- a/tiledb/sm/cpp_api/group.h +++ b/tiledb/sm/cpp_api/group.h @@ -473,13 +473,11 @@ class Group { std::string dump(const bool recursive) const { auto& ctx = ctx_.get(); tiledb_ctx_t* c_ctx = ctx.ptr().get(); - char* str; + tiledb_string_t* str; ctx.handle_error( - tiledb_group_dump_str(c_ctx, group_.get(), &str, recursive)); + tiledb_group_dump_str_v2(c_ctx, group_.get(), &str, recursive)); - std::string ret(str); - free(str); - return ret; + return impl::convert_to_string(&str).value(); } /** From 8a518ae20c8ba3408457a1bc6bd5ce48827e9171 Mon Sep 17 00:00:00 2001 From: Theodore Tsirpanis Date: Thu, 7 Nov 2024 22:10:47 +0200 Subject: [PATCH 2/5] Update list of deprecated APIs. --- doc/policy/api_changes.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/doc/policy/api_changes.md b/doc/policy/api_changes.md index 25c689d5221..9c91c880009 100644 --- a/doc/policy/api_changes.md +++ b/doc/policy/api_changes.md @@ -79,6 +79,10 @@ Function removal shall be announced one release cycle before removal, following - `tiledb_group_get_member_by_index` - `tiledb_group_get_member_by_name` +### 2.27.0..2.28.0 + +- `tiledb_group_dump_str` + ## Deprecation History Generation
From 84eb742075a015cdbf26db3b24a6cd37bb0da8c0 Mon Sep 17 00:00:00 2001 From: Theodore Tsirpanis Date: Wed, 13 Nov 2024 16:10:39 +0200 Subject: [PATCH 3/5] Update test. --- test/src/unit-capi-group.cc | 38 +++++++++++++++++++++++++++++-------- 1 file changed, 30 insertions(+), 8 deletions(-) diff --git a/test/src/unit-capi-group.cc b/test/src/unit-capi-group.cc index 920362990ef..52f82e89082 100644 --- a/test/src/unit-capi-group.cc +++ b/test/src/unit-capi-group.cc @@ -1563,14 +1563,36 @@ TEST_CASE_METHOD( rc = tiledb_group_open(ctx_, group1, TILEDB_READ); REQUIRE(rc == TILEDB_OK); - char* group_dump; - rc = tiledb_group_dump_str(ctx_, group1, &group_dump, 1); - REQUIRE(rc == TILEDB_OK); - REQUIRE(group_dump != nullptr); - REQUIRE( - std::string(group_dump) == - "group1 GROUP\n|-- group2 GROUP (does not exist)\n"); - free(group_dump); + std::string group_dump; + // Test both the new and the deprecated API. Remove the else part when the + // deprecated API gets removed. + bool use_dump_v2 = GENERATE(true, false); + if (use_dump_v2) { + tiledb_string_t* group_dump_handle; + rc = tiledb_group_dump_str_v2(ctx_, group1, &group_dump_handle, 1); + REQUIRE(rc == TILEDB_OK); + REQUIRE(group_dump_handle != nullptr); + const char* group_dump_ptr; + size_t group_dump_size; + rc = tiledb_string_view( + group_dump_handle, &group_dump_ptr, &group_dump_size); + REQUIRE(rc == TILEDB_OK); + REQUIRE(group_dump_ptr != nullptr); + group_dump = std::string(group_dump_ptr, group_dump_size); + rc = tiledb_string_free(&group_dump_handle); + REQUIRE(rc == TILEDB_OK); + } else { + char* group_dump_ptr; + rc = tiledb_group_dump_str(ctx_, group1, &group_dump_ptr, 1); + REQUIRE(rc == TILEDB_OK); + REQUIRE(group_dump_ptr != nullptr); + group_dump = std::string(group_dump_ptr); + // Usually it's not safe to call free() on this pointer, but since we are + // building everything at the same time, tiledb's and tiledb_unit's + // allocators are the same. + free(group_dump_ptr); + } + REQUIRE(group_dump == "group1 GROUP\n|-- group2 GROUP (does not exist)\n"); rc = tiledb_group_close(ctx_, group1); REQUIRE(rc == TILEDB_OK); From 9c00aa8e2fe16dbc682e2c113b21641d59056dee Mon Sep 17 00:00:00 2001 From: Theodore Tsirpanis Date: Wed, 13 Nov 2024 16:12:15 +0200 Subject: [PATCH 4/5] Update API documentation. --- tiledb/api/c_api/group/group_api_external.h | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/tiledb/api/c_api/group/group_api_external.h b/tiledb/api/c_api/group/group_api_external.h index 14ff01aac16..b4a7f995b49 100644 --- a/tiledb/api/c_api/group/group_api_external.h +++ b/tiledb/api/c_api/group/group_api_external.h @@ -623,7 +623,7 @@ TILEDB_EXPORT capi_return_t tiledb_group_get_query_type( /** * Dump a string representation of a group. * - * Deprecated, use tiledb_group_dump_str instead. + * Deprecated, use tiledb_group_dump_str_v2 instead. * * @param ctx The TileDB context. * @param group The group. @@ -643,7 +643,8 @@ TILEDB_DEPRECATED_EXPORT capi_return_t tiledb_group_dump_str( * * @param ctx The TileDB context. * @param group The group. - * @param dump_ascii The output string handle. + * @param dump_ascii The output string handle. The caller has the responsibility + * to free it. * @param recursive should we recurse into sub-groups * @return `TILEDB_OK` for success and `TILEDB_ERR` for error. */ From d78c442684498527e4bc144af5f814b707493a6e Mon Sep 17 00:00:00 2001 From: Theodore Tsirpanis Date: Thu, 14 Nov 2024 03:06:42 +0200 Subject: [PATCH 5/5] Address PR feedback. --- tiledb/api/c_api/group/group_api_external.h | 18 ++++++++++++++---- 1 file changed, 14 insertions(+), 4 deletions(-) diff --git a/tiledb/api/c_api/group/group_api_external.h b/tiledb/api/c_api/group/group_api_external.h index b4a7f995b49..f91418b3d48 100644 --- a/tiledb/api/c_api/group/group_api_external.h +++ b/tiledb/api/c_api/group/group_api_external.h @@ -629,7 +629,7 @@ TILEDB_EXPORT capi_return_t tiledb_group_get_query_type( * @param group The group. * @param dump_ascii The output string. The caller takes ownership * of the c-string. - * @param recursive should we recurse into sub-groups + * @param recursive True if the dump should recurse into subgroups. * @return `TILEDB_OK` for success and `TILEDB_ERR` for error. */ TILEDB_DEPRECATED_EXPORT capi_return_t tiledb_group_dump_str( @@ -641,11 +641,21 @@ TILEDB_DEPRECATED_EXPORT capi_return_t tiledb_group_dump_str( /** * Dump a string representation of a group. * + * The output string handle must be freed by the user after use. + * + * **Example:** + * + * @code{.c} + * tiledb_string_t* tdb_string; + * tiledb_group_dump_str_v2(ctx, group, &tdb_string); + * // Use the string + * tiledb_string_free(&tdb_string); + * @endcode + * * @param ctx The TileDB context. * @param group The group. - * @param dump_ascii The output string handle. The caller has the responsibility - * to free it. - * @param recursive should we recurse into sub-groups + * @param dump_ascii The output string handle. + * @param recursive True if the dump should recurse into subgroups. * @return `TILEDB_OK` for success and `TILEDB_ERR` for error. */ TILEDB_EXPORT capi_return_t tiledb_group_dump_str_v2(