Address reverse MultiScan follow-up comments

[jaykorean]

Summary:

• 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.

Differential Revision: D111127403

[meta-codesync]

@jaykorean has exported this pull request. If you are a Meta employee, you can view the originating Diff in D111127403.

[github-actions]

✅ clang-tidy: No findings on changed lines

Completed in 97.2s.

[github-actions]

🟡 Codex Code Review

Auto-triggered after CI passed — reviewing commit 72d7808

---

❌ Codex review failed before producing findings.

WARNING: proceeding, even though we could not create PATH aliases: Refusing to create helper binaries under temporary dir "/tmp" (codex_home: AbsolutePathBuf("/tmp/codex-home"))
error: the argument '--base <BRANCH>' cannot be used with '[PROMPT]'

Usage: codex exec review --commit <SHA> --base <BRANCH> --title <TITLE> --model <MODEL> --config <key=value> --dangerously-bypass-approvals-and-sandbox --output-last-message <FILE> [PROMPT]

For more information, try '--help'.

---

ℹ️ About this response

Generated by Codex CLI.
Review methodology: claude_md/code_review.md

Limitations:

• Codex may miss context from files not in the diff
• Large PRs may be truncated
• Always apply human judgment to AI suggestions

Commands:

• /codex-review [context] — Request a code review
• /codex-query <question> — Ask about the PR or codebase

[github-actions]

✅ Claude Code Review

Auto-triggered after CI passed — reviewing commit 72d7808

---

Summary

Clean follow-up PR that addresses review comments on reverse MultiScan. All five changes are well-motivated: restoring source compatibility for Scan::Reset, making HasMoreScanRanges() defensive instead of asserting, broadening forward test coverage, decoupling stress flags, and adding a release note. The changes are correct and low-risk.

High-severity findings (0):

No high-severity findings.

Full review (click to expand)

Findings

🟡 MEDIUM

M1. Test run_reverse condition does not filter on comparator -- block_based_table_reader_test.cc:1218

• Issue: The old parameterization explicitly restricted to BytewiseComparator() via .WithComparators({BytewiseComparator()}). The new run_reverse condition checks compression_type_ == kNoCompression && !use_direct_reads_ && kBinarySearch && threads == 1 && dict_bytes == 0 but does NOT check the comparator. However, the new parameterization's default builder uses comparators = {BytewiseComparator()} (line 2228), so in practice only BytewiseComparator is used. This is safe but fragile -- if someone later adds comparators to the default builder, reverse would silently run with untested comparators.
• Root cause: The default builder only includes BytewiseComparator, so the filter is implicitly satisfied. But the intent is not explicit in the run_reverse condition.
• Suggested fix: Consider adding a comment noting that the default builder only uses BytewiseComparator, or add an explicit comparator check to run_reverse for future-proofing. Low urgency since the current code is correct.

M2. Increased test matrix size may slow CI -- block_based_table_reader_test.cc:2399

• Issue: The old parameterization was tightly restricted (1 compression x 1 direct-read x 1 index-type x 1 thread x 1 dict x 1 comparator x Bool() fill_cache x IOUringFlags() async = small). The new parameterization uses defaults for most axes: GetSupportedCompressions() (typically 4-7) x Bool() direct-reads x 3 index-types x 2 thread-counts x 2 dict-byte-counts x 1 comparator x Bool() fill_cache x IOUringFlags() async. This is roughly a 50-170x increase in test cases depending on platform.
• Root cause: Intentional broadening of forward coverage is good, but the combinatorial explosion may be significant.
• Suggested fix: Verify CI execution time is acceptable. If too slow, consider restricting some axes like parallel_compression_thread_counts or compression_dict_byte_counts while keeping the index type coverage.

🟢 LOW / NIT

L1. HasMoreScanRanges() comment could note unreachability -- multi_scan_index_iterator.h:98

• Issue: The comment is good but could additionally note that this code path is currently unreachable in reverse because Prev() sets valid_ = false directly without calling SetExhausted(), so IsScanRangeExhausted() will never return true.
• Suggested fix: Minor doc improvement, not required.

L2. Release note is minimal -- unreleased_history/public_api_changes/reverse_multiscan.md

• Issue: Clear and concise. No action needed.

Cross-Component Analysis

Context	Does code execute?	Assumptions hold?	Action needed?
Forward MultiScan	YES	YES - HasMoreScanRanges() works as before	None
Reverse MultiScan	YES (defensive path)	YES - returns false, caller falls through to return	None
WritePreparedTxnDB	N/A - MultiScan is orthogonal	N/A	None
ReadOnly DB	YES	YES	None
User-defined timestamps	YES	YES	None

Assumption stress-test:

Claim: "HasMoreScanRanges() is unreachable in reverse"

• IsScanRangeExhausted() must return true for HasMoreScanRanges() to be called (block_based_table_iterator.cc:692-694).
• scan_range_exhausted_ is set to true ONLY by SetExhausted().
• SetExhausted() is called from: Seek() (forward-only, line 119 guards), Next() (line 340), and SeekToBlock() (forward-only). None called in reverse.
• Prev() does NOT call SetExhausted() -- sets valid_ = false directly (line 469).
• SeekForPrev() resets scan_range_exhausted_ = false (line 386).
• Verified: claim holds. The defensive check is a safety net for future changes.

Claim: "Decoupling multiscan_reverse from test_backward_scan is safe"

• FLAGS_test_backward_scan gates backward scan in TestIterate (no_batched_ops_stress.cc). It does NOT gate any infrastructure used by TestMultiScan.
• TestMultiScan sets iterate_lower_bound independently when reverse (line 2261-2263).
• Verified: claim holds.

Positive Observations

• Assert-to-defensive-check in HasMoreScanRanges() is the right pattern -- asserts are no-ops in release builds.
• Default parameter on Scan::Reset matches the constructor's signature -- good API consistency.
• Test restructuring wisely broadens forward coverage while keeping reverse on the cheapest representative config.
• Stress tool decoupling is correct -- reverse MultiScan doesn't share infrastructure with backward scan.

---

ℹ️ About this response

Generated by Claude Code.
Review methodology: claude_md/code_review.md

Limitations:

• Claude may miss context from files not in the diff
• Large PRs may be truncated
• Always apply human judgment to AI suggestions

Commands:

• /claude-review [context] — Request a code review
• /claude-query <question> — Ask about the PR or codebase

[meta-codesync]

This pull request has been merged in f32f747.