Skip to content

[Java] Using functions for mapping#1007

Merged
rapids-bot[bot] merged 17 commits intorapidsai:branch-25.08from
ldematte:java/mapping-as-function
Jul 6, 2025
Merged

[Java] Using functions for mapping#1007
rapids-bot[bot] merged 17 commits intorapidsai:branch-25.08from
ldematte:java/mapping-as-function

Conversation

@ldematte
Copy link
Contributor

@ldematte ldematte commented Jun 11, 2025

This PR replaces "array" based ID mappings with a more free-form mapping based on functions.
This should be more efficient both when the number of IDs is huge (you don't need to actually allocate big list for all IDs to map) and in general (as it won't need to perform boxing/unboxing of integers).
It should also be more flexible; as an example, I have introduced a simple helper to wrap a list, so that the previous approach can still be used. The same can be done for maps etc.

Relates to #699

@copy-pr-bot
Copy link

copy-pr-bot bot commented Jun 11, 2025

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

@cjnolet cjnolet added improvement Improves an existing functionality non-breaking Introduces a non-breaking change labels Jun 11, 2025
@cjnolet
Copy link
Member

cjnolet commented Jun 11, 2025

/ok to test b3856a1

@cjnolet
Copy link
Member

cjnolet commented Jun 11, 2025

/ok to test 6a46323

@ldematte ldematte changed the title [WIP][Java] Using functions for mapping [Review][Java] Using functions for mapping Jun 16, 2025
@chatman
Copy link
Contributor

chatman commented Jun 25, 2025

@ldematte One of my wishlist items was to compute this mapping on the GPU (when possible) to cut down on time to return the results from the various algorithms. Maybe something like a transform operation (https://nvidia.github.io/cccl/thrust/api/function_group__transformations_1ga62a6227031c9c811c5b497b347398a8a.html), or something similar, if available in Raft/RAPIDS.

Towards that, maybe we could bring back (in addition to this function) a int[] mapping (not the currently available List<Integer> though) at a later point in time, and keep this function based mapping for the usecases where CPU is being used?

Alternatively, the same function can be used to construct an int[] for the mapping on the GPU. However, constructing it on every request could be as bad as doing the entire mapping application on the CPU itself, so maybe some strategy involving caching the constructed mapping int[] could be explored.

Copy link
Contributor

@chatman chatman left a comment

Choose a reason for hiding this comment

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

Looks good, thanks!

@mythrocks
Copy link
Contributor

/ok to test 76264ea

@mythrocks
Copy link
Contributor

/ok to test 726c5dc

@mythrocks
Copy link
Contributor

This PR seems to be labeled as Non-breaking. But the public interfaces for BruteForceQuery, CagraQuery, etc. are changing.

Any objection to labeling this as Breaking?

@mythrocks mythrocks added breaking Introduces a breaking change and removed non-breaking Introduces a non-breaking change labels Jun 27, 2025
Copy link
Contributor

@mythrocks mythrocks left a 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, but this looks good to go. Thank you for this change, @ldematte!

@ChrisHegarty
Copy link
Contributor

This PR seems to be labeled as Non-breaking. But the public interfaces for BruteForceQuery, CagraQuery, etc. are changing.

Any objection to labeling this as Breaking?

The change in the java API signature is clearly an incompatible change, no question. However, the API is experimental, and as such is still evolving before being made final. So it't not a breaking change. We're likely going to have several of these kinds of change in the next release.

@mythrocks
Copy link
Contributor

/ok to test eb8a15c

@ldematte ldematte changed the title [Review][Java] Using functions for mapping [Java] Using functions for mapping Jun 30, 2025
@mythrocks
Copy link
Contributor

I've merged #1049. I'm afraid this PR will need a rebase.

ldematte added 2 commits July 2, 2025 12:41
…g-as-function

# Conflicts:
#	java/cuvs-java/src/main/java/com/nvidia/cuvs/BruteForceQuery.java
#	java/cuvs-java/src/main/java/com/nvidia/cuvs/CagraQuery.java
#	java/cuvs-java/src/main/java/com/nvidia/cuvs/HnswQuery.java
#	java/cuvs-java/src/main/java/com/nvidia/cuvs/SearchResults.java
#	java/cuvs-java/src/main/java22/com/nvidia/cuvs/internal/BruteForceSearchResults.java
#	java/cuvs-java/src/main/java22/com/nvidia/cuvs/internal/CagraIndexImpl.java
#	java/cuvs-java/src/main/java22/com/nvidia/cuvs/internal/CagraSearchResults.java
#	java/cuvs-java/src/main/java22/com/nvidia/cuvs/internal/HnswSearchResults.java
#	java/cuvs-java/src/main/java22/com/nvidia/cuvs/internal/common/SearchResultsImpl.java
#	java/cuvs-java/src/test/java/com/nvidia/cuvs/BruteForceAndSearchIT.java
#	java/cuvs-java/src/test/java/com/nvidia/cuvs/CagraBuildAndSearchIT.java
#	java/cuvs-java/src/test/java/com/nvidia/cuvs/HnswBuildAndSearchIT.java
@ldematte
Copy link
Contributor Author

ldematte commented Jul 2, 2025

I've merged #1049. I'm afraid this PR will need a rebase.

All of the pending PRs will need that :)
Done!

@mythrocks
Copy link
Contributor

/ok to test f4b40a8

…g-as-function

# Conflicts:
#	java/cuvs-java/src/test/java/com/nvidia/cuvs/CagraBuildAndSearchIT.java
@mythrocks
Copy link
Contributor

/ok to test 4db9f63

@mythrocks
Copy link
Contributor

/ok to test 142050a

@mythrocks
Copy link
Contributor

/merge

@rapids-bot rapids-bot bot merged commit 7462cbd into rapidsai:branch-25.08 Jul 6, 2025
96 of 102 checks passed
@mythrocks
Copy link
Contributor

Thank you for this change, @ldematte. I've merged this change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

breaking Introduces a breaking change improvement Improves an existing functionality

Development

Successfully merging this pull request may close these issues.

5 participants