Use int64_t for getters in the cagra/ivf_flat c-api#1272
Merged
rapids-bot[bot] merged 5 commits intorapidsai:branch-25.10from Sep 17, 2025
Merged
Use int64_t for getters in the cagra/ivf_flat c-api#1272rapids-bot[bot] merged 5 commits intorapidsai:branch-25.10from
rapids-bot[bot] merged 5 commits intorapidsai:branch-25.10from
Conversation
Java doesn't support unsigned types well, and this is leading to us only supporting 2**31 items in the cagra index - instead of 2**32. Fix by changing the getters in the c-api to return int64_t values for the size/dim etc, instead of uint32_t. We were also not consistent on returning error codes in ivf_flat/ivf_pq for these getters - with cagra returning an error code, and the ivf* methods just returning the value directly. Change to always return an error code to be consistent. (Note that rapidsai#1123 is also making this change for ivf-pq - and I'm skipping ivf-pq to avoid merge conflicts).
mythrocks
reviewed
Aug 21, 2025
| extern "C" cuvsError_t cuvsCagraIndexGetDims(cuvsCagraIndex_t index, int64_t* dim) | ||
| { | ||
| return cuvs::core::translate_exceptions([=] { | ||
| auto index_ptr = reinterpret_cast<cuvs::neighbors::cagra::index<float, uint32_t>*>(index->addr); |
Contributor
There was a problem hiding this comment.
Tangential question: How come index->addr is a uintprt_t and not a void*?
(Functionally, it's likely equivalent, except for the ability to use static_cast instead of reinterpret_cast.)
mythrocks
approved these changes
Aug 21, 2025
Contributor
mythrocks
left a comment
mythrocks
left a comment
There was a problem hiding this comment.
C++ and Java changes: 👍
Python changes: Look good to my eye, but should probably be reviewed by an expert.
tarang-jain
reviewed
Aug 22, 2025
| if (index->dtype.code == kDLFloat && index->dtype.bits == 32) { | ||
| auto index_ptr = | ||
| reinterpret_cast<cuvs::neighbors::ivf_flat::index<float, int64_t>*>(index->addr); | ||
| *n_lists = index_ptr->n_lists(); |
Contributor
There was a problem hiding this comment.
Since this is a non-narrowing conversion (uint32_t -> int64_t), an explicit static_cast is not required, but explicit casting can still help with readability and safety.
Same comment for all the other getters (such as cuvsIvfFlatIndexGetDim) that are doing an implicit casting.
mythrocks
approved these changes
Sep 10, 2025
cjnolet
approved these changes
Sep 16, 2025
Contributor
Author
|
/merge |
github-project-automation
Bot
moved this from Todo
to Done
in Unstructured Data Processing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Java doesn't support unsigned types well, and this is leading to us only supporting 2^31 items in the cagra index - instead of 2^32.
Fix by changing the getters in the c-api to return int64_t values for the size/dim etc, instead of uint32_t.
We were also not consistent on returning error codes in ivf_flat/ivf_pq for these getters - with cagra returning an error code, and the ivf* methods just returning the value directly. Change to always return an error code to be consistent. (Note that #1123 is also making this change for ivf-pq - and I'm skipping ivf-pq to avoid merge conflicts).