zeroshade commented on code in PR #1519:
URL: https://github.com/apache/arrow-go/pull/1519#discussion_r4543239729
##########
arrow/decimal128/decimal128.go:
##########
@@ -289,31 +288,16 @@ func FromString(v string, prec, scale int32) (n Num, err
error) {
n = FromBigInt(val)
n, _ = n.Div(scaleMultipliers[-scale])
} else {
- // Since we're going to truncate this to get an integer, we
need to round
- // the value instead because of edge cases so that we match how
other implementations
- // (e.g. C++) handles Decimal values. So if we're negative
we'll subtract 0.5 and if
- // we're positive we'll add 0.5.
p := (&big.Float{}).SetInt(scaleMultipliers[scale].BigInt())
out.SetPrec(precInBits).Mul(out, p)
// Reject before rounding and converting, which would allocate
as many
// bits as the value has.
if out.IsInf() || out.MantExp(nil) > 128 {
return Num{}, errors.New("bitlen too large for
decimal128")
}
- // A magnitude below 1/4 rounds to zero. Clearing it skips the
- // rounding below, which allocates in proportion to the gap
between
- // the value's exponent and 0.5's.
- if out.MantExp(nil) < -1 {
- out.SetInt64(0)
- }
- if out.Signbit() {
- out.Sub(out, pt5)
- } else {
- out.Add(out, pt5)
- }
-
- var tmp big.Int
- val, _ := out.Int(&tmp)
+ // Round from the digits of v, so exact ties are resolved
consistently
+ // rather than by the error in out's binary approximation.
+ val := decimalstr.RoundScaled(out, v, int64(scale))
Review Comment:
This parses every value twice: `big.ParseFloat` plus the scaling multiply
for the size checks, then the digits again here. `FromString` runs once per
value in the CSV reader, `FromJSON`, `AppendValueFromString` and `ParseScalar`,
so the cost matters. `decimal128.FromString` at scale 4, Apple M4:
| Input | main | this PR |
|---|---|---|
| `12345.6789` | 230 ns, 296 B, 11 allocs | 308 ns, 368 B, 15 allocs |
| `0.35` | 198 ns, 296 B, 11 allocs | 260 ns, 408 B, 16 allocs |
| `-98765432109876543210.123456789` | 316 ns, 304 B, 11 allocs | 471 ns, 360
B, 14 allocs |
Could you parse the digits first, and when that succeeds skip
`big.ParseFloat` and the multiply entirely? The size checks can come from the
number of integer digits after scaling. The float path would then only be
needed for inputs that aren't plain decimal literals, such as a `p` exponent.
Done that way, this could be faster than main. The same applies to the
`decimal256` and `Decimal32`/`Decimal64` call sites.
##########
arrow/decimal128/decimal128.go:
##########
@@ -289,31 +288,16 @@ func FromString(v string, prec, scale int32) (n Num, err
error) {
n = FromBigInt(val)
n, _ = n.Div(scaleMultipliers[-scale])
Review Comment:
Negative scales still truncate: `FromString("19", 38, -1)` returns 1, and so
does `"15"`, for both `decimal128` and `Decimal64`. #1518 asks for one
consistent rounding rule, and a positive scale rounding while a negative scale
truncates is a bigger inconsistency than the tie cases. `ScaleAndRound` already
handles a negative scale arithmetically, so this branch could use it too,
keeping the existing bit-size check first.
##########
arrow/internal/decimalstr/decimalstr.go:
##########
@@ -0,0 +1,171 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+// Package decimalstr holds helpers shared by the decimal packages for
+// working with decimal string literals.
+package decimalstr
+
+import (
+ "math/big"
+ "strconv"
+)
+
+// maxDigits bounds the number of integer digits ScaleAndRound will
+// materialize. It is well above the 77 digits a 256-bit value can hold, so it
+// only guards callers that skipped their own magnitude check.
+const maxDigits = 1024
+
+// ScaleAndRound returns the decimal literal s multiplied by 10^scale and
+// rounded to an integer, with ties rounded away from zero.
+//
+// It works on the decimal digits of s, so an exact halfway value such as
+// "0.15" or "0.35" is always recognized as a tie. Parsing into a binary
+// float first would leave it just above or just below one half, and the
+// rounding direction would depend on that error.
+//
+// ok is false when s is not a plain decimal literal (an optional sign,
+// digits with an optional fraction, and an optional decimal exponent) or
+// when the result would have more than maxDigits digits; callers should then
+// fall back to their own conversion. Callers are expected to have bounded
+// the magnitude of the result already.
+func ScaleAndRound(s string, scale int64) (val *big.Int, ok bool) {
+ neg := false
+ if len(s) > 0 && (s[0] == '+' || s[0] == '-') {
+ neg = s[0] == '-'
+ s = s[1:]
+ }
+
+ intEnd := 0
+ for intEnd < len(s) && isDigit(s[intEnd]) {
+ intEnd++
+ }
+ intPart, rest := s[:intEnd], s[intEnd:]
+
+ var fracPart string
+ if len(rest) > 0 && rest[0] == '.' {
+ fracEnd := 1
+ for fracEnd < len(rest) && isDigit(rest[fracEnd]) {
+ fracEnd++
+ }
+ fracPart, rest = rest[1:fracEnd], rest[fracEnd:]
+ }
+ if len(intPart)+len(fracPart) == 0 {
+ return nil, false
+ }
+
+ var exp int64
+ if len(rest) > 0 {
+ if rest[0] != 'e' && rest[0] != 'E' {
+ return nil, false
+ }
+ digits := rest[1:]
+ if len(digits) > 0 && (digits[0] == '+' || digits[0] == '-') {
+ digits = digits[1:]
+ }
+ if len(digits) == 0 {
+ return nil, false
+ }
+ for i := 0; i < len(digits); i++ {
+ if !isDigit(digits[i]) {
+ return nil, false
+ }
+ }
+ var err error
+ if exp, err = strconv.ParseInt(rest[1:], 10, 64); err != nil {
+ return nil, false
+ }
+ }
+
+ // The value is digits * 10^shift, where digits is the significand with
+ // the decimal point removed.
+ digits := trimLeadingZeros(intPart + fracPart)
Review Comment:
nit: for the hot path, joining `intPart+fracPart` allocates a new string,
and `big.Int.Exp` below allocates as well. Setting the big.Int from the two
parts and using the existing powers-of-ten tables would avoid both.
--
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]
