zeroshade commented on code in PR #1501: URL: https://github.com/apache/arrow-go/pull/1501#discussion_r4527837807
########## 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: For `decimal128`/`decimal256`, `ToBigFloat` indexes a 39- or 77-entry table of powers of ten, so this fast path still panics for scales between the table size and 1000. Calling `ValueStr(0)` on a `decimal128(38, s)` array holding 12345: | scale | result | |---|---| | 39 | panic: index out of range [39] with length 39 | | 500 | panic: index out of range [500] | | 1000 | panic: index out of range [1000] | | 1001 | `1.2345e-997` | So scales above 1000 work, while scales 39–1000 still panic. Could this take the coefficient-and-exponent path whenever the scale is outside the type's table range? Or drop the `Decimal128`/`Decimal256` `ValueStr` claim from this PR and handle it separately. The `Decimal128` scalar's `String()`/`CastTo(string)` doesn't use this helper and panics the same way (for example at scale 100000). ########## 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: nit: `>=` now also rejects `MinInt32`/`MinInt64`. That's fine, because a precision of at most 9 or 18 digits can't hold them anyway, but a short comment would explain why `>=` is intentional. ########## 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: nit: `prec+scale+1` is int32 arithmetic and wraps for scales near `MaxInt32`. The resulting negative float converted to `uint` behaves differently per CPU: arm64 saturates to 0, so the precision becomes 1 bit, while amd64 produces a huge value that `SetPrec` caps. The `MaxInt32` test case still errors on both, but by different routes. Computing in int64/float64 removes the dependence. ########## arrow/compute/scalar_nested.go: ########## @@ -94,8 +94,7 @@ type listElementFunction struct { func listElementScalarResultSupported(typ arrow.DataType, nullResult bool) bool { switch typ.ID() { - case arrow.BINARY_VIEW, arrow.STRING_VIEW, arrow.LIST_VIEW, arrow.LARGE_LIST_VIEW, - arrow.DECIMAL32, arrow.DECIMAL64: + case arrow.BINARY_VIEW, arrow.STRING_VIEW, arrow.LIST_VIEW, arrow.LARGE_LIST_VIEW: Review Comment: Heads up: #1490 edits these same `case` lines (here and in `listElementScalarArrayValueSupported`) to remove the view types, so whichever lands second will conflict. After both, only `LIST_VIEW`/`LARGE_LIST_VIEW` should remain. -- 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]
