JingsongLi commented on code in PR #9245:
URL: https://github.com/apache/paimon/pull/9245#discussion_r3802140083
##########
paimon-core/src/main/java/org/apache/paimon/iceberg/IcebergCommitCallback.java:
##########
@@ -2001,24 +2028,121 @@ private boolean isSameFormatVersion(int
baseFormatVersion) {
/**
* Row-lineage bookkeeping for a new snapshot, mandatory in Iceberg format
version 3: the
- * snapshot's first-row-id starts at the base metadata's next-row-id
watermark and the table's
- * next-row-id advances by the snapshot's added records. For format
version 2 all fields stay
- * null so nothing is written.
+ * snapshot's first-row-id starts at the base metadata's next-row-id
watermark. The snapshot's
+ * added-rows and the table's next-row-id are NOT derived here: they
depend on how many rows
+ * {@link #assignManifestFirstRowIds} actually assigns (which can exceed
this commit's added
+ * records when a carried-over manifest is assigned for the first time,
e.g. a Layer-1-written
+ * manifest being upgraded), so callers must recompute them from the
assignment's result. For
+ * format version 2 the field stays null so nothing is written.
+ */
+ @Nullable
+ private Long computeSnapshotFirstRowId(long baseNextRowId) {
+ return formatVersion >= IcebergMetadata.FORMAT_VERSION_V3 ?
baseNextRowId : null;
+ }
+
+ /**
+ * Result of {@link #assignManifestFirstRowIds}: the manifests with
first_row_id assigned, and
+ * the total number of rows actually consumed from the row-id space by
that assignment (which
+ * may be larger than this commit's added-records count; see the
class-level note there).
+ */
+ private static class ManifestRowIdAssignment {
+ private final List<IcebergManifestFileMeta> manifests;
+ private final long assignedRows;
+
+ private ManifestRowIdAssignment(
+ List<IcebergManifestFileMeta> manifests, long assignedRows) {
+ this.manifests = manifests;
+ this.assignedRows = assignedRows;
+ }
+ }
+
+ /**
+ * Iceberg v3: assign first_row_id (field 520) to data manifests that do
not have one yet.
+ * Manifests carried over from base metadata that are already assigned
keep their value; delete
+ * manifests stay null. The watermark starts at the snapshot's
first-row-id and advances by each
+ * newly-assigned manifest's TRUE inheriting-rows count (see {@link
#trueInheritingRowsCount}),
+ * returned as {@link ManifestRowIdAssignment#assignedRows}.
+ *
+ * <p>A manifest written entirely by Layer 2 (this commit or a later one)
satisfies "null-142
+ * rows == ADDED rows", so {@code addedRowsCount()} is exact for it. But a
manifest carried over
+ * from before manifest-level assignment existed (a "Layer-1" manifest)
may reach here
+ * unassigned with existing/deleted entries whose per-file field 142 is
also still null; for
+ * those, {@code addedRowsCount()} alone would undercount the rows this
assignment must cover,
+ * silently shrinking the range handed out and colliding with the next
commit's ids. Callers
+ * MUST use {@code assignedRows} (not this commit's added-records count)
to advance the
+ * snapshot's added-rows / table next-row-id, precisely because of that
mismatch.
+ */
+ private ManifestRowIdAssignment assignManifestFirstRowIds(
+ List<IcebergManifestFileMeta> manifests, @Nullable Long
snapshotFirstRowId) {
+ if (snapshotFirstRowId == null) {
+ return new ManifestRowIdAssignment(manifests, 0L);
+ }
+ List<IcebergManifestFileMeta> result = new ArrayList<>();
+ long watermark = snapshotFirstRowId;
+ for (IcebergManifestFileMeta meta : manifests) {
+ if (meta.content() == IcebergManifestFileMeta.Content.DATA
+ && meta.firstRowId() == null) {
+ result.add(meta.withFirstRowId(watermark));
Review Comment:
Thanks for clarifying that this is intentional. That makes the current PR
claim of completing writer-side v3 compliance with stable row IDs inaccurate,
but it does not remove the correctness issue. A pure COMPACT is an Iceberg
replace operation: unchanged rows moved to replacement files must retain both
lineage values. This path emits replacement files as ADDED with field 142 unset
and the current sequence numbers, so those values change. Iceberg requires
existing rows moved for any reason to copy _row_id, while unmodified rows
retain _last_updated_sequence_number
(https://iceberg.apache.org/spec/#row-lineage). Please either preserve both
values and add a GA before/after test per logical row, or fail/keep v3
publication explicitly unsupported for data-rewrite operations. Making the
synthetic behavior another opt-in would still not make the resulting v3 table
compliant.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
