fix(mem-wal): reject zero manifest scan batch size - #7929
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: QUIET Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe change rejects zero ChangesManifest scan validation
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
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 winAssert 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()returnsNone.🤖 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 winAssert that the stale hint was actually persisted.
write_version_hintis 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
📒 Files selected for processing (2)
rust/lance/src/dataset/mem_wal/manifest.rsrust/lance/src/dataset/mem_wal/write.rs
| /// Batch size for parallel HEAD requests when scanning for manifest versions. | ||
| /// | ||
| /// Higher values scan faster but use more parallel requests. | ||
| /// Must be greater than zero. Higher values scan faster but use more parallel | ||
| /// requests. | ||
| /// Default: 2 | ||
| pub manifest_scan_batch_size: usize, |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Complete the public API documentation.
The changed public field and builder method state the > 0 contract but do not provide the required synchronized example and links to the corresponding configuration API. Add a small example using ShardWriterConfig::new(...).with_manifest_scan_batch_size(...) and cross-link the field and method.
As per coding guidelines, **/*.{rs,md,rst}: Document all public APIs with examples and links to relevant structs and methods; keep examples synchronized with actual signatures.
Also applies to: 331-336
🤖 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 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.
Source: Coding guidelines
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
d4391fc to
00f53d5
Compare
hamersaw
left a comment
hamersaw
left a comment
There was a problem hiding this comment.
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.
|
That makes sense. I will remove One compatibility detail: this removes a public |
00f53d5 to
347f27e
Compare
347f27e to
0438d9e
Compare
2 participants
Summary
manifest_scan_batch_size=0atShardWriter::openbefore manifest or epoch side effectsInvalidInputassertionsFixes #7928.
Test Plan
cargo fmt --all -- --checkcargo test -p lance manifest_scan_batch_size --lib -- --nocapturecargo test -p lance dataset::mem_wal::manifest::tests --lib -- --nocapturecargo clippy --all --tests --benches -- -D warnings