stevenzwu commented on code in PR #12298:
URL: https://github.com/apache/iceberg/pull/12298#discussion_r2382634064
##########
data/src/main/java/org/apache/iceberg/data/GenericAppenderFactory.java:
##########
@@ -71,6 +73,26 @@ public GenericAppenderFactory(
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
+ */
+ public GenericAppenderFactory(
+ Table table,
+ Schema schema,
+ PartitionSpec spec,
+ Map<String, String> config,
+ int[] equalityFieldIds,
+ Schema eqDeleteRowSchema) {
+ this(table, schema, spec, config, equalityFieldIds, eqDeleteRowSchema,
null);
Review Comment:
When the deprecated constructor is removed in 1.12.0, how would this
constructor work? we will need to copy the code here?
##########
spark/v3.5/spark/src/main/java/org/apache/iceberg/spark/source/SparkFileWriterFactory.java:
##########
@@ -293,4 +217,48 @@ SparkFileWriterFactory build() {
writeProperties);
}
}
+
+ private static StructType calculateSparkTypeForDelete(StructType sparkType,
Schema schema) {
+ if (sparkType != null) {
+ // The delete types need to have the correct metadata columns.
+ if (sparkType.fields().length < 3) {
+ return PATH_POS_TYPE;
+ } else {
+ StructField rowField =
+
sparkType.fields()[sparkType.fieldIndex(MetadataColumns.DELETE_FILE_ROW_FIELD_NAME)];
+ return new StructType(
+ new StructField[] {
+ new StructField(
+ MetadataColumns.DELETE_FILE_PATH.name(),
+ DataTypes.StringType,
+ false,
+ Metadata.empty()),
+ new StructField(
+ MetadataColumns.DELETE_FILE_POS.name(),
+ DataTypes.LongType,
+ false,
+ Metadata.empty()),
+ new StructField(
+ MetadataColumns.DELETE_FILE_ROW_FIELD_NAME,
+ rowField.dataType(),
+ false,
+ Metadata.empty())
+ });
+ }
+ } else if (schema != null) {
+ return SparkSchemaUtil.convert(schema);
+ } else {
+ return null;
+ }
+ }
+
+ private static StructType calculateSparkType(StructType sparkType, Schema
schema) {
+ if (sparkType != null) {
+ return sparkType;
+ } else if (schema != null) {
+ return SparkSchemaUtil.convert(schema);
+ } else {
+ return null;
Review Comment:
equality delete also requires one of them be not null. position delete can
or should have null row schema. I would rather have a different method for
position delete than letting the validation ignored and cause NPE later in some
other code.
##########
parquet/src/main/java/org/apache/iceberg/parquet/Parquet.java:
##########
@@ -260,21 +269,147 @@ public WriteBuilder overwrite() {
}
public WriteBuilder overwrite(boolean enabled) {
- this.writeMode = enabled ? ParquetFileWriter.Mode.OVERWRITE :
ParquetFileWriter.Mode.CREATE;
+ impl.writeMode = enabled ? ParquetFileWriter.Mode.OVERWRITE :
ParquetFileWriter.Mode.CREATE;
return this;
}
public WriteBuilder writerVersion(WriterVersion version) {
- this.writerVersion = version;
+ impl.set(WRITER_VERSION_KEY, version.name());
return this;
}
public WriteBuilder withFileEncryptionKey(ByteBuffer encryptionKey) {
- this.fileEncryptionKey = encryptionKey;
+ impl.fileEncryptionKey(encryptionKey);
return this;
}
public WriteBuilder withAADPrefix(ByteBuffer aadPrefix) {
+ impl.fileAADPrefix(aadPrefix);
+ return this;
+ }
+
+ /*
+ * Sets the writer version. Default value is PARQUET_1_0 (v1).
+ */
+ @VisibleForTesting
+ WriteBuilder withWriterVersion(WriterVersion version) {
Review Comment:
oh. missed it in the removed code.
##########
data/src/main/java/org/apache/iceberg/data/GenericFileWriterFactory.java:
##########
@@ -50,62 +65,166 @@ class GenericFileWriterFactory extends
BaseFileWriterFactory<Record> {
super(
table,
dataFileFormat,
+ Record.class,
dataSchema,
dataSortOrder,
deleteFileFormat,
equalityFieldIds,
equalityDeleteRowSchema,
equalityDeleteSortOrder,
- positionDeleteRowSchema);
+ ImmutableMap.of(),
+ dataSchema,
+ equalityDeleteRowSchema);
+ this.table = table;
+ this.format = dataFileFormat;
+ this.positionDeleteRowSchema = positionDeleteRowSchema;
}
static Builder builderFor(Table table) {
return new Builder(table);
}
- @Override
+ /**
+ * @deprecated Since 1.10.0, will be removed in 1.11.0. It won't be called
starting 1.10.0 as the
+ * configuration is done by the {@link FormatModelRegistry}.
+ */
+ @Deprecated
protected void configureDataWrite(Avro.DataWriteBuilder builder) {
- builder.createWriterFunc(DataWriter::create);
+ throwUnsupportedOperationException();
}
- @Override
+ /**
+ * @deprecated Since 1.10.0, will be removed in 1.11.0. It won't be called
starting 1.10.0 as the
+ * configuration is done by the {@link FormatModelRegistry}.
+ */
+ @Deprecated
protected void configureEqualityDelete(Avro.DeleteWriteBuilder builder) {
- builder.createWriterFunc(DataWriter::create);
+ throwUnsupportedOperationException();
}
- @Override
+ /**
+ * @deprecated Since 1.10.0, will be removed in 1.11.0. It won't be called
starting 1.10.0 as the
+ * configuration is done by the {@link FormatModelRegistry}.
+ */
+ @Deprecated
protected void configurePositionDelete(Avro.DeleteWriteBuilder builder) {
- builder.createWriterFunc(DataWriter::create);
+ throwUnsupportedOperationException();
}
- @Override
+ /**
+ * @deprecated Since 1.10.0, will be removed in 1.11.0. It won't be called
starting 1.10.0 as the
+ * configuration is done by the {@link FormatModelRegistry}.
+ */
+ @Deprecated
protected void configureDataWrite(Parquet.DataWriteBuilder builder) {
- builder.createWriterFunc(GenericParquetWriter::create);
+ throwUnsupportedOperationException();
}
- @Override
+ /**
+ * @deprecated Since 1.10.0, will be removed in 1.11.0. It won't be called
starting 1.10.0 as the
+ * configuration is done by the {@link FormatModelRegistry}.
+ */
+ @Deprecated
protected void configureEqualityDelete(Parquet.DeleteWriteBuilder builder) {
- builder.createWriterFunc(GenericParquetWriter::create);
+ throwUnsupportedOperationException();
}
- @Override
+ /**
+ * @deprecated Since 1.10.0, will be removed in 1.11.0. It won't be called
starting 1.10.0 as the
+ * configuration is done by the {@link FormatModelRegistry}.
+ */
+ @Deprecated
protected void configurePositionDelete(Parquet.DeleteWriteBuilder builder) {
- builder.createWriterFunc(GenericParquetWriter::create);
+ throwUnsupportedOperationException();
}
- @Override
+ /**
+ * @deprecated Since 1.10.0, will be removed in 1.11.0. It won't be called
starting 1.10.0 as the
+ * configuration is done by the {@link FormatModelRegistry}.
+ */
+ @Deprecated
protected void configureDataWrite(ORC.DataWriteBuilder builder) {
- builder.createWriterFunc(GenericOrcWriter::buildWriter);
+ throwUnsupportedOperationException();
}
- @Override
+ /**
+ * @deprecated Since 1.10.0, will be removed in 1.11.0. It won't be called
starting 1.10.0 as the
+ * configuration is done by the {@link FormatModelRegistry}.
+ */
+ @Deprecated
protected void configureEqualityDelete(ORC.DeleteWriteBuilder builder) {
- builder.createWriterFunc(GenericOrcWriter::buildWriter);
+ throwUnsupportedOperationException();
}
- @Override
+ /**
+ * @deprecated Since 1.10.0, will be removed in 1.11.0. It won't be called
starting 1.10.0 as the
+ * configuration is done by the {@link FormatModelRegistry}.
+ */
+ @Deprecated
protected void configurePositionDelete(ORC.DeleteWriteBuilder builder) {
- builder.createWriterFunc(GenericOrcWriter::buildWriter);
+ throwUnsupportedOperationException();
+ }
+
+ private void throwUnsupportedOperationException() {
+ throw new UnsupportedOperationException(
+ "Method is deprecated and should not be called. "
+ + "Configuration is already done by the ObjectModelRegistry.");
+ }
+
+ @Override
+ public PositionDeleteWriter<Record> newPositionDeleteWriter(
+ EncryptedOutputFile file, PartitionSpec spec, StructLike partition) {
+ if (positionDeleteRowSchema == null) {
+ return super.newPositionDeleteWriter(file, spec, partition);
+ } else {
+ LOG.info(
Review Comment:
Isn't this a new API? the existing `configure...` APIs are marked as
deprecated above.
##########
data/src/test/java/org/apache/iceberg/io/TestAppenderFactory.java:
##########
@@ -259,6 +260,7 @@ public void testPosDeleteWriter() throws IOException {
.isEqualTo(expectedRowSet(expected));
}
+ @Disabled("Deprecated API")
Review Comment:
I am saying that we can make the code change for deprecation annotation and
test adjustment in a separate PR to reduce the complexity of this PR. It is a
separate matter and can be addressed separately.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
