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.

Sign up for GitHub

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

Jump to bottom

Decimal cleanup (#2637) #2865

Merged
merged 4 commits into from Oct 14, 2022
Merged

Decimal cleanup (#2637) #2865

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
5bf214a
Decimal cleanup (#2637)
tustvold Oct 13, 2022
7987be7
Merge remote-tracking branch 'upstream/master' into decimal-cleanup
tustvold Oct 14, 2022
1d3f434
Format
tustvold Oct 14, 2022
db46910
Merge remote-tracking branch 'upstream/master' into decimal-cleanup
tustvold Oct 14, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
10 changes: 9 additions & 1 deletion .github/workflows/arrow.yml
View file
Open in desktop
Expand Up @@ -186,6 +186,14 @@ jobs:
- name: Setup Clippy
run: |
rustup component add clippy
- name: Run clippy
- name: Clippy arrow-buffer with all features
Copy link
Contributor Author

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

run: cargo clippy -p arrow-buffer --all-features
- name: Clippy arrow-data with all features
run: cargo clippy -p arrow-data --all-features
- name: Clippy arrow-schema with all features
run: cargo clippy -p arrow-schema --all-features
- name: Clippy arrow-array with all features
run: cargo clippy -p arrow-array --all-features
- name: Clippy arrow
run: |
cargo clippy -p arrow --features=prettyprint,csv,ipc,test_utils,ffi,ipc_compression,dyn_cmp_dict,dyn_arith_dict --all-targets -- -D warnings
96 changes: 44 additions & 52 deletions arrow-array/src/array/primitive_array.rs
View file
Open in desktop
Expand Up @@ -16,7 +16,6 @@
// under the License.

use crate::builder::{BooleanBufferBuilder, BufferBuilder, PrimitiveBuilder};
use crate::decimal::Decimal;
use crate::iterator::PrimitiveIter;
use crate::raw_pointer::RawPtrBox;
use crate::temporal_conversions::{as_date, as_datetime, as_duration, as_time};
Expand Down Expand Up @@ -917,67 +916,62 @@ impl<T: DecimalType + ArrowPrimitiveType> PrimitiveArray<T> {
})
}

pub fn value_as_string(&self, row: usize) -> Result<String, ArrowError> {
let p = self.precision()?;
let s = self.scale()?;
Ok(Decimal::<T>::new(p, s, &T::to_native(self.value(row))).to_string())
/// Returns [`Self::value`] formatted as a string
pub fn value_as_string(&self, row: usize) -> String {
T::format_decimal(self.value(row), self.precision(), self.scale())
}

pub fn precision(&self) -> Result<u8, ArrowError> {
/// Returns the decimal precision of this array
pub fn precision(&self) -> u8 {
Copy link
Contributor Author

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

match T::BYTE_LENGTH {
16 => {
if let DataType::Decimal128(p, _) = self.data().data_type() {
Ok(*p)
*p
} else {
Err(ArrowError::InvalidArgumentError(format!(
unreachable!(
"Decimal128Array datatype is not DataType::Decimal128 but {}",
self.data_type()
)))
)
}
}
32 => {
if let DataType::Decimal256(p, _) = self.data().data_type() {
Ok(*p)
*p
} else {
Err(ArrowError::InvalidArgumentError(format!(
unreachable!(
"Decimal256Array datatype is not DataType::Decimal256 but {}",
self.data_type()
)))
)
}
}
other => Err(ArrowError::InvalidArgumentError(format!(
"Unsupported byte length for decimal array {}",
other
))),
other => unreachable!("Unsupported byte length for decimal array {}", other),
}
}

pub fn scale(&self) -> Result<u8, ArrowError> {
/// Returns the decimal scale of this array
pub fn scale(&self) -> u8 {
match T::BYTE_LENGTH {
16 => {
if let DataType::Decimal128(_, s) = self.data().data_type() {
Ok(*s)
*s
} else {
Err(ArrowError::InvalidArgumentError(format!(
unreachable!(
"Decimal128Array datatype is not DataType::Decimal128 but {}",
self.data_type()
)))
)
}
}
32 => {
if let DataType::Decimal256(_, s) = self.data().data_type() {
Ok(*s)
*s
} else {
Err(ArrowError::InvalidArgumentError(format!(
unreachable!(
"Decimal256Array datatype is not DataType::Decimal256 but {}",
self.data_type()
)))
)
}
}
other => Err(ArrowError::InvalidArgumentError(format!(
"Unsupported byte length for decimal array {}",
other
))),
other => unreachable!("Unsupported byte length for decimal array {}", other),
}
}
}
Expand Down Expand Up @@ -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));
Copy link
Contributor Author

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

assert_eq!(-8_887_000_000_i128, decimal_array.value(1));
}

#[test]
Expand All @@ -1576,7 +1570,7 @@ mod tests {
let result = decimal_builder.finish().with_precision_and_scale(5, 3);
assert!(result.is_ok());
let arr = result.unwrap();
assert_eq!("12.345", arr.value_as_string(1).unwrap());
assert_eq!("12.345", arr.value_as_string(1));

// Validate it explicitly
let result = arr.validate_decimal_precision(5);
Expand All @@ -1594,8 +1588,8 @@ mod tests {
let result = decimal_builder.finish().with_precision_and_scale(2, 1);
assert!(result.is_ok());
let arr = result.unwrap();
assert_eq!("9.9", arr.value_as_string(1).unwrap());
assert_eq!("-9.9", arr.value_as_string(3).unwrap());
assert_eq!("9.9", arr.value_as_string(1));
assert_eq!("-9.9", arr.value_as_string(3));

// Validate it explicitly
let result = arr.validate_decimal_precision(2);
Expand All @@ -1611,11 +1605,11 @@ mod tests {
let array = Decimal128Array::from_iter_values(vec![-100, 0, 101].into_iter());
assert_eq!(array.len(), 3);
assert_eq!(array.data_type(), &DataType::Decimal128(38, 10));
assert_eq!(-100_i128, array.value(0).into());
assert_eq!(-100_i128, array.value(0));
assert!(!array.is_null(0));
assert_eq!(0_i128, array.value(1).into());
assert_eq!(0_i128, array.value(1));
assert!(!array.is_null(1));
assert_eq!(101_i128, array.value(2).into());
assert_eq!(101_i128, array.value(2));
assert!(!array.is_null(2));
}

Expand All @@ -1625,10 +1619,10 @@ mod tests {
vec![Some(-100), None, Some(101)].into_iter().collect();
assert_eq!(array.len(), 3);
assert_eq!(array.data_type(), &DataType::Decimal128(38, 10));
assert_eq!(-100_i128, array.value(0).into());
assert_eq!(-100_i128, array.value(0));
assert!(!array.is_null(0));
assert!(array.is_null(1));
assert_eq!(101_i128, array.value(2).into());
assert_eq!(101_i128, array.value(2));
assert!(!array.is_null(2));
}

Expand Down Expand Up @@ -1661,13 +1655,13 @@ mod tests {
.with_precision_and_scale(6, 3)
.unwrap();

assert_eq!("123.450", arr.value_as_string(0).unwrap());
assert_eq!("-123.450", arr.value_as_string(1).unwrap());
assert_eq!("0.100", arr.value_as_string(2).unwrap());
assert_eq!("-0.100", arr.value_as_string(3).unwrap());
assert_eq!("0.010", arr.value_as_string(4).unwrap());
assert_eq!("-0.010", arr.value_as_string(5).unwrap());
assert_eq!("0.000", arr.value_as_string(6).unwrap());
assert_eq!("123.450", arr.value_as_string(0));
assert_eq!("-123.450", arr.value_as_string(1));
assert_eq!("0.100", arr.value_as_string(2));
assert_eq!("-0.100", arr.value_as_string(3));
assert_eq!("0.010", arr.value_as_string(4));
assert_eq!("-0.010", arr.value_as_string(5));
assert_eq!("0.000", arr.value_as_string(6));
}

#[test]
Expand All @@ -1677,11 +1671,11 @@ mod tests {
.unwrap();

assert_eq!(arr.data_type(), &DataType::Decimal128(20, 2));
assert_eq!(arr.precision().unwrap(), 20);
assert_eq!(arr.scale().unwrap(), 2);
assert_eq!(arr.precision(), 20);
assert_eq!(arr.scale(), 2);

let actual: Vec<_> = (0..arr.len())
.map(|i| arr.value_as_string(i).unwrap())
.map(|i| arr.value_as_string(i))
.collect();
let expected = vec!["123.45", "4.56", "78.90", "-1232234234324.32"];

Expand Down Expand Up @@ -1748,9 +1742,7 @@ mod tests {
let value2 = i256::from_i128(56789);

let mut array: Decimal256Array =
vec![Some(value1.clone()), None, Some(value2.clone())]
.into_iter()
.collect();
vec![Some(value1), None, Some(value2)].into_iter().collect();
array = array.with_precision_and_scale(76, 10).unwrap();
assert_eq!(array.len(), 3);
assert_eq!(array.data_type(), &DataType::Decimal256(76, 10));
Expand All @@ -1768,10 +1760,10 @@ mod tests {
array = array.with_precision_and_scale(38, 10).unwrap();
assert_eq!(array.len(), 3);
assert_eq!(array.data_type(), &DataType::Decimal128(38, 10));
assert_eq!(-100_i128, array.value(0).into());
assert_eq!(-100_i128, array.value(0));
assert!(!array.is_null(0));
assert!(array.is_null(1));
assert_eq!(101_i128, array.value(2).into());
assert_eq!(101_i128, array.value(2));
assert!(!array.is_null(2));
}

Expand Down