refactor(table): move transaction.rs into lance-table - #8054
Draft
wjones127 wants to merge 2 commits into
Draft
Conversation
…saction.rs test_build_manifest_partial_last_updated_rewrite_columns_stable_row_ids is the only test in transaction.rs that needs Dataset, WriteParams and Session. The other 69 build their inputs by hand. Moving it to dataset_transactions.rs, where the dataset-level transaction tests already live, lets transaction.rs move to lance-table unchanged. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Building a manifest from a transaction reads and writes only table metadata. The file has no dependency on Dataset, Session, DataFusion, or async I/O beyond restore_old_manifest, which uses only ObjectStore and CommitHandler, so it belongs at the table layer. The file moves unchanged apart from import paths. lance::dataset::transaction becomes a re-export, so no caller in lance, the Python bindings, or the Java bindings changes. build_manifest, restore_old_manifest, modifies_same_metadata and upsert_key_conflict widen from pub(crate) to pub, since 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 split into a module tree is deliberately left for a follow-up so this step stays a single detected rename. 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
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.
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.rshas no dependency onDataset,Session, DataFusion, or async I/O —restore_old_manifestis the only thing that touches storage at all, and it uses onlyObjectStoreandCommitHandler. So the file moves tolance-table.The file itself is unchanged apart from import paths.
lance::dataset::transactionbecomes a re-export, so nothing changes at any call site — not inlance, 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 transactionindataset.rsrather 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_metadataandupsert_key_conflictwiden frompub(crate)topub, because their callers inio/commit.rsandio/commit/conflict_resolver.rsare now in another crate.lance's own public API is unchanged.default_build_config()helper, sinceManifestWriteConfigstays inlanceand itsDefaultis what the 22build_manifesttest call sites used.ManifestWriteConfig::default()now describes the default config without naming a type from a higher crate.The one test that needed
Datasetmoves todataset_transactions.rsin the first commit, so the second commit is the rename alone. Test counts confirm nothing was dropped: 58 tests before, 57 inlance-tableafter, plus that one.Not included
The split into a module tree —
operation.rs,conflicts.rs,manifest_build.rs,proto.rsand friends — is the next PR in the stack. This PR leaves a single 6488-line file inlance-table.Collapsing the new
lance_table::transaction::Transactionwith the existinglance_table::format::Transaction(a thinpb::Transactionwrapper whose doc comment says it exists so that "lance-table does not depend on higher layers" — the exact inversion this removes) would change the publicCommitHandlertrait signature, so it is left for later.