pvary commented on code in PR #12366:
URL: https://github.com/apache/iceberg/pull/12366#discussion_r1971909256
##########
data/src/main/java/org/apache/iceberg/data/GenericAppenderFactory.java:
##########
@@ -66,19 +69,66 @@ public GenericAppenderFactory(
int[] equalityFieldIds,
Schema eqDeleteRowSchema,
Schema posDeleteRowSchema) {
- this.schema = schema;
- this.spec = spec;
+ this(null, schema, spec, null, equalityFieldIds, eqDeleteRowSchema,
posDeleteRowSchema);
+ }
+
+ /**
+ * 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,
+ PartitionSpec spec,
+ Map<String, String> config,
+ int[] equalityFieldIds,
+ Schema eqDeleteRowSchema,
+ Schema posDeleteRowSchema) {
+ this.table = table;
+ this.config = config == null ? Maps.newHashMap() : config;
+
+ 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.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 when the table is
provided. Use table properties instead.",
+ property));
+ }
+
config.put(property, value);
return this;
}
public GenericAppenderFactory setAll(Map<String, String> 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.");
+ }
Review Comment:
Maybe do this check only if the table is set
##########
data/src/main/java/org/apache/iceberg/data/GenericAppenderFactory.java:
##########
@@ -66,19 +69,66 @@ public GenericAppenderFactory(
int[] equalityFieldIds,
Schema eqDeleteRowSchema,
Schema posDeleteRowSchema) {
- this.schema = schema;
- this.spec = spec;
+ this(null, schema, spec, null, equalityFieldIds, eqDeleteRowSchema,
posDeleteRowSchema);
+ }
+
+ /**
+ * 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,
+ PartitionSpec spec,
+ Map<String, String> config,
+ int[] equalityFieldIds,
+ Schema eqDeleteRowSchema,
+ Schema posDeleteRowSchema) {
+ this.table = table;
+ this.config = config == null ? Maps.newHashMap() : config;
+
+ 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.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 when the table is
provided. Use table properties instead.",
+ property));
+ }
Review Comment:
This is the same as we check in the `validateMetricsConfig`. Can we reuse
the code somehow?
--
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-unsubscr...@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org
For additional commands, e-mail: issues-h...@iceberg.apache.org
