Fix the bug with using the user comparator to compare prefix. #12862
Conversation
wqshr12345
force-pushed
the
bugfix-comparator-error-use
branch
from
94c63be
to
d1453e3
Compare
| @@ -1280,11 +1280,9 @@ void LevelIterator::Seek(const Slice& target) { | |||
| ts_sz); | |||
| if (prefix_extractor_->InDomain(target_user_key_without_ts) && | |||
| (!prefix_extractor_->InDomain(next_file_first_user_key_without_ts) || | |||
| user_comparator_.CompareWithoutTimestamp( | |||
There was a problem hiding this comment.
Thanks for the fix. I think the original code should work fine too. A prefix is a continuous group, when determine if two groups are actually the same group, it should be ok to use the user comparator's logic.
This is checking the result to be equal or not, which is the same as what BytewiseComparator does and what ReverseBytewiseComparator does:
Line 37 in b800b5e
Line 160 in b800b5e
I'm checking this since this seems to be a very hot code path, if something is wrong with it, it would be great to include a unit test to show case what bug this fixes.
There was a problem hiding this comment.
Hello, the original code performs well in the two comparators you provided. But what I want to express is that the comparator is not responsible for comparing the values of the prefix (at least there is no such requirement in its comments). If it is a user-defined comparator, it may not support comparing prefixes, resulting in errors. Actually, our custom comparator does not support comparing prefixes, so an error occurred here.
There was a problem hiding this comment.
Just want to understand the issue better, would you mind giving an example of what the custom comparator is doing and what kind of error is encountered for comparing which prefixes etc?
|
@jowlyzhang has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
|
@jowlyzhang merged this pull request in 755010f. |
Fixes #12855