You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "tanmayrauth (via GitHub)" <gi...@apache.org> on 2026/05/05 23:45:33 UTC
[PR] feat(table): assign row IDs on write for v3 row lineage [iceberg-go]
tanmayrauth opened a new pull request, #1021:
URL: https://github.com/apache/iceberg-go/pull/1021
Assigns per-row _row_id during v3 appends using a pre-claimed range starting at next-row-id. Sets manifest entry first_row_id to the smallest assigned ID and advances metadata next-row-id by total row count at commit time. Mirrors Java's approach.
Closes: #998
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(table): assign row IDs on write for v3 row lineage [iceberg-go]
Posted by "tanmayrauth (via GitHub)" <gi...@apache.org>.
tanmayrauth closed pull request #1021: feat(table): assign row IDs on write for v3 row lineage
URL: https://github.com/apache/iceberg-go/pull/1021
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(table): assign row IDs on write for v3 row lineage [iceberg-go]
Posted by "tanmayrauth (via GitHub)" <gi...@apache.org>.
tanmayrauth commented on code in PR #1021:
URL: https://github.com/apache/iceberg-go/pull/1021#discussion_r3204693327
##########
table/snapshot_producers.go:
##########
@@ -733,7 +733,20 @@ func (sp *snapshotProducer) manifestProducer(content iceberg.ManifestContent, fi
}
defer internal.CheckedClose(wr, &err)
+ // For v3 data manifests, assign first_row_id to each data file.
+ // Each file claims a contiguous range of row IDs starting from NextRowID.
+ assignRowIDs := sp.txn.meta.formatVersion >= 3 && content == iceberg.ManifestContentData
+ nextRowID := sp.txn.meta.NextRowID()
Review Comment:
Yeah, this was a real correctness issue. The manifest-list rebuild on retry intentionally treats data manifests as immutable, and baking in per-file IDs at producer-build time breaks that invariant. On contention, stale IDs would get rejected by validateAndUpdateRowLineage deterministically, making retries useless.
##########
table/snapshot_producers.go:
##########
@@ -733,7 +733,20 @@ func (sp *snapshotProducer) manifestProducer(content iceberg.ManifestContent, fi
}
defer internal.CheckedClose(wr, &err)
+ // For v3 data manifests, assign first_row_id to each data file.
+ // Each file claims a contiguous range of row IDs starting from NextRowID.
+ assignRowIDs := sp.txn.meta.formatVersion >= 3 && content == iceberg.ManifestContentData
Review Comment:
You're correct, and I should have read the spec more carefully here. I went back and re-read the "First Row ID Inheritance" section - ADDED files carry null, manifest-list writer assigns the manifest-level value, readers derive per-file from record_count sums. We already implement both halves of that (list writer at manifest.go:1508, reader at manifest.go:785). So this was just duplicating existing machinery while also creating a second source of truth that would break Java replay.
##########
manifest.go:
##########
@@ -2187,6 +2187,19 @@ func (b *DataFileBuilder) FirstRowID(id int64) *DataFileBuilder {
return b
}
+// SetDataFileFirstRowID sets the first_row_id on an existing DataFile.
+// This is used by the snapshot producer to assign row IDs at write time for v3 tables.
+// Returns false if the DataFile implementation does not support mutation.
+func SetDataFileFirstRowID(df DataFile, id int64) bool {
Review Comment:
Yeah, you're right - this was a hack. Type-asserting to the unexported concrete type and silently returning false is not a contract anyone should have to reason about. I've removed it entirely in the latest push since the per-file assignment approach is gone now.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(table): assign row IDs on write for v3 row lineage [iceberg-go]
Posted by "laskoviymishka (via GitHub)" <gi...@apache.org>.
laskoviymishka commented on code in PR #1021:
URL: https://github.com/apache/iceberg-go/pull/1021#discussion_r3199996281
##########
manifest.go:
##########
@@ -2187,6 +2187,19 @@ func (b *DataFileBuilder) FirstRowID(id int64) *DataFileBuilder {
return b
}
+// SetDataFileFirstRowID sets the first_row_id on an existing DataFile.
+// This is used by the snapshot producer to assign row IDs at write time for v3 tables.
+// Returns false if the DataFile implementation does not support mutation.
+func SetDataFileFirstRowID(df DataFile, id int64) bool {
Review Comment:
I'd reconsider exposing this as a top-level mutator. The function type-asserts to the unexported `*dataFile`, returns `false` silently for any other implementation, and aliases the caller's pointer — so a `DataFile` the caller still holds gets its `FirstRowIDField` mutated as a side effect. That's a surprising contract for a public API, and the `bool` return treats what is really an invariant (`*dataFile` is the only impl in this package) as a soft optional.
Could we either fold this into `DataFileBuilder` (build a fresh `*dataFile` with `FirstRowID` set, no in-place mutation) or keep the helper unexported within the package? wdyt?
##########
table/snapshot_producers.go:
##########
@@ -733,7 +733,20 @@ func (sp *snapshotProducer) manifestProducer(content iceberg.ManifestContent, fi
}
defer internal.CheckedClose(wr, &err)
+ // For v3 data manifests, assign first_row_id to each data file.
+ // Each file claims a contiguous range of row IDs starting from NextRowID.
+ assignRowIDs := sp.txn.meta.formatVersion >= 3 && content == iceberg.ManifestContentData
+ nextRowID := sp.txn.meta.NextRowID()
Review Comment:
Even if we keep the per-file assignment, this read happens once at producer-build time and the values get baked into the data-manifest avro. On OCC retry, `rebuildSnapshotUpdates` (`table/table.go`) only rewrites the manifest *list*, not the data manifest entries — so under contention the per-file IDs stay at the stale `NextRowID` from the first attempt while metadata's `NextRowID` has moved.
`validateAndUpdateRowLineage` (`table/metadata.go:474-478`) would then reject the retry deterministically. The recently-landed manifest-list rebuild was designed assuming data manifests can be reused as-is on retry; pre-assigning IDs breaks that invariant.
Worth a v3 OCC retry test either way — the existing `occ_scenario_test.go` patterns should fit. Could we either move the assignment to commit time (after retries settle) or scrap pre-assignment in favor of inheritance?
##########
table/snapshot_producers.go:
##########
@@ -733,7 +733,20 @@ func (sp *snapshotProducer) manifestProducer(content iceberg.ManifestContent, fi
}
defer internal.CheckedClose(wr, &err)
+ // For v3 data manifests, assign first_row_id to each data file.
+ // Each file claims a contiguous range of row IDs starting from NextRowID.
+ assignRowIDs := sp.txn.meta.formatVersion >= 3 && content == iceberg.ManifestContentData
Review Comment:
I think the bigger question is whether we should be assigning per-file `first_row_id` at all on ADDED entries. The v3 spec ("First Row ID Inheritance") says ADDED data files carry null `first_row_id`; the manifest-list writer assigns the manifest-level value, and readers inherit per-file IDs by summing preceding `record_count`s. Java's `MergingSnapshotProducer.appendManifest` explicitly rejects inputs where `manifest.firstRowId() != null`.
iceberg-go already implements both halves of the inheritance model — list writer at `manifest.go:1419-1425`, reader at `manifest.go:734-742`. After this PR, manifests we write would carry non-null per-file IDs that Java's `appendManifest` would refuse to replay, and the per-file values become a second source of truth alongside the manifest-level one.
Is there a reason we're diverging from inheritance here? Or could we drop the per-file assignment and rely on what's already wired up?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org