From f32f7471251695c8001f7b191cb2dec87aa52100 Mon Sep 17 00:00:00 2001 From: Jay Huh Date: Wed, 8 Jul 2026 20:56:36 -0700 Subject: [PATCH] Address reverse MultiScan follow-up comments (#14922) Summary: Pull Request resolved: https://github.com/facebook/rocksdb/pull/14922 - Preserve source compatibility for `Scan::Reset` by restoring the default `reverse` argument. - Make `HasMoreScanRanges()` defensive for reverse prepared scans instead of asserting. - Restore broader forward coverage in `MultiScanAsyncIOTest` while keeping reverse coverage on the representative async IO matrix. - Let `--multiscan_reverse` drive reverse MultiScan stress coverage independently of `--test_backward_scan`. - Add the reverse MultiScan public API release note. Reviewed By: xingbowang Differential Revision: D111127403 fbshipit-source-id: 1a17d0a36aaf66e8cbc51f445d0f85e145fd653c --- db_stress_tool/db_stress_gflags.cc | 3 ++- db_stress_tool/db_stress_test_base.cc | 3 +-- include/rocksdb/multi_scan.h | 2 +- .../block_based_table_reader_test.cc | 18 ++++++++++++------ table/block_based/multi_scan_index_iterator.h | 6 +++++- .../public_api_changes/reverse_multiscan.md | 1 + 6 files changed, 22 insertions(+), 11 deletions(-) create mode 100644 unreleased_history/public_api_changes/reverse_multiscan.md diff --git a/db_stress_tool/db_stress_gflags.cc b/db_stress_tool/db_stress_gflags.cc index b975263ead..bd2e030f0b 100644 --- a/db_stress_tool/db_stress_gflags.cc +++ b/db_stress_tool/db_stress_gflags.cc @@ -1725,7 +1725,8 @@ DEFINE_bool(use_multiscan, false, DEFINE_bool(multiscan_reverse, false, "If set with use_multiscan, scan each MultiScan range in reverse " - "using SeekForPrev and Prev."); + "using SeekForPrev and Prev. This does not require " + "test_backward_scan."); DEFINE_bool(multiscan_use_async_io, false, "If set, enable async_io for MultiScan operations."); diff --git a/db_stress_tool/db_stress_test_base.cc b/db_stress_tool/db_stress_test_base.cc index 60c84b21ed..95a3bad832 100644 --- a/db_stress_tool/db_stress_test_base.cc +++ b/db_stress_tool/db_stress_test_base.cc @@ -2237,8 +2237,7 @@ Status StressTest::TestMultiScan(ThreadState* thread, std::vector end_key_strs; // TODO support reverse BytewiseComparator in the stress test MultiScanArgs scan_opts(options_.comparator); - const bool reverse_multiscan = - FLAGS_multiscan_reverse && FLAGS_test_backward_scan; + const bool reverse_multiscan = FLAGS_multiscan_reverse; scan_opts.reverse = reverse_multiscan; scan_opts.use_async_io = FLAGS_multiscan_use_async_io && diff --git a/include/rocksdb/multi_scan.h b/include/rocksdb/multi_scan.h index b544daa17c..e785dabf19 100644 --- a/include/rocksdb/multi_scan.h +++ b/include/rocksdb/multi_scan.h @@ -74,7 +74,7 @@ class Scan { explicit Scan(Iterator* db_iter, bool reverse = false) : db_iter_(db_iter), reverse_(reverse) {} - void Reset(Iterator* db_iter, bool reverse) { + void Reset(Iterator* db_iter, bool reverse = false) { db_iter_ = db_iter; reverse_ = reverse; } diff --git a/table/block_based/block_based_table_reader_test.cc b/table/block_based/block_based_table_reader_test.cc index 9d92057896..e3b9e28e83 100644 --- a/table/block_based/block_based_table_reader_test.cc +++ b/table/block_based/block_based_table_reader_test.cc @@ -1215,8 +1215,16 @@ TEST_P(BlockBasedTableReaderMultiScanAsyncIOTest, MultiScanPrepare) { } ASSERT_OK(iter->status()); }; + const bool run_reverse = + compression_type_ == kNoCompression && !use_direct_reads_ && + param.index_type == BlockBasedTableOptions::IndexType::kBinarySearch && + param.compression_parallel_threads == 1 && + param.compression_dict_bytes == 0; for (const bool reverse : {false, true}) { + if (reverse && !run_reverse) { + continue; + } SCOPED_TRACE(reverse ? "reverse" : "forward"); options_.statistics = CreateDBStatistics(); std::string table_name = "BlockBasedTableReaderTest_NewIterator" + @@ -2401,12 +2409,10 @@ INSTANTIATE_TEST_CASE_P( BlockBasedTableReaderMultiScanAsyncIOTest, ::testing::ValuesIn( BlockBasedTableReaderTestParamBuilder() - .WithCompressionTypes({kNoCompression}) - .WithUseDirectReadFlags({false}) - .WithIndexTypes({BlockBasedTableOptions::IndexType::kBinarySearch}) - .WithParallelCompressionThreadCounts({1}) - .WithCompressionDictByteCounts({0}) - .WithComparators({BytewiseComparator()}) + .WithIndexTypes( + {BlockBasedTableOptions::IndexType::kBinarySearch, + BlockBasedTableOptions::IndexType::kHashSearch, + BlockBasedTableOptions::IndexType::kTwoLevelIndexSearch}) .WithFillCacheFlags(Bool()) .WithUseAsyncIoFlags(IOUringFlags()) .build())); diff --git a/table/block_based/multi_scan_index_iterator.h b/table/block_based/multi_scan_index_iterator.h index 845dd305ad..b76298b007 100644 --- a/table/block_based/multi_scan_index_iterator.h +++ b/table/block_based/multi_scan_index_iterator.h @@ -95,7 +95,11 @@ class MultiScanIndexIterator : public InternalIteratorBase { // Returns true if there are more scan ranges after the current one. bool HasMoreScanRanges() const { - assert(!scan_opts_->reverse); + // Reverse MultiScan advances ranges through SeekForPrev(), not this + // forward-only range transition path. + if (scan_opts_->reverse) { + return false; + } return next_scan_idx_ < block_index_ranges_per_scan_.size(); } diff --git a/unreleased_history/public_api_changes/reverse_multiscan.md b/unreleased_history/public_api_changes/reverse_multiscan.md new file mode 100644 index 0000000000..b12fc33e49 --- /dev/null +++ b/unreleased_history/public_api_changes/reverse_multiscan.md @@ -0,0 +1 @@ +Added `MultiScanArgs::reverse` to support reverse MultiScan reads over bounded scan ranges.