-
Notifications
You must be signed in to change notification settings - Fork 841
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
Decimal cleanup (#2637) #2865
Decimal cleanup (#2637) #2865
Conversation
@@ -186,6 +186,14 @@ jobs: | |||
- name: Setup Clippy | |||
run: | | |||
rustup component add clippy | |||
- name: Run clippy | |||
- name: Clippy arrow-buffer with all features |
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 was a consequence of #2594 and was preventing linting of the split apart crates
} | ||
|
||
pub fn precision(&self) -> Result<u8, ArrowError> { | ||
/// Returns the decimal precision of this array | ||
pub fn precision(&self) -> u8 { |
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 methods can be made infallible as the error case is actually unreachable, as of #2835 we consistently verify the data type
@@ -1564,8 +1558,8 @@ mod tests { | |||
.build() | |||
.unwrap(); | |||
let decimal_array = Decimal128Array::from(array_data); | |||
assert_eq!(8_887_000_000_i128, decimal_array.value(0).into()); | |||
assert_eq!(-8_887_000_000_i128, decimal_array.value(1).into()); | |||
assert_eq!(8_887_000_000_i128, decimal_array.value(0)); |
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 Into::into
conversions are redundant and were making clippy unhappy
@@ -569,6 +565,26 @@ impl ArrowPrimitiveType for Decimal256Type { | |||
const DATA_TYPE: DataType = <Self as DecimalType>::DEFAULT_TYPE; | |||
} | |||
|
|||
fn format_decimal_str(value_str: &str, precision: usize, scale: usize) -> String { |
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 based on the logic from Decimal::to_string
, adding this allows removing the now unnecessary Decimal
construct.
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.
Thanks @tustvold for the cleanups. I will review this in next few days.
Benchmark runs are scheduled for baseline = d7f994c and contender = d67d5fb. d67d5fb 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?
Part of #2637
Rationale for this change
Follow on to #2857 that takes the opportunity to perform some further cleanups
What changes are included in this PR?
Makes some APIs infallible, and removes the no longer needed
Decimal
abstractionAre there any user-facing changes?
No, #2857 hasn't been released yet
FYI @alamb it would be good for some version of this to make the next release if possible, to avoid API churn