-
Notifications
You must be signed in to change notification settings - Fork 759
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
Add validation for Decimal256 #2113
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2113 +/- ##
==========================================
- Coverage 83.71% 83.70% -0.01%
==========================================
Files 225 225
Lines 59563 59625 +62
==========================================
+ Hits 49862 49911 +49
- Misses 9701 9714 +13
Help us with your feedback. Take ten seconds to tell us how you rate us. |
c36f2df
to
21e8457
Compare
@@ -298,6 +299,49 @@ pub const MAX_DECIMAL_FOR_EACH_PRECISION: [i128; 38] = [ | |||
99999999999999999999999999999999999999, | |||
]; | |||
|
|||
/// `MAX_DECIMAL_FOR_LARGER_PRECISION[p]` holds the maximum integer value | |||
/// that can be stored in [DataType::Decimal256] value of precision `p` > 38 |
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.
Depends on #2093 which adds DataType::Decimal256
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.
So this causes doc failure in CI.
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 took the liberty of merging the latest apache/master
into this PR
arrow/src/array/data.rs
Outdated
@@ -1018,6 +1018,7 @@ impl ArrayData { | |||
} | |||
Ok(()) | |||
} | |||
// TODO: call validate_decimal256_precision for Decimal256 type |
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.
Is this still a TODO for this 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.
I was waiting for #2093 where added Decimal256
type, so I can call validate_decimal256_precision
here. Let me update this.
11b2958
to
a9bf22a
Compare
@@ -352,7 +352,7 @@ impl From<ArrayData> for Decimal256Array { | |||
assert_eq!( | |||
data.buffers().len(), | |||
1, | |||
"Decimal128Array data should contain 1 buffer only (values)" | |||
"Decimal256Array data should contain 1 buffer only (values)" |
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.
fix typo
integration-testing/src/lib.rs
Outdated
b.append_value(&decimal)? | ||
} | ||
_ => b.append_null(), | ||
}?; | ||
}; |
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.
Not sure why this was not caught in CI previously.
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 think it was a logical conflict: #2121
a9bf22a
to
6dfd0b0
Compare
Thanks @alamb |
Benchmark runs are scheduled for baseline = c3e019f and contender = 34f58f9. 34f58f9 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 #2112.
Rationale for this change
What changes are included in this PR?
Are there any user-facing changes?