refactor: rename MemWAL compaction progress - #7957
Merged
jackye1995 merged 2 commits intoJul 24, 2026
Merged
jackye1995 merged 2 commits into
jackye1995 merged 2 commits into
Conversation
Contributor
📝 WalkthroughWalkthroughMemWAL progress tracking is renamed from merged generations to compacted SSTables across protobuf contracts, Rust transaction/index logic, merge-insert APIs, Java and Python bindings, conflict resolution, tests, and documentation. Storage, freshness, compaction, and read-planning terminology is updated accordingly. ChangesMemWAL compaction tracking
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested labels: 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 |
Contributor
|
Important This PR touches the Lance format specification. Substantive changes to the format specification — the If this is a meaningful format change:
|
jackye1995
force-pushed
the
codex/update-memwal-sstable-compaction
branch
from
July 24, 2026 06:54
43f9815 to
c56f0c0
Compare
jackye1995
marked this pull request as ready for review
July 24, 2026 07:20
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
Note
Quiet mode is enabled, so only the most important comments were posted inline. Other review comments are grouped below.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
java/src/test/java/org/lance/memwal/MemWalTest.java (1)
523-554: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAssert that compaction progress was committed.
The row-count assertion does not verify
CompactedSsTablepropagation; omitting it from the transaction would still pass. Assert thatmerged.memWalIndexDetails()contains(shardId, 1)after the merge.As per coding guidelines, “Every bugfix and feature must have corresponding tests.”
🤖 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 `@java/src/test/java/org/lance/memwal/MemWalTest.java` around lines 523 - 554, The test method testMergeInsertMarkSstablesAsCompacted must verify that compaction metadata is committed, not only that rows were merged. After obtaining merged from mergeInsert, assert that merged.memWalIndexDetails() contains the CompactedSsTable entry identified by shardId and sequence 1, while preserving the existing row-count assertion and cleanup.Source: Coding guidelines
rust/lance/src/dataset/transaction.rs (1)
3379-3403: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick winReplace
.unwrap()with?when deserializingcompacted_sstablesfrom protobuf.Both occurrences call
CompactedSsTable::try_from(m).unwrap()insideTryFrom<pb::Transaction> for Transaction, which returnsResult<Self>.CompactedSsTable::try_fromcan fail (Error::invalid_inputwhenshard_idis missing), so a corrupted or older transaction file will panic the reader instead of returning an error — unlike the adjacentFragment::try_from(...).collect::<Result<Vec<_>>>()?calls in the same arms.🐛 Proposed fix for both occurrences
fields_modified, - compacted_sstables: compacted_sstables - .into_iter() - .map(|m| CompactedSsTable::try_from(m).unwrap()) - .collect(), + compacted_sstables: compacted_sstables + .into_iter() + .map(CompactedSsTable::try_from) + .collect::<Result<Vec<_>>>()?, fields_for_preserving_frag_bitmap,)) => Operation::UpdateMemWalState { - compacted_sstables: compacted_sstables - .into_iter() - .map(|m| CompactedSsTable::try_from(m).unwrap()) - .collect(), + compacted_sstables: compacted_sstables + .into_iter() + .map(CompactedSsTable::try_from) + .collect::<Result<Vec<_>>>()?, },Also applies to: 3519-3526
🤖 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/transaction.rs` around lines 3379 - 3403, In both Update-operation deserialization arms of TryFrom<pb::Transaction> for Transaction, replace the CompactedSsTable::try_from(m).unwrap() collection with fallible collection that propagates conversion errors via ?. Match the adjacent Fragment conversion pattern so invalid or incomplete protobuf data returns the existing Result error instead of panicking.Source: Coding guidelines
rust/lance/src/io/commit/conflict_resolver.rs (1)
1561-1568: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winFinish the terminology migration in local bindings.
Line 1561 retains
committed_mg/to_commit_mgfrom the removedMergedGenerationmodel. Rename them tocommitted_sstable/to_commit_sstable.As per coding guidelines, “When renaming a type, struct, or enum, update all references, including methods, fields, variables, and test names.”
Proposed fix
- for committed_mg in committed { - for to_commit_mg in to_commit { - if committed_mg.shard_id == to_commit_mg.shard_id { + for committed_sstable in committed { + for to_commit_sstable in to_commit { + if committed_sstable.shard_id == to_commit_sstable.shard_id { - if committed_mg.generation >= to_commit_mg.generation { + if committed_sstable.generation >= to_commit_sstable.generation {🤖 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/io/commit/conflict_resolver.rs` around lines 1561 - 1568, Rename the local bindings in the nested loops from committed_mg and to_commit_mg to committed_sstable and to_commit_sstable, and update every reference within the conflict-resolution logic to use the new names consistently.Source: Coding guidelines
🟡 Other comments (1)
python/src/mem_wal.rs-156-159 (1)
156-159: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winInclude the invalid
shard_idvalue in the error.Line 158 only reports the parser error, which is ambiguous when converting multiple SSTables. Include
self.shard_idin the message so callers can identify the failing descriptor.Proposed fix
- .map_err(|e| PyValueError::new_err(format!("Invalid shard_id UUID: {}", e)))?; + .map_err(|e| { + PyValueError::new_err(format!( + "Invalid shard_id UUID {:?}: {}", + self.shard_id, e + )) + })?;As per coding guidelines, error messages must include full context such as variable names and values.
🤖 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 `@python/src/mem_wal.rs` around lines 156 - 159, Update the error mapping in MemWal::to_lance so the invalid self.shard_id value is included alongside the parser error in the PyValueError message, allowing callers to identify the failing descriptor.Source: Coding guidelines
🧹 Nitpick comments (1)
python/python/tests/test_mem_wal.py (1)
77-95: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winConsider asserting the recorded
compacted_sstablesstate, not just row count.The test verifies
mark_sstables_as_compacteddoesn't break merge-insert row counts, but doesn't check that the shard/generation was actually recorded in the MemWAL index'scompacted_sstables. If there's an accessor (e.g. dataset/index stats) to read that state, asserting on it would make this a real regression test for the renamed API rather than only an incidental smoke test.🤖 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 `@python/python/tests/test_mem_wal.py` around lines 77 - 95, The test_mark_sstables_as_compacted test only verifies the resulting row count, not that mark_sstables_as_compacted records the supplied shard and generation. Use the available dataset or MemWAL index state accessor after execute to assert compacted_sstables contains CompactedSsTable(shard_id, 1), while preserving the existing row-count assertion.
🤖 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 `@java/lance-jni/src/merge_insert.rs`:
- Around line 252-260: Validate CompactedSsTable inputs at both API boundaries:
in java/src/main/java/org/lance/memwal/CompactedSsTable.java lines 33-36, reject
malformed shardId UUIDs and negative generation values with descriptive errors;
in java/lance-jni/src/merge_insert.rs lines 252-260, replace the unchecked
generation cast with fallible u64::try_from, rejecting negatives while including
the generation value in the error, then construct CompactedSsTable only after
validation.
In `@java/src/main/java/org/lance/memwal/CompactedSsTable.java`:
- Around line 25-45: Preserve the renamed public APIs with deprecated adapters:
in java/src/main/java/org/lance/memwal/CompactedSsTable.java lines 25-45, retain
deprecated MergedGeneration with its former constructor and accessors while
delegating or mapping to the compacted-SSTable representation; in
java/src/main/java/org/lance/merge/MergeInsertParams.java lines 256-267, retain
deprecated markGenerationsAsMerged(...) delegating to
markSstablesAsCompacted(...); and in
rust/lance/src/dataset/write/merge_insert.rs lines 605-612, retain deprecated
MergedGeneration and mark_generations_as_merged(...) adapters delegating to the
replacement APIs.
In `@java/src/main/java/org/lance/merge/MergeInsertParams.java`:
- Around line 247-267: Update MergeInsertParams.markSstablesAsCompacted and its
Javadoc to document atomic commit behavior and valid SSTable coordinate
constraints, linking to CompactedSsTable and relevant merge-insert APIs. In
rust/lance/src/dataset/write/merge_insert.rs:605-612, add a compiling
synchronized usage example that demonstrates the actual CompactedSsTable and
merge-insert commit APIs; ensure both sites reflect the same signatures,
guarantees, and constraints.
In `@protos/table.proto`:
- Around line 645-652: Reserve field number 7 in the ShardManifest message so
future schema changes cannot reuse it, while preserving the existing
current_generation and sstables fields unchanged.
In `@python/python/lance/dataset.py`:
- Around line 751-766: Preserve the renamed Python APIs during a deprecation
window: in python/python/lance/dataset.py lines 751-766, add deprecated
mark_generations_as_merged(...) as a wrapper around
mark_sstables_as_compacted(...); in python/python/lance/mem_wal.py lines 43-43
and 127-143, retain the MergedGeneration export and a compatibility type or
alias with the existing fields; in python/python/lance/__init__.py lines 53-57
and 127, continue re-exporting MergedGeneration and include the deprecated name
in __all__. Add regression tests covering these compatibility APIs.
In `@python/python/lance/mem_wal.py`:
- Around line 630-634: Update the _to_raw_compacted_sstables return annotation
from bare list to a parameterized list[_CompactedSsTable], preserving the
existing conversion logic and Python-to-PyO3 element type.
---
Outside diff comments:
In `@java/src/test/java/org/lance/memwal/MemWalTest.java`:
- Around line 523-554: The test method testMergeInsertMarkSstablesAsCompacted
must verify that compaction metadata is committed, not only that rows were
merged. After obtaining merged from mergeInsert, assert that
merged.memWalIndexDetails() contains the CompactedSsTable entry identified by
shardId and sequence 1, while preserving the existing row-count assertion and
cleanup.
In `@rust/lance/src/dataset/transaction.rs`:
- Around line 3379-3403: In both Update-operation deserialization arms of
TryFrom<pb::Transaction> for Transaction, replace the
CompactedSsTable::try_from(m).unwrap() collection with fallible collection that
propagates conversion errors via ?. Match the adjacent Fragment conversion
pattern so invalid or incomplete protobuf data returns the existing Result error
instead of panicking.
In `@rust/lance/src/io/commit/conflict_resolver.rs`:
- Around line 1561-1568: Rename the local bindings in the nested loops from
committed_mg and to_commit_mg to committed_sstable and to_commit_sstable, and
update every reference within the conflict-resolution logic to use the new names
consistently.
---
Other comments:
In `@python/src/mem_wal.rs`:
- Around line 156-159: Update the error mapping in MemWal::to_lance so the
invalid self.shard_id value is included alongside the parser error in the
PyValueError message, allowing callers to identify the failing descriptor.
---
Nitpick comments:
In `@python/python/tests/test_mem_wal.py`:
- Around line 77-95: The test_mark_sstables_as_compacted test only verifies the
resulting row count, not that mark_sstables_as_compacted records the supplied
shard and generation. Use the available dataset or MemWAL index state accessor
after execute to assert compacted_sstables contains CompactedSsTable(shard_id,
1), while preserving the existing row-count assertion.
🪄 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: b56ea08d-eba5-49d8-9ea2-625aeb39833a
⛔ Files ignored due to path filters (3)
docs/src/images/mem_wal_overview.pngis excluded by!**/*.pngdocs/src/images/mem_wal_regional.pngis excluded by!**/*.pngdocs/src/images/mem_wal_shard.pngis excluded by!**/*.png
📒 Files selected for processing (35)
docs/src/format/index/system/mem_wal.mddocs/src/format/table/mem_wal.mdjava/lance-jni/src/merge_insert.rsjava/lance-jni/src/transaction.rsjava/src/main/java/org/lance/memwal/CompactedSsTable.javajava/src/main/java/org/lance/merge/MergeInsertParams.javajava/src/test/java/org/lance/memwal/MemWalTest.javaprotos/table.protoprotos/transaction.protopython/python/lance/__init__.pypython/python/lance/dataset.pypython/python/lance/lance/__init__.pyipython/python/lance/mem_wal.pypython/python/tests/test_mem_wal.pypython/src/dataset.rspython/src/lib.rspython/src/mem_wal.rspython/src/transaction.rsrust/lance-index/src/mem_wal.rsrust/lance-table/src/system_index/mem_wal.rsrust/lance/benches/mem_wal/point_lookup/mem_wal_point_lookup_bench.rsrust/lance/src/dataset/fragment.rsrust/lance/src/dataset/mem_wal/scanner.rsrust/lance/src/dataset/mem_wal/scanner/builder.rsrust/lance/src/dataset/mem_wal/scanner/collector.rsrust/lance/src/dataset/mem_wal/scanner/data_source.rsrust/lance/src/dataset/mem_wal/write.rsrust/lance/src/dataset/transaction.rsrust/lance/src/dataset/write/commit.rsrust/lance/src/dataset/write/merge_insert.rsrust/lance/src/dataset/write/merge_insert/exec/delete.rsrust/lance/src/dataset/write/merge_insert/exec/write.rsrust/lance/src/dataset/write/update.rsrust/lance/src/index/mem_wal.rsrust/lance/src/io/commit/conflict_resolver.rs
Comment on lines
252
to
+260
| let shard_id: JString = env | ||
| .call_method(&obj, "shardId", "()Ljava/lang/String;", &[])? | ||
| .call_method(&obj, "getShardId", "()Ljava/lang/String;", &[])? | ||
| .l()? | ||
| .into(); | ||
| let shard_id = shard_id.extract(env)?; | ||
| let generation = env.call_method(&obj, "generation", "()J", &[])?.j()? as u64; | ||
| let generation = env.call_method(&obj, "getGeneration", "()J", &[])?.j()? as u64; | ||
| let uuid = Uuid::parse_str(&shard_id) | ||
| .map_err(|e| Error::input_error(format!("Invalid shard_id UUID: {}", e)))?; | ||
| Ok(MergedGeneration::new(uuid, generation)) | ||
| Ok(CompactedSsTable::new(uuid, generation)) |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Reject invalid compaction coordinates before conversion.
A negative Java generation is cast at Line 257 to u64, becoming a huge generation value. Validate UUID-form shardId and non-negative generation in Java, and use a fallible u64::try_from with the rejected generation value in JNI.
java/lance-jni/src/merge_insert.rs#L252-L260: reject negativegenerationbefore constructingCompactedSsTable.java/src/main/java/org/lance/memwal/CompactedSsTable.java#L33-L36: reject malformedshardIdand negativegenerationat construction.
As per coding guidelines, “Validate inputs at API boundaries and reject invalid values with descriptive errors; never silently clamp or adjust them,” and “Include full error context, including variable names, values, sizes, and types.”
📍 Affects 2 files
java/lance-jni/src/merge_insert.rs#L252-L260(this comment)java/src/main/java/org/lance/memwal/CompactedSsTable.java#L33-L36
🤖 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 `@java/lance-jni/src/merge_insert.rs` around lines 252 - 260, Validate
CompactedSsTable inputs at both API boundaries: in
java/src/main/java/org/lance/memwal/CompactedSsTable.java lines 33-36, reject
malformed shardId UUIDs and negative generation values with descriptive errors;
in java/lance-jni/src/merge_insert.rs lines 252-260, replace the unchecked
generation cast with fallible u64::try_from, rejecting negatives while including
the generation value in the error, then construct CompactedSsTable only after
validation.
Source: Coding guidelines
Comment on lines
+25
to
+45
| public class CompactedSsTable { | ||
| private final String shardId; | ||
| private final long generation; | ||
|
|
||
| /** | ||
| * @param shardId UUID string for the write shard | ||
| * @param generation generation number from {@link ShardSnapshot#sstables()} | ||
| */ | ||
| public MergedGeneration(String shardId, long generation) { | ||
| public CompactedSsTable(String shardId, long generation) { | ||
| Preconditions.checkNotNull(shardId, "shardId must not be null"); | ||
| this.shardId = shardId; | ||
| this.generation = generation; | ||
| } | ||
|
|
||
| /** UUID string for the write shard. */ | ||
| public String shardId() { | ||
| public String getShardId() { | ||
| return shardId; | ||
| } | ||
|
|
||
| /** The merged generation number. */ | ||
| public long generation() { | ||
| /** The compacted SSTable's generation number. */ | ||
| public long getGeneration() { |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Preserve the renamed public APIs through deprecation adapters.
This removes both Java and Rust public symbols used by existing callers. Keep deprecated MergedGeneration / markGenerationsAsMerged and MergedGeneration / mark_generations_as_merged adapters delegating to the new compacted-SSTable APIs.
java/src/main/java/org/lance/memwal/CompactedSsTable.java#L25-L45: retain a deprecatedMergedGenerationcompatibility type with the former constructor/accessors.java/src/main/java/org/lance/merge/MergeInsertParams.java#L256-L267: retain deprecatedmarkGenerationsAsMerged(...)delegating tomarkSstablesAsCompacted(...).rust/lance/src/dataset/write/merge_insert.rs#L605-L612: retain deprecatedMergedGenerationandmark_generations_as_merged(...)compatibility adapters.
As per coding guidelines, “Do not break public API signatures; deprecate old APIs with #[deprecated] or @deprecated and add a replacement.”
📍 Affects 3 files
java/src/main/java/org/lance/memwal/CompactedSsTable.java#L25-L45(this comment)java/src/main/java/org/lance/merge/MergeInsertParams.java#L256-L267rust/lance/src/dataset/write/merge_insert.rs#L605-L612
🤖 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 `@java/src/main/java/org/lance/memwal/CompactedSsTable.java` around lines 25 -
45, Preserve the renamed public APIs with deprecated adapters: in
java/src/main/java/org/lance/memwal/CompactedSsTable.java lines 25-45, retain
deprecated MergedGeneration with its former constructor and accessors while
delegating or mapping to the compacted-SSTable representation; in
java/src/main/java/org/lance/merge/MergeInsertParams.java lines 256-267, retain
deprecated markGenerationsAsMerged(...) delegating to
markSstablesAsCompacted(...); and in
rust/lance/src/dataset/write/merge_insert.rs lines 605-612, retain deprecated
MergedGeneration and mark_generations_as_merged(...) adapters delegating to the
replacement APIs.
Source: Coding guidelines
Comment on lines
247
to
+267
| /** | ||
| * Mark MemWAL generations as merged into the base table. | ||
| * Mark MemWAL SSTables as compacted into the base table. | ||
| * | ||
| * <p>Use this when the merge insert incorporates data from MemWAL flushed generations. It updates | ||
| * the MemWAL generation tracking to prevent the same generations from being merged again. | ||
| * <p>Use this when merge insert compacts MemWAL SSTables. It updates MemWAL compaction progress | ||
| * to prevent the same SSTables from being compacted again. | ||
| * | ||
| * @param generations the flushed generations being merged | ||
| * @param sstables the SSTables being compacted | ||
| * @return This MergeInsertParams instance | ||
| */ | ||
| public MergeInsertParams markGenerationsAsMerged(List<MergedGeneration> generations) { | ||
| Preconditions.checkNotNull(generations, "generations must not be null"); | ||
| this.markedGenerations = generations; | ||
| public MergeInsertParams markSstablesAsCompacted(List<CompactedSsTable> sstables) { | ||
| Preconditions.checkNotNull(sstables, "sstables must not be null"); | ||
| this.compactedSstables = sstables; | ||
| return this; | ||
| } | ||
|
|
||
| public List<String> on() { | ||
| return on; | ||
| } | ||
|
|
||
| public List<MergedGeneration> markedGenerations() { | ||
| return markedGenerations; | ||
| public List<CompactedSsTable> getCompactedSstables() { | ||
| return compactedSstables; |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Add synchronized API documentation for compaction marking.
Document the atomic commit guarantee, valid SSTable coordinates, and a synchronized Rust usage example linking CompactedSsTable and the relevant merge-insert APIs.
java/src/main/java/org/lance/merge/MergeInsertParams.java#L247-L267: mirror the Rust API’s atomicity and input constraints in Javadoc.rust/lance/src/dataset/write/merge_insert.rs#L605-L612: add a compiling usage example and links toCompactedSsTableand the commit behavior.
As per coding guidelines, “Document all public APIs with examples and links to relevant structs and methods; keep examples synchronized with actual signatures,” and “Copy Rust documentation about defaults, constraints, and invariants into Javadoc for binding classes.”
📍 Affects 2 files
java/src/main/java/org/lance/merge/MergeInsertParams.java#L247-L267(this comment)rust/lance/src/dataset/write/merge_insert.rs#L605-L612
🤖 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 `@java/src/main/java/org/lance/merge/MergeInsertParams.java` around lines 247 -
267, Update MergeInsertParams.markSstablesAsCompacted and its Javadoc to
document atomic commit behavior and valid SSTable coordinate constraints,
linking to CompactedSsTable and relevant merge-insert APIs. In
rust/lance/src/dataset/write/merge_insert.rs:605-612, add a compiling
synchronized usage example that demonstrates the actual CompactedSsTable and
merge-insert commit APIs; ensure both sites reflect the same signatures,
guarantees, and constraints.
Source: Coding guidelines
Comment on lines
+645
to
652
| // Generation to assign to the next SSTable (incremented after each MemTable flush). | ||
| uint64 current_generation = 6; | ||
|
|
||
| // Field 7 removed: merged_generation moved to MemWalIndexDetails.merged_generations | ||
| // which is the authoritative source for merge progress. | ||
| // Field 7 removed: compaction progress lives in | ||
| // MemWalIndexDetails.compacted_sstables. | ||
|
|
||
| // List of SSTables (flushed MemTable generations) and their directory paths. | ||
| // List of SSTables created by flushing MemTables and their directory paths. | ||
| repeated SsTable sstables = 8; |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== candidate proto files =="
fd -a 'protos/(table|transaction)\.proto$' . | sed 's#^\./##'
echo
echo "== ShardManifest occurrences =="
rg -n "message ShardManifest|compacted_sstables|current_generation|reserved (200|202|7)|SsTable \[" protos || true
echo
echo "== table proto around ShardManifest =="
sed -n '560,680p' protos/table.proto | cat -n
echo
echo "== transaction proto reserved lines =="
rg -n -C 2 "reserved (200|202)|message .*Transaction" protos/transaction.proto || true
echo
echo "== search ShardManifest usages in repo =="
rg -n "ShardManifest" . --glob '!*.lock' --glob '!node_modules/**' || true
echo
echo "== changed files/stat =="
git diff --stat || true
git diff --name-only || trueRepository: lance-format/lance
Length of output: 17135
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== ShardManifest protobuf mapping =="
sed -n '170,270p' rust/lance-table/src/system_index/mem_wal.rs | cat -n
echo
echo "== mem_wal format docs around ShardManifest =="
sed -n '260,315p' docs/src/format/table/mem_wal.md | cat -n
echo
echo "== field-number assignments in ShardManifestStore write/read =="
rg -n -C 2 "ShardManifest \\{|From<ShardManifest> for pb::ShardManifest|pb::ShardManifest|read_version|read_latest|write\\(" rust/lance/src/dataset/mem_wal/manifest.rs protos/table.proto || true
echo
echo "== deterministic check: field-7 removal/reservation invariant in ShardManifest proto =="
python3 - <<'PY'
from pathlib import Path
import re
path = Path("protos/table.proto")
text = path.read_text()
m = re.search(r"message ShardManifest \{(?P<body>.*?)\n\}", text, re.S)
if not m:
raise SystemExit("ShardManifest message not found")
body = m.group("body")
def fields(regex):
pat = re.compile(regex + r"\s+\w+\s*=\s*(\d+).*")
return {int(number) for number in pat.findall(body)}
removed = fields(r"Field \d+ removed")
reserved_numbers = set()
for name, field in re.findall(r"reserved\s+(.+?);", body, re.S):
# handle "reserved 7;" and "reserved 200, 202;"
reserved_numbers.update(int(x.strip()) for x in re.findall(r"\d+", name))
# handle optional quoted reserved names, though current original has no name reservation
for name, field in re.findall(r"reserved\s+(?P<num_str>[^;]+?);", body):
reserved_numbers.update(int(x.strip()) for x in re.findall(r"\d+", name))
present = fields(r"=\s*\d+")
print({"removed": "field 7" in body and 7 not in present, "removed_has_reservation": "field 7" in body and 7 in reserved_numbers, "present": sorted(present), "reserved": sorted(reserved_numbers)})
PYRepository: lance-format/lance
Length of output: 24655
Reserve removed field 7 in ShardManifest.
Field 7 is documented as removed, but it is not added to reserved, so future additions could reuse that number and misinterpret old persisted ShardManifest bytes that still contain field 7.
🛡️ Proposed fix
// Generation to assign to the next SSTable (incremented after each MemTable flush).
uint64 current_generation = 6;
- // Field 7 removed: compaction progress lives in
- // MemWalIndexDetails.compacted_sstables.
+ // Field 7 removed: compaction progress lives in
+ // MemWalIndexDetails.compacted_sstables.
+ reserved 7;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // Generation to assign to the next SSTable (incremented after each MemTable flush). | |
| uint64 current_generation = 6; | |
| // Field 7 removed: merged_generation moved to MemWalIndexDetails.merged_generations | |
| // which is the authoritative source for merge progress. | |
| // Field 7 removed: compaction progress lives in | |
| // MemWalIndexDetails.compacted_sstables. | |
| // List of SSTables (flushed MemTable generations) and their directory paths. | |
| // List of SSTables created by flushing MemTables and their directory paths. | |
| repeated SsTable sstables = 8; | |
| // Generation to assign to the next SSTable (incremented after each MemTable flush). | |
| uint64 current_generation = 6; | |
| // Field 7 removed: compaction progress lives in | |
| // MemWalIndexDetails.compacted_sstables. | |
| reserved 7; | |
| // List of SSTables created by flushing MemTables and their directory paths. | |
| repeated SsTable sstables = 8; |
🤖 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 `@protos/table.proto` around lines 645 - 652, Reserve field number 7 in the
ShardManifest message so future schema changes cannot reuse it, while preserving
the existing current_generation and sstables fields unchanged.
Source: Coding guidelines
Comment on lines
+751
to
+766
| def mark_sstables_as_compacted( | ||
| self, sstables: "List[mem_wal.CompactedSsTable]" | ||
| ) -> "MergeInsertBuilder": | ||
| """Mark MemWAL generations as merged into the base table. | ||
| """Mark MemWAL SSTables as compacted into the base table. | ||
|
|
||
| Call this before executing the merge_insert when the source data | ||
| includes rows from MemWAL flushed generations. | ||
| Call this before executing merge_insert when it compacts MemWAL SSTables. | ||
|
|
||
| Parameters | ||
| ---------- | ||
| generations : list of MergedGeneration | ||
| Generations to mark as merged. | ||
| sstables : list of CompactedSsTable | ||
| SSTables to mark as compacted. | ||
| """ | ||
| from .mem_wal import _to_raw_merged_generations | ||
| from .mem_wal import _to_raw_compacted_sstables | ||
|
|
||
| raw_gens = _to_raw_merged_generations(generations) | ||
| super(MergeInsertBuilder, self).mark_generations_as_merged(raw_gens) | ||
| raw_sstables = _to_raw_compacted_sstables(sstables) | ||
| super(MergeInsertBuilder, self).mark_sstables_as_compacted(raw_sstables) |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Preserve the renamed Python APIs through a deprecation window.
This change removes public names rather than deprecating them, so existing users will see import failures or AttributeError. Keep compatibility aliases/wrappers, mark them deprecated, and add regression tests.
python/python/lance/dataset.py#L751-L766: retainmark_generations_as_merged(...)as a deprecated wrapper aroundmark_sstables_as_compacted(...).python/python/lance/mem_wal.py#L43-L43: keepMergedGenerationin the module exports.python/python/lance/mem_wal.py#L127-L143: retain a compatibility type/alias with the existing fields.python/python/lance/__init__.py#L53-L57: continue re-exportingMergedGeneration.python/python/lance/__init__.py#L127-L127: keep the deprecated name in__all__.
As per coding guidelines, public APIs must not be broken; old APIs should be deprecated with replacements.
📍 Affects 3 files
python/python/lance/dataset.py#L751-L766(this comment)python/python/lance/mem_wal.py#L43-L43python/python/lance/mem_wal.py#L127-L143python/python/lance/__init__.py#L53-L57python/python/lance/__init__.py#L127-L127
🤖 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 `@python/python/lance/dataset.py` around lines 751 - 766, Preserve the renamed
Python APIs during a deprecation window: in python/python/lance/dataset.py lines
751-766, add deprecated mark_generations_as_merged(...) as a wrapper around
mark_sstables_as_compacted(...); in python/python/lance/mem_wal.py lines 43-43
and 127-143, retain the MergedGeneration export and a compatibility type or
alias with the existing fields; in python/python/lance/__init__.py lines 53-57
and 127, continue re-exporting MergedGeneration and include the deprecated name
in __all__. Add regression tests covering these compatibility APIs.
Source: Coding guidelines
Comment on lines
+630
to
+634
| def _to_raw_compacted_sstables( | ||
| sstables: Iterable[CompactedSsTable], | ||
| ) -> list: | ||
| """Convert Python MergedGeneration list to PyO3 _MergedGeneration list.""" | ||
| return [_MergedGeneration(g.shard_id, g.generation) for g in generations] | ||
| """Convert Python CompactedSsTable list to PyO3 _CompactedSsTable list.""" | ||
| return [_CompactedSsTable(s.shard_id, s.generation) for s in sstables] |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Parameterize the helper’s return type.
The new helper returns a bare list. Use an element type such as list[_CompactedSsTable] to keep the Python-to-PyO3 boundary type-safe.
As per coding guidelines, Python annotations must use parameterized type hints and never bare generics.
🤖 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 `@python/python/lance/mem_wal.py` around lines 630 - 634, Update the
_to_raw_compacted_sstables return annotation from bare list to a parameterized
list[_CompactedSsTable], preserving the existing conversion logic and
Python-to-PyO3 element type.
Source: Coding guidelines
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
hamersaw
approved these changes
Jul 24, 2026
Xuanwo
pushed a commit
that referenced
this pull request
Aug 10, 2026
`main` does not compile its tests. `cargo test -p lance --lib` stops at
three uses of a field that no longer exists:
error[E0559]: variant `dataset::transaction::Operation::Update` has no
field named `merged_generations`
--> rust/lance/src/dataset/transaction.rs:5453:17
|
5453 | merged_generations: vec![],
| ^^^^^^^^^^^^^^^^^^ `dataset::transaction::Operation::Update` does not
have this field
|
= note: available fields are: `compacted_sstables`
#7957 renamed that field to `compacted_sstables` in `Operation::Update`
and in `pb::transaction::Update`. #7432 was branched before it and its
tests still name the old field, so the merge produced code that no
compiler had seen: neither branch was wrong on its own base, and git had
no textual conflict to report.
All three sites pass an empty list, so they take the new name unchanged.
With it, `cargo test -p lance --lib dataset::transaction::tests` is back
to 64 passed.
Co-authored-by: Vova Kolmakov <wombatukun@apache.org>
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
2 participants
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
CompactedSsTable/compacted_sstablesacross protobuf, Rust, Python, and JavaValidation
cargo clippy --all --tests --benches -- -D warningscargo test -p lance compacted_sstablesuv run mkdocs build