pre-release hardening: fix bugs and remove dead code - #103
Merged
Conversation
Fixes from deep 4-agent code review across all crates: - fix(python): return PyBytes for ByteArray columns instead of lossy UTF-8 String — prevents silent binary data corruption on round-trip - fix(store): LocalFileStore::get_range now seeks+reads instead of loading the entire file into memory - fix(store): CachedStore::get tolerates ENOENT from races with LRU eviction instead of panicking - fix(iceberg): read_manifest_payload eliminates TOCTOU by attempting read directly and falling back on NotFound - fix(iceberg): export path also rejects absolute paths (not just "..") - fix(flush): use SEQNUM_MAX constant instead of fragile u64::MAX >> 8 - fix(wal): remove unnecessary double-boxing in WalWriter::close - remove: dead arena module and bumpalo dependency (zero callers) - remove: dead _meta_touch function in translate.rs - remove: dead seen_latest variable in compaction iterator - remove: unused object_store_url field from OpenOptions API - remove: unused SeqNum and ParquetFileMeta imports
|
Thanks for the contribution! Before we can merge this, we need @SreeramGarlapati to sign the Salesforce Inc. Contributor License Agreement. |
SreeramGarlapati
added a commit
that referenced
this pull request
Jun 16, 2026
## Summary Fixes from deep 4-agent parallel code review across all crates (engine, iceberg, memtable/WAL/types, parquet/sql/store/python). Each finding was debated for legitimacy before action. ## Bugs fixed - **Python ByteArray corruption**: `field_value_to_py` used `from_utf8_lossy` for all `Bytes` columns, silently replacing invalid UTF-8 with U+FFFD. Now returns `PyBytes` for lossless round-trip. - **LocalFileStore::get_range**: Read entire file into memory then sliced. Now seeks+reads only the requested range. - **CachedStore TOCTOU race**: `get()` checked `exists()` then `read()` — a concurrent LRU eviction between the two caused ENOENT panic. Now attempts read directly, falls back on NotFound. - **Iceberg read_manifest_payload TOCTOU**: `pb_path.exists()` + `read()` race window. Now attempts read, falls back to JSON on NotFound. - **Export path traversal**: Also rejects absolute paths (`/etc/passwd`), not just `..` sequences. - **flush.rs fragile SeqNum**: Used `u64::MAX >> 8` instead of the `SEQNUM_MAX` constant. - **WAL writer double-box**: `Box::new(self.sink).close()` where `self.sink` is already `Box<dyn WalSink>` — unnecessary heap allocation. ## Dead code removed - `memtable/arena.rs` + `bumpalo` dependency (zero callers, module was documented as having an unsound API) - `_meta_touch` dead_code stub in translate.rs - `seen_latest` dead variable in compaction iterator - `object_store_url` field in `OpenOptions` (never wired to anything) - Unused imports (`SeqNum`, `ParquetFileMeta`) ## Review findings acknowledged but deferred (0.1 scope) - Sync I/O on async runtime in read_path (architectural; needs full async read_path rewrite) - `range_scan` loads entire DB into memory (known; streaming merge is post-0.1) - S3Store has no retry/backoff (needs retry policy design) - Parquet writer double-pass for kv_index (performance; correctness-safe) - DV uses 32-bit roaring (4B row limit per file; documented) ## Test plan - [x] `cargo test --package merutable` — 374 pass - [x] `cargo clippy --package merutable --tests -- -D warnings` — clean - [x] `cargo fmt -- --check` — clean - [x] `cargo check --package merutable-python` — compiles Co-authored-by: Sreeram Garlapati <sreeramg@salesforce.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes from deep 4-agent parallel code review across all crates (engine, iceberg, memtable/WAL/types, parquet/sql/store/python). Each finding was debated for legitimacy before action.
Bugs fixed
field_value_to_pyusedfrom_utf8_lossyfor allBytescolumns, silently replacing invalid UTF-8 with U+FFFD. Now returnsPyBytesfor lossless round-trip.get()checkedexists()thenread()— a concurrent LRU eviction between the two caused ENOENT panic. Now attempts read directly, falls back on NotFound.pb_path.exists()+read()race window. Now attempts read, falls back to JSON on NotFound./etc/passwd), not just..sequences.u64::MAX >> 8instead of theSEQNUM_MAXconstant.Box::new(self.sink).close()whereself.sinkis alreadyBox<dyn WalSink>— unnecessary heap allocation.Dead code removed
memtable/arena.rs+bumpalodependency (zero callers, module was documented as having an unsound API)_meta_touchdead_code stub in translate.rsseen_latestdead variable in compaction iteratorobject_store_urlfield inOpenOptions(never wired to anything)SeqNum,ParquetFileMeta)Review findings acknowledged but deferred (0.1 scope)
range_scanloads entire DB into memory (known; streaming merge is post-0.1)Test plan
cargo test --package merutable— 374 passcargo clippy --package merutable --tests -- -D warnings— cleancargo fmt -- --check— cleancargo check --package merutable-python— compiles