Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

GH-43956: [Go][Format] Add initial Decimal32/Decimal64 implementation #43958

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

zeroshade
Copy link
Member

@zeroshade zeroshade commented Sep 5, 2024

Rationale for this change

Widening the Decimal128/256 type to allow for bitwidths of 32 and 64 allows for more interoperability with other libraries and utilities which already support these types. This provides even more opportunities for zero-copy interactions between things such as libcudf and various databases.

What changes are included in this PR?

This PR contains the basic Go implementations for Decimal32/Decimal64 types, arrays, builders and scalars. It also includes the minimum necessary to get everything compiling and tests passing without also extending the acero kernels and parquet handling (both of which will be handled in follow-up PRs).

Are these changes tested?

Yes, tests were extended where applicable to add decimal32/decimal64 cases.

Copy link
Member

@lidavidm lidavidm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: PR description still says "C++" 🙂

Also, should we be making this PR against arrow-go now?

@github-actions github-actions bot added awaiting changes Awaiting changes and removed awaiting committer review Awaiting committer review labels Sep 6, 2024
Comment on lines 36 to 43
type decimalTypes interface {
decimal.Decimal32 | decimal.Decimal64 | decimal.Decimal128 | decimal.Decimal256
}

type baseDecimal[T interface {
decimalTypes
decimal.Num[T]
}] struct {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
type decimalTypes interface {
decimal.Decimal32 | decimal.Decimal64 | decimal.Decimal128 | decimal.Decimal256
}
type baseDecimal[T interface {
decimalTypes
decimal.Num[T]
}] struct {
type decimalType[T any] interface {
arrow.FixedWidthType
decimal.Num[T]
}
type baseDecimal[T decimalType[T]] struct {

What would you think consolidating to this decimalType interface and using it wherever interface { decimalTypes; decimal.Num[T] } is currently used? The only functionality the decimal array uses that's not defined by decimal.Num AFAICT is GetData, so intersecting the interface with the arrow.FixedWidthType constraint seems appropriate.

This would also require relaxing the type constraint in decimal.Num's definition i.e. type Num[T any] interface {...}, which seems ok IMO as it would allow other decimal implementations (such as Decimal8/16) to be added in the future without needing to update the list of valid decimal types.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think I want to allow relaxing to T any for the decimal definition, but to avoid having multiple spots that we'd have to update for adding future impls (such as Decimal8/16) I'll create a DecimalTypes interface in the decimal package that is exported and then use that everywhere. That way there's only one place that would need to be updated if we add those new implementations.

I'm also going to shift the decTraits interface and implementations into the decimal package in order to simplify some code.

Comment on lines 155 to 163
func (n Decimal32) ToString(scale int32) string {
f := (&big.Float{}).SetInt64(int64(n))
if scale < 0 {
f.SetPrec(32).Mul(f, (&big.Float{}).SetInt64(int64(intPow(10, -scale))))
} else {
f.SetPrec(32).Quo(f, (&big.Float{}).SetInt64(int64(intPow(10, scale))))
}
return f.Text('f', int(scale))
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
func (n Decimal32) ToString(scale int32) string {
f := (&big.Float{}).SetInt64(int64(n))
if scale < 0 {
f.SetPrec(32).Mul(f, (&big.Float{}).SetInt64(int64(intPow(10, -scale))))
} else {
f.SetPrec(32).Quo(f, (&big.Float{}).SetInt64(int64(intPow(10, scale))))
}
return f.Text('f', int(scale))
}
func (n Decimal32) ToString(scale int32) string {
return n.ToBigFloat(scale).Text('f', int(scale))
}

Seems like this logic can be reused. Same for Decimal64.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

simplified

@github-actions github-actions bot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Sep 9, 2024
Copy link
Member

@joellubi joellubi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

minor nit

go/arrow/internal/arrjson/arrjson.go Outdated Show resolved Hide resolved
go/arrow/internal/arrjson/arrjson.go Outdated Show resolved Hide resolved
@github-actions github-actions bot added awaiting changes Awaiting changes and removed awaiting change review Awaiting change review labels Sep 9, 2024
@github-actions github-actions bot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Sep 9, 2024
Copy link
Member

@joellubi joellubi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great!

@github-actions github-actions bot added awaiting merge Awaiting merge and removed awaiting change review Awaiting change review labels Sep 9, 2024
@kou
Copy link
Member

kou commented Sep 9, 2024

Can we work on this in apache/arrow-go instead of here?

Is the TinyGo CI job a blocker for this?

@zeroshade
Copy link
Member Author

@kou The TinyGo CI job isn't a blocker (since I already got it passing here) and I was planning on shifting this to the apache/arrow-go repo rather than merging here. I just didn't feel like changing all the import paths yet until it was ready.

Assuming no one has any further comments, I'll shift it over to the apache/arrow-go repo tomorrow.

@kou
Copy link
Member

kou commented Sep 10, 2024

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants