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

Added getNumElements + getVector #64

Merged
merged 4 commits into from
Apr 9, 2024
Merged

Conversation

samek
Copy link
Contributor

@samek samek commented Mar 27, 2024

As reported in issues #3 inserting string vector with the same name doesn't update it but it adds another one.

I've modified inserting so that it checks for the key before adding it to the index.

If this change is not wanted I could maybe create upsertItem upsertItems which would perform this way.

Added getNumElements
Added getVector
Added getNumElements
Added getVector
Added getNumElements
Added getVector
Copy link
Contributor

@markkohdev markkohdev left a comment

Choose a reason for hiding this comment

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

Hey @samek thanks for another contribution! You're now a star supporter of this OSS project :) haha

Exposing getNumElements and getVector are 👍

In regards to upserting, after chatting with @dylanrb123 about this change, we don't think that this approach is how we would like to see this implemented. The implementation you have here results in traversal of the entire names list upon every addition in the worst case.

In order to support upserting functionality I think we would need a slightly more involved solution. I'll be implementing StringIndex in the C++ core soon enough and will implement this functionality there (and deprecate this class).

I would recommend removing the upsert functionality for the moment and reducing the scope of this PR. However, if you are very motivated to enable this functionality before StringIndex support in voyager core comes along we can discuss how we might support this using a naive implementation in Java (effectively utilizing parallel HashMaps instead of an ArrayList for managing the strings)

@samek samek changed the title StringIndex upsert + getNumElements + getVector Added getNumElements + getVector Mar 30, 2024
@samek
Copy link
Contributor Author

samek commented Mar 30, 2024

Ok, I've removed the functionality.
But I'm keen on making this until we have whole string index in C++ core.
What did you have in mind should we use ConcurrentHashMap ?

@samek samek requested a review from markkohdev March 30, 2024 09:52
Copy link
Contributor

@markkohdev markkohdev left a comment

Choose a reason for hiding this comment

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

👍 @samek I'm gonna merge this PR and we can discuss the upsert functionality in #3

@markkohdev markkohdev merged commit 26dd460 into spotify:main Apr 9, 2024
53 checks passed
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