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

Add API to dump groups that returns a string handle. #5367

Merged
merged 5 commits into from
Nov 14, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions doc/policy/api_changes.md
Original file line number Diff line number Diff line change
Expand Up @@ -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

<details>
Expand Down
11 changes: 7 additions & 4 deletions examples/c_api/groups.c
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
Expand Down
38 changes: 30 additions & 8 deletions test/src/unit-capi-group.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
23 changes: 23 additions & 0 deletions tiledb/api/c_api/group/group_api.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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<tiledb::api::tiledb_group_dump_str_v2>(
ctx, group, dump_ascii, recursive);
}

CAPI_INTERFACE(
serialize_group,
tiledb_ctx_t* ctx,
Expand Down
34 changes: 31 additions & 3 deletions tiledb/api/c_api/group/group_api_external.h
Original file line number Diff line number Diff line change
Expand Up @@ -621,21 +621,49 @@ 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_v2 instead.
*
* @param ctx The TileDB context.
* @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_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.
Copy link
Member

Choose a reason for hiding this comment

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

We need to mention the caller has the responsibility to free the allocated tiledb_string_t.

Copy link
Member Author

Choose a reason for hiding this comment

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

Mentioned.

*
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing example. The example could highlight the user's responsibility to free dump_ascii.

Copy link
Contributor

Choose a reason for hiding this comment

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

It's more clear to add an @note The caller must free the returned char (dump_ascii). here, before the params, rather than an inline note that could easily be overlooked.

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated documentation to match other dump APIs.

/**
* Dumps the contents of a dimension in ASCII form to the selected string
* output.
*
* The output string handle must be freed by the user after use.
*
* **Example:**
*
* @code{.c}
* tiledb_string_t* tdb_string;
* tiledb_dimension_dump_str(ctx, dimension, &tdb_string);
* // Use the string
* tiledb_string_free(&tdb_string);
* @endcode
*
* @param ctx The TileDB context.
* @param dimension The dimension.
* @param out The output string.
* @return `TILEDB_OK` for success and `TILEDB_ERR` for error.
*/

* 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.
* @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(
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.
*
Expand Down
8 changes: 3 additions & 5 deletions tiledb/sm/cpp_api/group.h
Original file line number Diff line number Diff line change
Expand Up @@ -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));
Copy link
Collaborator

Choose a reason for hiding this comment

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

I really don't see that much into this code, but don't we need to free it here since we copy it into string? or does the convert_to_string logic frees the string?

Copy link
Member Author

Choose a reason for hiding this comment

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

The latter. We move the handle to a class which frees it when it gets destroyed.

inline std::optional<std::string> convert_to_string(tiledb_string_t** handle) {
if (*handle == nullptr) {
return std::nullopt;
}
return CAPIString(handle).str();
}


std::string ret(str);
free(str);
return ret;
return impl::convert_to_string(&str).value();
}

/**
Expand Down
Loading