Skip to content

Commit

Permalink
Be consistent in key range overlap check (facebook#12315)
Browse files Browse the repository at this point in the history
Summary:
We should be consistent in how we check key range overlap in memtables and in sst files. While all the sst file key range overlap check compares the user key without timestamp, for example:
https://github.com/facebook/rocksdb/blob/377eee77f8da3f5d232cf014db0c4ca232352883/db/version_set.cc#L129-L130

This key range overlap check for memtable is comparing the whole user key. Currently it happen to achieve the same effect because this function is only called by `ExternalSstFileIngestionJob` and `DBImpl::CompactRange`, which takes a user key without timestamp as the range end, pad a max or min timestamp to it depending on whether the end is exclusive. So use `Compartor::Compare` here is working too, but we should update it to `Comparator::CompareWithoutTimestamp` to be consistent with all the other file key range overlapping check functions.

Pull Request resolved: facebook#12315

Test Plan: existing tests

Reviewed By: ltamasi

Differential Revision: D53273456

Pulled By: jowlyzhang

fbshipit-source-id: c094ae1f0c195d52542124c4fb03fdca14241e85
  • Loading branch information
jowlyzhang authored and facebook-github-bot committed Jan 31, 2024
1 parent acf77e1 commit d11584e
Showing 1 changed file with 2 additions and 1 deletion.
3 changes: 2 additions & 1 deletion db/column_family.cc
Original file line number Diff line number Diff line change
Expand Up @@ -1223,7 +1223,8 @@ Status ColumnFamilyData::RangesOverlapWithMemtables(

if (status.ok()) {
if (memtable_iter->Valid() &&
ucmp->Compare(seek_result.user_key, ranges[i].limit) <= 0) {
ucmp->CompareWithoutTimestamp(seek_result.user_key,
ranges[i].limit) <= 0) {
*overlap = true;
} else if (range_del_agg.IsRangeOverlapped(ranges[i].start,
ranges[i].limit)) {
Expand Down

0 comments on commit d11584e

Please sign in to comment.