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

Optionally disable validate_decimal_precision check in DecimalBuilder.append_value for interop test #1767

Merged
merged 10 commits into from
Jun 3, 2022
Merged

Optionally disable validate_decimal_precision check in DecimalBuilder.append_value for interop test #1767

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
6367da4
Remove precision check
viirya May 31, 2022
e2ad586
Remove test
viirya May 31, 2022
da44930
Fix clippy
viirya May 31, 2022
3a221ce
Move decimal precision check to ArrayData full validation and add test
viirya May 31, 2022
b289f11
Increase precision to pass existing test
viirya May 31, 2022
2d8a69c
Fix clippy
viirya May 31, 2022
5665071
Fix tests
viirya May 31, 2022
d6eeba5
Add disable_value_validation to optionally disable value validation a…
viirya Jun 3, 2022
48798f0
Simplify validation code
viirya Jun 3, 2022
7a6be90
Update test
viirya Jun 3, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 1 addition & 33 deletions arrow/src/array/array_binary.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1056,7 +1056,7 @@ mod tests {
use std::sync::Arc;

use crate::{
array::{DecimalBuilder, LargeListArray, ListArray},
array::{LargeListArray, ListArray},
datatypes::{Field, Schema},
record_batch::RecordBatch,
};
Expand Down Expand Up @@ -1497,38 +1497,6 @@ mod tests {
assert_eq!(16, decimal_array.value_length());
}

#[test]
fn test_decimal_append_error_value() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I recommend we change this test to show that it is ok to store these values in a decimal rather than removing it completely (aka change the test to validate there is no error).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed the test. As I move the precision check to ArrayData full validation, I add another test for that too.

let mut decimal_builder = DecimalBuilder::new(10, 5, 3);
let mut result = decimal_builder.append_value(123456);
let mut error = result.unwrap_err();
assert_eq!(
"Invalid argument error: 123456 is too large to store in a Decimal of precision 5. Max is 99999",
error.to_string()
);
decimal_builder.append_value(12345).unwrap();
let arr = decimal_builder.finish();
assert_eq!("12.345", arr.value_as_string(0));

decimal_builder = DecimalBuilder::new(10, 2, 1);
result = decimal_builder.append_value(100);
error = result.unwrap_err();
assert_eq!(
"Invalid argument error: 100 is too large to store in a Decimal of precision 2. Max is 99",
error.to_string()
);
decimal_builder.append_value(99).unwrap();
result = decimal_builder.append_value(-100);
error = result.unwrap_err();
assert_eq!(
"Invalid argument error: -100 is too small to store in a Decimal of precision 2. Min is -99",
error.to_string()
);
decimal_builder.append_value(-99).unwrap();
let arr = decimal_builder.finish();
assert_eq!("9.9", arr.value_as_string(0));
assert_eq!("-9.9", arr.value_as_string(1));
}
#[test]
fn test_decimal_from_iter_values() {
let array = DecimalArray::from_iter_values(vec![-100, 0, 101].into_iter());
Expand Down
1 change: 0 additions & 1 deletion arrow/src/array/builder.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1462,7 +1462,6 @@ impl DecimalBuilder {
/// distinct array element.
#[inline]
pub fn append_value(&mut self, value: i128) -> Result<()> {
let value = validate_decimal_precision(value, self.precision)?;
let value_as_bytes = Self::from_i128_to_fixed_size_bytes(
value,
self.builder.value_length() as usize,
Expand Down