refactor(table): move transaction helpers down into lance-table - #8053
Merged
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
geruh
reviewed
Jul 28, 2026
geruh
left a comment
Contributor
There was a problem hiding this comment.
Hey Will, did a pass here and looks like a straightforward refactor with everything moved and everything is lined up. Also, checked out locally and ran some tests. Just a left a few small nits, nothing blocking. Let me know what you think.
The function only compares an IndexMetadata name against the two system index name constants, both of which already live in lance-table's system_index module. Moving it there puts it next to the constants it reads; lance-index re-exports it so callers are unaffected. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
KeyExistenceFilter and friends were defined in the lance crate but depend only on arrow, lance-core's bloom filter, and the transaction protobuf in lance-table. The filter is serialized into that protobuf, so the table layer is where it belongs. lance::dataset::write::merge_insert::inserted_rows becomes a re-export, so callers are unaffected. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Deciding which rows an overlay makes stale with respect to an index reads only fragment and index metadata: the coverage bitmaps, the overlay committed_version, and the indexed field ids. None of that needs the read path, so it moves to lance-table alongside the overlay format itself. lance::dataset::overlay keeps the read-resolution half and re-exports the three functions its callers use. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
load_mem_wal_index_details, open_mem_wal_index, new_mem_wal_index_meta and update_mem_wal_index_compacted_sstables read and write the MemWAL index's IndexMetadata entry. Every type they touch already lives in lance-table's system_index module, so they join the data structures they serialize. lance::index::mem_wal keeps the dataset-level operations and re-exports the four helpers at their previous visibility. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
build_manifest and restore_old_manifest took ManifestWriteConfig, whose timestamp field is the lance crate's mockable SystemTime. That mock is cfg(test) of the lance crate, so resolving the timestamp inside a lower crate would silently un-mock it. This adds lance_table::format::ManifestBuildConfig, which carries the timestamp already resolved to nanoseconds, and has ManifestWriteConfig convert into it at the call sites. The conversion stays in the lance crate, so the clock is still mockable. Conversion happens per attempt inside the commit retry loops, matching the previous behavior of resolving the timestamp on each build. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
- Use a named re-export instead of a glob for key_existence - Restore MemWAL module docs dropped in the earlier metadata-helper move - Fully qualify DatasetPreFilter::new in the fragment-coverage comment
wjones127
force-pushed
the
refactor/transaction-prereqs
branch
from
August 17, 2026 21:48
97a8c50 to
5794df2
Compare
wjones127
marked this pull request as ready for review
August 17, 2026 22:21
Contributor
There was a problem hiding this comment.
✅ Gate recommendation: approve.
This moves table-format and system-index logic to the owning crate while preserving existing caller paths and durable formats. Resolving timestamps in lance before passing nanoseconds down keeps the mockable clock and per-attempt behavior without adding lower-layer clock coupling.
westonpace
approved these changes
Aug 18, 2026
This was referenced Aug 18, 2026
Xuanwo
pushed a commit
that referenced
this pull request
Aug 19, 2026
`main` does not compile, which fails the Rust, Python, and Java workflows on main and on every open PR. Two PRs that were each green on their own collided semantically. #8521 added a `migration_next_row_id` option to `ManifestWriteConfig` and read it while building the manifest. #8053 then moved that config down into `lance-table` as the new `ManifestBuildConfig`; because it branched before #8521, the new struct had no such field. Git merged both cleanly, so the break only appeared once the second one landed: ``` error[E0609]: no field `migration_next_row_id` on type `&lance_table::format::ManifestBuildConfig` --> rust/lance/src/dataset/transaction.rs:2230:23 ``` This PR adds the missing field to `ManifestBuildConfig` and passes it through `ManifestWriteConfig::to_build_config`. The code that reads it is already correct and is unchanged. No new test: the five `migrate_to_stable_row_ids` tests added by #8521 already cover this behavior, and they pass again now that the crate compiles. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
wjones127
added a commit
that referenced
this pull request
Aug 19, 2026
Stacked on #8053. Review that one first; this PR targets its branch. Building a manifest from a transaction reads and writes only table metadata. Now that #8053 has moved the helpers it depended on, `transaction.rs` has no dependency on `Dataset`, `Session`, DataFusion, or async I/O — `restore_old_manifest` is the only thing that touches storage at all, and it uses only `ObjectStore` and `CommitHandler`. So the file moves to `lance-table`. The file itself is unchanged apart from import paths. `lance::dataset::transaction` becomes a re-export, so nothing changes at any call site — not in `lance`, not in the Python bindings, not in the Java bindings. Git reports the move as a **97% rename**, so the review surface is the import block rather than 6500 added and 6500 deleted lines. Keeping it that way is why the file arrives here whole and gets split into a module tree in a separate follow-up, and why the shim is an inline `pub mod transaction` in `dataset.rs` rather than a file at the old path — a file there would have left the addition with nothing to pair against, and the rename would not have been detected. Beyond import paths, the moved file changes in three ways, all visible in the rename diff: - `build_manifest`, `restore_old_manifest`, `modifies_same_metadata` and `upsert_key_conflict` widen from `pub(crate)` to `pub`, because their callers in `io/commit.rs` and `io/commit/conflict_resolver.rs` are now in another crate. `lance`'s own public API is unchanged. - The test module gains a `default_build_config()` helper, since `ManifestWriteConfig` stays in `lance` and its `Default` is what the 22 `build_manifest` test call sites used. - One comment that named `ManifestWriteConfig::default()` now describes the default config without naming a type from a higher crate. The one test that needed `Dataset` moves to `dataset_transactions.rs` in the first commit, so the second commit is the rename alone. Test counts confirm nothing was dropped: 58 tests before, 57 in `lance-table` after, plus that one. ## Not included The split into a module tree — `operation.rs`, `conflicts.rs`, `manifest_build.rs`, `proto.rs` and friends — is the next PR in the stack. This PR leaves a single 6488-line file in `lance-table`. Collapsing the new `lance_table::transaction::Transaction` with the existing `lance_table::format::Transaction` (a thin `pb::Transaction` wrapper whose doc comment says it exists so that "lance-table does not depend on higher layers" — the exact inversion this removes) would change the public `CommitHandler` trait signature, so it is left for later. --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
wjones127
added a commit
that referenced
this pull request
Aug 19, 2026
Stacked on #8054, which is stacked on #8053. Review those first; this PR targets #8054's branch. `lance_table::transaction` arrived as a single 6488-line file. This splits it into nine modules along the lines the code was already divided by, leaving `transaction.rs` as declarations, re-exports, and a map of where each concern lives: | module | lines | what it answers | | --- | --- | --- | | `builder` | 90 | what a transaction is: an operation plus the version it was based on | | `operation` | 304 | the vocabulary of changes an operation can describe | | `update_map` | 128 | incremental edits to the manifest's string maps | | `validate` | 357 | pre-commit checks against the manifest being replaced | | `manifest_build` | 1676 | applying an operation to produce the next manifest | | `index_maintenance` | 1027 | how that narrows or drops index metadata | | `row_version` | 1139 | how it assigns row ids and per-row version metadata | | `conflicts` | 1027 | whether two operations collide, for the commit retry path | | `proto` | 816 | the persisted protobuf encoding of all of the above | Each of the nine commits extracts one module, so the "was any logic altered?" question is answerable a module at a time rather than across a 4000-line redistribution. Test counts hold at 57 throughout, and each commit compiles and passes on its own. The 57 tests move with the code they cover. Six fixtures used by more than one module's tests live in a `test_support` module rather than being duplicated. Nothing outside `lance-table` sees a change: the re-export list in `transaction.rs` is the same set of names the module exported before. Items used across submodules are `pub(super)` rather than `pub(crate)`, since the submodules are private — clippy's `pub(crate)`-inside-a-private-module lint is what settles that. One deliberate non-change: `PartialEq for Operation` and `PartialEq for RewriteGroup` each define their own local `compare_vec`. That duplication was there before and is left alone to keep every commit a pure move. ## Not included `manifest_build` stays the outlier at 1676 lines, of which `build_manifest` is about 890 and its tests about 700. The original plan was to break its 15-arm match into per-operation appliers in a `manifest_build/` subdirectory, and I stopped short of it deliberately: unlike everything else here, that is not a move. Each arm mutates four or five pieces of shared state (`final_fragments`, `final_indices`, `next_row_id`, `fragment_id`), so extracting them means threading that state through `&mut` parameters, and a free function taking five `&mut` arguments is not obviously easier to read than the match arm it replaced. Worth doing as its own PR if we want it, where the signatures can be discussed on their merits rather than riding along with a mechanical split. --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.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
3 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.
lance/src/dataset/transaction.rsis 6767 lines and is the next thing we want to move down intolance-table. Its production code turns out to have no dependency onDataset,Session, DataFusion, or async I/O — only six couplings to code abovelance-table. This PR clears five of them so the move itself can be a plain file rename.Each commit moves one self-contained piece down to the layer that already owns the types it touches, and leaves a re-export behind so no caller changes:
is_system_indexcompared anIndexMetadataname against two constants that already live inlance-table'ssystem_indexmodule. It now sits next to them;lance-indexre-exports it.KeyExistenceFilterand friends, previouslymerge_insert/inserted_rows.rs) depends only on arrow,lance-core's bloom filter, and the transaction protobuf. It is serialized into that protobuf, so it moves tolance-table.committed_version, and indexed field ids.lance::dataset::overlaykeeps the read-resolution half.IndexMetadataentry. Every type they touch was already inlance-table, so they join the data structures they serialize.ManifestBuildConfigis new.build_manifesttookManifestWriteConfig, whosetimestampfield is thelancecrate's mockableSystemTime— and that mock iscfg(test)of thelancecrate, so resolving the timestamp inside a lower crate would silently un-mock it. The new config carries the timestamp already resolved to nanoseconds, andManifestWriteConfigconverts into it at the call sites, keeping the clock mockable.The sixth coupling,
ManifestWriteConfigitself, deliberately stays inlancefor that reason.Not included
The move of
transaction.rsintolance-table, and its split into a module tree, come as two follow-up PRs stacked on this one. Splitting them keeps the cross-crate move reviewable as a detected rename rather than a 6767-line add/delete pair.io/commit/conflict_resolver.rsis the other half of the transaction story and a natural later target, but it depends onDatasetandDatasetIndexExt, so it stays put.Testing
The one behavioral question here is whether the mock clock still works, since that is what
ManifestBuildConfigexists to protect. Verified locally: theMockClock-based suites (dataset::cleanup,dataset::delta,dataset::tests::dataset_versioning) pass, 67 tests. Theto_build_config()conversion is called inside the two commit retry loops rather than hoisted above them, so each retry still resolves its own timestamp as before.