zeroshade commented on code in PR #1162:
URL: https://github.com/apache/arrow-go/pull/1162#discussion_r3856934760
##########
arrow/datatype_fixedwidth.go:
##########
@@ -590,22 +589,32 @@ func NarrowestDecimalType(prec, scale int32)
(DecimalType, error) {
}
func NewDecimalType(id Type, prec, scale int32) (DecimalType, error) {
+ var (
+ dtype DecimalType
+ maxPrecision int32
+ )
switch id {
case DECIMAL32:
- debug.Assert(prec <=
int32(decimal.MaxPrecision[decimal.Decimal32]()), "invalid precision for
decimal32")
- return &Decimal32Type{Precision: prec, Scale: scale}, nil
+ dtype = &Decimal32Type{Precision: prec, Scale: scale}
+ maxPrecision = int32(decimal.MaxPrecision[decimal.Decimal32]())
case DECIMAL64:
- debug.Assert(prec <=
int32(decimal.MaxPrecision[decimal.Decimal64]()), "invalid precision for
decimal64")
- return &Decimal64Type{Precision: prec, Scale: scale}, nil
+ dtype = &Decimal64Type{Precision: prec, Scale: scale}
+ maxPrecision = int32(decimal.MaxPrecision[decimal.Decimal64]())
case DECIMAL128:
- debug.Assert(prec <=
int32(decimal.MaxPrecision[decimal.Decimal128]()), "invalid precision for
decimal128")
- return &Decimal128Type{Precision: prec, Scale: scale}, nil
+ dtype = &Decimal128Type{Precision: prec, Scale: scale}
+ maxPrecision = int32(decimal.MaxPrecision[decimal.Decimal128]())
case DECIMAL256:
- debug.Assert(prec <=
int32(decimal.MaxPrecision[decimal.Decimal256]()), "invalid precision for
decimal256")
- return &Decimal256Type{Precision: prec, Scale: scale}, nil
+ dtype = &Decimal256Type{Precision: prec, Scale: scale}
+ maxPrecision = int32(decimal.MaxPrecision[decimal.Decimal256]())
default:
return nil, fmt.Errorf("%w: must use one of the DECIMAL IDs to
create a DecimalType", ErrInvalid)
}
+
+ if prec <= 0 || prec > maxPrecision {
+ return nil, fmt.Errorf("%w: precision for %s must be between 1
and %d, got %d",
Review Comment:
**Blocking:** This new error path makes the existing caller in
`arrow/avro/schema.go:475` unsafe: it does `dt, _ = arrow.NewDecimalType(...)`.
For an Avro decimal with precision `77`, the actual `ErrInvalid` is discarded,
`dt` becomes nil, and schema construction later returns only `invalid avro
schema: arrow: field with nil DataType`. Consequently, callers cannot use
`errors.Is(err, arrow.ErrInvalid)` or see the invalid precision. Please handle
and propagate the constructor error at that call site—panicking with it would
work with the existing Avro recovery boundary—and add an Avro regression test
preserving the precision error.
--
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]
