Core: Enable row lineage for all v3 tables #12593

rdblue · 2025-03-20T23:00:14Z

This removes sections of #11948 that allow row lineage to be enabled or disabled. Instead, row lineage is always on for v3 tables.

Note: This is a work-in-progress and merging should wait until after the discussion about making v3 row lineage always-on concludes.

rdblue · 2025-03-20T23:02:09Z

core/src/main/java/org/apache/iceberg/TableMetadata.java

-      boolean rowLineageEnabled,
-      long nextRowId) {
+      long nextRowId,
+      List<MetadataUpdate> changes) {


Not that this PR also restores the convention that changes are passed last to the TableMetadata constructor. This is because changes are accumulated in TableMetadata.Builder and are not part of the metadata definition. I'd prefer not to mix changes into the actual table metadata.

core/src/test/java/org/apache/iceberg/TestRowLineageMetadata.java

core/src/test/java/org/apache/iceberg/TestTableMetadata.java

core/src/test/java/org/apache/iceberg/TestRowLineageMetadata.java

core/src/test/java/org/apache/iceberg/TestTableMetadata.java

core/src/main/java/org/apache/iceberg/util/JsonUtil.java

RussellSpitzer

Looks mostly correct to me except for the noted test removals which need to be pared back a bit.

rdblue · 2025-03-24T23:07:47Z

core/src/test/java/org/apache/iceberg/TestRowLineageMetadata.java

  @TestTemplate
-  public void testEnableRowLineageViaPropertyAtTableCreation() {
-    assumeThat(formatVersion).isGreaterThanOrEqualTo(TableMetadata.MIN_FORMAT_VERSION_ROW_LINEAGE);
+  public void testReplace() {


Updated this test because DataOperation.REPLACE row counts are now handled correctly.

Rolled this back. I'll fix it when I update how first-row-id gets assigned.

rdblue · 2025-03-24T23:09:03Z

core/src/test/java/org/apache/iceberg/TestRowLineageMetadata.java

  @TestTemplate
-  public void testReplace() {
-    assumeThat(formatVersion).isGreaterThanOrEqualTo(TableMetadata.MIN_FORMAT_VERSION_ROW_LINEAGE);
+  public void testPositionDeletes() {


@RussellSpitzer, I added tests for position and equality deletes. These were previously failing because the record counts from delete manifests were also used. Please take a look at the new test cases and the fix.

rdblue · 2025-03-24T23:09:37Z

core/src/test/java/org/apache/iceberg/TestRowLineageMetadata.java

-          .hasMessageContaining("Cannot use row lineage");
-    }
+  @Test
+  public void testSnapshotRowIDValidation() {


This checks the new validations in Snapshot.

rdblue · 2025-03-24T23:12:48Z

core/src/main/java/org/apache/iceberg/TableMetadata.java

-            "Cannot add a snapshot with a null 'added-rows' field when row lineage is enabled");
-        Preconditions.checkArgument(
-            snapshot.addedRows() >= 0,
-            "Cannot decrease 'last-row-id'. 'last-row-id' must increase monotonically. Snapshot reports %s added rows");


@RussellSpitzer, I moved these validations in to Snapshot rather than leaving them here. Now this only validates that first-row-id is set for any Snapshot in a v3 table and that the first-row-id is increasing. Both of those can't be checked by the Snapshot itself.

Also, I thought about keeping the checks here rather than moving them to Snapshot so that we can always read metadata, even when written incorrectly. However, when a snapshot is missing first-row-id, it isn't safe to read the snapshot so I think it is okay to fail when reading metadata rather than when using the metadata.

rdblue · 2025-03-24T23:15:13Z

core/src/main/java/org/apache/iceberg/SnapshotProducer.java

+      String operation, Map<String, String> summary, List<ManifestFile> manifests) {
+    if (summary != null) {
+      long addedRecords =
+          PropertyUtil.propertyAsLong(summary, SnapshotSummary.ADDED_RECORDS_PROP, 0L);


When the summary is defined, we trust the added-records and deleted-records values, which should match the counts calculated below.

I also added a check to handle the DataOperations.REPLACE case. For a replace, the added records should always be 0. If we want, we can also add checks for data file metadata that assert there are no null values for the _row_id column. What do you think @RussellSpitzer?

I've actually been thinking about this a bit more and I'm not sure we can do this. If Summary != null we can't assume that Added_Records_Prop is defined. I think if it is null we probably should still fall back to manifest inspection

We can trust the summary to be correct given that the operation produces it. However, I think it is best not to rely on this and to use actual rows assigned by the writers instead. I'll follow up with a fix.

For now, I've reverted these changes.

rdblue · 2025-03-24T23:15:50Z

core/src/main/java/org/apache/iceberg/BaseSnapshot.java

    this.v1ManifestLocations = null;
    this.firstRowId = firstRowId;
-    this.addedRows = addedRows;
+    this.addedRows = firstRowId != null ? addedRows : null;


To be slightly more permissive, I'm allowing addedRows to be passed, but it is only propagated if firstRowId is non-null.

core/src/main/java/org/apache/iceberg/SnapshotProducer.java

core/src/main/java/org/apache/iceberg/TableMetadata.java

RussellSpitzer

Have a few style comments but change looks good. Thanks for fixing the DeleteFile handling

RussellSpitzer · 2025-03-24T23:39:20Z

Pending Tests and Spec Change vote of course :)

amogh-jahagirdar

Thanks @rdblue , beyond the mailing list discussion, I think the remaining aspect is some of the assertions need to be restored , but looks like this is known!

core/src/main/java/org/apache/iceberg/SnapshotProducer.java

core/src/test/java/org/apache/iceberg/TestTableMetadata.java

core/src/main/java/org/apache/iceberg/SnapshotProducer.java

core/src/main/java/org/apache/iceberg/TableMetadataParser.java

core/src/test/java/org/apache/iceberg/TestRowLineageMetadata.java

nastra

the proposed changes LGTM. I actually thought that the Row Lineage stuff went into main and that we could just revert the metadata update API and the TableMetadata updates without breaking anything, but it looks like it went into 1.8.0 (1fcd6a9), so we'll need to address the RevAPI failures and also figure out how we want to deal with the removal of the EnableRowLineage metadata update

nastra · 2025-04-04T07:05:33Z

core/src/main/java/org/apache/iceberg/TableMetadata.java

+      }
+
+      // otherwise this is a no-op
+      return null;


I would have expected that this method would return this instead of null as otherwise library users using this method would now get a NPE

nastra

LGTM but I would have expected TableMetadata#enableRowLineage() to return this instead of null, which would potentially cause a NPE for library users already using this method

rdblue · 2025-04-07T16:53:09Z

Now that the vote has passed, I'll merge this. I'm following up in #12672 to add more core functionality so I'll fix the TableMetadata return value in that PR. Thanks for the reviews, @RussellSpitzer, @amogh-jahagirdar, and @nastra!

With apache/iceberg#12593 Row lineage is required and Spark uses Java 1.8.0 that does not write the required `start-row-id` field in the snapshot. Therefore, I think it would be good to just use the SNAPSHOT for now until it gets released.

With apache/iceberg#12593 Row lineage is required and Spark uses Java 1.8.0 that does not write the required `first-row-id` field in the snapshot. Therefore, I think it would be good to just use the SNAPSHOT for now until it gets released. We can easily revert the PR once that's done. Closes #1898    # Rationale for this change # Are these changes tested? # Are there any user-facing changes?

With apache/iceberg#12593 Row lineage is required and Spark uses Java 1.8.0 that does not write the required `first-row-id` field in the snapshot. Therefore, I think it would be good to just use the SNAPSHOT for now until it gets released. We can easily revert the PR once that's done. Closes apache#1898    # Rationale for this change # Are these changes tested? # Are there any user-facing changes?

Core: Enable row lineage for all v3 tables.

a66942b

github-actions bot added the core label Mar 20, 2025

rdblue commented Mar 20, 2025

View reviewed changes

core/src/test/java/org/apache/iceberg/TestRowLineageMetadata.java Show resolved Hide resolved

rdblue commented Mar 20, 2025

View reviewed changes

core/src/test/java/org/apache/iceberg/TestTableMetadata.java Show resolved Hide resolved