fix(table): validate numeric Parquet writer properties - #1672
Conversation
fallintoplace
changed the title
9babb5c to
e1495cf
Compare
zeroshade
left a comment
zeroshade
left a comment
There was a problem hiding this comment.
Thanks for centralizing numeric writer-property validation. The positive checks are useful, the bloom-filter bounds correctly include 32 bytes through 128 MiB, and there is no unsupported FPP property to validate.
Two blocking validation gaps remain inline: compression levels need codec-specific bounds, and the 32-bit int ceiling must not reject the row-group byte target that remains int64 downstream. I also called out the empty-string compatibility change and the page-size relationship separately.
Please add codec boundary matrices with exact range errors, nil/unset/default cases, a 32-bit large-row-group case, and proof that invalid properties fail before file creation.
| } | ||
| } | ||
|
|
||
| if value, ok := props[ParquetCompressionLevelKey]; ok { |
There was a problem hiding this 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.
| 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) { |
There was a problem hiding this 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.
| return strings.EqualFold(val, "true") | ||
| } | ||
|
|
||
| func ValidateParquetWriteProperties(props iceberg.Properties) error { |
There was a problem hiding this 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.
| continue | ||
| } | ||
|
|
||
| parsed, err := strconv.ParseInt(value, 10, 64) |
There was a problem hiding this 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.
2 participants
Summary
Validate numeric Parquet writer properties before creating an output file.
Why
Malformed, zero, negative, or out-of-range values were accepted by permissive parsing and then passed to Arrow.
What changed
Row-group, page, dictionary, and bloom-filter sizes are checked before the writer is created. Existing defaults are unchanged.
Tests