Skip to content

Test : move tests for parse_string_decimal_native to parse_decimal #7177

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

Closed
himadripal wants to merge 4 commits into from
Closed

Test : move tests for parse_string_decimal_native to parse_decimal #7177

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
746387a
change test to use parse decimal
himadripal Feb 22, 2025
7738768
move the mod.rs tests for parse_string_to_decimal_native to parse_dec…
himadripal Feb 22, 2025
359a5ec
revert the code change, only test move, comment the failed test.
himadripal Feb 22, 2025
296e0fd
disable a test for scale=0
himadripal Feb 22, 2025
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
37 changes: 14 additions & 23 deletions arrow-cast/src/cast/decimal.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -590,43 +590,34 @@ where
#[cfg(test)]
mod tests {
use super::*;
use crate::parse::parse_decimal;

#[test]
fn test_parse_string_to_decimal_native() -> Result<(), ArrowError> {
assert_eq!(
parse_string_to_decimal_native::<Decimal128Type>("0", 0)?,
0_i128
);
assert_eq!(
parse_string_to_decimal_native::<Decimal128Type>("0", 5)?,
0_i128
);
assert_eq!(parse_decimal::<Decimal128Type>("0", 38, 0)?, 0_i128);
assert_eq!(parse_decimal::<Decimal128Type>("0", 38, 5)?, 0_i128);

assert_eq!(parse_decimal::<Decimal128Type>("123", 38, 0)?, 123_i128);
assert_eq!(
parse_string_to_decimal_native::<Decimal128Type>("123", 0)?,
123_i128
);
assert_eq!(
parse_string_to_decimal_native::<Decimal128Type>("123", 5)?,
parse_decimal::<Decimal128Type>("123", 38, 5)?,
12300000_i128
);

// `parse_decimal` does not handle scale=0 correctly. will enable it as part of code change PR.
// assert_eq!(parse_decimal::<Decimal128Type>("123.45", 38, 0)?, 123_i128);
Copy link
Contributor Author

@himadripal himadripal Feb 22, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

parse_decimal does not behave correctly when scale=0 when there are decimal digits in the original string. fix is in this PR #7179

assert_eq!(
parse_string_to_decimal_native::<Decimal128Type>("123.45", 0)?,
123_i128
);
assert_eq!(
parse_string_to_decimal_native::<Decimal128Type>("123.45", 5)?,
parse_decimal::<Decimal128Type>("123.45", 38, 5)?,
12345000_i128
);

assert_eq!(
parse_string_to_decimal_native::<Decimal128Type>("123.4567891", 0)?,
//scale = 0 is not handled correctly in parse_decimal, next PR will fix it and enable this.
Copy link
Contributor

@alamb alamb Feb 26, 2025

Choose a reason for hiding this comment

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

Doens't this comment out existing tests? Why would you reduce test coverage?

Copy link
Contributor Author

@himadripal himadripal Feb 26, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

So, this is commented as parse_decimal do not have handling of scale 0 well, Fix for scale 0 in parse_decimal is here #7179 and this test is enabled back there - all of these changes are all together in #6905

Copy link
Contributor

@alamb alamb Feb 26, 2025

Choose a reason for hiding this comment

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

But my point is that this PR actually changes what is tested (it isn't just a migration / refactor).
I expect a refactor to change how the code is structured but not change the coverage

This comment was marked as outdated.

Sign in to view

This comment was marked as outdated.

Sign in to view

Copy link
Contributor Author

@himadripal himadripal Feb 27, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Now, we can close this PR, as I added additional test in #7179 to check the values generated by parse_string_to_decimal_native and parse_decimal are same. So no change in existing test or coverage.

/*assert_eq!(
parse_decimal::<Decimal128Type>("123.4567891", 38, 0)?,
123_i128
);
);*/
assert_eq!(
parse_string_to_decimal_native::<Decimal128Type>("123.4567891", 5)?,
12345679_i128
parse_decimal::<Decimal128Type>("123.4567891", 38, 5)?,
12345678_i128
);
Ok(())
}
Expand Down
29 changes: 15 additions & 14 deletions arrow-cast/src/cast/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2383,6 +2383,7 @@ where
#[cfg(test)]
mod tests {
use super::*;
use crate::parse::parse_decimal;
use arrow_buffer::{Buffer, IntervalDayTime, NullBuffer};
use chrono::NaiveDate;
use half::f16;
Expand Down Expand Up @@ -8416,92 +8417,92 @@ mod tests {
fn test_parse_string_to_decimal() {
assert_eq!(
Decimal128Type::format_decimal(
parse_string_to_decimal_native::<Decimal128Type>("123.45", 2).unwrap(),
parse_decimal::<Decimal128Type>("123.45", 38, 2).unwrap(),
Copy link
Contributor

@alamb alamb Feb 26, 2025

Choose a reason for hiding this comment

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

This test is now testing a different function. I am not familar with the difference betwen parse_string_to_decimal and parse_decima

Copy link
Contributor Author

@himadripal himadripal Feb 26, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Idea is to move from parse_string_to_decimal_native and to use parse_decimal while doing string to decimal casting here. Main reason is that parse_decimal has support for e-notation and is also being used in readers like (i.e arrow-csv, arrow-json and also performant, some discussion here

Copy link
Contributor

@alamb alamb Feb 26, 2025

Choose a reason for hiding this comment

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

so is your plan to remove the parse_string_to_decimal function?

Copy link
Contributor Author

@himadripal himadripal Feb 26, 2025

Choose a reason for hiding this comment

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

Yes, to be consistent, readers and cast will be using same function.

38,
2,
),
"123.45"
);
assert_eq!(
Decimal128Type::format_decimal(
parse_string_to_decimal_native::<Decimal128Type>("12345", 2).unwrap(),
parse_decimal::<Decimal128Type>("12345", 38, 2).unwrap(),
38,
2,
),
"12345.00"
);
assert_eq!(
Decimal128Type::format_decimal(
parse_string_to_decimal_native::<Decimal128Type>("0.12345", 2).unwrap(),
parse_decimal::<Decimal128Type>("0.12345", 38, 2).unwrap(),
38,
2,
),
"0.12"
);
assert_eq!(
Decimal128Type::format_decimal(
parse_string_to_decimal_native::<Decimal128Type>(".12345", 2).unwrap(),
parse_decimal::<Decimal128Type>(".12345", 38, 2).unwrap(),
38,
2,
),
"0.12"
);
assert_eq!(
Decimal128Type::format_decimal(
parse_string_to_decimal_native::<Decimal128Type>(".1265", 2).unwrap(),
parse_decimal::<Decimal128Type>(".1265", 38, 2).unwrap(),
Copy link
Contributor Author

@himadripal himadripal Feb 22, 2025

Choose a reason for hiding this comment

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

parse_decimal does not support rounding yet. it does truncate instead

38,
2,
),
"0.13"
"0.12"
);
assert_eq!(
Decimal128Type::format_decimal(
parse_string_to_decimal_native::<Decimal128Type>(".1265", 2).unwrap(),
parse_decimal::<Decimal128Type>(".1265", 38, 2).unwrap(),
38,
2,
),
"0.13"
"0.12"
Copy link
Contributor Author

@himadripal himadripal Feb 22, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

parse_decimal does not support rounding yet. it does truncate instead, fix is in this PR #7179. This change is put back in the PR #7179

);

assert_eq!(
Decimal256Type::format_decimal(
parse_string_to_decimal_native::<Decimal256Type>("123.45", 3).unwrap(),
parse_decimal::<Decimal256Type>("123.45", 38, 3).unwrap(),
38,
3,
),
"123.450"
);
assert_eq!(
Decimal256Type::format_decimal(
parse_string_to_decimal_native::<Decimal256Type>("12345", 3).unwrap(),
parse_decimal::<Decimal256Type>("12345", 38, 3).unwrap(),
38,
3,
),
"12345.000"
);
assert_eq!(
Decimal256Type::format_decimal(
parse_string_to_decimal_native::<Decimal256Type>("0.12345", 3).unwrap(),
parse_decimal::<Decimal256Type>("0.12345", 38, 3).unwrap(),
38,
3,
),
"0.123"
);
assert_eq!(
Decimal256Type::format_decimal(
parse_string_to_decimal_native::<Decimal256Type>(".12345", 3).unwrap(),
parse_decimal::<Decimal256Type>(".12345", 38, 3).unwrap(),
38,
3,
),
"0.123"
);
assert_eq!(
Decimal256Type::format_decimal(
parse_string_to_decimal_native::<Decimal256Type>(".1265", 3).unwrap(),
parse_decimal::<Decimal256Type>(".1265", 38, 3).unwrap(),
38,
3,
),
"0.127"
"0.126"
Copy link
Contributor Author

@himadripal himadripal Feb 22, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

parse_decimal does not support rounding yet. it does truncate instead

);
}

Expand Down
Loading