Skip to content

feat(transaction): add the AddBase action and the conflict substrate - #8058

Closed
wjones127 wants to merge 20 commits into
will/oss-1530-draft-action-based-transactionfrom
will/oss-1542-add-base-action
Closed

wjones127 wants to merge 20 commits into
will/oss-1530-draft-action-based-transactionfrom
will/oss-1542-add-base-action

Conversation

@wjones127

Copy link
Copy Markdown
Contributor

Stacked on #7954, which drafts the wire format. This is the first working slice of action-based transactions (Transaction V2), implementing OSS-1542's AddBase and the machinery every later action reuses.

Today a transaction is one Operation describing one kind of change, and deciding whether two of them collide means consulting a pairwise matrix of check_*_txn functions — 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::UserOperation carries an ordered list of steps that expand into granular Action deltas and commit atomically. Newly allocated ids (base ids here, field and fragment ids later) travel as Local placeholder 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.

AddBase is deliberately the smallest useful first action: it writes only base_paths, which is the one region where none of the existing rebase machinery needs to do extra work.

Example

let operation = Operation::UserOperation(UserOperation {
    description: "ALTER TABLE t ADD BASE".into(),
    uuid: Uuid::new_v4().to_string(),
    read_version: dataset.version().version,
    actions: vec![UserAction {
        description: "register base".into(),
        actions: vec![Action::AddBase(AddBase {
            local: 0,
            name: Some("warm".into()),
            is_dataset_root: false,
            path: "s3://bucket/warm".into(),
        })],
    }],
});

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_txn ordering is the safety-critical part. A concurrent action-based transaction is answered by footprint before any legacy dispatch. Several check_*_txn bodies 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_EFFECTS gates 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_conflicts already 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 said Compatible. 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 ‖ Overwrite differs 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> translates UpdateBases, 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

  • The fail-closed decode contract moved down one level. feat(transaction): action-based transaction wire format (Transaction V2) #7954 rejected any UserOperation on load, because that version had no support for it. This PR supports it, so the rejection now lives in Action'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_manifest takes the transaction's tag. 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.
  • The dispatch sits just after the stable-row-id guard, not at the very first line of build_manifest, so the action path inherits that check rather than duplicating it.
  • TxnContext::resolve_base is not included. Nothing in this slice reads a base binding — AddDataFile is the first consumer — so it would be dead code. bind_base and its distinctness check are here, as is a test-only accessor asserting the right id lands under the right token.
  • Action::conflicts_with is not included. It would forward to self.writes().conflicts_with(&other.writes()); callers use the mask directly.
  • Unnamed bases no longer collide. The action path compares names only when a name is actually set. The legacy apply path compares name == name unconditionally, 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: MergedGeneration was renamed to CompactedSsTable upstream after that branch was cut, and #7954 recreated transaction.proto from a pre-rename snapshot, so the protos no longer compiled. Fixed on #7954's branch (force-pushed, base retargeted to refactor/transaction-module-split), which also renames the draft UpdateMergedGenerations action to UpdateCompactedSsTables so 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:

  1. Base ids are not really a counter. There is no max_base_id in Manifest or table.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.
  2. Restore silently drops concurrently-added bases. restore_old_manifest replaces the manifest wholesale without merging base_paths forward, yet check_add_bases_txn returns Ok for it — so UpdateBases ‖ Restore loses the base today. Pre-existing; not fixed here. The action path gives Restore an 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), relocating Local tokens 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 ManifestMask level, 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 main until the format vote (OSS-757) passes. The branch-level hold is the release gate, which is why there is no Cargo feature here.

wjones127 and others added 20 commits July 28, 2026 12:38
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>
@github-actions github-actions Bot added enhancement New feature or request A-python Python bindings labels Jul 28, 2026
@wjones127
wjones127 force-pushed the will/oss-1530-draft-action-based-transaction branch from 5422956 to 132c3b4 Compare August 19, 2026 20:30
@wjones127 wjones127 closed this Aug 21, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-python Python bindings enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant