You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "fallintoplace (via GitHub)" <gi...@apache.org> on 2026/08/05 22:38:11 UTC
[PR] fix(table): validate parquet writer properties [iceberg-go]
fallintoplace opened a new pull request, #1672:
URL: https://github.com/apache/iceberg-go/pull/1672
Several numeric Parquet writer properties were parsed with permissive defaults and then passed to Arrow without checking for malformed, zero, negative, or out-of-range values.
This validates the configured row-group, page, dictionary, and bloom-filter sizes before creating an output file. Existing defaults are unchanged.
Tests: go test ./table/internal -count=1
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] fix(table): validate numeric Parquet writer properties [iceberg-go]
Posted by "zeroshade (via GitHub)" <gi...@apache.org>.
zeroshade commented on code in PR #1672:
URL: https://github.com/apache/iceberg-go/pull/1672#discussion_r3732106968
##########
table/internal/parquet_files.go:
##########
@@ -354,6 +357,47 @@ func javaBool(val string) bool {
return strings.EqualFold(val, "true")
}
+func ValidateParquetWriteProperties(props iceberg.Properties) error {
+ for _, key := range []string{
+ ParquetRowGroupSizeBytesKey,
+ ParquetRowGroupLimitKey,
+ ParquetPageSizeBytesKey,
+ ParquetPageRowLimitKey,
+ ParquetDictSizeBytesKey,
+ ParquetBloomFilterMaxBytesKey,
+ } {
+ value, ok := props[key]
+ if !ok {
+ continue
+ }
+
+ parsed, err := strconv.ParseInt(value, 10, 64)
+ if err != nil {
+ return fmt.Errorf("%w: invalid %s value %q: %v", iceberg.ErrInvalidArgument, key, value, err)
+ }
+ if parsed <= 0 {
+ return fmt.Errorf("%w: %s must be greater than 0, got %d", iceberg.ErrInvalidArgument, key, parsed)
+ }
+ if key == ParquetBloomFilterMaxBytesKey &&
+ (parsed < parquetBloomFilterMaxBytesMin || parsed > parquetBloomFilterMaxBytesMax) {
+ return fmt.Errorf("%w: %s must be between %d and %d bytes, got %d",
+ iceberg.ErrInvalidArgument, key, parquetBloomFilterMaxBytesMin, parquetBloomFilterMaxBytesMax, parsed)
+ }
+ if strconv.IntSize == 32 && parsed > int64(^uint(0)>>1) {
Review Comment:
This blanket 32-bit `int` ceiling also applies to `write.parquet.row-group-size-bytes`, even though that value remains `int64` through `ParquetRowGroupTargetSizeBytes`, `ParquetFileWriter.rowGroupBytes`, and the row-group comparison. On 32-bit builds this rejects legitimate `int64` row-group targets.
Suggested fix: exempt the row-group-size property from the `int` limit, or migrate all of its downstream consumers consistently if an `int` bound is actually required.
##########
table/internal/parquet_files.go:
##########
@@ -354,6 +357,47 @@ func javaBool(val string) bool {
return strings.EqualFold(val, "true")
}
+func ValidateParquetWriteProperties(props iceberg.Properties) error {
+ for _, key := range []string{
+ ParquetRowGroupSizeBytesKey,
+ ParquetRowGroupLimitKey,
+ ParquetPageSizeBytesKey,
+ ParquetPageRowLimitKey,
+ ParquetDictSizeBytesKey,
+ ParquetBloomFilterMaxBytesKey,
+ } {
+ value, ok := props[key]
+ if !ok {
+ continue
+ }
+
+ parsed, err := strconv.ParseInt(value, 10, 64)
+ if err != nil {
+ return fmt.Errorf("%w: invalid %s value %q: %v", iceberg.ErrInvalidArgument, key, value, err)
+ }
+ if parsed <= 0 {
+ return fmt.Errorf("%w: %s must be greater than 0, got %d", iceberg.ErrInvalidArgument, key, parsed)
+ }
+ if key == ParquetBloomFilterMaxBytesKey &&
+ (parsed < parquetBloomFilterMaxBytesMin || parsed > parquetBloomFilterMaxBytesMax) {
+ return fmt.Errorf("%w: %s must be between %d and %d bytes, got %d",
+ iceberg.ErrInvalidArgument, key, parquetBloomFilterMaxBytesMin, parquetBloomFilterMaxBytesMax, parsed)
+ }
+ if strconv.IntSize == 32 && parsed > int64(^uint(0)>>1) {
+ return fmt.Errorf("%w: %s value %d exceeds int range", iceberg.ErrInvalidArgument, key, parsed)
+ }
+ }
+
+ if value, ok := props[ParquetCompressionLevelKey]; ok {
Review Comment:
`strconv.Atoi` only proves that the level is an integer; it still accepts invalid codec levels such as gzip 10 and zstd 23. Gzip can panic later, while zstd silently maps arbitrary values, so invalid configuration is not being rejected safely here.
Suggested fix: validate the level against the selected codec's supported range, preserve the default `-1`, and report the property name, codec, and valid range in the error.
##########
table/internal/parquet_files.go:
##########
@@ -354,6 +357,47 @@ func javaBool(val string) bool {
return strings.EqualFold(val, "true")
}
+func ValidateParquetWriteProperties(props iceberg.Properties) error {
+ for _, key := range []string{
+ ParquetRowGroupSizeBytesKey,
+ ParquetRowGroupLimitKey,
+ ParquetPageSizeBytesKey,
+ ParquetPageRowLimitKey,
+ ParquetDictSizeBytesKey,
+ ParquetBloomFilterMaxBytesKey,
+ } {
+ value, ok := props[key]
+ if !ok {
+ continue
+ }
+
+ parsed, err := strconv.ParseInt(value, 10, 64)
Review Comment:
An explicitly configured empty string now returns a parse error, whereas it previously selected the default. That may be the desired stricter behavior, but it is a compatibility change.
Suggested fix: document the change and add explicit tests distinguishing absent/unset properties, empty strings, and default-valued properties.
##########
table/internal/parquet_files.go:
##########
@@ -354,6 +357,47 @@ func javaBool(val string) bool {
return strings.EqualFold(val, "true")
}
+func ValidateParquetWriteProperties(props iceberg.Properties) error {
Review Comment:
One constraint should deliberately remain absent: under Arrow/Parquet, page size does not need to be less than or equal to the row-group target.
Suggested fix: do not add that cross-property relationship as a hard validity rule; validate each property's actual consumer constraints instead.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] fix(table): validate numeric Parquet writer properties [iceberg-go]
Posted by "zeroshade (via GitHub)" <gi...@apache.org>.
zeroshade merged PR #1672:
URL: https://github.com/apache/iceberg-go/pull/1672
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org