Conversation
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>
…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>
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>
…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>
…ction V2)
Draft the full action vocabulary for action-based transactions (Transaction
V2) directly in canonical `transaction.proto`, so it can drive the OSS-1529
squash/merge spike and the OSS-757 PMC vote.
A `UserOperation` (a new `Transaction.operation` oneof arm, field 116) is an
ordered list of `UserAction` steps, each expanding to granular `Action`
deltas. Actions record the *change* to the manifest (not a post-image), which
is what makes compound commits and branch merge fall out uniformly. Minted
identifiers (field/fragment/base ids) carry a `Local` token via a single
`Ref { committed | local }` so they relocate on merge/rebase; reference-stable
changes key off stable coordinates. Computed conflict footprints and large
derivable row-level deltas stay off the wire.
Library support is intentionally READ-side fail-closed only: a transaction
carrying a `UserOperation` is rejected on load with a clear "not supported"
error, and there is no write path, no `apply`, no translation, and no conflict
resolution yet. This keeps older writers safe (a concurrent V2 commit in the
conflict window aborts an in-flight commit rather than being silently skipped).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…tory Reorganize the action-based transaction (Transaction V2) wire draft into its own files under protos/transaction/ so the design reads clearly and the shared building blocks have a proper home: - protos/transaction/actions.proto: the V2 vocabulary (Ref, UserOperation, UserAction, Action + action messages), as top-level messages. The design rationale is summarized in an in-tree file header (deltas vs post-images, minting vs reference-stable, Ref/Local resolution, field-level schema, index segments, what stays off the wire) so it stands on its own. - protos/transaction/common.proto: UpdateMap/UpdateMapEntry and KeyExistenceFilter/ExactKeySetFilter/BloomFilter, promoted from nested Transaction messages to top-level so both the legacy operations and the V2 actions can reference them without a circular import. Wire-compatible: field numbers unchanged and none of these types are Any-packed, so the fully-qualified name change is invisible on the wire. - protos/transaction.proto moves to protos/transaction/transaction.proto and keeps only the Transaction envelope, legacy operations, and the user_operation oneof arm. Comments use block style for IDE folding. Rust references to the promoted types are repointed from pb::transaction::X to pb::X (mechanical, compiler-checked); the hand-written dataset::transaction domain types are unaffected. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Refinements to the Transaction V2 wire draft (protos/transaction/actions.proto), no behavior change: - AddFragment / SetDeletionFile: resolve a contradiction. AddFragment's doc implied a freshly-minted (Local) fragment could take a deletion file via SetDeletionFile, but SetDeletionFile.fragment is a committed-only uint64. Clarify that a new fragment has no deletion vector and deletions arrive in a later operation once the id is committed, and document why SetDeletionFile takes no Ref. - data_change: document the marker on every carrier (previously only on AddFragment), cross-referencing the canonical definition. Spell out its non-obvious meaning on AddIndexSegment / RemoveIndexSegment, where it refers to the indexed data rather than table rows. - AssertUniqueKeys: note that key_fields is authoritative and the embedded filter.field_ids (an artifact of the shared KeyExistenceFilter type) is ignored. - AdjustIndexCoverage: rename bare add / remove fields to add_fragments / remove_fragments for self-documentation and consistency with AddIndexSegment.covered_fragments. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Adds the first vertical slice of action-based transactions (Transaction V2): the in-memory vocabulary, an apply path, and footprint-based conflict resolution, with `AddBase` as the only action. `Operation::UserOperation` carries an ordered list of `UserAction` steps that expand to granular `Action` deltas and commit atomically. Minted ids travel as `Local` tokens and are allocated at apply, so two concurrent operations cannot collide on an id, and a retry simply re-mints from the newer manifest rather than needing rebase state. Conflicts are decided by intersecting `ManifestMask` footprints instead of matching operation pairs: each action and each legacy `Operation` declares the manifest regions it writes, and the intersection is computed once in `mask.rs`. Exact-key overlap (a taken base name or path) is incompatible; coarser overlap is retryable. `check_txn` answers a V2 `other` by footprint before any legacy dispatch, because several `check_*_txn` bodies mutate rebase state derived from the other operation's contents and would otherwise permit it silently while skipping that side effect. The existing `test_conflicts` table now doubles as an oracle for `Operation::writes()`: disjoint footprints must mean the table said `Compatible`, which catches an under-declared mask. `TryFrom<&Operation> for Vec<Action>` translates `UpdateBases` and backs a round-trip parity test asserting the legacy and action paths build identical manifests. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
wjones127
force-pushed
the
will/oss-1530-draft-action-based-transaction
branch
from
August 19, 2026 20:30
5422956 to
132c3b4
Compare
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 #7954, which drafts the wire format. This is the first working slice of action-based transactions (Transaction V2), implementing OSS-1542's
AddBaseand the machinery every later action reuses.Today a transaction is one
Operationdescribing one kind of change, and deciding whether two of them collide means consulting a pairwise matrix ofcheck_*_txnfunctions — one row and column per operation. That matrix is why compound commits and branch merges are hard: there is no way to say "this transaction does two things", and no way to combine two transactions that each minted an id.This PR adds a second, composable vocabulary alongside it.
Operation::UserOperationcarries an ordered list of steps that expand into granularActiondeltas and commit atomically. Newly allocated ids (base ids here, field and fragment ids later) travel asLocalplaceholder tokens and are assigned when the transaction is applied. That is what makes them safe under concurrency: each side allocates from whatever the counter holds at its own apply time, so two concurrent transactions can never pick the same id, and a retry just re-allocates against the newer manifest instead of needing rebase bookkeeping.Conflicts are decided by comparing footprints rather than operation pairs. Each action, and each existing
Operation, declares which regions of the manifest it writes (ManifestMask), and two transactions conflict exactly when those declarations intersect — computed once, in one place. Adding an action costs one declaration instead of a row and a column. Claiming a specific name (a base name or path, a config key) is an exclusive claim, so overlap there is incompatible; anything coarser is retryable, because a retry re-runs apply and apply re-validates.AddBaseis deliberately the smallest useful first action: it writes onlybase_paths, which is the one region where none of the existing rebase machinery needs to do extra work.Example
Two such transactions planned against the same version, adding different bases, both commit and receive distinct ids without any rebase step.
Notes for review
check_txnordering is the safety-critical part. A concurrent action-based transaction is answered by footprint before any legacy dispatch. Severalcheck_*_txnbodies do not merely decide — they also record state derived from the other transaction (fragments needing a deletion-file rewrite, conflicting frag-reuse indices, compacted SSTables) — and most end in a permissive arm, so one reaching them would be quietly allowed while the work it should have triggered never happened.REGIONS_WITH_REBASE_SIDE_EFFECTSgates the regions that still need those side effects; a later slice has to supply them before its region comes off the list. Every one of those legacy bodies also gained an explicit fail-safe arm, since Rust requires the match to be exhaustive and falling through to a permissive arm is exactly the failure being guarded against.The existing conflict table is now an oracle.
test_conflictsalready encodes the legacy verdict for every operation pair.Operation::writes()restates the same information as footprints, so the test now also asserts that disjoint footprints imply the table saidCompatible. Over-declaring passes (a spurious retry is harmless); an under-declared footprint, the direction that could let two colliding commits through, fails. As the plan predicted, one wrinkle is real: the legacy table is direction-sensitive (ReserveFragments ‖ Overwritediffers by direction) while intersection is symmetric, so the stricter direction wins. That is the acceptable failure mode, and the assertion still holds.Round-trip parity is the test that matters most.
TryFrom<&Operation> for Vec<Action>translatesUpdateBases, and a test drives the same change down both the legacy and the action path and compares the resulting manifests field by field. It is what would catch the action path quietly diverging.Deviations from the implementation plan
UserOperationon load, because that version had no support for it. This PR supports it, so the rejection now lives inAction's decode: an action this version does not recognize errors out rather than being skipped. That is the level that matters, since a leniently parsed action would vanish from conflict detection and let two colliding commits both succeed. The renamed test still covers it.UserOperation::build_manifesttakes the transaction'stag. The signature in the plan omitted it, but the plan also asks for the same scaffolding the legacy path applies, which includes the tag. Without the parameter every action-based commit would silently lose it.build_manifest, so the action path inherits that check rather than duplicating it.TxnContext::resolve_baseis not included. Nothing in this slice reads a base binding —AddDataFileis the first consumer — so it would be dead code.bind_baseand its distinctness check are here, as is a test-only accessor asserting the right id lands under the right token.Action::conflicts_withis not included. It would forward toself.writes().conflicts_with(&other.writes()); callers use the mask directly.name == nameunconditionally, so adding a second unnamed base to a dataset that already has one is wrongly rejected today. The legacy conflict check (check_add_bases_txn) already gets this right, so the action path agrees with it; the legacy apply path is untouched. There's a test for the corrected behavior.Rebase of #7954
Rebasing #7954 onto the refactor stack turned up real drift:
MergedGenerationwas renamed toCompactedSsTableupstream after that branch was cut, and #7954 recreatedtransaction.protofrom a pre-rename snapshot, so the protos no longer compiled. Fixed on #7954's branch (force-pushed, base retargeted torefactor/transaction-module-split), which also renames the draftUpdateMergedGenerationsaction toUpdateCompactedSsTablesso the action matches the type it carries. Two rustfmt fixes from the same rename went there too, so that branch is fmt-clean on its own.For Will
Two things from the plan I did not resolve on my own:
max_base_idinManifestortable.proto; the next id is derived from the current maximum key, and it only stays monotone because nothing in the tree ever removes a base. That is fine within a single lineage, which is all this PR does, but OSS-1529's relocation-by-watermark model assumes a never-reused counter, so it will need either a real counter field or a written-down never-remove invariant. Noted in a comment at the mint site. The irony is worth flagging: the mask design argues counters never conflict because they are monotone and never reused, which is exactly the property base ids lack on paper.Restoresilently drops concurrently-added bases.restore_old_manifestreplaces the manifest wholesale without mergingbase_pathsforward, yetcheck_add_bases_txnreturnsOkfor it — soUpdateBases ‖ Restoreloses the base today. Pre-existing; not fixed here. The action path givesRestorean everything-footprint, which makes the pairing retryable (re-adding on top of the restored state is correct).Not included
Every other action,
reads()(write-versus-write only for now; the plan records why, and the trap to avoid when read sets land), relocatingLocaltokens against a different target's counters for branch merge and squash, and Python/Java surfaces — the bindings just reject action-based operations.One test I could not write: the interception half of the frontier rule. Triggering it needs a concurrent action-based transaction whose footprint touches fragments, indices, or generations, and no action in this PR does. The gate's logic is covered at the
ManifestMasklevel, and the path it protects becomes reachable — and testable — with the first fragment-level action.Also worth stating plainly: nothing from #7954 upward should merge to
mainuntil the format vote (OSS-757) passes. The branch-level hold is the release gate, which is why there is no Cargo feature here.