feat(storage): support reverse scan #12570
Conversation
Codecov Report
@@ Coverage Diff @@
## main #12570 +/- ##
==========================================
+ Coverage 69.32% 69.37% +0.04%
==========================================
Files 1470 1471 +1
Lines 241541 242154 +613
==========================================
+ Hits 167448 167991 +543
- Misses 74093 74163 +70
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 19 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
Signed-off-by: Little-Wallace <[email protected]>
Signed-off-by: Little-Wallace <[email protected]>
Signed-off-by: Little-Wallace <[email protected]>
Little-Wallace
force-pushed
the
reverse-iter
branch
from
6fee500 to
f4b5eca
Compare
Signed-off-by: Little-Wallace <[email protected]>
Signed-off-by: Little-Wallace <[email protected]>
Signed-off-by: Little-Wallace <[email protected]>
Signed-off-by: Little-Wallace <[email protected]>
Signed-off-by: Little-Wallace <[email protected]>
Signed-off-by: Little-Wallace <[email protected]>
The reverse-scan performs much worse than normal scan |
Signed-off-by: Little-Wallace <[email protected]>
For short scan, the are similar
|
Signed-off-by: Little-Wallace <[email protected]>
Signed-off-by: Little-Wallace <[email protected]>
Signed-off-by: Little-Wallace <[email protected]>
Signed-off-by: Little-Wallace <[email protected]>
Signed-off-by: Little-Wallace <[email protected]>
Signed-off-by: Little-Wallace <[email protected]>
Signed-off-by: Little-Wallace <[email protected]>
| if first_full_key > key { | ||
| // The semantic of `seek_fn` will ensure that `first_key` <= table_key of `key`. | ||
| // At the beginning we have checked that `self.table_id` <= table_id of `key`. | ||
| // Therefore, when `first_full_key` > `key`, the only possibility is that | ||
| // `first_key` == table_key of `key`, and `self.table_id` == table_id of `key`, | ||
| // the `self.epoch` > epoch of `key`. | ||
| assert_eq!(first_key, key.user_key.table_key); | ||
| } | ||
| self.iter = Some((RustIteratorOfBuilder::Seek(iter), first_key, first_value)); |
There was a problem hiding this comment.
Instead of checking first_full_key > key, I think we should check first_key == key && key.epoch_with_gap > self.epoch instead. When the condition is hit, we need to call iter.next.
Example:
Memtable in epoch2:
k1
k2
rev iter seek(FullKey(k2, epoch3)) should point to k1 instead of k2
because (k1, epoch2) < (k2, epoch3) < k2, epoch2)
Note that we cannot simply check fisrt_full_key < key because this won't guarantee that first_key == key under the semantic of reverse iter.
There was a problem hiding this comment.
We should add a test case for this as well.
Signed-off-by: Little-Wallace <[email protected]>
Signed-off-by: Little-Wallace <[email protected]>
| DirectionEnum::Backward => { | ||
| assert_lt!(next_key, first_key); | ||
| } |
There was a problem hiding this comment.
Although it fixes the corner case mentioned here, it introduces a bug for a case that work before:
Memtable in epoch2:
k1
rev iter seek(FullKey(k2, epoch2))
The code will panic in L466 because (k1, epoch2) < (k2, epoch2) but k1 != k2.
This is exactly why I said in my previous comment:
Note that we cannot simply check fisrt_full_key < key because this won't guarantee that first_key == key under the semantic of reverse iter.
If the UT passes with the current code, I think we lack a UT for this straight-forward case.
There was a problem hiding this comment.
I added some test for this case
Signed-off-by: Little-Wallace <[email protected]>
Signed-off-by: Little-Wallace <[email protected]>
Signed-off-by: Little-Wallace <[email protected]>
| match iter.next() { | ||
| Some((first_key, first_value)) => { | ||
| if first_key.eq(&key.user_key.table_key) && self.epoch < key.epoch_with_gap { | ||
| // The semantic of `seek_fn` will ensure that `first_key` >= table_key of `key`. |
There was a problem hiding this comment.
nits: should be first_key <= table_key of key in the comment.
| Some((first_key, first_value)) => { | ||
| if first_key.eq(&key.user_key.table_key) && self.epoch < key.epoch_with_gap { | ||
| // The semantic of `seek_fn` will ensure that `first_key` >= table_key of `key`. | ||
| // At the beginning we have checked that `self.table_id` >= table_id of `key`. |
There was a problem hiding this comment.
nits: should be self.table_id <= table_id of key in the comment
Signed-off-by: Little-Wallace <[email protected]>
Signed-off-by: Little-Wallace <[email protected]>
Signed-off-by: Little-Wallace <[email protected]>
Signed-off-by: Little-Wallace <[email protected]>
I hereby agree to the terms of the RisingWave Labs, Inc. Contributor License Agreement.
What's changed and what's your intention?
support reverse scan for range cache expand.
Maybe in some batch query just like
select a from table order by b desc limit 1, we can also use reverse-scan to optimize time complexChecklist
./risedev check(or alias,./risedev c)Documentation
Release note
If this PR includes changes that directly affect users or other significant modifications relevant to the community, kindly draft a release note to provide a concise summary of these changes. Please prioritize highlighting the impact these changes will have on users.