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

Clearer names for dense storage implementations #3712

Merged
merged 1 commit into from
Feb 28, 2024

Conversation

agourlay
Copy link
Member

This PR is a bunch of mechanical renaming for dense storage related types and files.

The motivation is that we want to add soon new types of storages for multi-vector points, so it is better to makes things explicit.

Copy link
Member

@timvisee timvisee left a comment

Choose a reason for hiding this comment

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

Just quickly skimmed it: are we sure this doesn't break compatibility, in terms of loading existing storage from disk?

@agourlay
Copy link
Member Author

agourlay commented Feb 28, 2024

Just quickly skimmed it: are we sure this doesn't break compatibility, in terms of loading existing storage from disk?

This part is tested by our storage compatibility tests.
Only types are changed, no field names, I do not foresee any compatibility issues.

Do you maybe a specific scenario in mind?

@timvisee
Copy link
Member

This part is tested by our storage compatibility tests. Only types are changes, no field names, I do not foresee any compatibility issues.

You're right. Brilliant! Thanks for confirming.

Do you maybe a specific scenario in mind?

Nope! Not anything other than what you've already covered.

@agourlay
Copy link
Member Author

agourlay commented Feb 28, 2024

I just double checked, it seems the compatibility tests are not covering the memap & appendable memap configurations.
Will check how to add it quickly.

@generall
Copy link
Member

I think it is safe to merge, cause those structs do not implement Deserialize anyway

@agourlay
Copy link
Member Author

@generall excellent observation 👍

Then feel free to approve, I will work on improving the compat. tests independently from this PR.

@agourlay agourlay merged commit d5c089a into dev Feb 28, 2024
17 checks passed
@agourlay agourlay deleted the dense-storage-renamings branch February 28, 2024 11:30
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.

None yet

3 participants