mxm commented on code in PR #15700:
URL: https://github.com/apache/iceberg/pull/15700#discussion_r3080553364
##########
parquet/src/main/java/org/apache/iceberg/parquet/Parquet.java:
##########
@@ -297,6 +300,7 @@ private <T> WriteSupport<T> getWriteSupport(MessageType
type) {
*/
@VisibleForTesting
WriteBuilder withWriterVersion(WriterVersion version) {
+ Preconditions.checkNotNull(version, "Writer version cannot be null");
this.writerVersion = version;
return this;
}
Review Comment:
Can we clean up this method? We already have `writerVersion(WriterVersion
version)` above.
##########
parquet/src/main/java/org/apache/iceberg/parquet/Parquet.java:
##########
@@ -168,7 +170,7 @@ public static class WriteBuilder implements
InternalData.WriteBuilder {
private BiFunction<Schema, MessageType, ParquetValueWriter<?>>
createWriterFunc = null;
private MetricsConfig metricsConfig = MetricsConfig.getDefault();
private ParquetFileWriter.Mode writeMode = ParquetFileWriter.Mode.CREATE;
- private WriterVersion writerVersion = WriterVersion.PARQUET_1_0;
+ private WriterVersion writerVersion = null;
Review Comment:
Could we add a comment to state where the default comes from?
##########
core/src/main/java/org/apache/iceberg/TableProperties.java:
##########
@@ -135,6 +135,9 @@ private TableProperties() {}
"write.delete.parquet.page-size-bytes";
public static final int PARQUET_PAGE_SIZE_BYTES_DEFAULT = 1024 * 1024; // 1
MB
+ public static final String PARQUET_PAGE_VERSION =
"write.parquet.page-version";
+ public static final String PARQUET_PAGE_VERSION_DEFAULT = "1";
Review Comment:
I was thinking about this as well, but why would we use a different format
for delete files? IMHO fine to skip the delete file counterpart to keep the set
of config options minimal.
##########
parquet/src/main/java/org/apache/iceberg/parquet/Parquet.java:
##########
@@ -364,6 +368,8 @@ public <D> FileAppender<D> build() throws IOException {
// Map Iceberg properties to pass down to the Parquet writer
Context context = createContextFunc.apply(config);
+ WriterVersion parquetWriterVersion =
+ writerVersion != null ? writerVersion : context.writerVersion();
Review Comment:
NIT
```suggestion
WriterVersion parquetWriterVersion =
MoreObjects.firstNonNull(writerVersion, context.writerVersion());
```
--
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]
