-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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: [C++][Format] Add initial Decimal32/Decimal64 implementations #43957
Conversation
|
@@ -171,7 +171,8 @@ using PrimitiveArrowTypes = | |||
using TemporalArrowTypes = | |||
::testing::Types<Date32Type, Date64Type, TimestampType, Time32Type, Time64Type>; | |||
|
|||
using DecimalArrowTypes = ::testing::Types<Decimal128Type, Decimal256Type>; | |||
using DecimalArrowTypes = | |||
::testing::Types</*Decimal32Type, Decimal64Type,*/ Decimal128Type, Decimal256Type>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto here. (Should we file issues to come back to these?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are commented out because we didn't implement casting for the new decimal types. This is mentioned in the issue as check boxes to do rather than as an entirely separate issue currently.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But it's going to be a separate PR, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, i didn't want to make this already large PR even larger. I'll implement the cast kernels and so on as a follow-up PR
I'm afraid this may massively break user code. I would suggest another approach:
|
I just have the same concern. +1 on the proposed workaround. |
Co-authored-by: Antoine Pitrou <pitrou@free.fr>
154ea65
to
af8c722
Compare
After merging your PR, Conbench analyzed the 4 benchmarking runs that have been run so far on merge-commit d55d4c6. There were no benchmark performance regressions. 🎉 The full Conbench report has more details. It also includes information about 27 possible false positives for unstable benchmarks that are known to sometimes produce them. |
Hmm, did you notice the UBSAN failure in (you can easily run this build locally using |
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 C++ 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.
Are there any user-facing changes?
Currently if a user is using
decimal(precision, scale)
rather thandecimal128(precision, scale)
they will get aDecimal128Type
if the precision is <= 38 (max precision for Decimal128) andDecimal256Type
if the precision is higher. Following the same pattern, this change means that usingdecimal(precision, scale)
instead of the specificdecimal32
/decimal64
/decimal128
/decimal256
functions results in the following functionality:Decimal32Type
Decimal64Type
Decimal128Type
Decimal256Type
While many of our tests currently make the assumption that
decimal
with a low precision would beDecimal128
and had to be updated, this may cause an initial surprise if users are making the same assumptions.