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

fix: secondary index range #214

Merged
merged 1 commit into from
Aug 24, 2024
Merged

fix: secondary index range #214

merged 1 commit into from
Aug 24, 2024

Conversation

vincent-herlemont
Copy link
Owner

@vincent-herlemont vincent-herlemont commented Aug 18, 2024

The bug is related to the formation of secondary keys, which were created from the primary keys in cases where the secondary key is not unique.

The problems encountered in this issue #208 show that this causes a problem when trying to scan with an inclusive range. This is because the secondary key concatenated with the primary key is always greater than the inclusive range (and falls outside the interval). To address this problem, I attempted to implement a byte delimiter by setting one byte to 0 when creating the key, and adding a byte set to 255 when creating the inclusive range. Since 255 is always greater than 0, the inclusive range will have the correct values.

@vincent-herlemont vincent-herlemont merged commit 488a620 into main Aug 24, 2024
9 checks passed
@vincent-herlemont vincent-herlemont deleted the fix_secondary_index_2 branch August 24, 2024 09:24
@vincent-herlemont
Copy link
Owner Author

vincent-herlemont commented Aug 24, 2024

The solution described in this comment was not good, because in all cases, a bug could appear by shifting the bytes—see the test: test_low_level_scan_range. So, it was not a good solution.

The adopted solution is to use MultimapTable to manage secondary indexes, which avoids concatenating keys and provides a much more stable solution.

An automatic migration was also coded so that users do not have to do anything and no data is lost.

Copy link

@C4Phoenix C4Phoenix left a comment

Choose a reason for hiding this comment

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

I don't have much time to contribute atm but I appreciate the work being put into the fix. Reassures my choice too use this as a db for my project. I had a question at a certain line that could be a logical slip up or me being ignorant about the context.

let start = start.to_key();
let mut end = end.to_key();
// Add 255 to the end key to include the last key
end.extend(&Key::new(vec![255]));

Choose a reason for hiding this comment

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

There is no difference between this and the one above? Should this be start.extend ?

Copy link
Owner Author

Choose a reason for hiding this comment

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

Well spotted, you're absolutely right to find this code strange; it's one of the things that needs refactoring and will be removed.

By the way, just a little background on this legacy code: in the case of Rust ranges and the workarounds put in place here, it’s all about the inclusive end range because we use start..=end.

Copy link
Owner Author

@vincent-herlemont vincent-herlemont Sep 1, 2024

Choose a reason for hiding this comment

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

@C4Phoenix Here is the cleaned code a0b2e8c, in fact, part of the code might not be supported due to the syntax of ranges in Rust.

@vincent-herlemont vincent-herlemont added this to the 0.8.0 milestone Sep 7, 2024
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.

2 participants