support lower_bound for iterator #2504
Conversation
zhangjinpeng87
changed the title
zhangjinpeng87
changed the title
| _ => unreachable!(), | ||
| }; | ||
|
|
||
| self.statistics_cache.add(self.scanner.get_statistics()); | ||
| let lower_bound = Some(Key::from_raw(&self.range.start).encoded().to_vec()); |
There was a problem hiding this comment.
It seems the same with L51-L65?
There was a problem hiding this comment.
L51-L65 depends if desc is set, but here is not.
|
|
||
| self.statistics_cache.add(self.scanner.get_statistics()); | ||
| let lower_bound = Some(Key::from_raw(&self.range.start).encoded().to_vec()); | ||
| let upper_bound = Some(Key::from_raw(&self.range.end).encoded().to_vec()); |
There was a problem hiding this comment.
What happen if it reverse seek upper_bound?
There was a problem hiding this comment.
Inside the RocksDB, SeekForPrev(upper_bound) will return the largest valid key, it is equal to SeekToLast(). I see we use SeekForPrev to implement reverse seek, so the behavior is the same.
| }; | ||
| iter_opt.set_lower_bound(lower_bound) | ||
| } | ||
|
|
||
| fn set_upper_bound(iter_opt: IterOption, region: &Region) -> IterOption { |
There was a problem hiding this comment.
Can it take a &mut IterOption instead?
| @@ -48,12 +48,18 @@ impl Scanner { | |||
| key_only: bool, | |||
| range: KeyRange, | |||
| ) -> Result<Scanner> { | |||
| let (scan_mode, seek_key, upper_bound) = if desc { | |||
| (ScanMode::Backward, range.get_end().to_vec(), None) | |||
| let (scan_mode, seek_key, lower_bound, upper_bound) = if desc { | |||
There was a problem hiding this comment.
Here why not set both lower_bound and upper_bound, like we do at line 92?
| _ => keys::enc_end_key(region), | ||
| Some(k) if !k.is_empty() => { | ||
| let k = keys::data_key(k); | ||
| if k < region_end_key { |
|
LGTM. |
|
LGTM |
| @@ -591,5 +616,22 @@ mod tests { | |||
| let mut expect = test_data.clone(); | |||
| expect.reverse(); | |||
| assert_eq!(res, expect); | |||
|
|
|||
| // test lower bound | |||
| let store = new_peer_storage(engine.clone(), raft_engine.clone(), ®ion); | |||
There was a problem hiding this comment.
Could we define an independent function to test lower bound?
|
/run-all-tests |
@BusyJay @AndreMouche PTAL