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

Support var length dimension labels from Python #4142

Merged
merged 7 commits into from
Jul 5, 2023

Conversation

shaunrd0
Copy link
Contributor

@shaunrd0 shaunrd0 commented Jun 27, 2023

Adds result_buffer_elements_nullable to support variable size dimension labels in TileDB-Py.


TYPE: FEATURE
DESC: Adds support for var length dimension label reads from Python

@shortcut-integration
Copy link

This pull request has been linked to Shortcut Story #29317: Dimension label Python API String read support.

@shaunrd0 shaunrd0 force-pushed the smr/sc-29317/py-var-size-dim-labels branch from 4903c72 to 107803e Compare June 27, 2023 18:26
Copy link
Contributor

@jp-dark jp-dark left a comment

Choose a reason for hiding this comment

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

As discussed previously, you may want to consider how to make the changes here work for both range and data dimension label queries to prevent issues down the line. A query can have either or both for a single dimension label.

I also made some comments about moving things to the experimental C++ API. I don't know how careful we want to be about not adding experimental C API to the C++ API, so you may want to check with someone else on that. Feel free to ignore those comments if we don't care as a rule.

tiledb/sm/cpp_api/array_schema.h Outdated Show resolved Hide resolved
tiledb/sm/cpp_api/array_schema.h Outdated Show resolved Hide resolved
tiledb/sm/cpp_api/array_schema.h Outdated Show resolved Hide resolved
tiledb/sm/cpp_api/query.h Outdated Show resolved Hide resolved
tiledb/sm/cpp_api/query.h Outdated Show resolved Hide resolved
@shaunrd0 shaunrd0 requested a review from jp-dark June 28, 2023 17:15
tiledb/sm/c_api/tiledb_dimension_label_experimental.h Outdated Show resolved Hide resolved
tiledb/sm/cpp_api/query.h Outdated Show resolved Hide resolved
@ihnorton ihnorton requested a review from KiterLuc June 29, 2023 18:21
@shaunrd0
Copy link
Contributor Author

I was working on tests for est_result_size_label variants yesterday and noticed it only works after the query is initialized. Before Query::init(), Query::dim_label_queries_ is null. I did some more testing this morning and found that the only required change to support var length labels in python was result_buffer_elements. I updated my branch for TileDB-Py this morning and CI seems to agree.

All done making changes here though, will watch for review / CI failures.

@ihnorton ihnorton merged commit bc46385 into dev Jul 5, 2023
@ihnorton ihnorton deleted the smr/sc-29317/py-var-size-dim-labels branch July 5, 2023 13:42
github-actions bot pushed a commit that referenced this pull request Jul 5, 2023
Adds [result_buffer_elements_nullable](https://github.com/TileDB-Inc/TileDB-Py/blob/dev/tiledb/core.cc#L717) to support variable size dimension labels in TileDB-Py.

---
TYPE: FEATURE
DESC: Adds support for var length dimension label reads from Python
ihnorton pushed a commit that referenced this pull request Jul 5, 2023
Adds [result_buffer_elements_nullable](https://github.com/TileDB-Inc/TileDB-Py/blob/dev/tiledb/core.cc#L717) to support variable size dimension labels in TileDB-Py.

---
TYPE: FEATURE
DESC: Adds support for var length dimension label reads from Python

Co-authored-by: Shaun M Reed <shaunrd0@gmail.com>
* If the query has not been submitted, an empty map is returned.
*/
static std::unordered_map<std::string, std::pair<uint64_t, uint64_t>>
result_buffer_elements(const Query& query) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems like it's operating only on dimension labels? If so, the function name should reflect that. The comment should also be updated. Same below.

* Throws if there is no label data query on dim_idx.
*
* @param dim_idx Dimension index for label data query.
* @returns Vector of DimensionLabelQuery on dim_idx
Copy link
Contributor

Choose a reason for hiding this comment

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

NIT: double space.

ihnorton pushed a commit that referenced this pull request Jul 10, 2023
Rename `result_buffer_elements_labels` and update doxygen from review on #4142 

---
TYPE: IMPROVEMENT
DESC: Rename `result_buffer_elements_labels` and update doxygen from review on #4142
github-actions bot pushed a commit that referenced this pull request Jul 10, 2023
Rename `result_buffer_elements_labels` and update doxygen from review on #4142 

---
TYPE: IMPROVEMENT
DESC: Rename `result_buffer_elements_labels` and update doxygen from review on #4142
ihnorton pushed a commit that referenced this pull request Jul 10, 2023
Rename `result_buffer_elements_labels` and update doxygen from review on #4142 

---
TYPE: IMPROVEMENT
DESC: Rename `result_buffer_elements_labels` and update doxygen from review on #4142

Co-authored-by: Shaun M Reed <shaunrd0@gmail.com>
ihnorton pushed a commit to TileDB-Inc/TileDB-Py that referenced this pull request Jul 17, 2023
Adds support for variable length dimension label reads. This change requires TileDB-Inc/TileDB#4142 to add dimension label support for [result_buffer_elements_nullable](https://github.com/TileDB-Inc/TileDB-Py/blob/dev/tiledb/core.cc#L717) to size read offset buffers. The experimental dimension label variant of this function is defined as `QueryExperimental::result_buffer_elements_nullable_labels` and used in this PR to determine the number of offsets read by the var-size dimension label query.
ihnorton pushed a commit to TileDB-Inc/TileDB-Py that referenced this pull request Jul 17, 2023
Adds support for variable length dimension label reads. This change requires TileDB-Inc/TileDB#4142 to add dimension label support for [result_buffer_elements_nullable](https://github.com/TileDB-Inc/TileDB-Py/blob/dev/tiledb/core.cc#L717) to size read offset buffers. The experimental dimension label variant of this function is defined as `QueryExperimental::result_buffer_elements_nullable_labels` and used in this PR to determine the number of offsets read by the var-size dimension label query.
ihnorton pushed a commit to TileDB-Inc/TileDB-Py that referenced this pull request Jul 17, 2023
Adds support for variable length dimension label reads. This change requires TileDB-Inc/TileDB#4142 to add dimension label support for [result_buffer_elements_nullable](https://github.com/TileDB-Inc/TileDB-Py/blob/dev/tiledb/core.cc#L717) to size read offset buffers. The experimental dimension label variant of this function is defined as `QueryExperimental::result_buffer_elements_nullable_labels` and used in this PR to determine the number of offsets read by the var-size dimension label query.
ihnorton pushed a commit to TileDB-Inc/TileDB-Py that referenced this pull request Jul 17, 2023
Adds support for variable length dimension label reads. This change requires TileDB-Inc/TileDB#4142 to add dimension label support for [result_buffer_elements_nullable](https://github.com/TileDB-Inc/TileDB-Py/blob/dev/tiledb/core.cc#L717) to size read offset buffers. The experimental dimension label variant of this function is defined as `QueryExperimental::result_buffer_elements_nullable_labels` and used in this PR to determine the number of offsets read by the var-size dimension label query.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants