fix(table): update spec reading stale metadata in chained transactions

[badalprasadsingh]

Description

Fixes #1642

Now, UpdateSpec reads table schema and metadata from the transaction's staged metadata snapshot (captured at construction) - similar to what we have in PyIceberg and Iceberg-Java.

This unblocks fundamental workflows such as adding a column and partitioning by it within a single transaction.

Testing

Added the necessary tests for it.

[zeroshade]

Thanks for making UpdateSpec read the staged schema and partition state; that correctly enables partitioning by a column added or renamed earlier in the same transaction.

The commit requirements are now inconsistent across chained spec updates. At table/transaction.go:170-240, requirement deduplication uses the full JSON for AssertLastAssignedPartitionID, so the original and staged values are considered distinct and both reach the final commit. They are mutually contradictory assertions against one catalog snapshot, and retry only rewrites ref-snapshot assertions, so retry cannot repair this. Suggested fix: build this concurrency requirement from us.txn.tbl.Metadata() (the original snapshot), or deduplicate this singleton assertion by type while preserving the first/base value.

This overlaps with #1641 in table/update_spec.go: staged metadata is correct for historical lookup, but the original catalog snapshot is required for concurrency assertions. Suggested fix: keep those responsibilities on separate metadata sources. The chained-update test also needs to exercise a real catalog commit and retry behavior, as noted inline.

[zeroshade]

On the second staged spec update, us.meta contains the first update's last partition-field ID, while the first update already staged a requirement for the original value. Both requirements survive deduplication and are validated against the same catalog snapshot, so they contradict each other and cannot be repaired by retry. Suggested fix: derive this requirement from us.txn.tbl.Metadata() so every staged update asserts the same original/base value, or make this a singleton requirement that retains the first value.

[zeroshade]

This stops at StagedTable(), before the contradictory requirements are sent to the catalog. Suggested fix: follow the two staged updates with txn.Commit(ctx), assert that both fields are committed with exactly one base last-assigned-partition-id requirement, and cover the retry path as well.

[laskoviymishka]

Thanks for this. Reading UpdateSpec from the staged transaction metadata is the right fix, and the new tests around partitioning by a freshly-added or renamed column are a nice touch. I'd hold it before merging though.

I agree with the concurrency-requirement concern zeroshade already raised: now that AssertLastAssignedPartitionID is sourced from staged metadata, a second chained UpdateSpec.Commit() asserts a last-partition-id the first call already advanced, and both contradictory requirements reach the catalog. I'd fix it the way zeroshade suggested, building that one requirement from the original us.txn.tbl.Metadata() snapshot while keeping staged metadata for field resolution.

Two things beyond that thread that I'd want addressed. First, that same line dereferences us.meta.LastPartitionSpecID() (a *int) with no nil guard, even though the constructor guards exactly this case a few lines up, so it's a latent panic on the builder path. Second, the spec-dedup loop and isNewPartitionSpec() now read specs from staged metadata too, so a spec that's net-new relative to the committed catalog can look already-present and we'd drop the AddPartitionSpecUpdate that has to be sent.

And the tests: the chained-update case asserts against StagedTable() but never commits to a catalog, so the contradictory-requirements bug is invisible; it'd pass whether or not the fix is correct. A real commit plus a check that only one assert-last-assigned-partition-id survives would guard this properly.

A couple of smaller things I left inline (the construction-time snapshot semantics on the new meta field, a hard-coded field id in the tests).

Once the requirement baseline, the nil guard, and the test are sorted, happy to take another pass.

[laskoviymishka]

This is the root of the conflict zeroshade flagged. Sourcing the requirement from us.meta means a second chained UpdateSpec.Commit() asserts the staged last-partition-id that the first call already advanced, so we emit two AssertLastAssignedPartitionID requirements with different values and both survive dedup and reach the catalog.

The concurrency baseline has to be the value the catalog actually holds, so I'd read this one from us.txn.tbl.Metadata().LastPartitionSpecID() (the pre-transaction snapshot) even though everything else in here correctly moved to us.meta. Staged state is right for field resolution, wrong for the assertion.

[laskoviymishka]

Separate from the baseline question: LastPartitionSpecID() returns *int and we dereference it unconditionally on the next line. The constructor guards exactly this case a few lines up (if lastAssignedFieldId == nil falling back to PartitionDataIDStart - 1), and the MetadataBuilder path can hand us a nil last-partition-id, so this is a latent panic. I'd mirror the constructor's guard here before the deref.

[laskoviymishka]

This subtest stages two specs and asserts against StagedTable(), but it never calls txn.Commit(ctx), so the contradictory-requirements bug is invisible; the test passes whether or not the requirement baseline is correct.

Same ask as zeroshade's inline: I'd add a real catalog commit (in-memory catalog is fine) after the two UpdateSpec.Commit() calls, assert it succeeds, and assert exactly one assert-last-assigned-partition-id survives with the original value. That's the assertion that actually guards this fix.

[laskoviymishka]

This dedup loop and isNewPartitionSpec() (line 454) now read partition specs from the staged us.meta. After the first chained Commit(), a spec that's genuinely new relative to the committed catalog can already appear in us.meta, so isNewPartitionSpec returns false for a net-new spec and we'd suppress the AddPartitionSpecUpdate that has to reach the catalog, or select the wrong newSpecId.

Field resolution should keep using us.meta, but these existing-spec lookups want the committed snapshot. wdyt?

[laskoviymishka]

One design thing on the new meta field: it's snapshotted once in NewUpdateSpec via meta.Build() and then frozen, but BuildUpdates()/Apply() run lazily later. If anything else stages a change on the transaction between constructing this UpdateSpec and committing it, the snapshot is stale, and there's no doc on the field saying it intentionally freezes at construction.

I'd consider building the snapshot at the start of BuildUpdates() instead, so it reflects transaction state at apply time the way transaction.apply() does. If we keep it at construction, I'd at least document that on the field. wdyt?

[laskoviymishka]

The literal 8 here (and at line 325) leans on testNonPartitionedTable's schema occupying ids 1-7, so if that schema ever changes this silently returns an empty slice and fails on the Len with a confusing message. I'd resolve the id from the staged schema instead, e.g. f, _ := stagedTbl.Schema().FindFieldByName("new_col"); spec.FieldsBySourceID(f.ID).

[laskoviymishka]

Small thing: the So, opener reads a little awkwardly for a doc comment. The earlier single-sentence phrasing was cleaner, maybe ...captured when the transaction began, so that columns and specs added earlier in the same transaction are visible immediately.