Hi Steve,

Glad this was useful. It also looks like quite a bit of this is already
covered on the Rust side.

On prepare_commit: I think consuming the transaction is the right default.
Rust gets a useful guarantee here that we had to build manually in Go.

In Go, TableCommit() only exposes the commit payload. Nothing prevents a
caller from adding the transaction to a batch and then calling Commit() on
the same transaction as well. We ended up adding a committed flag and an
explicit MarkCommitted() call from the multi-table wrapper after the batch
succeeds.

Rust ownership prevents that at compile time, so I would keep it that way.

It also fits the error model. A 5xx means the commit state is unknown: it
may have applied, or it may not have. Retrying the same transaction is
therefore unsafe. Consuming it makes it harder for callers to accidentally
rebuild and resubmit from the same object.

The only thing I would check is whether callers can still build several
transactions first and then submit them as one batch. You already confirmed
that works, so I do not see a problem with the API shape.

Everything below is in apache/iceberg-go on main.

*Multi-table commit*

   -

   #787 - adds the TransactionalCatalog interface and REST implementation.
   TableCommit() and MarkCommitted() are in table/commit.go.
   CommitTransaction() is in catalog/rest/rest.go. The status handling is
   inline there: 409 maps to retryable ErrCommitFailed, while the relevant
   5xx responses map to ErrCommitStateUnknown.
   -

   #817 - adds CommitAndReload() in catalog/multi_table_transaction.go,
   including the reload after a successful 204 and the 404 handling.

*Partition-scoped conflict detection*

   -

   #983 by Masa - replaces AlwaysTrue with partition-scoped conflict
   detection. validateNoConflictingDataFilesInPartitions() in
   table/conflict_validation.go is the entry point.
   eqDeletePartitionsToFilter() turns the equality-delete partition tuples
   into an OR-of-ANDs expression, and validateAddedDataFilesMatchingFilter()
   handles per-spec projection and manifest-summary pruning. It is called from
   RowDelta.validate() in table/row_delta.go. Unpartitioned tables still
   fall back to AlwaysTrue.
   -

   #1428 by Tobias Pütz - adds the same partition scoping for
   position-delete rewrites.
   -

   #1528 by Minh Vu - fixes comparison of binary and fixed partition
   values, which previously caused the filter to over- or under-match.

The last two were follow-up fixes after the original change merged. The
part I would pay particular attention to is spec evolution: each concurrent
manifest needs to be projected using its own spec ID.

*Dangling equality deletes after compaction*

   -

   #947 - DecideDeadEqualityDeletes() in
   table/compaction/eq_delete_decision.go contains the predicate. An
   equality delete is dead when no surviving data file can still be affected
   by it. The rule is: the delete sequence number is greater than the data
   sequence number, and the partitions match, unless either side is
   unpartitioned.
   -

   CollectDeadEqualityDeletes() in eq_delete_collect.go does two manifest
   passes: first collecting candidate delete files, then checking surviving
   data files while excluding the files being rewritten.
   -

   The result is passed through
   RewriteDataFilesOptions.ExtraDeleteFilesToRemove in
   table/rewrite_data_files.go.
   -

   RewriteResult.RemovedEqualityDeleteFiles reports how many were removed.

One detail we were careful about was not including spec ID in the
applies-to predicate. That predicate needs to match reader behavior, rather
than the writer’s bookkeeping.

Thanks,
Andrei

On Fri, Jul 10, 2026 at 5:55 PM <[email protected]> wrote:

> Hi, Andrei.
>
>
>
> Many thanks for your review, extremely useful! I pushed code updates for
> three of your points:
>
>
>
>    1. Content-based op selection (you were right, we had overwrite
>    hardcoded — and it was ~15 lines)
>
>
>
>    2. Write-time rejection of empty/unknown/float-double equality keys,
>    and an explicit note on the action that BaseRowDelta's conflict-validation
>    surface is deferred.
>
>
>
>    3. On #2786, we documented the 204-reload contract and the 409-vs-5xx
>    atomicity split, and gave 503 an explicit commit-state-unknown arm. Your
>    "retrying those blindly double-applies" line is now paraphrased in the doc
>    comment.
>
>
>
> Some of your feedback could be happily checked off as done. The
> strict-greater sequence rule was already in the Rust reader, and the PR's
> round-trip test (test_row_delta_write_and_read_back) turns out to exercise
> your delete-and-reinsert-same-PK case specifically, through the scan path
> with equality deletes applied. We hit the same worry about position-only
> tests hiding stubbed equality reads. equality_ids is int on our side, and
> the null-key reader fix landed in the base branch just before our PR.
>
>
>
> One design divergence worth flagging: our prepare_commit consumes the
> transaction rather than inspecting it. Rust's ownership makes that the
> safer default — a prepared transaction can't also be committed directly,
> and the build-N-then-batch pattern still works. Curious whether that caused
> you any friction in practice on the Go side.
>
>
>
> I'd take you up on the diffs — the partition-scoped conflict detection
> especially, since that's the piece we deferred, and the
> dangling-equality-deletes-after-compaction cleanup, which we'll need
> downstream. Which repo/branches should I look at?
>
>
>
> Best,
>
> Steve
>
>
>
> *From:* Andrei Tserakhau via dev <[email protected]>
> *Sent:* Thursday, July 9, 2026 1:10 PM
> *To:* [email protected]
> *Cc:* Andrei Tserakhau <[email protected]>
> *Subject:* Re: [DISCUSS] Row-delta commits and multi-table transactions
> in iceberg-rust ? patterns from a CDC producer
>
>
>
> Hi Steve,
>
> I built this same thing in iceberg-go last year, RowDelta commit plus the
> REST multi-table commit, so here's what bit us. Most of it showed up after
> the initial API merged, in the read path and the edge cases, so worth
> knowing
> up front.
>
> On the RowDelta PR (#2785):
>
> The op-type selection is basically the whole trick and it's cheap, maybe 15
> lines. Data only is append, deletes only is delete, both is overwrite. Same
> as Java's BaseRowDelta.
>
> The sequence-number rule is where correctness lives. Equality deletes apply
> only to data files with a strictly lower seq number (position deletes use
> >=,
> equality use >). That's what keeps the deletes in a RowDelta from killing
> the
> new rows added in the same commit. We test it directly: insert PK=3, then
> in
> one RowDelta delete PK=3 and re-insert it, scan should still see PK=3.
> Easy to
> get wrong and it fails silently.
>
> The commit side is small, ~250 lines. The read side is where the weeks go.
> We
> shipped the commit API well before the scanner could actually apply
> equality
> deletes (that's a hash-based anti-join, the hardest piece by far). That's
> fine
> for interop testing against Lakekeeper/Spark, but watch out: a round-trip
> test
> that only uses position deletes will pass while equality reads are still
> stubbed, so it hides the gap.
>
> Conflict validation you can defer, just say so in the comment. Flink skips
> most of it anyway since seq numbers carry correctness. It came back for us
> later as partition-scoped conflict detection, because the first cut used an
> always-true filter that serialized every concurrent writer.
>
> Things you'll hit next, all post-merge fixes for us:
>   - null keys in equality delete files (null vs absent in the key encoding)
>   - reject empty equality-key sets at write time
>   - reject float/double equality columns (NaN and +-0 make equality
> undefined)
>   - match deletes to data files by partition transform, not raw value
>   - drop dangling equality deletes after compaction or they leak forever
>   - equality_ids avro element type is int, not long (breaks cross-engine
> reads)
>
> On the multi-table commit PR (#2786):
>
> Keep it a separate optional trait, don't widen the base Catalog. Only REST
> implements it and callers type-assert.
>
> Split building the commit from committing it. We have a TableCommit() that
> pulls {identifier, requirements, updates} out of a transaction without
> consuming it, so you build N per-table txns and then batch the payloads.
> Your
> prepare_commit() looks like the same idea, just keep it inspect-only.
>
> Watch the 204: no refreshed metadata comes back and post-commit hooks can't
> run, so callers have to reload afterward. And the error mapping is the
> atomicity surface, 409 is commit-failed and retryable, but 500/502/503/504
> are
> commit-state-unknown since the commit may or may not have applied. Retrying
> those blindly double-applies.
>
> Happy to point at the specific diffs if any of this is useful.
>
> Thanks,
> Andrei
>
>
>
> On Wed, Jul 8, 2026 at 7:04 PM <[email protected]> wrote:
>
> Hi all,
>
> I'm building an open-source Postgres→Iceberg CDC pipeline on iceberg-rust,
> and two capabilities came up that felt broadly useful, so I've opened them
> as draft PRs to discuss:
>
>    1. RowDeltaAction (apache/iceberg-rust#2785
>    <https://github.com/apache/iceberg-rust/pull/2785>) — the commit-side
>    equivalent of Java's Table.newRowDelta(): data files + delete files
>    (equality/position) in one overwrite snapshot. The crate already reads
>    delete files; this adds the missing write/commit side. It relates to #2218
>    (it's the commit-side complement of a DeltaWriter) and #2186. Includes an
>    end-to-end merge-on-read round-trip test.
>    2. Multi-table transaction commits (apache/iceberg-rust#2786
>    <https://github.com/apache/iceberg-rust/pull/2786>, issue #2784) —
>    Transaction::prepare_commit() plus RestCatalog::commit_transaction(Vec)
>    driving the REST spec's /v1/{prefix}/transactions/commit. Our use case:
>    committing per-cadence row deltas to many tables plus a changelog table
>    atomically, validated against Lakekeeper.
>
>
>
> Both are drafts and I'm happy to adapt the APIs to wherever the
> merge-on-read work in #2186 is heading. Feedback very welcome.
>
>
>
> Thanks,
>
> Steve
>
>
>
>

Reply via email to