Conversation
…n rows Positional remapping requires a group's rewritten-row count to equal the row count of its new fragments, and `CompactFragReuseIndex::try_new` rejects the group otherwise. The per-row map it replaced had no such check -- its `zip` silently truncated -- so a payload that violates the rule used to load and now does not. That is reachable, and not only in theory. Lance 0.30.0 through 4.0.0-beta.6 wrote exactly this shape for a stable-row-id dataset whose index remap was deferred: an empty set of rewritten addresses, alongside new fragments carrying real row counts. The writer stopped doing it in 4.0.0-beta.6's successor, which made row addresses mandatory on that path, but the payloads it wrote are on disk. Since the index is opened inside `load_indices`, such a dataset fails every scan, validate and commit. Recognise the shape in `try_new` -- no rewritten addresses, non-empty new fragments -- and build the group with no new fragments, warning once per group. The per-row map resolved every address in the group's old fragments to deleted for this input, so that is what is preserved: the rows are unreachable either way until the index is rebuilt, and opening succeeds. A group that genuinely deleted everything carries no new fragments and is untouched. Fresh compaction builds go through `RowAddrRemap::compact` directly and keep the strict check.
There was a problem hiding this comment.
✅ Gate recommendation: approve.
The read-side fallback restores version-0 payload compatibility lost in #8887 while leaving current writers and non-empty layout/count validation strict. Interpreting this historical empty-bitmap shape as deleting the covered old rows is compact, confined to the legacy reader, and covered alongside genuine empty rewrites.
|
Closing this: we do not have datasets of the affected vintage, so we are not in a position to keep the change current. Leaving the finding on record, since it is still live on Reproducer, as a test in #[test]
fn test_compact_fri_opens_legacy_group_with_no_rewritten_rows() {
let details = FragReuseIndexDetails {
versions: vec![FragReuseVersion {
dataset_version: 1,
groups: vec![FragReuseGroup {
changed_row_addrs: serialize_changed([]),
old_frags: vec![digest(0, 400), digest(1, 400)],
new_frags: vec![digest(2, 800)],
}],
}],
};
CompactFragReuseIndex::try_new(Uuid::new_v4(), details).unwrap();
}The fix in the closed commit recognises the shape (no rewritten addresses, non-empty new fragments) and builds the group with no new fragments, which makes every address in its old fragments read as deleted, matching what the per-row map produced. Anyone picking this up is welcome to the commit. |
Problem
CompactFragReuseIndex::try_newmaps a group's rewritten old rows positionally onto its new fragments, so it requires the rewritten-row count to equal the new fragments' total row count and rejects the group otherwise. The per-rowHashMapopen path it replaced had no such check; itszipsilently truncated.Lance 0.30.0 through 4.0.0-beta.6 wrote exactly the shape that check rejects: for a stable-row-id dataset whose index remap was deferred, a fragment-reuse group with an empty set of rewritten addresses alongside new fragments carrying real row counts. The writer stopped doing so when row addresses became mandatory on that path, but the payloads it wrote are on disk. Because the index is opened inside
load_indices, a dataset carrying one of those payloads now fails every scan, validate and commit withChange
try_newrecognises the shape, no rewritten addresses together with non-empty new fragments, and builds the group with no new fragments, logging a warning per group. The per-row map resolved every address in the group's old fragments to deleted for this input, so that is what is preserved: the rows are unreachable either way until the index is rebuilt, and opening succeeds. A group that genuinely deleted everything carries no new fragments and takes the normal path. Fresh compaction builds callRowAddrRemap::compactdirectly and keep the strict check; only the reader of persisted payloads is relaxed.Tests
test_compact_fri_opens_legacy_group_with_no_rewritten_rowsbuilds the legacy payload, fails on the previous code with the error above, and asserts that covered addresses resolve to deleted while unclaimed fragments are untouched.test_compact_fri_genuinely_emptied_group_is_unaffected_by_the_legacy_pathpins the boundary.Compatibility
Read-side only. No format change; no change for payloads whose counts agree.
Tracking: Ported in part from rerun-io#59.