tanmayrauth commented on code in PR #1405:
URL: https://github.com/apache/iceberg-go/pull/1405#discussion_r3525801484
##########
types.go:
##########
@@ -578,9 +588,30 @@ func (f FixedType) String() string { return
fmt.Sprintf("fixed[%d]", f.len) }
func (f FixedType) primitive() {}
func DecimalTypeOf(prec, scale int) DecimalType {
+ if err := validateDecimalPrecisionScale(prec, scale); err != nil {
+ panic(fmt.Errorf("%w: %w", ErrInvalidArgument, err))
+ }
+
return DecimalType{precision: prec, scale: scale}
}
+func validateDecimalPrecisionScale(precision, scale int) error {
+ if precision <= 0 {
+ return fmt.Errorf("invalid precision %d: must be greater than
0", precision)
+ }
+ if precision >= 40 {
Review Comment:
The spec (and Java) cap decimal precision at 38 — that's the max for
16-byte decimal128. `>= 40` still admits `decimal(39,x)`, which other engines
will reject. Should this be `> 38`?
##########
types.go:
##########
@@ -197,8 +197,18 @@ func (t *typeIFace) UnmarshalJSON(b []byte) error {
return fmt.Errorf("%w: %s",
ErrInvalidTypeString, typename)
}
- prec, _ := strconv.Atoi(matches[1])
- scale, _ := strconv.Atoi(matches[2])
+ prec, err := strconv.Atoi(matches[1])
Review Comment:
nit: with the anchored `^decimal\(\s*(\d+)...\)$` regex the captures are
always `\d+`, so `Atoi` here can only fail on integer overflow (e.g.
`decimal(99999999999,2)`) — `validateDecimalPrecisionScale` does the real work.
Fine to keep, maybe just a comment noting overflow is the only failure path.
##########
types.go:
##########
@@ -578,9 +588,30 @@ func (f FixedType) String() string { return
fmt.Sprintf("fixed[%d]", f.len) }
func (f FixedType) primitive() {}
func DecimalTypeOf(prec, scale int) DecimalType {
+ if err := validateDecimalPrecisionScale(prec, scale); err != nil {
+ panic(fmt.Errorf("%w: %w", ErrInvalidArgument, err))
Review Comment:
This panic regresses `DecimalLiteral.Type()`, which returns
`DecimalTypeOf(9, d.Scale)` (literals.go:1254 — hardcoded precision 9 per
#1028). Any decimal literal with scale > 9 now panics here because scale >
precision, and Iceberg allows scale up to 38. I think that's why the tests in
this PR had to drop `decimal(38,10)`→`decimal(38,9)` and
`DecimalTypeOf(8,9)`→`DecimalTypeOf(8,2)` — those edits are working around the
panic rather than the code being safe.
It's also reachable outside substrait: exprs.go:521 does
`lit.Type().Equals(b.field.Type)` in the literal-binding path, so filtering on
a high-scale decimal column hits the same panic.
Could we avoid panicking on this path? Options: have
`DecimalLiteral.Type()` construct the struct directly (as before), or make
`DecimalTypeOf` return an error / stay lenient and validate only on the parse
path.
--
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]
