feat(mem-wal): adapt manifest scan concurrency

[u70b3]

Summary

• replace the user-tuned manifest scan batch size with internal exponential growth from 2 to 64
• recover the latest manifest when the version hint is stale, including legacy configurations that supplied a zero batch size
• retain deprecated Rust, Python, and Java configuration entry points as ignored compatibility shims
• stop persisting the obsolete writer default

Fixes #7928.

Test Plan

• cargo test -p lance dataset::mem_wal::manifest::tests --lib -- --nocapture
• cargo check -p lance --tests --benches
• cargo clippy --all --tests --benches -- -D warnings
• cargo test -p lance --doc ShardManifestStore
• cargo clippy --tests --manifest-path java/lance-jni/Cargo.toml -- -D warnings
• cd java && ./mvnw spotless:check
• cd java && ./mvnw test
• cd python && uv run make build
• cd python && uv run make lint

Relationship to #8640

#8640 eliminated hot-path manifest scans by serving position from the writing store. This PR optimizes what remains: the cold-scan paths (stale-hint catch-up and claim), where the adaptive 2→64 batch growth cuts catch-up round trips from O(K/2) to O(log K)+O(K/64). Precise-hint reads issue the same requests as before.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: QUIET

Plan: Pro Plus

Run ID: 9f1a549d-0ff8-4aa3-9c36-0c770001f247

📥 Commits

Reviewing files that changed from the base of the PR and between d4391fc and 00f53d5.

📒 Files selected for processing (2)

• rust/lance/src/dataset/mem_wal/manifest.rs
• rust/lance/src/dataset/mem_wal/write.rs

---

📝 Walkthrough

Walkthrough

The change rejects zero manifest_scan_batch_size values during manifest reads and shard writer opening. Manifest reads validate before using version hints, and tests cover stale hints and writer-open failures with contextual invalid-input errors.

Changes

Manifest scan validation

Layer / File(s)	Summary
Manifest reader validation rust/lance/src/dataset/mem_wal/manifest.rs	Adds shared validation before manifest hint lookup or version scanning, with coverage for zero batch sizes and stale hints.
Shard writer open validation rust/lance/src/dataset/mem_wal/write.rs	Documents and enforces the positive batch-size requirement when opening a shard writer, with invalid-input test coverage.

Estimated code review effort: 2 (Simple) | ~10 minutes

Suggested reviewers: hamersaw, jackye1995

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly matches the main change: rejecting zero manifest scan batch size.
Description check	✅ Passed	The description is directly related to the PR and describes the same validation and regression work.
Linked Issues check	✅ Passed	The changes reject zero at writer open and in manifest discovery, with regression tests for stale-hint handling.
Out of Scope Changes check	✅ Passed	The diff stays within the bug fix and test coverage scope without introducing unrelated changes.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

Actionable comments posted: 1

Note

Quiet mode is enabled, so only the most important comments were posted inline. Other review comments are grouped below.

🟡 Other comments (2)

rust/lance/src/dataset/mem_wal/write.rs-6130-6149 (1)

6130-6149: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win

Assert that rejected open leaves no manifest or epoch side effects.

The PR objective requires validation before claiming the shard, but this test only checks the returned error. A future regression that claims an epoch before validating would still pass. Reuse the store after the failed open and assert that read_latest() returns None.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@rust/lance/src/dataset/mem_wal/write.rs` around lines 6130 - 6149, Extend
test_open_rejects_zero_manifest_scan_batch_size to reuse the store after the
rejected ShardWriter::open and assert that read_latest() returns None, verifying
validation leaves no manifest or epoch side effects while preserving the
existing error assertions.

rust/lance/src/dataset/mem_wal/manifest.rs-697-699 (1)

697-699: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Assert that the stale hint was actually persisted.

write_version_hint is best-effort and returns no error, so this call can fail silently; the test would still pass because zero is rejected regardless of the hint. Verify the fixture before constructing the zero-batch reader.

Suggested test assertion

 manifest_store.write_version_hint(1).await;
+assert_eq!(manifest_store.read_version_hint().await, Some(1));

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@rust/lance/src/dataset/mem_wal/manifest.rs` around lines 697 - 699, Update
the test around ShardManifestStore::write_version_hint to verify that version
hint 1 was persisted before constructing the zero-batch reader. Read or inspect
the manifest store’s persisted hint using the existing fixture/assertion
mechanism, and assert it equals 1 so the test cannot pass solely because the
zero-batch reader rejects zero.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@rust/lance/src/dataset/mem_wal/write.rs`:
- Around line 134-139: Complete the documentation for the public field
manifest_scan_batch_size and its builder method with a synchronized example
using ShardWriterConfig::new(...) followed by
with_manifest_scan_batch_size(...), keeping the example aligned with the actual
signatures. Add cross-links between the field and method, and preserve the
documented greater-than-zero constraint.

---

Other comments:
In `@rust/lance/src/dataset/mem_wal/manifest.rs`:
- Around line 697-699: Update the test around
ShardManifestStore::write_version_hint to verify that version hint 1 was
persisted before constructing the zero-batch reader. Read or inspect the
manifest store’s persisted hint using the existing fixture/assertion mechanism,
and assert it equals 1 so the test cannot pass solely because the zero-batch
reader rejects zero.

In `@rust/lance/src/dataset/mem_wal/write.rs`:
- Around line 6130-6149: Extend test_open_rejects_zero_manifest_scan_batch_size
to reuse the store after the rejected ShardWriter::open and assert that
read_latest() returns None, verifying validation leaves no manifest or epoch
side effects while preserving the existing error assertions.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: QUIET

Plan: Pro Plus

Run ID: 65160c7f-2148-41f5-81ba-6346f6051df5

📥 Commits

Reviewing files that changed from the base of the PR and between 9726f88 and d4391fc.

📒 Files selected for processing (2)

• rust/lance/src/dataset/mem_wal/manifest.rs
• rust/lance/src/dataset/mem_wal/write.rs

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

[hamersaw]

I think rather than validating the configuration on this, we should remove it all together. Users are not going to update this because it's such a nuanced configuration option. Instead, we should update the batch size to be exponential with lower and upper bounds, for example the first check is 2, then 4, then 8, then 16 (maybe up to 64?). This way we get the best performance possible.

[u70b3]

That makes sense. I will remove manifest_scan_batch_size from the user-facing configuration and make manifest discovery use an internal exponentially growing batch size, starting at 2 and capped at 64. I will replace the zero-value validation tests with coverage for stale-hint recovery and the batch growth/cap.

One compatibility detail: this removes a public ShardWriterConfig field and builder method. Since MemWAL is still evolving, I plan to remove them directly unless you would prefer a deprecation path.

[u70b3]

@hamersaw I rebased this PR onto the latest main and updated it to use the internal adaptive manifest scan policy you requested (2 → 4 → … → 64). I also corrected the Rust deprecation version and added a regression assertion that the obsolete writer default is no longer persisted.

Local validation completed successfully:

• cargo fmt --all -- --check
• cargo test -p lance dataset::mem_wal::manifest::tests --lib
• cargo test -p lance test_initialize_mem_wal_records_writer_config_defaults --lib
• cargo test -p lance --doc ShardManifestStore
• cargo clippy --all --tests --benches -- -D warnings
• cd python && uv run make build && uv run make lint
• cd java && ./mvnw spotless:check
• cd java && cargo clippy --tests --manifest-path ./lance-jni/Cargo.toml -- -D warnings
• cd java && ./mvnw test

Could you please take another look and approve if this now addresses your requested changes?

[u70b3]

Rebased onto latest main and resolved the conflict with #8640: the store keeps the latest position cache, the adaptive batch policy now applies to the remaining cold-scan/claim paths (scan_latest via refresh_latest), and the deprecated new() shim initializes the cache field. Deprecation versions updated to 12.0.0 to match the workspace. Full mem_wal suite and clippy pass locally (the FTS tiered-merge test is intermittently timing-flaky under parallel load, unrelated to this change).

@hamersaw your CHANGES_REQUESTED direction — drop the user-facing knob, grow internally 2→64 — is what this PR now implements. Could you take another look?

[u70b3]

The remaining CI failures here are tracked in #8789 — systematic main-level test failures (not caused by this PR). Will retrigger once main's own CI recovers.

[u70b3]

@Xuanwo could you help review this when you have a moment? This implements exactly what the CHANGES_REQUESTED asked for — drop the user-facing batch-size knob and grow the manifest scan internally (2→64 adaptive). It's rebased onto current main: conflicts with #8640 resolved (the adaptive policy now applies to the remaining cold-scan/claim paths), deprecations dated to 12.0.0, and the only CI failures are the tracked fleet flakes from #8789. hamersaw hasn't responded since 2026-07-27 despite re-requests, so a fresh review would be much appreciated.

[lance-gatekeeper]

⚠️ Gate recommendation: approve with a non-blocking risk.

The internal policy removes the zero-batch stale-manifest path while preserving the gap-free/CAS position contract from #8640 and retaining the Rust, Python, and Java compatibility shims.

Cold scans with a stale hint trade fewer sequential round trips for more speculative HEADs: a one-version lag performs six successor HEADs instead of four with a fixed batch of two. The 64-concurrent-request cap bounds each batch, but no request-count/latency benchmark covers this trade-off. Follow-up request-count coverage could determine whether growth should stop after a partially filled batch.