-
Notifications
You must be signed in to change notification settings - Fork 143
[Java] Support for tiered index #1028
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
[Java] Support for tiered index #1028
Conversation
|
Thank you for working on this. I'm taking a look at this now. |
|
@mythrocks Resolved the conflicts. |
mythrocks
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A couple of comments. Still reviewing.
java/cuvs-java/src/main/java22/com/nvidia/cuvs/internal/TieredIndexImpl.java
Outdated
Show resolved
Hide resolved
java/cuvs-java/src/main/java22/com/nvidia/cuvs/internal/TieredIndexImpl.java
Outdated
Show resolved
Hide resolved
java/cuvs-java/src/main/java22/com/nvidia/cuvs/internal/TieredIndexImpl.java
Outdated
Show resolved
Hide resolved
java/cuvs-java/src/main/java22/com/nvidia/cuvs/internal/TieredIndexImpl.java
Outdated
Show resolved
Hide resolved
java/cuvs-java/src/main/java22/com/nvidia/cuvs/internal/TieredIndexImpl.java
Show resolved
Hide resolved
java/cuvs-java/src/main/java22/com/nvidia/cuvs/internal/TieredIndexImpl.java
Outdated
Show resolved
Hide resolved
mythrocks
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need to fix how the tiered index MemorySegment is used to build the tiered index.
-extracting index pointer and using it
|
I'm doing another pass on this PR. It looks good so far. I hope to approve this shortly. |
java/cuvs-java/src/test/java/com/nvidia/cuvs/TieredIndexIT.java
Outdated
Show resolved
Hide resolved
java/cuvs-java/src/test/java/com/nvidia/cuvs/TieredIndexIT.java
Outdated
Show resolved
Hide resolved
java/cuvs-java/src/test/java/com/nvidia/cuvs/TieredIndexIT.java
Outdated
Show resolved
Hide resolved
java/cuvs-java/src/main/java22/com/nvidia/cuvs/internal/TieredIndexImpl.java
Outdated
Show resolved
Hide resolved
mythrocks
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A couple of nitpicks more, but it looks good otherwise.
It will need a rebase again.
-Cleaned up the test and added some asserts
…puneet/expose-tiered-index
|
/ok to test 9918bc3 |
|
Linking cjnolet/nv_elastic#11 |
|
Apologies, I haven't gotten a moment to review the latest change. I'll go over it shortly. |
|
/ok to test 553d6a0 |
mythrocks
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alright, this looks a whole lot better. Thank you for your patience, and for incorporating the review changes.
|
/merge |
28d2de6
into
rapidsai:branch-25.08
|
I've merged this change. Thank you for this one, @punAhuja. |
TieredIndex exposed in the Java layer.
Code is written and tested for CAGRA algorithm only.
Serialization, Deserialization and merge is still not supported in the C layer.
Fixes: #1014