dramaticlly commented on code in PR #16094:
URL: https://github.com/apache/iceberg/pull/16094#discussion_r4483978242
##########
parquet/src/main/java/org/apache/iceberg/parquet/ParquetWriter.java:
##########
@@ -259,22 +264,39 @@ private void startRowGroup() {
this.recordCount = 0;
this.rowGroupUncompressedSize = 0;
- this.pageStore =
- new ColumnChunkPageWriteStore(
- compressor,
- parquetSchema,
- props.getAllocator(),
- this.columnIndexTruncateLength,
- ParquetProperties.DEFAULT_PAGE_WRITE_CHECKSUM_ENABLED,
- fileEncryptor,
- rowGroupOrdinal);
+ this.pageStore = newColumnChunkPageWriteStore();
this.rowGroupOrdinal++;
this.writeStore = props.newColumnWriteStore(parquetSchema, pageStore,
pageStore);
model.setColumnStore(writeStore);
}
+ private ColumnChunkPageWriteStore newColumnChunkPageWriteStore() {
+ return ColumnChunkPageWriteStore.builder()
+ .withCompressorProvider(this::resolveCompressor)
Review Comment:
I think we can get rid of `compressorsByColumn` here and directly with
```suggestion
.withCompressorProvider(this::newCompressor)
```
the codecFactory.getCompressor method below already cache with
`org.apache.iceberg.parquet.ParquetCodecFactory`
##########
docs/docs/configuration.md:
##########
@@ -54,6 +54,8 @@ Iceberg tables support table properties to configure table
behavior, like the de
| write.parquet.dict-size-bytes | 2097152 (2 MB)
| Parquet dictionary page size
|
| write.parquet.compression-codec | zstd
| Parquet compression codec: zstd, brotli, lz4, gzip, snappy, uncompressed
|
| write.parquet.compression-level | null
| Parquet compression level
|
+| write.parquet.compression-codec.column.col1 | (not set)
| Per-column compression codec override for column 'col1': zstd, brotli,
lz4, gzip, snappy, uncompressed
|
+| write.parquet.compression-level.column.col1 | (not set)
| Per-column compression level override for column 'col1'. Takes an
integer valid for the column's codec: 0-9 for gzip, 1-22 for zstd, 0-11 for
brotli; unlike 'write.parquet.compression-level', gzip does not accept names
such as BEST_SPEED. When a column overrides the codec but not the level,
'write.parquet.compression-level' is applied only if the overriding codec
matches the table codec; otherwise the overriding codec's default level is used
|
Review Comment:
trying to see if this is the best place to state the compression level, it
can possible drift from parquet or actual compressor like zstd over time,
something like GZIP also accepts -1 (default), and ZSTD accepts negative levels
down
##########
parquet/src/main/java/org/apache/iceberg/parquet/Parquet.java:
##########
@@ -374,6 +376,46 @@ private void setColumnStatsConfig(
});
}
+ private void setColumnCompressionConfig(
+ Context context,
+ Map<String, String> colNameToParquetPathMap,
+ BiConsumer<String, CompressionCodecName> withCompressionCodec,
+ BiConsumer<String, Integer> withCompressionLevel) {
+
+ Map<String, String> columnCompressionCodec =
context.columnCompressionCodec();
+ Map<String, String> columnCompressionLevel =
context.columnCompressionLevel();
+
+ Sets.union(columnCompressionCodec.keySet(),
columnCompressionLevel.keySet())
Review Comment:
from parquet the level seems to be dependent on the codec
> The valid range is codec-specific (e.g. ZSTD: negative levels trade ratio
for speed, up to 22 for best compression (default 3), GZIP: 0–9 or -1 (default
6), BROTLI: 0–11 (default 1)). Pass null to unset a previously configured level
for that column.
shall we add a out of range level in test as well?
--
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]
