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 an additional C API example "aggregates_string" which demonstrates running min/max on string dimension and emitting coordinates from the same query #5302

Open
wants to merge 12 commits into
base: dev
Choose a base branch
from

Conversation

rroelke
Copy link
Contributor

@rroelke rroelke commented Sep 12, 2024

An example like this would have been a useful reference when building the query aggregate API in tiledb-rs (and in fact it shows that we did a few things wrong over there).

So, we combine the quickstart_sparse_string.c and aggregates.c examples into a new example which demonstrates the above.

I have also added tiledb_examples.h with some functions/macros to do error checking. Whenever I have played with one of the examples to make a bug reproducer or learn a bit I am adding a bunch of error checks to see if I am doing anything wrong. Error checking should be a standard part of the examples and this header provides a fairly non-intrusive way to do it.


TYPE: IMPROVEMENT
DESC: Add aggregates_string.c example and tiledb_examples.h for common example code

Copy link
Member

@teo-tsirpanis teo-tsirpanis left a comment

Choose a reason for hiding this comment

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

Thanks for working on this! Left some comments, and it might be a good idea to add a corresponding C++ example as well.

examples/c_api/tiledb_examples.h Outdated Show resolved Hide resolved
examples/c_api/aggregates_string.c Outdated Show resolved Hide resolved
examples/c_api/aggregates_string.c Outdated Show resolved Hide resolved
examples/c_api/aggregates_string.c Show resolved Hide resolved
examples/c_api/aggregates_string.c Outdated Show resolved Hide resolved
examples/c_api/aggregates_string.c Outdated Show resolved Hide resolved
Comment on lines +292 to +294
TRY(ctx,
tiledb_query_set_offsets_buffer(
ctx, query, "Min(rows)", min_offsets, &min_offsets_size));
Copy link
Member

Choose a reason for hiding this comment

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

What is the purpose of min_offsets? Since Min(rows) is an aggregate it will have only one value and its size can be obtained through min_size set above, but I'm seeing min_offsets[0] being used below; shouldn't it always be zero?

If the Core requires an offsets buffer for a variable-sized aggregate field to be set, that should be fixed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's right, the offsets will always be written as a single array whose contents are zero.

The offsets buffer is required.

Both of these aspects are also true if you were to query a single variable-length cell; while I agree that there's no data to be communicated, it has the same structure as existing code. So I'm not sure I agree with you that anything is worth changing here.

examples/c_api/aggregates_string.c Outdated Show resolved Hide resolved
examples/c_api/tiledb_examples.h Outdated Show resolved Hide resolved
@KiterLuc
Copy link
Contributor

+1 on adding the matching CPP API example.

@rroelke
Copy link
Contributor Author

rroelke commented Sep 17, 2024

I have added a corresponding C++ example

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants