wypoon commented on code in PR #6799:
URL: https://github.com/apache/iceberg/pull/6799#discussion_r1332249764
##########
core/src/main/java/org/apache/iceberg/ManifestWriter.java:
##########
@@ -229,18 +252,34 @@ protected ManifestEntry<DataFile>
prepare(ManifestEntry<DataFile> entry) {
@Override
protected FileAppender<ManifestEntry<DataFile>> newAppender(
PartitionSpec spec, OutputFile file) {
+ return newAppender(spec, file, null, null);
+ }
+
+ @Override
+ protected FileAppender<ManifestEntry<DataFile>> newAppender(
+ PartitionSpec spec, OutputFile file, String compressionCodec, Integer
compressionLevel) {
Schema manifestSchema = V2Metadata.entrySchema(spec.partitionType());
try {
- return Avro.write(file)
- .schema(manifestSchema)
- .named("manifest_entry")
- .meta("schema", SchemaParser.toJson(spec.schema()))
- .meta("partition-spec", PartitionSpecParser.toJsonFields(spec))
- .meta("partition-spec-id", String.valueOf(spec.specId()))
- .meta("format-version", "2")
- .meta("content", "data")
- .overwrite()
- .build();
+ Avro.WriteBuilder builder =
+ Avro.write(file)
+ .schema(manifestSchema)
+ .named("manifest_entry")
+ .meta("schema", SchemaParser.toJson(spec.schema()))
+ .meta("partition-spec", PartitionSpecParser.toJsonFields(spec))
+ .meta("partition-spec-id", String.valueOf(spec.specId()))
+ .meta("format-version", "2")
+ .meta("content", "data")
+ .overwrite();
+
+ if (compressionCodec != null) {
+ builder.set(TableProperties.AVRO_COMPRESSION, compressionCodec);
+ }
+
+ if (compressionLevel != null) {
+ builder.set(TableProperties.AVRO_COMPRESSION_LEVEL,
compressionLevel.toString());
Review Comment:
In the original PR (https://github.com/apache/iceberg/pull/5893),
@sumeetgajjar used a String for compressionLevel in the API initially, and
@rdblue commented that compression level is an int, not a string. Sumeet then
changed it to an Integer, to allow for the fact that some codecs do not have
compression level (so it can be null).
I think that conceptually, it makes sense for compressionLevel to be an
Integer in the API. In the implementation, it so happens that we call
`Avro.WriteBuilder` and that makes us use a `set(String, String)` method to
configure it, and ironically `Avro.WriteBuilder.Context` has to convert the
String for compression level back into an int. But that is all implementation
detail and an accident of the implementation.
--
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]
