zeroshade commented on code in PR #1519:
URL: https://github.com/apache/arrow-go/pull/1519#discussion_r4553252764
##########
arrow/decimal128/decimal128.go:
##########
@@ -238,9 +239,41 @@ func FromFloat64(v float64, prec, scale int32) (Num,
error) {
return fromPositiveFloat64(v, prec, scale)
}
-var pt5 = big.NewFloat(0.5)
-
func FromString(v string, prec, scale int32) (n Num, err error) {
+ if lit, ok := decimalstr.Parse(v); ok {
+ return fromDecimalLiteral(lit, prec, scale)
+ }
+ return fromFloatLiteral(v, prec, scale)
+}
+
+func fromDecimalLiteral(lit decimalstr.Parsed, prec, scale int32) (n Num, err
error) {
+ if scale < 0 {
+ // Reject an unscaled magnitude that would not fit in signed
128-bit
+ // storage, before using the scale.
+ if lit.AtLeastTwoPower(0, 127) {
+ return Num{}, errors.New("bitlen too large for
decimal128")
+ }
+ if scale < -MaxScale {
+ return Num{}, fmt.Errorf("scale %d out of range for
decimal128, must be between %d and %d", scale, -MaxScale, MaxScale)
+ }
+ } else if scale > MaxScale {
+ return Num{}, fmt.Errorf("scale %d out of range for decimal128,
must be between %d and %d", scale, -MaxScale, MaxScale)
+ } else if lit.AtLeastTwoPower(int64(scale), 128) {
+ return Num{}, errors.New("bitlen too large for decimal128")
+ }
+
+ val, ok := lit.ScaleAndRound(int64(scale))
Review Comment:
Long fractions now fail here. `ScaleAndRound` returns `ok=false` when more
than `maxDigits` (1024) digits have to be dropped, and this turns any `!ok`
into a bit-length error even though the value is tiny:
| Input at scale 2 | main | this PR |
|---|---|---|
| `0.` + 1000 sevens | 78 | 78 |
| `0.` + 1030 sevens | 78 | `bitlen too large for decimal128` |
| `0.` + 5000 sevens | 78 | same error |
The helper's doc says callers should fall back to their own conversion when
`ok` is false. Either fall back to `fromFloatLiteral` here, or drop the `down >
maxDigits` limit in `ScaleAndRound`. With ties rounding away from zero, only
the first dropped digit matters, so the digits can be cut as a string with no
division. The same applies to `decimal256` and `Decimal32`/`Decimal64`. Could
you also add a test with a fraction longer than 1024 digits?
--
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]
