hcrosse commented on code in PR #1501: URL: https://github.com/apache/arrow-go/pull/1501#discussion_r4528294792
########## internal/utils/decimal_format.go: ########## @@ -0,0 +1,46 @@ +// 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 utils + +import ( + "fmt" + "math/big" + "strconv" + "strings" +) + +// FormatDecimalG formats an Arrow decimal in 'g' style. At absolute scales +// above 1000, it rounds the exact coefficient and shifts the exponent, +// avoiding conversion work quadratic in the scale. Exact ties round to even. +func FormatDecimalG[T interface{ ToBigFloat(int32) *big.Float }](n T, precision, scale int32) string { + if scale >= -1000 && scale <= 1000 { Review Comment: Thanks. I removed the `Decimal128` and `Decimal256` `ValueStr` sentence from the description and left the code unchanged. I opened an issue for formatting at every scale past their tables, including the existing scalar `String` and string casts: https://github.com/apache/arrow-go/issues/1507 ########## arrow/decimal/decimal.go: ########## @@ -190,12 +196,18 @@ func decimalFromString[T interface { return n, fmt.Errorf("bitlen too large for decimal%d", nbits) } - n = T(val.Int64() / int64(math.Pow10(int(-scale)))) + // |val| < 2^64 < 10^20, so a larger divisor gives the same quotient. + div := new(big.Int).Exp(big.NewInt(10), big.NewInt(min(-int64(scale), 20)), nil) + n = T(val.Quo(val, div).Int64()) } else { var precInBits = uint(math.Round(float64(prec+scale+1)/math.Log10(2))) + 1 Review Comment: I now compute the sum in `int64` and clamp before converting to `uint`, keeping the final precision between 1 and `big.MaxPrec` on every CPU. ########## arrow/decimal/decimal.go: ########## @@ -204,7 +216,7 @@ func decimalFromString[T interface { var tmp big.Int val, _ := out.Int(&tmp) - if val.BitLen() > int(nbits) { + if val.BitLen() >= int(nbits) { Review Comment: I added a comment explaining why `>=` also rejects the signed minimum. -- 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]
