Skip to content

Core: Fix non-setting row-lineage from table properties on initial table creation #12307

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
RussellSpitzer merged 1 commit into from
Feb 18, 2025
Merged

Core: Fix non-setting row-lineage from table properties on initial table creation #12307

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions core/src/main/java/org/apache/iceberg/TableMetadata.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -133,6 +133,11 @@ static TableMetadata newTableMetadata(
int freshSortOrderId = sortOrder.isUnsorted() ? sortOrder.orderId() : INITIAL_SORT_ORDER_ID;
SortOrder freshSortOrder = freshSortOrder(freshSortOrderId, freshSchema, sortOrder);

// configure row lineage using table properties
Boolean rowLineage =
PropertyUtil.propertyAsBoolean(
properties, TableProperties.ROW_LINEAGE, DEFAULT_ROW_LINEAGE);

// Validate the metrics configuration. Note: we only do this on new tables to we don't
// break existing tables.
MetricsConfig.fromProperties(properties).validateReferencedColumns(schema);
Expand All @@ -146,6 +151,7 @@ static TableMetadata newTableMetadata(
.setDefaultSortOrder(freshSortOrder)
.setLocation(location)
.setProperties(properties)
.setRowLineage(rowLineage)
Copy link
Member

@RussellSpitzer RussellSpitzer Feb 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't want to set RowLineage to false if it is unset. So we have 2 options here

  1. Set it to false but only if the table is V3
  2. Set it to true only if it is true and leave it absent otherwise.

Copy link
Contributor Author

@tomtongue tomtongue Feb 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, thanks for checking. Let me fix this.

Copy link
Member

@RussellSpitzer RussellSpitzer Feb 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You actually don't have to do anything! I forgot I already did this in "setRowLineage". It won't set the field unless it's getting set to true.

Copy link
Contributor Author

@tomtongue tomtongue Feb 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@RussellSpitzer If you don't set row-lineage in the table properties, it's unset, I mean it's kept empty because setRowLineage can handle null argument such as:

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

Lines 1525 to 1528 in bcbbd03

private Builder setRowLineage(Boolean newRowLineage) {
if (newRowLineage == null) {
return this;
}

Copy link
Contributor Author

@tomtongue tomtongue Feb 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh yes, it is. Thanks for letting me know. So I believe this part should be fine.

.build();
}

Expand Down
15 changes: 15 additions & 0 deletions core/src/test/java/org/apache/iceberg/TestRowLineageMetadata.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@
import java.util.List;
import java.util.concurrent.atomic.AtomicInteger;
import org.apache.iceberg.exceptions.ValidationException;
import org.apache.iceberg.relocated.com.google.common.collect.ImmutableMap;
import org.apache.iceberg.relocated.com.google.common.primitives.Ints;
import org.apache.iceberg.types.Types;
import org.junit.jupiter.api.AfterEach;
Expand Down Expand Up @@ -306,6 +307,20 @@ public void testEnableRowLineageViaProperty() {
assertThat(table.ops().current().rowLineageEnabled()).isTrue();
}

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

TestTables.TestTable table =
TestTables.create(
tableDir,
"test",
TEST_SCHEMA,
ImmutableMap.of(TableProperties.ROW_LINEAGE, "true"),
formatVersion);
assertThat(table.ops().current().rowLineageEnabled()).isTrue();
}

private final AtomicInteger fileNum = new AtomicInteger(0);

private DataFile fileWithRows(long numRows) {
Expand Down
20 changes: 20 additions & 0 deletions core/src/test/java/org/apache/iceberg/TestTables.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,26 @@ public static TestTable create(
return create(temp, name, schema, spec, SortOrder.unsorted(), formatVersion);
}

public static TestTable create(
File temp, String name, Schema schema, Map<String, String> properties, int formatVersion) {
TestTableOperations ops = new TestTableOperations(name, temp);
if (ops.current() != null) {
throw new AlreadyExistsException("Table %s already exists at location: %s", name, temp);
}

ops.commit(
null,
newTableMetadata(
schema,
PartitionSpec.unpartitioned(),
SortOrder.unsorted(),
temp.toString(),
properties,
formatVersion));

return new TestTable(ops, name);
}

public static TestTable create(
File temp,
String name,
Expand Down