git-hulk commented on PR #1367: URL: https://github.com/apache/incubator-kvrocks/pull/1367#issuecomment-1497049958
We didn't add the version into the prefix, since it may cause bugs in some test cases like [hash_test.cc#L203](https://github.com/apache/incubator-kvrocks/blob/unstable/tests/cppunit/types/hash_test.cc#L203): ```C++ /Users/hulk/code/cxx/incubator-kvrocks/tests/cppunit/types/hash_test.cc:315: Failure Expected equality of these values: 4 + 26 Which is: 30 result.size() Which is: 0 ``` And found it was caused by the upper bound in `SeekToLast` after diving into the reason. The root cause is the `Seek` API will also use the bloom filter to probe the existence of the prefix when enabling the prefix extractor, then it will be false if we added the version into the prefix when seeking with the upper bound(which is the version + 1). ```C++ void DBIter::SeekToLast() { if (iterate_upper_bound_ != nullptr) { // Seek to last key strictly less than ReadOptions.iterate_upper_bound. SeekForPrev(*iterate_upper_bound_); const bool is_ikey = (timestamp_size_ > 0 && timestamp_lb_ != nullptr); Slice k = Valid() ? key() : Slice(); if (is_ikey && Valid()) { k.remove_suffix(kNumInternalBytes + timestamp_size_); } ``` We can work around this issue by removing the version like the current implementation, but add the version would help a lot when there have many delete tombstones. So another way is to avoid use the upper bound when using `SeekToLast`. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
