From 00f3e0269e7ffb091ee9f108b5add552bbf13e59 Mon Sep 17 00:00:00 2001 From: Xavier Bai Date: Sat, 22 Feb 2025 00:12:41 +0800 Subject: [PATCH 1/5] keep previous constructors --- .../iceberg/data/GenericAppenderFactory.java | 45 ++++++++++++++++--- 1 file changed, 39 insertions(+), 6 deletions(-) diff --git a/data/src/main/java/org/apache/iceberg/data/GenericAppenderFactory.java b/data/src/main/java/org/apache/iceberg/data/GenericAppenderFactory.java index abad5d261d51..d4c10885ce7c 100644 --- a/data/src/main/java/org/apache/iceberg/data/GenericAppenderFactory.java +++ b/data/src/main/java/org/apache/iceberg/data/GenericAppenderFactory.java @@ -26,6 +26,7 @@ import org.apache.iceberg.PartitionSpec; import org.apache.iceberg.Schema; import org.apache.iceberg.StructLike; +import org.apache.iceberg.Table; import org.apache.iceberg.avro.Avro; import org.apache.iceberg.data.avro.DataWriter; import org.apache.iceberg.data.orc.GenericOrcWriter; @@ -44,7 +45,7 @@ /** Factory to create a new {@link FileAppender} to write {@link Record}s. */ public class GenericAppenderFactory implements FileAppenderFactory { - + private final Table table; private final Schema schema; private final PartitionSpec spec; private final int[] equalityFieldIds; @@ -52,22 +53,48 @@ public class GenericAppenderFactory implements FileAppenderFactory { private final Schema posDeleteRowSchema; private final Map config = Maps.newHashMap(); + @Deprecated public GenericAppenderFactory(Schema schema) { - this(schema, PartitionSpec.unpartitioned(), null, null, null); + this(null, schema, PartitionSpec.unpartitioned(), null, null, null); } + @Deprecated public GenericAppenderFactory(Schema schema, PartitionSpec spec) { - this(schema, spec, null, null, null); + this(null, schema, spec, null, null, null); + } + + @Deprecated + public GenericAppenderFactory( + Schema schema, + PartitionSpec spec, + int[] equalityFieldIds, + Schema eqDeleteRowSchema, + Schema posDeleteRowSchema) { + this(null, schema, spec, equalityFieldIds, eqDeleteRowSchema, posDeleteRowSchema); + } + + public GenericAppenderFactory(Table table) { + this(table, null, null, null, null, null); } public GenericAppenderFactory( + Table table, Schema schema, PartitionSpec spec, int[] equalityFieldIds, Schema eqDeleteRowSchema, Schema posDeleteRowSchema) { - this.schema = schema; - this.spec = spec; + this.table = table; + if (table != null && schema == null) { + this.schema = table.schema(); + } else { + this.schema = schema; + } + if (table != null && spec == null) { + this.spec = table.spec(); + } else { + this.spec = spec; + } this.equalityFieldIds = equalityFieldIds; this.eqDeleteRowSchema = eqDeleteRowSchema; this.posDeleteRowSchema = posDeleteRowSchema; @@ -91,7 +118,13 @@ public FileAppender newAppender(OutputFile outputFile, FileFormat fileFo @Override public FileAppender newAppender( EncryptedOutputFile encryptedOutputFile, FileFormat fileFormat) { - MetricsConfig metricsConfig = MetricsConfig.fromProperties(config); + MetricsConfig metricsConfig; + if (table == null) { + metricsConfig = MetricsConfig.fromProperties(config); + } else { + metricsConfig = MetricsConfig.forTable(table); + } + try { switch (fileFormat) { case AVRO: From e90dad910f8240bf6303fa60eda4947583224678 Mon Sep 17 00:00:00 2001 From: Xavier Bai Date: Sat, 22 Feb 2025 00:33:31 +0800 Subject: [PATCH 2/5] update review --- .../iceberg/data/GenericAppenderFactory.java | 25 ++++++++++++------- 1 file changed, 16 insertions(+), 9 deletions(-) diff --git a/data/src/main/java/org/apache/iceberg/data/GenericAppenderFactory.java b/data/src/main/java/org/apache/iceberg/data/GenericAppenderFactory.java index d4c10885ce7c..d77a61a87d53 100644 --- a/data/src/main/java/org/apache/iceberg/data/GenericAppenderFactory.java +++ b/data/src/main/java/org/apache/iceberg/data/GenericAppenderFactory.java @@ -90,11 +90,13 @@ public GenericAppenderFactory( } else { this.schema = schema; } + if (table != null && spec == null) { this.spec = table.spec(); } else { this.spec = spec; } + this.equalityFieldIds = equalityFieldIds; this.eqDeleteRowSchema = eqDeleteRowSchema; this.posDeleteRowSchema = posDeleteRowSchema; @@ -118,12 +120,7 @@ public FileAppender newAppender(OutputFile outputFile, FileFormat fileFo @Override public FileAppender newAppender( EncryptedOutputFile encryptedOutputFile, FileFormat fileFormat) { - MetricsConfig metricsConfig; - if (table == null) { - metricsConfig = MetricsConfig.fromProperties(config); - } else { - metricsConfig = MetricsConfig.forTable(table); - } + MetricsConfig metricsConfig = metricsConfig(); try { switch (fileFormat) { @@ -184,8 +181,7 @@ public EqualityDeleteWriter newEqDeleteWriter( Preconditions.checkNotNull( eqDeleteRowSchema, "Equality delete row schema shouldn't be null when creating equality-delete writer"); - - MetricsConfig metricsConfig = MetricsConfig.fromProperties(config); + MetricsConfig metricsConfig = metricsConfig(); try { switch (format) { @@ -239,7 +235,7 @@ public EqualityDeleteWriter newEqDeleteWriter( @Override public PositionDeleteWriter newPosDeleteWriter( EncryptedOutputFile file, FileFormat format, StructLike partition) { - MetricsConfig metricsConfig = MetricsConfig.fromProperties(config); + MetricsConfig metricsConfig = metricsConfig(); try { switch (format) { @@ -285,4 +281,15 @@ public PositionDeleteWriter newPosDeleteWriter( throw new UncheckedIOException(e); } } + + private MetricsConfig metricsConfig() { + MetricsConfig metricsConfig; + if (table == null) { + metricsConfig = MetricsConfig.fromProperties(config); + } else { + metricsConfig = MetricsConfig.forTable(table); + } + + return metricsConfig; + } } From 27263c170e8d464adec8ba854725cb0615f4ec43 Mon Sep 17 00:00:00 2001 From: Xavier Bai Date: Sat, 22 Feb 2025 01:32:09 +0800 Subject: [PATCH 3/5] add tests --- .../iceberg/data/GenericAppenderFactory.java | 37 ++++++++--- .../iceberg/TestGenericAppenderFactory.java | 64 +++++++++++++++++++ 2 files changed, 91 insertions(+), 10 deletions(-) diff --git a/data/src/main/java/org/apache/iceberg/data/GenericAppenderFactory.java b/data/src/main/java/org/apache/iceberg/data/GenericAppenderFactory.java index d77a61a87d53..109ff93b2cdc 100644 --- a/data/src/main/java/org/apache/iceberg/data/GenericAppenderFactory.java +++ b/data/src/main/java/org/apache/iceberg/data/GenericAppenderFactory.java @@ -21,6 +21,7 @@ import java.io.IOException; import java.io.UncheckedIOException; import java.util.Map; +import java.util.function.Supplier; import org.apache.iceberg.FileFormat; import org.apache.iceberg.MetricsConfig; import org.apache.iceberg.PartitionSpec; @@ -51,16 +52,18 @@ public class GenericAppenderFactory implements FileAppenderFactory { private final int[] equalityFieldIds; private final Schema eqDeleteRowSchema; private final Schema posDeleteRowSchema; - private final Map config = Maps.newHashMap(); + private final Map config; + + private static final String WRITE_METRICS_PREFIX = "write.metadata.metrics."; @Deprecated public GenericAppenderFactory(Schema schema) { - this(null, schema, PartitionSpec.unpartitioned(), null, null, null); + this(schema, PartitionSpec.unpartitioned()); } @Deprecated public GenericAppenderFactory(Schema schema, PartitionSpec spec) { - this(null, schema, spec, null, null, null); + this(schema, spec, null, null, null); } @Deprecated @@ -70,17 +73,18 @@ public GenericAppenderFactory( int[] equalityFieldIds, Schema eqDeleteRowSchema, Schema posDeleteRowSchema) { - this(null, schema, spec, equalityFieldIds, eqDeleteRowSchema, posDeleteRowSchema); + this(null, schema, spec, null, equalityFieldIds, eqDeleteRowSchema, posDeleteRowSchema); } public GenericAppenderFactory(Table table) { - this(table, null, null, null, null, null); + this(table, null, null, null, null, null, null); } public GenericAppenderFactory( Table table, Schema schema, PartitionSpec spec, + Map config, int[] equalityFieldIds, Schema eqDeleteRowSchema, Schema posDeleteRowSchema) { @@ -97,17 +101,30 @@ public GenericAppenderFactory( this.spec = spec; } + this.config = config == null ? Maps.newHashMap() : config; this.equalityFieldIds = equalityFieldIds; this.eqDeleteRowSchema = eqDeleteRowSchema; this.posDeleteRowSchema = posDeleteRowSchema; } public GenericAppenderFactory set(String property, String value) { + if (property.startsWith(WRITE_METRICS_PREFIX) && table != null) { + throw new IllegalArgumentException( + String.format( + "Cannot set metrics property: %s directly. Use table properties instead.", property)); + } + config.put(property, value); return this; } public GenericAppenderFactory setAll(Map properties) { + if (properties.keySet().stream().anyMatch(k -> k.startsWith(WRITE_METRICS_PREFIX)) + && table != null) { + throw new IllegalArgumentException( + "Cannot set metrics properties directly. Use table properties instead."); + } + config.putAll(properties); return this; } @@ -120,7 +137,7 @@ public FileAppender newAppender(OutputFile outputFile, FileFormat fileFo @Override public FileAppender newAppender( EncryptedOutputFile encryptedOutputFile, FileFormat fileFormat) { - MetricsConfig metricsConfig = metricsConfig(); + MetricsConfig metricsConfig = applyMetricsConfig(() -> MetricsConfig.forTable(table)); try { switch (fileFormat) { @@ -181,7 +198,7 @@ public EqualityDeleteWriter newEqDeleteWriter( Preconditions.checkNotNull( eqDeleteRowSchema, "Equality delete row schema shouldn't be null when creating equality-delete writer"); - MetricsConfig metricsConfig = metricsConfig(); + MetricsConfig metricsConfig = applyMetricsConfig(() -> MetricsConfig.forTable(table)); try { switch (format) { @@ -235,7 +252,7 @@ public EqualityDeleteWriter newEqDeleteWriter( @Override public PositionDeleteWriter newPosDeleteWriter( EncryptedOutputFile file, FileFormat format, StructLike partition) { - MetricsConfig metricsConfig = metricsConfig(); + MetricsConfig metricsConfig = applyMetricsConfig(() -> MetricsConfig.forPositionDelete(table)); try { switch (format) { @@ -282,12 +299,12 @@ public PositionDeleteWriter newPosDeleteWriter( } } - private MetricsConfig metricsConfig() { + private MetricsConfig applyMetricsConfig(Supplier metricsConfigSupplier) { MetricsConfig metricsConfig; if (table == null) { metricsConfig = MetricsConfig.fromProperties(config); } else { - metricsConfig = MetricsConfig.forTable(table); + metricsConfig = metricsConfigSupplier.get(); } return metricsConfig; diff --git a/data/src/test/java/org/apache/iceberg/TestGenericAppenderFactory.java b/data/src/test/java/org/apache/iceberg/TestGenericAppenderFactory.java index 2e4a7b885903..707144deb238 100644 --- a/data/src/test/java/org/apache/iceberg/TestGenericAppenderFactory.java +++ b/data/src/test/java/org/apache/iceberg/TestGenericAppenderFactory.java @@ -18,15 +18,21 @@ */ package org.apache.iceberg; +import static org.assertj.core.api.Assertions.assertThatNoException; +import static org.assertj.core.api.Assertions.assertThatThrownBy; + import java.util.List; +import java.util.Map; import org.apache.iceberg.data.GenericAppenderFactory; import org.apache.iceberg.data.GenericRecord; import org.apache.iceberg.data.Record; import org.apache.iceberg.io.FileAppenderFactory; import org.apache.iceberg.io.TestAppenderFactory; import org.apache.iceberg.relocated.com.google.common.collect.ImmutableMap; +import org.apache.iceberg.relocated.com.google.common.collect.Maps; import org.apache.iceberg.util.ArrayUtil; import org.apache.iceberg.util.StructLikeSet; +import org.junit.jupiter.api.TestTemplate; public class TestGenericAppenderFactory extends TestAppenderFactory { @@ -36,8 +42,10 @@ public class TestGenericAppenderFactory extends TestAppenderFactory { protected FileAppenderFactory createAppenderFactory( List equalityFieldIds, Schema eqDeleteSchema, Schema posDeleteRowSchema) { return new GenericAppenderFactory( + table, table.schema(), table.spec(), + Maps.newHashMap(), ArrayUtil.toIntArray(equalityFieldIds), eqDeleteSchema, posDeleteRowSchema); @@ -54,4 +62,60 @@ protected StructLikeSet expectedRowSet(Iterable records) { records.forEach(set::add); return set; } + + @TestTemplate + void illegalSetConfig() { + GenericAppenderFactory appenderFactory = + (GenericAppenderFactory) createAppenderFactory(null, null, null); + + assertThatThrownBy( + () -> + appenderFactory.set( + TableProperties.METRICS_MAX_INFERRED_COLUMN_DEFAULTS, + MetricsModes.None.get().toString())) + .as("Should not allow setting metrics property if the table was provided") + .isInstanceOf(IllegalArgumentException.class) + .hasMessageContaining( + "Cannot set metrics property: " + TableProperties.METRICS_MAX_INFERRED_COLUMN_DEFAULTS); + } + + @TestTemplate + void illegalSetAllConfigs() { + GenericAppenderFactory appenderFactory = + (GenericAppenderFactory) createAppenderFactory(null, null, null); + + Map properties = + ImmutableMap.of( + TableProperties.METRICS_MAX_INFERRED_COLUMN_DEFAULTS, + "10", + TableProperties.METRICS_MODE_COLUMN_CONF_PREFIX + "id", + MetricsModes.Full.get().toString()); + + assertThatThrownBy(() -> appenderFactory.setAll(properties)) + .as("Should not allow setting metrics property if the table was provided") + .isInstanceOf(IllegalArgumentException.class) + .hasMessageContaining("Cannot set metrics properties directly"); + } + + @TestTemplate + void setConfigExcludeMetrics() { + GenericAppenderFactory appenderFactory = + (GenericAppenderFactory) createAppenderFactory(null, null, null); + assertThatNoException().isThrownBy(() -> appenderFactory.set("key1", "value1")); + assertThatNoException() + .isThrownBy(() -> appenderFactory.setAll(ImmutableMap.of("key2", "value2"))); + } + + @TestTemplate + void setConfigWithoutTable() { + GenericAppenderFactory appenderFactory = new GenericAppenderFactory(SCHEMA); + assertThatNoException() + .isThrownBy( + () -> appenderFactory.set(TableProperties.METRICS_MAX_INFERRED_COLUMN_DEFAULTS, "10")); + assertThatNoException() + .isThrownBy( + () -> + appenderFactory.setAll( + ImmutableMap.of(TableProperties.DEFAULT_WRITE_METRICS_MODE, "full"))); + } } From 4a3375a1bef66beeaf50448e261c9d5f8c2bdd3d Mon Sep 17 00:00:00 2001 From: Xavier Bai Date: Tue, 25 Feb 2025 13:33:22 +0800 Subject: [PATCH 4/5] fix review fix checkstyle --- .../iceberg/data/GenericAppenderFactory.java | 74 ++++++++++++------- .../iceberg/TestGenericAppenderFactory.java | 19 +++++ 2 files changed, 65 insertions(+), 28 deletions(-) diff --git a/data/src/main/java/org/apache/iceberg/data/GenericAppenderFactory.java b/data/src/main/java/org/apache/iceberg/data/GenericAppenderFactory.java index 109ff93b2cdc..82991cecea96 100644 --- a/data/src/main/java/org/apache/iceberg/data/GenericAppenderFactory.java +++ b/data/src/main/java/org/apache/iceberg/data/GenericAppenderFactory.java @@ -21,7 +21,6 @@ import java.io.IOException; import java.io.UncheckedIOException; import java.util.Map; -import java.util.function.Supplier; import org.apache.iceberg.FileFormat; import org.apache.iceberg.MetricsConfig; import org.apache.iceberg.PartitionSpec; @@ -56,17 +55,14 @@ public class GenericAppenderFactory implements FileAppenderFactory { private static final String WRITE_METRICS_PREFIX = "write.metadata.metrics."; - @Deprecated public GenericAppenderFactory(Schema schema) { this(schema, PartitionSpec.unpartitioned()); } - @Deprecated public GenericAppenderFactory(Schema schema, PartitionSpec spec) { this(schema, spec, null, null, null); } - @Deprecated public GenericAppenderFactory( Schema schema, PartitionSpec spec, @@ -76,10 +72,17 @@ public GenericAppenderFactory( this(null, schema, spec, null, equalityFieldIds, eqDeleteRowSchema, posDeleteRowSchema); } - public GenericAppenderFactory(Table table) { - this(table, null, null, null, null, null, null); - } - + /** + * Constructor for GenericAppenderFactory. + * + * @param table iceberg table + * @param schema the schema of the records to write + * @param spec the partition spec of the records + * @param config the configuration for the writer + * @param equalityFieldIds the field ids for equality delete + * @param eqDeleteRowSchema the schema for equality delete rows + * @param posDeleteRowSchema the schema for position delete rows + */ public GenericAppenderFactory( Table table, Schema schema, @@ -89,19 +92,19 @@ public GenericAppenderFactory( Schema eqDeleteRowSchema, Schema posDeleteRowSchema) { this.table = table; - if (table != null && schema == null) { - this.schema = table.schema(); - } else { - this.schema = schema; - } + this.config = config == null ? Maps.newHashMap() : config; - if (table != null && spec == null) { - this.spec = table.spec(); + if (table != null) { + // If the table is provided and schema and spec are not provided, derive them from the table + this.schema = schema == null ? table.schema() : schema; + this.spec = spec == null ? table.spec() : spec; + // Validate that the metrics config doesn't have conflict with table properties + validateMetricsConfig(table.properties()); } else { + this.schema = schema; this.spec = spec; } - this.config = config == null ? Maps.newHashMap() : config; this.equalityFieldIds = equalityFieldIds; this.eqDeleteRowSchema = eqDeleteRowSchema; this.posDeleteRowSchema = posDeleteRowSchema; @@ -111,7 +114,8 @@ public GenericAppenderFactory set(String property, String value) { if (property.startsWith(WRITE_METRICS_PREFIX) && table != null) { throw new IllegalArgumentException( String.format( - "Cannot set metrics property: %s directly. Use table properties instead.", property)); + "Cannot set metrics property: %s directly when the table is provided. Use table properties instead.", + property)); } config.put(property, value); @@ -122,7 +126,7 @@ public GenericAppenderFactory setAll(Map properties) { if (properties.keySet().stream().anyMatch(k -> k.startsWith(WRITE_METRICS_PREFIX)) && table != null) { throw new IllegalArgumentException( - "Cannot set metrics properties directly. Use table properties instead."); + "Cannot set metrics properties directly when the table is provided. Use table properties instead."); } config.putAll(properties); @@ -137,7 +141,8 @@ public FileAppender newAppender(OutputFile outputFile, FileFormat fileFo @Override public FileAppender newAppender( EncryptedOutputFile encryptedOutputFile, FileFormat fileFormat) { - MetricsConfig metricsConfig = applyMetricsConfig(() -> MetricsConfig.forTable(table)); + MetricsConfig metricsConfig = + table != null ? MetricsConfig.forTable(table) : MetricsConfig.fromProperties(config); try { switch (fileFormat) { @@ -198,7 +203,8 @@ public EqualityDeleteWriter newEqDeleteWriter( Preconditions.checkNotNull( eqDeleteRowSchema, "Equality delete row schema shouldn't be null when creating equality-delete writer"); - MetricsConfig metricsConfig = applyMetricsConfig(() -> MetricsConfig.forTable(table)); + MetricsConfig metricsConfig = + table != null ? MetricsConfig.forTable(table) : MetricsConfig.fromProperties(config); try { switch (format) { @@ -252,7 +258,10 @@ public EqualityDeleteWriter newEqDeleteWriter( @Override public PositionDeleteWriter newPosDeleteWriter( EncryptedOutputFile file, FileFormat format, StructLike partition) { - MetricsConfig metricsConfig = applyMetricsConfig(() -> MetricsConfig.forPositionDelete(table)); + MetricsConfig metricsConfig = + table != null + ? MetricsConfig.forPositionDelete(table) + : MetricsConfig.fromProperties(config); try { switch (format) { @@ -299,14 +308,23 @@ public PositionDeleteWriter newPosDeleteWriter( } } - private MetricsConfig applyMetricsConfig(Supplier metricsConfigSupplier) { - MetricsConfig metricsConfig; - if (table == null) { - metricsConfig = MetricsConfig.fromProperties(config); - } else { - metricsConfig = metricsConfigSupplier.get(); + private void validateMetricsConfig(Map properties) { + if (config.isEmpty()) { + return; } - return metricsConfig; + config.keySet().stream() + .filter(k -> k.startsWith(WRITE_METRICS_PREFIX)) + .forEach( + k -> { + String configValue = config.get(k); + String propertyValue = properties.get(k); + if (propertyValue != null && !propertyValue.equals(configValue)) { + throw new IllegalArgumentException( + String.format( + "Cannot set metrics property: %s to %s, as it conflicts with the table property value: %s", + k, configValue, propertyValue)); + } + }); } } diff --git a/data/src/test/java/org/apache/iceberg/TestGenericAppenderFactory.java b/data/src/test/java/org/apache/iceberg/TestGenericAppenderFactory.java index 707144deb238..832d6ccc353c 100644 --- a/data/src/test/java/org/apache/iceberg/TestGenericAppenderFactory.java +++ b/data/src/test/java/org/apache/iceberg/TestGenericAppenderFactory.java @@ -118,4 +118,23 @@ void setConfigWithoutTable() { appenderFactory.setAll( ImmutableMap.of(TableProperties.DEFAULT_WRITE_METRICS_MODE, "full"))); } + + @TestTemplate + void createFactoryWithConflictConfig() { + table + .updateProperties() + .set(TableProperties.DEFAULT_WRITE_METRICS_MODE, MetricsModes.Full.get().toString()) + .commit(); + Map config = + ImmutableMap.of( + TableProperties.DEFAULT_WRITE_METRICS_MODE, MetricsModes.None.get().toString()); + + assertThatThrownBy( + () -> new GenericAppenderFactory(table, SCHEMA, SPEC, config, null, null, null)) + .isInstanceOf(IllegalArgumentException.class) + .hasMessageContaining( + String.format( + "Cannot set metrics property: %s to %s, as it conflicts with the table property", + TableProperties.DEFAULT_WRITE_METRICS_MODE, MetricsModes.None.get().toString())); + } } From 16a7b3d66346cf06bfb71a209b78cc693b08678a Mon Sep 17 00:00:00 2001 From: Xavier Bai Date: Thu, 27 Feb 2025 11:01:00 +0800 Subject: [PATCH 5/5] update --- .../iceberg/data/GenericAppenderFactory.java | 42 +++++-------------- .../iceberg/TestGenericAppenderFactory.java | 9 ++-- 2 files changed, 14 insertions(+), 37 deletions(-) diff --git a/data/src/main/java/org/apache/iceberg/data/GenericAppenderFactory.java b/data/src/main/java/org/apache/iceberg/data/GenericAppenderFactory.java index 82991cecea96..53a02856c9d3 100644 --- a/data/src/main/java/org/apache/iceberg/data/GenericAppenderFactory.java +++ b/data/src/main/java/org/apache/iceberg/data/GenericAppenderFactory.java @@ -41,6 +41,7 @@ import org.apache.iceberg.orc.ORC; import org.apache.iceberg.parquet.Parquet; import org.apache.iceberg.relocated.com.google.common.base.Preconditions; +import org.apache.iceberg.relocated.com.google.common.collect.ImmutableMap; import org.apache.iceberg.relocated.com.google.common.collect.Maps; /** Factory to create a new {@link FileAppender} to write {@link Record}s. */ @@ -53,8 +54,6 @@ public class GenericAppenderFactory implements FileAppenderFactory { private final Schema posDeleteRowSchema; private final Map config; - private static final String WRITE_METRICS_PREFIX = "write.metadata.metrics."; - public GenericAppenderFactory(Schema schema) { this(schema, PartitionSpec.unpartitioned()); } @@ -98,8 +97,7 @@ public GenericAppenderFactory( // If the table is provided and schema and spec are not provided, derive them from the table this.schema = schema == null ? table.schema() : schema; this.spec = spec == null ? table.spec() : spec; - // Validate that the metrics config doesn't have conflict with table properties - validateMetricsConfig(table.properties()); + validateMetricsConfig(this.config); } else { this.schema = schema; this.spec = spec; @@ -111,24 +109,13 @@ public GenericAppenderFactory( } public GenericAppenderFactory set(String property, String value) { - if (property.startsWith(WRITE_METRICS_PREFIX) && table != null) { - throw new IllegalArgumentException( - String.format( - "Cannot set metrics property: %s directly when the table is provided. Use table properties instead.", - property)); - } - + validateMetricsConfig(ImmutableMap.of(property, value)); config.put(property, value); return this; } public GenericAppenderFactory setAll(Map properties) { - if (properties.keySet().stream().anyMatch(k -> k.startsWith(WRITE_METRICS_PREFIX)) - && table != null) { - throw new IllegalArgumentException( - "Cannot set metrics properties directly when the table is provided. Use table properties instead."); - } - + validateMetricsConfig(properties); config.putAll(properties); return this; } @@ -308,23 +295,14 @@ public PositionDeleteWriter newPosDeleteWriter( } } - private void validateMetricsConfig(Map properties) { - if (config.isEmpty()) { + private void validateMetricsConfig(Map writeConfig) { + if (table == null) { return; } - config.keySet().stream() - .filter(k -> k.startsWith(WRITE_METRICS_PREFIX)) - .forEach( - k -> { - String configValue = config.get(k); - String propertyValue = properties.get(k); - if (propertyValue != null && !propertyValue.equals(configValue)) { - throw new IllegalArgumentException( - String.format( - "Cannot set metrics property: %s to %s, as it conflicts with the table property value: %s", - k, configValue, propertyValue)); - } - }); + if (writeConfig.keySet().stream().anyMatch(k -> k.startsWith("write.metadata.metrics."))) { + throw new IllegalArgumentException( + "Cannot set metrics properties when the table is provided, use table properties instead"); + } } } diff --git a/data/src/test/java/org/apache/iceberg/TestGenericAppenderFactory.java b/data/src/test/java/org/apache/iceberg/TestGenericAppenderFactory.java index 832d6ccc353c..5d940adaec58 100644 --- a/data/src/test/java/org/apache/iceberg/TestGenericAppenderFactory.java +++ b/data/src/test/java/org/apache/iceberg/TestGenericAppenderFactory.java @@ -76,7 +76,7 @@ void illegalSetConfig() { .as("Should not allow setting metrics property if the table was provided") .isInstanceOf(IllegalArgumentException.class) .hasMessageContaining( - "Cannot set metrics property: " + TableProperties.METRICS_MAX_INFERRED_COLUMN_DEFAULTS); + "Cannot set metrics properties when the table is provided, use table properties instead"); } @TestTemplate @@ -94,7 +94,8 @@ void illegalSetAllConfigs() { assertThatThrownBy(() -> appenderFactory.setAll(properties)) .as("Should not allow setting metrics property if the table was provided") .isInstanceOf(IllegalArgumentException.class) - .hasMessageContaining("Cannot set metrics properties directly"); + .hasMessageContaining( + "Cannot set metrics properties when the table is provided, use table properties instead"); } @TestTemplate @@ -133,8 +134,6 @@ void createFactoryWithConflictConfig() { () -> new GenericAppenderFactory(table, SCHEMA, SPEC, config, null, null, null)) .isInstanceOf(IllegalArgumentException.class) .hasMessageContaining( - String.format( - "Cannot set metrics property: %s to %s, as it conflicts with the table property", - TableProperties.DEFAULT_WRITE_METRICS_MODE, MetricsModes.None.get().toString())); + "Cannot set metrics properties when the table is provided, use table properties instead"); } }