zeroshade commented on code in PR #2529:
URL: https://github.com/apache/iceberg-go/pull/2529#discussion_r4542921986
##########
table/writer.go:
##########
@@ -267,6 +268,62 @@ func metricsPropsFor(content iceberg.ManifestEntryContent,
props iceberg.Propert
return props
}
+// parquetPropsFor mirrors Java's deleteContext: delete settings override the
+// corresponding data settings, while absent overrides inherit the data values.
+// Metrics use the original table properties independently of this writer
config.
+func parquetPropsFor(content iceberg.ManifestEntryContent, props
iceberg.Properties) (iceberg.Properties, error) {
+ if content == iceberg.EntryContentData {
+ return props, tblutils.ValidateParquetWriteProperties(props)
+ }
+
+ effective := maps.Clone(props)
+ overrides := map[string]string{
+ DeleteParquetRowGroupSizeBytesKey: ParquetRowGroupSizeBytesKey,
+ DeleteParquetPageSizeBytesKey: ParquetPageSizeBytesKey,
+ DeleteParquetPageRowLimitKey: ParquetPageRowLimitKey,
+ DeleteParquetDictSizeBytesKey: ParquetDictSizeBytesKey,
+ DeleteParquetPageVersionKey: ParquetPageVersionKey,
+ DeleteParquetCompressionKey: ParquetCompressionKey,
+ DeleteParquetCompressionLevelKey: ParquetCompressionLevelKey,
+ }
+ for deleteKey, dataKey := range overrides {
+ if value, ok := props[deleteKey]; ok {
+ effective[dataKey] = value
+ }
+ }
+ if codec, overridden := props[DeleteParquetCompressionKey]; overridden {
Review Comment:
When `write.delete.parquet.compression-codec` is set but
`write.delete.parquet.compression-level` isn't, the data
`write.parquet.compression-level` carries over to the delete codec. With
`write.parquet.compression-codec=zstd`, `write.parquet.compression-level=19`
and `write.delete.parquet.compression-codec=gzip`,
`ValidateParquetWriteProperties` rejects 19 for gzip. Every delete write then
fails with `invalid write.parquet.compression-level value 19 for gzip`, while
data writes keep working, and the error doesn't mention the delete codec.
Java's `deleteContext` also inherits the level, but a level only makes sense
for the codec it was set for. #2524 resets the level to the codec default in
exactly this case for per-column overrides. Could you do the same: when the
delete codec is overridden, the delete level isn't, and the codec differs from
the data codec, set `effective[ParquetCompressionLevelKey]` to
`ParquetCompressionLevelDefault`? Please also add a test for that combination.
##########
table/writer.go:
##########
@@ -267,6 +268,62 @@ func metricsPropsFor(content iceberg.ManifestEntryContent,
props iceberg.Propert
return props
}
+// parquetPropsFor mirrors Java's deleteContext: delete settings override the
+// corresponding data settings, while absent overrides inherit the data values.
+// Metrics use the original table properties independently of this writer
config.
+func parquetPropsFor(content iceberg.ManifestEntryContent, props
iceberg.Properties) (iceberg.Properties, error) {
+ if content == iceberg.EntryContentData {
+ return props, tblutils.ValidateParquetWriteProperties(props)
+ }
+
+ effective := maps.Clone(props)
+ overrides := map[string]string{
+ DeleteParquetRowGroupSizeBytesKey: ParquetRowGroupSizeBytesKey,
+ DeleteParquetPageSizeBytesKey: ParquetPageSizeBytesKey,
+ DeleteParquetPageRowLimitKey: ParquetPageRowLimitKey,
+ DeleteParquetDictSizeBytesKey: ParquetDictSizeBytesKey,
+ DeleteParquetPageVersionKey: ParquetPageVersionKey,
+ DeleteParquetCompressionKey: ParquetCompressionKey,
+ DeleteParquetCompressionLevelKey: ParquetCompressionLevelKey,
+ }
+ for deleteKey, dataKey := range overrides {
+ if value, ok := props[deleteKey]; ok {
+ effective[dataKey] = value
+ }
+ }
+ if codec, overridden := props[DeleteParquetCompressionKey]; overridden {
+ switch strings.ToLower(codec) {
+ case "snappy", "zstd", "uncompressed", "gzip", "brotli", "lz4",
"lz4raw", "lz4_raw", "lzo":
+ default:
+ return nil, fmt.Errorf("%w: unsupported %s value %q",
iceberg.ErrInvalidArgument, DeleteParquetCompressionKey, codec)
+ }
+ }
+ if version, overridden := props[DeleteParquetPageVersionKey];
overridden {
+ switch strings.ToLower(version) {
+ case "1", "2", "v1", "v2":
+ default:
+ return nil, fmt.Errorf("%w: unsupported %s value %q",
iceberg.ErrInvalidArgument, DeleteParquetPageVersionKey, version)
+ }
+ }
+ // Java does not apply data-column Bloom filters or dictionary
overrides to
+ // delete files, whose schema may differ from the data schema.
+ maps.DeleteFunc(effective, func(key, _ string) bool {
+ return strings.HasPrefix(key,
ParquetBloomFilterColumnEnabledKeyPrefix+".") ||
+ strings.HasPrefix(key,
tblutils.ParquetDictEncodingColumnEnabledKeyPrefix+".")
+ })
+ if err := tblutils.ValidateParquetWriteProperties(effective); err !=
nil {
+ for deleteKey, dataKey := range overrides {
+ if _, overridden := props[deleteKey]; overridden &&
strings.Contains(err.Error(), dataKey) {
Review Comment:
nit: finding the delete key with `strings.Contains(err.Error(), dataKey)`
depends on the exact wording of `ValidateParquetWriteProperties`'s messages, so
it breaks if they change. Could you validate the overridden delete keys
directly before copying them over, or have the validator return the offending
key, so the `write.delete.parquet.*` name doesn't depend on string matching?
--
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]
