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: [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]
