Skip to content

refactor(table): split transaction.rs into a module tree - #8056

Merged
wjones127 merged 13 commits into
mainfrom
refactor/transaction-module-split
Aug 19, 2026
Merged

refactor(table): split transaction.rs into a module tree#8056
wjones127 merged 13 commits into
mainfrom
refactor/transaction-module-split

Conversation

@wjones127

Copy link
Copy Markdown
Contributor

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.

@geruh geruh left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks clean to me! Just left a few nits below

Comment thread rust/lance-table/src/transaction.rs
Comment thread rust/lance-table/src/transaction/manifest_build.rs
@wjones127
wjones127 force-pushed the refactor/transaction-module-split branch from d661d9e to dfaccc4 Compare August 17, 2026 21:48
wjones127 added a commit that referenced this pull request Aug 17, 2026
…ents

register_pure_rewrite_rows_update_frags_in_indices dropped the
results_are_row_addrs() guard when it was extracted from transaction.rs
into index_maintenance.rs. Without it, a RewriteRows update wrongly
extended ZoneMap/BloomFilter coverage to the new fragment, whose
physical addresses those indices cannot follow.

Fixes the CI failures on #8056 (test_addr_domain_index_does_not_cover_rewritten_update_fragment).
@wjones127
wjones127 force-pushed the refactor/transaction-module-split branch from dfaccc4 to cc03654 Compare August 17, 2026 22:50
@wjones127
wjones127 marked this pull request as ready for review August 18, 2026 15:39
@codecov

codecov Bot commented Aug 18, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

@geruh geruh left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@lance-gatekeeper lance-gatekeeper Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gate recommendation: approve.

The responsibility-based module split preserves transaction behavior and protobuf compatibility. It does make schema_fragments_modern_valid and schema_fragments_legacy_valid unreachable at their previous public paths.

Please mark this PR with the breaking-change label.

@lance-gatekeeper lance-gatekeeper Bot added the K-approved Latest Gatekeeper recommendation permits acceptance. label Aug 18, 2026
stack merge was automatically disabled August 18, 2026 21:51

Pull Request is not mergeable

stack merge was automatically disabled August 18, 2026 21:52

Pull Request is not mergeable

stack merge was automatically disabled August 18, 2026 21:54

Pull Request is not mergeable

wjones127 added a commit that referenced this pull request Aug 19, 2026
…ents

register_pure_rewrite_rows_update_frags_in_indices dropped the
results_are_row_addrs() guard when it was extracted from transaction.rs
into index_maintenance.rs. Without it, a RewriteRows update wrongly
extended ZoneMap/BloomFilter coverage to the new fragment, whose
physical addresses those indices cannot follow.

Fixes the CI failures on #8056 (test_addr_domain_index_does_not_cover_rewritten_update_fragment).
@wjones127
wjones127 force-pushed the refactor/transaction-module-split branch from cc03654 to cfefa98 Compare August 19, 2026 15:36
Base automatically changed from refactor/transaction-to-lance-table to main August 19, 2026 17:06
wjones127 added a commit that referenced this pull request Aug 19, 2026
…ents

register_pure_rewrite_rows_update_frags_in_indices dropped the
results_are_row_addrs() guard when it was extracted from transaction.rs
into index_maintenance.rs. Without it, a RewriteRows update wrongly
extended ZoneMap/BloomFilter coverage to the new fragment, whose
physical addresses those indices cannot follow.

Fixes the CI failures on #8056 (test_addr_domain_index_does_not_cover_rewritten_update_fragment).
@wjones127
wjones127 force-pushed the refactor/transaction-module-split branch from cfefa98 to f9a4a84 Compare August 19, 2026 17:06
wjones127 and others added 8 commits August 19, 2026 10:34
validate_operation and its four schema/fragment helpers check an operation
against the manifest it applies to. They have no other callers inside
transaction.rs beyond one call to merge_fragment_physically_rewritten from
build_manifest.

Also adds transaction::test_support for the three fixtures that more than one
submodule's tests will need, so later extractions have somewhere to reach for
them.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…n module

The thirteen conversions between the transaction types and pb::Transaction were
about 700 lines interleaved with the logic they serialize. They are the format
contract for this module -- a field added to an Operation is only durable once
it round-trips here -- so grouping them makes that contract reviewable in one
place.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
PartialEq for Operation is 836 lines: one arm per pair of operation variants,
hand-written because several operations carry Vec fields whose order is not
meaningful. It sat between the Operation definition and the logic that consumes
it, along with the four config-key helpers the commit retry path uses to decide
whether a concurrent operation touched metadata it depends on.

Grouping them puts every "do these two operations collide" question in one file.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
assign_row_ids, the created_at/last_updated resolution helpers and the run
encoder all serve one concern: which row ids the new fragments get, and what
per-row version metadata travels with them. Keeping created_at correct across
an update means tracing each new row back to the fragment and offset it came
from, which is the bulk of this code and reads better away from the manifest
assembly that calls it.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
An index entry claims coverage of a set of fragments and fields, and any
operation that rewrites data can invalidate part of that claim. The ten methods
that narrow a fragment bitmap, drop no-longer-described fields, or drop the
index outright were spread through the second half of impl Transaction.

Getting these rules wrong does not fail a commit, it silently returns stale rows
from the index, so they are worth reading as a group next to the 20 tests that
pin them down.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Dataset config, table metadata, schema metadata and per-field metadata are all
string maps updated the same way: entries where a None value deletes the key,
plus a flag choosing between merge and replace. The type, its four From
conversions and the three functions that apply it move together.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Operation, UpdateMode, the two group types, RewriteGroup, RewrittenIndex and
UpdatedFragmentOffsets are the data model the rest of the module is written
against. Separating the definitions from the code that applies them leaves each
side readable on its own: this file answers what a transaction can say, and
manifest_build answers what happens when it is applied.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The Transaction struct, its two constructors and TransactionBuilder are the
entry point to the module and read better away from the manifest assembly they
feed.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
wjones127 and others added 5 commits August 19, 2026 10:34
…facade

build_manifest and the helpers that feed it move to manifest_build, which
completes the split: transaction.rs is now module declarations, re-exports, and
a map of where each concern lives.

The re-export list is the same set of names the module exported before, so
nothing outside lance-table sees a change. Items used across submodules are
pub(super) rather than pub(crate), since the submodules themselves are private.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
- Fix misaligned doc comment columns in transaction.rs module map
- Remove duplicate #[cfg(test)] attribute in manifest_build.rs
- validate.rs: pub -> not pub(crate), redundant clone removed
- cargo fmt --all across touched transaction files
…ents

register_pure_rewrite_rows_update_frags_in_indices dropped the
results_are_row_addrs() guard when it was extracted from transaction.rs
into index_maintenance.rs. Without it, a RewriteRows update wrongly
extended ZoneMap/BloomFilter coverage to the new fragment, whose
physical addresses those indices cannot follow.

Fixes the CI failures on #8056 (test_addr_domain_index_does_not_cover_rewritten_update_fragment).
build_manifest_with_read_version's split into manifest_build.rs was
authored against a version of transaction.rs that predated
migrate_to_stable_row_ids (#8521). That feature added a
migration_next_row_id escape hatch through the 'cannot enable stable
row ids on existing dataset' guard and through the next_row_id
watermark computation, plus a check blocking migration while indexes
exist. The extraction silently dropped all three, since main's
transaction.rs already carried them by the time this branch rebased
onto it and the empty-incoming conflict resolution assumed the
destination file already matched.

Fixes the 4 failing dataset_migrations tests.
@wjones127
wjones127 force-pushed the refactor/transaction-module-split branch from f9a4a84 to 59acbcc Compare August 19, 2026 17:44
@wjones127
wjones127 merged commit 6f7b1c9 into main Aug 19, 2026
38 of 39 checks passed
@wjones127
wjones127 deleted the refactor/transaction-module-split branch August 19, 2026 18:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

K-approved Latest Gatekeeper recommendation permits acceptance.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants