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

teo-tsirpanis · 2024-11-07T20:44:24Z

This PR adds a new C API tiledb_group_dump_str_v2 that dumps a group into a tiledb_string_t*. This allows the string to be safely freed under certain circumstances (such as when debug and release CRT are mixed on Windows).

The existing tiledb_group_dump_str API was deprecated. Migrating to the new API is straightforward and requires no changes to the public surface of the higher-level APIs. The C++ API and the C groups example are migrated in this PR.

TYPE: C_API
DESC: Added tiledb_group_dump_str_v2 API that returns a tiledb_string_t*. The existing tiledb_group_dump_str API is deprecated.

ypatia

Looks good but a test is missing. Let's at least update the existing tests that use the deprecated API to also use the new one.

ypatia · 2024-11-11T09:02:55Z

tiledb/api/c_api/group/group_api_external.h

- * Dump a string representation of a group
+ * Dump a string representation of a group.
+ *
+ * Deprecated, use tiledb_group_dump_str instead.


tiledb_group_dump_str_v2 ?

Oops, thanks!

ypatia · 2024-11-11T09:17:24Z

tiledb/api/c_api/group/group_api_external.h

    tiledb_ctx_t* ctx,
    tiledb_group_t* group,
    char** dump_ascii,
    const uint8_t recursive) TILEDB_NOEXCEPT;

+/**
+ * Dump a string representation of a group.


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

teo-tsirpanis · 2024-11-13T14:38:30Z

Feedback addressed.

dudoslav · 2024-11-13T14:54:56Z

tiledb/sm/cpp_api/group.h

    ctx.handle_error(
-        tiledb_group_dump_str(c_ctx, group_.get(), &str, recursive));
+        tiledb_group_dump_str_v2(c_ctx, group_.get(), &str, recursive));


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?

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

TileDB/tiledb/sm/cpp_api/capi_string.h

Lines 102 to 107 in abca93c

inline std::optional<std::string> convert_to_string(tiledb_string_t** handle) {

if (*handle == nullptr) {

return std::nullopt;

}

return CAPIString(handle).str();

}

bekadavis9 · 2024-11-13T17:37:17Z

tiledb/api/c_api/group/group_api_external.h

+ * @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


Suggested change

* @param recursive should we recurse into sub-groups

* @param recursive True if the dump should recurse into subgroups.

bekadavis9 · 2024-11-13T17:37:26Z

tiledb/api/c_api/group/group_api_external.h

    tiledb_ctx_t* ctx,
    tiledb_group_t* group,
    char** dump_ascii,
    const uint8_t recursive) TILEDB_NOEXCEPT;

+/**
+ * Dump a string representation of a group.
+ *


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

bekadavis9 · 2024-11-13T17:43:57Z

tiledb/api/c_api/group/group_api_external.h

    tiledb_ctx_t* ctx,
    tiledb_group_t* group,
    char** dump_ascii,
    const uint8_t recursive) TILEDB_NOEXCEPT;

+/**
+ * Dump a string representation of a group.
+ *


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.

Updated documentation to match other dump APIs.

TileDB/tiledb/api/c_api/dimension/dimension_api_external.h

Lines 298 to 317 in abca93c

/**

* 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.

*/

bekadavis9

LGTM

teo-tsirpanis added 2 commits November 7, 2024 21:51

Add API to dump groups that returns a string handle.

ea4bdb3

Update list of deprecated APIs.

8a518ae

teo-tsirpanis requested a review from ihnorton November 7, 2024 20:44

ihnorton requested review from bekadavis9, kounelisagis and dudoslav and removed request for kounelisagis November 9, 2024 15:55

ypatia reviewed Nov 11, 2024

View reviewed changes

teo-tsirpanis added 2 commits November 13, 2024 16:37

Update test.

84eb742

Update API documentation.

9c00aa8

teo-tsirpanis requested a review from ypatia November 13, 2024 14:38

dudoslav reviewed Nov 13, 2024

View reviewed changes

bekadavis9 reviewed Nov 13, 2024

View reviewed changes

Address PR feedback.

d78c442

bekadavis9 approved these changes Nov 14, 2024

View reviewed changes

teo-tsirpanis merged commit 871d2ed into dev Nov 14, 2024
65 checks passed

teo-tsirpanis deleted the teo/group-dump-str-v2 branch November 14, 2024 15:00

ypatia approved these changes Nov 14, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

teo-tsirpanis commented Nov 7, 2024

ypatia left a comment

ypatia Nov 11, 2024

teo-tsirpanis Nov 13, 2024

ypatia Nov 11, 2024

teo-tsirpanis Nov 13, 2024

teo-tsirpanis commented Nov 13, 2024

dudoslav Nov 13, 2024

teo-tsirpanis Nov 13, 2024

bekadavis9 Nov 13, 2024

bekadavis9 Nov 13, 2024

bekadavis9 Nov 13, 2024

teo-tsirpanis Nov 14, 2024

bekadavis9 left a comment

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

	* @param recursive should we recurse into sub-groups
	* @param recursive True if the dump should recurse into subgroups.

	/**
	* 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.
	*/

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

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

Conversation

teo-tsirpanis commented Nov 7, 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

teo-tsirpanis commented Nov 13, 2024

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

bekadavis9 left a comment

Choose a reason for hiding this comment