ARROW-10600: [Go] Implement Decimal256 #13792
Conversation
zeroshade
requested review from
amol-,
andygrove,
kszucs,
pitrou and
emkornfield
go/arrow/decimal256/decimal256.go
Outdated
| func fromPositiveFloat32(v float32, prec, scale int32) (Num, error) { | ||
| var pscale float32 | ||
| if scale >= -76 && scale <= 76 { | ||
| pscale = float32PowersOfTen[scale+76] |
There was a problem hiding this comment.
Wouldn't it be better to cast v to float64 and then call fromPositive64? You would 1) remove a lot of code 2) get better precision (float32PowersOfTen contains some zeros and infinities for very small or large scales)
There was a problem hiding this comment.
So i tried that originally when I was writing the ToFloat functionalities for decimal128 and found (very) small precision issues which I assumed would carry through to the From functions. though right now trying this locally I can't reproduce those precision issues, maybe it was a windows thing? I don't know. That said, given that those precision issues would be outside the bounds of what is considered even reasonable for a float, I agree with you that it's probably just easier/better to cast to float64 and use the fromPositiveFloat64 / tofloat64Positive functions and remove the excess code. I'll go do that.
| if n == (Num{}) { | ||
| return 0 | ||
| } | ||
| return int(1 | (int64(n.arr[3]) >> 63)) |
There was a problem hiding this comment.
Is the purpose here to avoid branching? (otherwise why not a simple conditional?)
There was a problem hiding this comment.
yes, the purpose here was to minimize branching as this could be potentially called frequently. I actually pulled this from code in Go's internals
go/arrow/scalar/scalar.go
Outdated
|
|
||
| switch to.ID() { | ||
| case arrow.DECIMAL256: | ||
| return NewDecimal256Scalar(s.Value, to), nil |
There was a problem hiding this comment.
I think this is missing some casting of s.Value in case the scales don't match.
And even if the scales match, should probably check that the value still fits in the new precision.
There was a problem hiding this comment.
fixed this, calling Rescale and FitsInPrecision
go/arrow/scalar/scalar.go
Outdated
| @@ -297,6 +298,8 @@ func (s *Decimal128) CastTo(to arrow.DataType) (Scalar, error) { | |||
| switch to.ID() { | |||
| case arrow.DECIMAL128: | |||
| return NewDecimal128Scalar(s.Value, to), nil | |||
| case arrow.DECIMAL256: | |||
| return NewDecimal256Scalar(decimal256.FromDecimal128(s.Value), to), nil | |||
There was a problem hiding this comment.
Same as below: should take into account differences in scale or precision.
There was a problem hiding this comment.
fixed calling Rescale and FitsInPrecision
Co-authored-by: Antoine Pitrou <pitrou@free.fr>
Co-authored-by: Antoine Pitrou <pitrou@free.fr>
|
I'm gonna bug you again @wolfeidau to have a look as we still lack many Go developers here who can take a look at these. 😄 After this, the union PR and one more PR afterwards (implementing unions in the IPC handling) Go will officially support all of the Arrow data types! |
go/arrow/array/decimal256.go
Outdated
| // all values in v are appended and considered valid. | ||
| func (b *Decimal256Builder) AppendValues(v []decimal256.Num, valid []bool) { | ||
| if len(v) != len(valid) && len(valid) != 0 { | ||
| panic("len(v) != len(valid) && len(valid) != 0") |
There was a problem hiding this comment.
Is this missing the convention i have seen with the message being prefixed by "arrow: "?
There was a problem hiding this comment.
good point! I've added the prefixes.
Thanks for the reviews! technically Go has better Type support than C++ now! Since C++ doesn't support Float16 lol!
|
There seem to be a lot of Go CI failures suddenly. |
|
@pitrou Yea, when I merged the Union type PR it added new requirements to the Builder types which I hadn't implemented in the |
|
Benchmark runs are scheduled for baseline = 6a3fb97 and contender = 6d1bc62. 6d1bc62 is a master commit associated with this PR. Results will be available as each benchmark for each run completes. |
|
['Python', 'R'] benchmarks have high level of regressions. |
No description provided.