-
Notifications
You must be signed in to change notification settings - Fork 847
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
Pass generate_decimal256_case integration test, add DataType::Decimal256
#2094
Conversation
884e717
to
bd591f3
Compare
Codecov Report
@@ Coverage Diff @@
## master #2094 +/- ##
==========================================
- Coverage 83.73% 83.68% -0.05%
==========================================
Files 225 225
Lines 59412 59461 +49
==========================================
+ Hits 49748 49760 +12
- Misses 9664 9701 +37
Continue to review full report at Codecov.
|
bd591f3
to
1fa95e5
Compare
1fa95e5
to
9b005d3
Compare
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.
This is very cool @viirya
I wonder if anyone else has thoughts about how to name the new DataType?
@@ -195,6 +195,8 @@ pub enum DataType { | |||
/// | |||
/// For example the number 123.45 has precision 5 and scale 2. | |||
Decimal(usize, usize), | |||
/// Exact decimal value with 256 bits width | |||
Decimal256(usize, usize), |
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.
Should we also rename Decimal
to Decimal128
?
Or maybe use an extra mode in Decimal like `
enum BitWidth {
// 128 bits
Bits128,
// 256 bits
Bits256,
}
enum DataType {
..
Decimal(usize, usize, DecimalWidth)
}
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.
I thought to add a parameter to current Decimal
like you put above. But soon I realize that I need to modify many places. And we need to do bit width check there. As in C++ and Python implementations, they have Decimal128Type and Decimal256Type separately. I feel that it is simple to work with.
And, yea, we need to rename Decimal
to Decimal128
. Although I've not done it in this change as this is basically for Decimal256 interop test. I may work on it in another PR.
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.
And, yea, we need to rename Decimal to Decimal128.
I agree
I may work on it in another PR.
Thank you!
I see #2101 as well so 👍 |
DataType::Decimal256
Benchmark runs are scheduled for baseline = 576069a and contender = fd38664. fd38664 is a master commit associated with this PR. Results will be available as each benchmark for each run completes. |
Which issue does this PR close?
Closes #2093.
Rationale for this change
What changes are included in this PR?
Are there any user-facing changes?