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 tiledb_group_get_member_by(index|name)_v2. #4019

Merged
merged 33 commits into from
Nov 2, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
33 commits
Select commit Hold shift + click to select a range
4b6d799
Add `tiledb_group_get_member_by_***_v2`.
teo-tsirpanis Apr 3, 2023
6fc5d17
Deprecate the existing group member APIs.
teo-tsirpanis Apr 3, 2023
c0f62f6
Use the new group member APIs.
teo-tsirpanis Apr 3, 2023
8dde4aa
Add a dedicated class to manage lifetime of string handles.
teo-tsirpanis Apr 3, 2023
b5a07a2
Merge branch 'dev' into group-get-member-by-index-v2
teo-tsirpanis Apr 10, 2023
0ce0af4
Make `StringHandleHolder` RAII-compliant and rename it to `CAPIString`.
teo-tsirpanis Apr 10, 2023
6959e7b
Fix a potential memory leak.
teo-tsirpanis Apr 10, 2023
24847c1
Address more PR feedback.
teo-tsirpanis Apr 10, 2023
cc325c9
Mark `CAPIString`'s constructor with `noexcept`.
teo-tsirpanis Apr 10, 2023
68b7063
Use static methods to expose `CAPIString`'s functionality, explicitly…
teo-tsirpanis Apr 10, 2023
8c2fd53
Keep testing the old function.
teo-tsirpanis Apr 10, 2023
3f73c58
Update the test.
teo-tsirpanis May 2, 2023
5f48204
Update `CAPIStringHandle`.
teo-tsirpanis May 2, 2023
4660ebd
Use `auto` once more.
teo-tsirpanis May 2, 2023
948ca02
Rename `make_CAPIString` to `to_string`.
teo-tsirpanis May 2, 2023
3920281
Fix formatting.
teo-tsirpanis May 2, 2023
35038c6
Do not use `SECTION`s inside `GroupFx::read_group`.
teo-tsirpanis May 2, 2023
1b8f8eb
Merge branch 'dev' into group-get-member-by-index-v2
teo-tsirpanis Oct 4, 2023
f538bda
Stop using rvalue references, according to review feedback.
teo-tsirpanis Oct 4, 2023
d266ee7
Fix compile errors.
teo-tsirpanis Oct 4, 2023
308f7f9
Fix typo.
teo-tsirpanis Oct 4, 2023
7d3450f
Don't leak memory when testing the old APIs.
teo-tsirpanis Oct 9, 2023
23ecd15
Merge branch 'dev' into group-get-member-by-index-v2
teo-tsirpanis Oct 19, 2023
f4be46e
Address PR feedback.
teo-tsirpanis Oct 19, 2023
b11b572
Merge branch 'dev' into group-get-member-by-index-v2
teo-tsirpanis Oct 24, 2023
7b70576
Address most PR feedback.
teo-tsirpanis Oct 24, 2023
f756520
Merge branch 'dev' into group-get-member-by-index-v2
teo-tsirpanis Oct 26, 2023
002d98a
Add more tests.
teo-tsirpanis Oct 26, 2023
8c017f2
Test that accessing a freed string handle fails.
teo-tsirpanis Oct 26, 2023
388a38a
clang-format
teo-tsirpanis Oct 26, 2023
d9c858f
Fix compile errors.
teo-tsirpanis Oct 26, 2023
b348631
Merge branch 'dev' into group-get-member-by-index-v2
teo-tsirpanis Nov 1, 2023
c3eceb1
Address remaining feedback.
teo-tsirpanis Nov 1, 2023
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
5 changes: 5 additions & 0 deletions doc/policy/api_changes.md
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,11 @@ Function removal shall be announced one release cycle before removal, following

- `tiledb_query_submit_async`

### 2.18.0..2.19.0

- `tiledb_group_get_member_by_index`
- `tiledb_group_get_member_by_name`

eric-hughes-tiledb marked this conversation as resolved.
Show resolved Hide resolved
## Deprecation History Generation

<details>
Expand Down
62 changes: 49 additions & 13 deletions test/src/unit-capi-group.cc
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,7 @@ struct GroupFx {
std::string get_golden_ls(const std::string& path);
static std::string random_name(const std::string& prefix);
std::vector<std::pair<tiledb::sm::URI, tiledb_object_t>> read_group(
tiledb_group_t* group) const;
tiledb_group_t* group, bool use_get_member_by_index_v2 = true) const;
void set_group_timestamp(
tiledb_group_t* group, const uint64_t& timestamp) const;
void write_group_metadata(const char* group_uri) const;
Expand Down Expand Up @@ -230,20 +230,49 @@ void GroupFx::vacuum(const char* group_uri) const {
}

std::vector<std::pair<tiledb::sm::URI, tiledb_object_t>> GroupFx::read_group(
tiledb_group_t* group) const {
tiledb_group_t* group, bool use_get_member_by_index_v2) const {
Copy link
Contributor

Choose a reason for hiding this comment

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

This would better have been done as a template argument. It's adequate as-is, though. Using a template argument would make removal easier, because there's so little shared code that the two versions could have been defined with specializations instead of if or even if constexpr. Ultimate removal would then entail removing a whole function definition instead of editing one.

std::vector<std::pair<tiledb::sm::URI, tiledb_object_t>> ret;
uint64_t count = 0;
char* uri;
tiledb_object_t type;
char* name;
int rc = tiledb_group_get_member_count(ctx_, group, &count);
REQUIRE(rc == TILEDB_OK);
for (uint64_t i = 0; i < count; i++) {
rc = tiledb_group_get_member_by_index(ctx_, group, i, &uri, &type, &name);
REQUIRE(rc == TILEDB_OK);
ret.emplace_back(uri, type);
std::free(uri);
std::free(name);
eric-hughes-tiledb marked this conversation as resolved.
Show resolved Hide resolved
if (use_get_member_by_index_v2) {
for (uint64_t i = 0; i < count; i++) {
std::string uri;
tiledb_object_t type;
tiledb_string_t *uri_handle, *name_handle;
rc = tiledb_group_get_member_by_index_v2(
ctx_, group, i, &uri_handle, &type, &name_handle);
REQUIRE(rc == TILEDB_OK);
const char* uri_str;
size_t uri_size;
rc = tiledb_string_view(uri_handle, &uri_str, &uri_size);
CHECK(rc == TILEDB_OK);
uri = std::string(uri_str, uri_size);
rc = tiledb_string_free(&uri_handle);
CHECK(rc == TILEDB_OK);
if (name_handle) {
rc = tiledb_string_free(&name_handle);
CHECK(rc == TILEDB_OK);
}
ret.emplace_back(uri, type);
}
} else {
// When tiledb_group_get_member_by_index gets removed, the else part can
// simply be removed.
for (uint64_t i = 0; i < count; i++) {
std::string uri;
tiledb_object_t type;
char *uri_ptr, *name_ptr;
rc = tiledb_group_get_member_by_index(
ctx_, group, i, &uri_ptr, &type, &name_ptr);
REQUIRE(rc == TILEDB_OK);
uri = uri_ptr == nullptr ? "" : std::string(uri_ptr);
std::free(uri_ptr);
if (name_ptr) {
std::free(name_ptr);
}
ret.emplace_back(uri, type);
}
}
return ret;
}
Expand Down Expand Up @@ -549,6 +578,13 @@ TEST_CASE_METHOD(

TEST_CASE_METHOD(
GroupFx, "C API: Group, write/read", "[capi][group][metadata][read]") {
bool use_get_member_by_index_v2 = true;
SECTION("Using tiledb_group_get_member_by_index_v2") {
use_get_member_by_index_v2 = true;
}
SECTION("Using tiledb_group_get_member_by_index") {
use_get_member_by_index_v2 = false;
}
// Create and open group in write mode
// TODO: refactor for each supported FS.
std::string temp_dir = fs_vec_[0]->temp_dir();
Expand Down Expand Up @@ -618,12 +654,12 @@ TEST_CASE_METHOD(
REQUIRE(rc == TILEDB_OK);

std::vector<std::pair<tiledb::sm::URI, tiledb_object_t>> group1_received =
read_group(group1);
read_group(group1, use_get_member_by_index_v2);
REQUIRE_THAT(
group1_received, Catch::Matchers::UnorderedEquals(group1_expected));

std::vector<std::pair<tiledb::sm::URI, tiledb_object_t>> group2_received =
read_group(group2);
read_group(group2, use_get_member_by_index_v2);
REQUIRE_THAT(
group2_received, Catch::Matchers::UnorderedEquals(group2_expected));

Expand Down
1 change: 1 addition & 0 deletions tiledb/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -120,6 +120,7 @@ if (TILEDB_CPP_API)
${TILEDB_CORE_INCLUDE_DIR}/tiledb/sm/cpp_api/query_experimental.h
${TILEDB_CORE_INCLUDE_DIR}/tiledb/sm/cpp_api/schema_base.h
${TILEDB_CORE_INCLUDE_DIR}/tiledb/sm/cpp_api/stats.h
${TILEDB_CORE_INCLUDE_DIR}/tiledb/sm/cpp_api/capi_string.h
${TILEDB_CORE_INCLUDE_DIR}/tiledb/sm/cpp_api/subarray.h
${TILEDB_CORE_INCLUDE_DIR}/tiledb/sm/cpp_api/subarray_experimental.h
${TILEDB_CORE_INCLUDE_DIR}/tiledb/sm/cpp_api/type.h
Expand Down
1 change: 1 addition & 0 deletions tiledb/api/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ include(common-root)
add_subdirectory(c_api)
add_subdirectory(c_api_support)
add_subdirectory(c_api_test_support)
add_subdirectory(cpp_api_support)

get_gathered(API_SOURCES)
set(TILEDB_API_SOURCES ${API_SOURCES} PARENT_SCOPE)
80 changes: 80 additions & 0 deletions tiledb/api/c_api/group/group_api.cc
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@
#include "../buffer/buffer_api_internal.h"
#include "../config/config_api_internal.h"
#include "../context/context_api_internal.h"
#include "../string/string_api_internal.h"

#include "group_api_internal.h"

Expand Down Expand Up @@ -306,6 +307,10 @@ capi_return_t tiledb_group_get_member_by_index(
ensure_output_pointer_is_valid(type);
ensure_output_pointer_is_valid(name);

LOG_WARN(
"tiledb_group_get_member_by_index is deprecated. Please use "
"tiledb_group_get_member_by_index_v2 instead.");

char* tmp_uri = nullptr;
char* tmp_name = nullptr;

Expand Down Expand Up @@ -343,6 +348,34 @@ capi_return_t tiledb_group_get_member_by_index(
return TILEDB_OK;
}

capi_return_t tiledb_group_get_member_by_index_v2(
tiledb_group_handle_t* group,
uint64_t index,
tiledb_string_t** uri,
tiledb_object_t* type,
tiledb_string_t** name) {
ensure_group_is_valid(group);
ensure_output_pointer_is_valid(uri);
ensure_output_pointer_is_valid(type);
ensure_output_pointer_is_valid(name);

auto&& [uri_str, object_type, name_str] =
group->group().member_by_index(index);

*uri = tiledb_string_handle_t::make_handle(uri_str);
*type = static_cast<tiledb_object_t>(object_type);
try {
*name = name_str.has_value() ?
tiledb_string_handle_t::make_handle(*name_str) :
nullptr;
} catch (...) {
tiledb_string_handle_t::break_handle(*uri);
throw;
}

return TILEDB_OK;
}

capi_return_t tiledb_group_get_member_by_name(
tiledb_group_handle_t* group,
const char* name,
Expand All @@ -353,6 +386,10 @@ capi_return_t tiledb_group_get_member_by_name(
ensure_output_pointer_is_valid(uri);
ensure_output_pointer_is_valid(type);

LOG_WARN(
"tiledb_group_get_member_by_name is deprecated. Please use "
"tiledb_group_get_member_by_name_v2 instead.");

auto&& [uri_str, object_type, name_str, ignored_relative] =
group->group().member_by_name(name);

Expand All @@ -366,6 +403,26 @@ capi_return_t tiledb_group_get_member_by_name(
return TILEDB_OK;
}

capi_return_t tiledb_group_get_member_by_name_v2(
tiledb_group_handle_t* group,
const char* name,
tiledb_string_t** uri,
tiledb_object_t* type) {
ensure_group_is_valid(group);
ensure_output_pointer_is_valid(uri);
ensure_output_pointer_is_valid(type);

std::string uri_str;
sm::ObjectType object_type;
std::tie(uri_str, object_type, std::ignore, std::ignore) =
group->group().member_by_name(name);

*uri = tiledb_string_handle_t::make_handle(std::move(uri_str));
*type = static_cast<tiledb_object_t>(object_type);

return TILEDB_OK;
}

capi_return_t tiledb_group_get_is_relative_uri_by_name(
tiledb_group_handle_t* group, const char* name, uint8_t* is_relative) {
ensure_group_is_valid(group);
Expand Down Expand Up @@ -716,6 +773,18 @@ CAPI_INTERFACE(
ctx, group, index, uri, type, name);
}

CAPI_INTERFACE(
group_get_member_by_index_v2,
tiledb_ctx_t* ctx,
tiledb_group_t* group,
uint64_t index,
tiledb_string_t** uri,
tiledb_object_t* type,
tiledb_string_t** name) {
return api_entry_context<tiledb::api::tiledb_group_get_member_by_index_v2>(
ctx, group, index, uri, type, name);
}

CAPI_INTERFACE(
group_get_member_by_name,
tiledb_ctx_t* ctx,
Expand All @@ -727,6 +796,17 @@ CAPI_INTERFACE(
ctx, group, name, uri, type);
}

CAPI_INTERFACE(
group_get_member_by_name_v2,
tiledb_ctx_t* ctx,
tiledb_group_t* group,
const char* name,
tiledb_string_t** uri,
tiledb_object_t* type) {
return api_entry_context<tiledb::api::tiledb_group_get_member_by_name_v2>(
ctx, group, name, uri, type);
}

CAPI_INTERFACE(
group_get_is_relative_uri_by_name,
tiledb_ctx_t* ctx,
Expand Down
95 changes: 86 additions & 9 deletions tiledb/api/c_api/group/group_api_external_experimental.h
Original file line number Diff line number Diff line change
Expand Up @@ -400,7 +400,8 @@ TILEDB_EXPORT capi_return_t tiledb_group_get_member_count(
tiledb_ctx_t* ctx, tiledb_group_t* group, uint64_t* count) TILEDB_NOEXCEPT;

/**
* Get a member of a group by index and details of group
* Get a member of a group by index and details of group.
* Deprecated, use \p tiledb_group_get_member_by_index_v2 instead.
*
* **Example:**
*
Expand All @@ -424,14 +425,12 @@ TILEDB_EXPORT capi_return_t tiledb_group_get_member_count(
* @param ctx The TileDB context.
* @param group An group opened in READ mode.
* @param index index of member to fetch
* @param uri URI of member, The caller takes ownership
* of the c-string.
* @param uri URI of member.
* @param type type of member
* @param name name of member, The caller takes ownership
* of the c-string. NULL if name was not set
* @param name name of member. NULL if name was not set
* @return `TILEDB_OK` for success and `TILEDB_ERR` for error.
*/
TILEDB_EXPORT capi_return_t tiledb_group_get_member_by_index(
TILEDB_DEPRECATED_EXPORT capi_return_t tiledb_group_get_member_by_index(
tiledb_ctx_t* ctx,
tiledb_group_t* group,
uint64_t index,
Expand All @@ -448,6 +447,46 @@ TILEDB_EXPORT capi_return_t tiledb_group_get_member_by_index(
* tiledb_group_t* group;
* tiledb_group_alloc(ctx, "s3://tiledb_bucket/my_group", &group);
* tiledb_group_open(ctx, group, TILEDB_WRITE);
* tiledb_group_add_member(ctx, group, "s3://tiledb_bucket/my_array");
* tiledb_group_add_member(ctx, group, "s3://tiledb_bucket/my_group_2");
*
* tiledb_group_close(ctx, group);
* tiledb_group_open(ctx, group, TILEDB_READ);
* tiledb_string_t *uri, *name;
* tiledb_object_t type;
* tiledb_group_get_member_by_index_v2(ctx, group, 0, &uri, &type, &name);
*
* tiledb_string_free(uri);
* tiledb_string_free(name);
*
* @endcode
*
* @param ctx The TileDB context.
* @param group An group opened in READ mode.
* @param index index of member to fetch
* @param uri Handle to the URI of the member.
* @param type type of member
* @param name Handle to the name of the member. NULL if name was not set
* @return `TILEDB_OK` for success and `TILEDB_ERR` for error.
*/
TILEDB_EXPORT capi_return_t tiledb_group_get_member_by_index_v2(
tiledb_ctx_t* ctx,
tiledb_group_t* group,
uint64_t index,
tiledb_string_t** uri,
tiledb_object_t* type,
tiledb_string_t** name) TILEDB_NOEXCEPT;

/**
* Get a member of a group by name and details of group.
* Deprecated, use \p tiledb_group_get_member_by_name_v2.
*
* **Example:**
*
* @code{.c}
* tiledb_group_t* group;
* tiledb_group_alloc(ctx, "s3://tiledb_bucket/my_group", &group);
* tiledb_group_open(ctx, group, TILEDB_WRITE);
* tiledb_group_add_member(ctx, group, "s3://tiledb_bucket/my_array", "array1");
* tiledb_group_add_member(ctx, group, "s3://tiledb_bucket/my_group_2",
* "group2");
Expand All @@ -465,18 +504,56 @@ TILEDB_EXPORT capi_return_t tiledb_group_get_member_by_index(
* @param ctx The TileDB context.
* @param group An group opened in READ mode.
* @param name name of member to fetch
* @param uri URI of member, The caller takes ownership
* of the c-string.
* @param uri URI of member
* @param type type of member
* @return `TILEDB_OK` for success and `TILEDB_ERR` for error.
*/
TILEDB_EXPORT capi_return_t tiledb_group_get_member_by_name(
TILEDB_DEPRECATED_EXPORT capi_return_t tiledb_group_get_member_by_name(
tiledb_ctx_t* ctx,
tiledb_group_t* group,
const char* name,
char** uri,
tiledb_object_t* type) TILEDB_NOEXCEPT;

/**
* Get a member of a group by name and details of group.
*
* **Example:**
*
* @code{.c}
* tiledb_group_t* group;
* tiledb_group_alloc(ctx, "s3://tiledb_bucket/my_group", &group);
* tiledb_group_open(ctx, group, TILEDB_WRITE);
* tiledb_group_add_member(ctx, group, "s3://tiledb_bucket/my_array", "array1");
* tiledb_group_add_member(ctx, group, "s3://tiledb_bucket/my_group_2",
* "group2");
*
* tiledb_group_close(ctx, group);
* tiledb_group_open(ctx, group, TILEDB_READ);
* tilledb_string_t *uri;
* tiledb_object_t type;
* tiledb_group_get_member_by_name(ctx, group, "array1", &uri, &type);
*
* tiledb_string_free(uri);
*
* @endcode
*
* @param ctx The TileDB context.
* @param group An group opened in READ mode.
* @param name name of member to fetch
* @param uri Handle to the URI of the member.
* @param type type of member
* @return `TILEDB_OK` for success and `TILEDB_ERR` for error.
*/
TILEDB_EXPORT capi_return_t tiledb_group_get_member_by_name_v2(
tiledb_ctx_t* ctx,
tiledb_group_t* group,
const char* name,
tiledb_string_t** uri,
tiledb_object_t* type) TILEDB_NOEXCEPT;

/* (clang format was butchering the tiledb_group_add_member() calls) */
/* clang-format off */
/**
* Get a member of a group by name and relative characteristic of that name
*
Expand Down
Loading
Loading