Re: [PR] fix: liberal parsing of zero scale decimals [arrow-rs]
gruuya commented on PR #8700:
URL: https://github.com/apache/arrow-rs/pull/8700#issuecomment-3908756176
> > It's a valid question on what is the expected outcome here, for instance
PG will actually round it up
>
> I am wrapping up the final details for 58.0.0. Is there anything we need
to fix before I release this out into the world?
>
> * [Release arrow-rs / parquet Major version `58.0.0` (January 2026)
#8466](https://github.com/apache/arrow-rs/issues/8466)
I don't think this warrants an immediate fix, since it's still an
improvement on the previous approach which wold return `19` for
`parse_decimal::("1.9", 3, 0).unwrap()`.
Still, a good observation, so i opened a follow-up issue
https://github.com/apache/arrow-rs/issues/9422.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] fix: liberal parsing of zero scale decimals [arrow-rs]
alamb commented on PR #8700: URL: https://github.com/apache/arrow-rs/pull/8700#issuecomment-3907985160 > It's a valid question on what is the expected outcome here, for instance PG will actually round it up I am wrapping up the final details for 58.0.0. Is there anything we need to fix before I release this out into the world? - https://github.com/apache/arrow-rs/issues/8466 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] fix: liberal parsing of zero scale decimals [arrow-rs]
gruuya commented on PR #8700:
URL: https://github.com/apache/arrow-rs/pull/8700#issuecomment-3901573683
> Late drive-by -- it's not obvious to me whether the parsing truncates or
rounds a number like `1.9` and all of the unit tests have fractional parts less
than 0.5. Based on my quick skim of the code not turning up any obvious
references to rounding, I assume it truncates. But is that the desired behavior?
Yes, it will truncate it, i.e. this passes
```rust
assert_eq!(
parse_decimal::("1.9", 3, 0).unwrap(),
1,
);
```
It's a valid question on what is the expected outcome here, for instance PG
will actually round it up
```sql
postgres@localhost:postgres> select '1.9'::numeric(3, 0);
+-+
| numeric |
|-|
| 2 |
+-+
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] fix: liberal parsing of zero scale decimals [arrow-rs]
scovich commented on PR #8700: URL: https://github.com/apache/arrow-rs/pull/8700#issuecomment-3897948966 Late drive-by -- it's not obvious to me whether the parsing truncates or rounds a number like `1.9` and all of the unit tests have fractional parts less than 0.5. Based on my quick skim of the code not turning up any obvious references to rounding, I assume it truncates. But is that the desired behavior? -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] fix: liberal parsing of zero scale decimals [arrow-rs]
alamb merged PR #8700: URL: https://github.com/apache/arrow-rs/pull/8700 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] fix: liberal parsing of zero scale decimals [arrow-rs]
alamb commented on PR #8700: URL: https://github.com/apache/arrow-rs/pull/8700#issuecomment-3768787886 Thanks again for the review, code and work @Jefffrey @gruuya and @martin-g -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] fix: liberal parsing of zero scale decimals [arrow-rs]
alamb commented on code in PR #8700:
URL: https://github.com/apache/arrow-rs/pull/8700#discussion_r2705133133
##
arrow-cast/src/parse.rs:
##
@@ -861,19 +850,19 @@ pub fn parse_decimal(
let mut digits: u8 = 0;
let base = T::Native::usize_as(10);
+if matches!(s, "" | "-" | "+" | "." | "1e" | "1e+" | "1e-") {
Review Comment:
Looks good to me -- thank you
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] fix: liberal parsing of zero scale decimals [arrow-rs]
gruuya commented on code in PR #8700:
URL: https://github.com/apache/arrow-rs/pull/8700#discussion_r2703839494
##
arrow-cast/src/parse.rs:
##
@@ -861,19 +850,19 @@ pub fn parse_decimal(
let mut digits: u8 = 0;
let base = T::Native::usize_as(10);
+if matches!(s, "" | "-" | "+" | "." | "1e" | "1e+" | "1e-") {
Review Comment:
Fair point—yeah we could rely on checking whether the last character is a
digit, but we also need to disallow `"."`, while supporting these
https://github.com/apache/arrow-rs/blob/ba3446bb90cc652a45e909f1caf6a64c39a57609/arrow-cast/src/parse.rs#L2537-L2538
I've converged on this
```rust
if !bs
.last()
.is_some_and(|b| b.is_ascii_digit() || (b == &b'.' && s.len() > 1))
{
// If the last character is not a digit (or a decimal point prefixed
with some digits), then
// it's not a valid decimal.
return Err(ArrowError::ParseError(format!(
"can't parse the string value {s} to decimal"
)));
}
```
let me know how that looks.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] fix: liberal parsing of zero scale decimals [arrow-rs]
alamb commented on code in PR #8700:
URL: https://github.com/apache/arrow-rs/pull/8700#discussion_r2701256793
##
arrow-cast/src/parse.rs:
##
@@ -861,19 +850,19 @@ pub fn parse_decimal(
let mut digits: u8 = 0;
let base = T::Native::usize_as(10);
+if matches!(s, "" | "-" | "+" | "." | "1e" | "1e+" | "1e-") {
Review Comment:
this feels like an oddly specific list of error conditions to check -- It
seems like it should be checking that s has only digits and is not empty?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] fix: liberal parsing of zero scale decimals [arrow-rs]
Jefffrey commented on PR #8700: URL: https://github.com/apache/arrow-rs/pull/8700#issuecomment-3758532551 > > By chance would this PR also address > > ``` > > * [Parsing Decimal With Negative Exponent And Zero Scale Can Overflow #5762](https://github.com/apache/arrow-rs/issues/5762) > > ``` > > > > > > > > > > > > > > > > > > > > > > > > Or is it a different cause? > > No, actually this is the same problem I laid out here [#8700 (comment)](https://github.com/apache/arrow-rs/pull/8700#issuecomment-3696913488). > > And as mentioned one potential solution for it is #9170, where we could avoid redundant multiplication by base factor, followed immediately by division when we parse the e-notation (since then we'd know the exponent in advance, and could stop processing digits on time to avoid the overflow). Thanks for checking, I'll update the issue to link these two. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] fix: liberal parsing of zero scale decimals [arrow-rs]
gruuya commented on PR #8700: URL: https://github.com/apache/arrow-rs/pull/8700#issuecomment-3758379668 > By chance would this PR also address > > * [Parsing Decimal With Negative Exponent And Zero Scale Can Overflow #5762](https://github.com/apache/arrow-rs/issues/5762) > > > Or is it a different cause? No, actually this is the same problem I laid out here https://github.com/apache/arrow-rs/pull/8700#issuecomment-3696913488. And as mentioned one potential solution for it is https://github.com/apache/arrow-rs/issues/9170, where we could avoid redundant multiplication by base factor, followed immediately by division when we parse the e-notation (since then we'd know the exponent in advance, and could stop processing digits on time to avoid the overflow). -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] fix: liberal parsing of zero scale decimals [arrow-rs]
gruuya commented on code in PR #8700:
URL: https://github.com/apache/arrow-rs/pull/8700#discussion_r2697159027
##
arrow-cast/src/parse.rs:
##
@@ -2752,6 +2740,35 @@ mod tests {
let result = parse_decimal::(s, 76, scale);
assert_eq!(i, result.unwrap());
}
+
+let zero_scale_tests = [
+("0.123", 0, 3),
+("1.0", 1, 3),
+("1.2", 1, 3),
+("1.00", 1, 3),
+("1.23", 1, 3),
+("1.000", 1, 3),
+("1.123", 1, 3),
+("123.0", 123, 3),
+("123.4", 123, 3),
+("123.00", 123, 3),
+("123.45", 123, 3),
+("123.4", 123, 3),
+("0.123e2", 12, 3),
+("0.123e4", 1230, 10),
+("1.23e4", 12300, 10),
+("12.3e4", 123000, 10),
+("123e4", 123, 10),
+(
+"22.0",
+22,
+38,
+),
+];
+for (s, i, precision) in zero_scale_tests {
+let result_128 = parse_decimal::(s, precision,
0).unwrap();
+assert_eq!(i, result_128);
+}
Review Comment:
Nice, I've appended these cases to unsupported check and test cases.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] fix: liberal parsing of zero scale decimals [arrow-rs]
Jefffrey commented on PR #8700: URL: https://github.com/apache/arrow-rs/pull/8700#issuecomment-3757683061 By chance would this PR also address - https://github.com/apache/arrow-rs/issues/5762 Or is it a different cause? -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] fix: liberal parsing of zero scale decimals [arrow-rs]
alamb commented on PR #8700:
URL: https://github.com/apache/arrow-rs/pull/8700#issuecomment-3755820974
Spark 4 throws an error
```sql
spark-sql (default)> SELECT cast ('1e' as DECIMAL(5,0));
[CAST_INVALID_INPUT] The value '1e' of the type "STRING" cannot be cast to
"DECIMAL(5,0)" because it is malformed. Correct the value as per the syntax, or
change its target type. Use `try_cast` to tolerate malformed input and return
NULL instead. SQLSTATE: 22018
== SQL (line 1, position 8) ==
SELECT cast ('1e' as DECIMAL(5,0))
^^^
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] fix: liberal parsing of zero scale decimals [arrow-rs]
alamb commented on code in PR #8700:
URL: https://github.com/apache/arrow-rs/pull/8700#discussion_r2695158502
##
arrow-cast/src/parse.rs:
##
@@ -2752,6 +2740,35 @@ mod tests {
let result = parse_decimal::(s, 76, scale);
assert_eq!(i, result.unwrap());
}
+
+let zero_scale_tests = [
+("0.123", 0, 3),
+("1.0", 1, 3),
+("1.2", 1, 3),
+("1.00", 1, 3),
+("1.23", 1, 3),
+("1.000", 1, 3),
+("1.123", 1, 3),
+("123.0", 123, 3),
+("123.4", 123, 3),
+("123.00", 123, 3),
+("123.45", 123, 3),
+("123.4", 123, 3),
+("0.123e2", 12, 3),
+("0.123e4", 1230, 10),
+("1.23e4", 12300, 10),
+("12.3e4", 123000, 10),
+("123e4", 123, 10),
+(
+"22.0",
+22,
+38,
+),
+];
+for (s, i, precision) in zero_scale_tests {
+let result_128 = parse_decimal::(s, precision,
0).unwrap();
+assert_eq!(i, result_128);
+}
Review Comment:
> So if the matches! approach from above is fine with you, and if you agree
we should make these unsupported, then I can just append these patterns there.
Otherwise, let me know which ones would you think it'd make sense to parse.
I think that sounds good to me. I tested some other systems and they also
can't parse these examples
```
D select '1e'::decimal;
Conversion Error:
Could not convert string "1e" to DECIMAL(18,3)
LINE 1: select '1e'::decimal;
```
spark also doesn't parse them
```
spark-sql (default)> SELECT cast ('1' as DECIMAL(5,3));
1.000
Time taken: 0.048 seconds, Fetched 1 row(s)
spark-sql (default)> SELECT cast ('1e' as DECIMAL(5,3));
NULL
Time taken: 0.052 seconds, Fetched 1 row(s)
spark-sql (default)> SELECT cast ('e' as DECIMAL(5,3));
NULL
Time taken: 0.034 seconds, Fetched 1 row(s)
spark-sql (default)> SELECT cast ('1e+' as DECIMAL(5,3));
NULL
Time taken: 0.033 seconds, Fetched 1 row(s)
spark-sql (default)> SELECT cast ('1e' as DECIMAL(5,0));
NULL
Time taken: 0.038 seconds, Fetched 1 row(s)
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] fix: liberal parsing of zero scale decimals [arrow-rs]
alamb commented on code in PR #8700:
URL: https://github.com/apache/arrow-rs/pull/8700#discussion_r2695158502
##
arrow-cast/src/parse.rs:
##
@@ -2752,6 +2740,35 @@ mod tests {
let result = parse_decimal::(s, 76, scale);
assert_eq!(i, result.unwrap());
}
+
+let zero_scale_tests = [
+("0.123", 0, 3),
+("1.0", 1, 3),
+("1.2", 1, 3),
+("1.00", 1, 3),
+("1.23", 1, 3),
+("1.000", 1, 3),
+("1.123", 1, 3),
+("123.0", 123, 3),
+("123.4", 123, 3),
+("123.00", 123, 3),
+("123.45", 123, 3),
+("123.4", 123, 3),
+("0.123e2", 12, 3),
+("0.123e4", 1230, 10),
+("1.23e4", 12300, 10),
+("12.3e4", 123000, 10),
+("123e4", 123, 10),
+(
+"22.0",
+22,
+38,
+),
+];
+for (s, i, precision) in zero_scale_tests {
+let result_128 = parse_decimal::(s, precision,
0).unwrap();
+assert_eq!(i, result_128);
+}
Review Comment:
> So if the matches! approach from above is fine with you, and if you agree
we should make these unsupported, then I can just append these patterns there.
Otherwise, let me know which ones would you think it'd make sense to parse.
I think that sounds good to me. I tested some other systems and they also
can't parse these examples
```sql
D select '1e'::decimal;
Conversion Error:
Could not convert string "1e" to DECIMAL(18,3)
LINE 1: select '1e'::decimal;
```
spark also doesn't parse them
```sql
spark-sql (default)> SELECT cast ('1' as DECIMAL(5,3));
1.000
Time taken: 0.048 seconds, Fetched 1 row(s)
spark-sql (default)> SELECT cast ('1e' as DECIMAL(5,3));
NULL
Time taken: 0.052 seconds, Fetched 1 row(s)
spark-sql (default)> SELECT cast ('e' as DECIMAL(5,3));
NULL
Time taken: 0.034 seconds, Fetched 1 row(s)
spark-sql (default)> SELECT cast ('1e+' as DECIMAL(5,3));
NULL
Time taken: 0.033 seconds, Fetched 1 row(s)
spark-sql (default)> SELECT cast ('1e' as DECIMAL(5,0));
NULL
Time taken: 0.038 seconds, Fetched 1 row(s)
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] fix: liberal parsing of zero scale decimals [arrow-rs]
gruuya commented on code in PR #8700:
URL: https://github.com/apache/arrow-rs/pull/8700#discussion_r2693688957
##
arrow-cast/src/parse.rs:
##
@@ -2752,6 +2740,35 @@ mod tests {
let result = parse_decimal::(s, 76, scale);
assert_eq!(i, result.unwrap());
}
+
+let zero_scale_tests = [
+("0.123", 0, 3),
+("1.0", 1, 3),
+("1.2", 1, 3),
+("1.00", 1, 3),
+("1.23", 1, 3),
+("1.000", 1, 3),
+("1.123", 1, 3),
+("123.0", 123, 3),
+("123.4", 123, 3),
+("123.00", 123, 3),
+("123.45", 123, 3),
+("123.4", 123, 3),
+("0.123e2", 12, 3),
+("0.123e4", 1230, 10),
+("1.23e4", 12300, 10),
+("12.3e4", 123000, 10),
+("123e4", 123, 10),
+(
+"22.0",
+22,
+38,
+),
+];
+for (s, i, precision) in zero_scale_tests {
+let result_128 = parse_decimal::(s, precision,
0).unwrap();
+assert_eq!(i, result_128);
+}
Review Comment:
Thanks, these cases turned out to be very valuable.
For one thing `"."` case wasn't being correctly handled for zero scale
decimals, I fixed that now by factoring out various validation points
throughout the code into a common unsupported check at the start
```rust
if matches!(s, "" | "-" | "+" | ".") {
return Err(ArrowError::ParseError(format!(
"can't parse the string value {s} to decimal"
)));
}
```
I found this is a lot easier to grok, since previously these validations
were scattered throughout `parse_decimal` and deduced implicitly based on
side-effects (e.g. we finished parsing a decimal with a decimal point but the
digit count is zero). Let me know if you have performance concerns about this,
and I can resort to the old (implicit) manner on deducing unsupported strings.
For another thing, the test cases `"1e"`, `"1e+"` and '"1e-"` are curious.
Strictly speaking they aren't valid, but I wouldn't be surprised if some system
can parse/generate these. The question is what should `parse_decimal` output
for them. I tested these on both main and this branch:
```
| input | output(main)
| output(this branch) |
||---|-|
| parse_decimal::("1e", 5, 0)| Err(ParseError("can't
parse the string value 1e to decimal")) | Ok(1) |
| parse_decimal::("1e+", 5, 0) | Ok(1)
| Ok(1) |
| parse_decimal::("1e-", 5, 0) | Ok(1)
| Ok(1) |
| parse_decimal::("1e", 5, 2)| Err(ParseError("can't
parse the string value 1e to decimal")) | Ok(100) |
| parse_decimal::("1e+", 5, 2) | Ok(100)
| Ok(100) |
| parse_decimal::("1e-", 5, 2) | Ok(100)
| Ok(100) |
```
To make these consistent I think we should error out on all of them (that's
what Postgres does), at least until there's a worthwhile precedent to parse
some of these.
So _if_ the `matches!` approach from above is fine with you, and _if_ you
agree we should make these unsupported, then I can just append these patterns
there. Otherwise, let me know which ones would you think it'd make sense to
parse.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] fix: liberal parsing of zero scale decimals [arrow-rs]
gruuya commented on code in PR #8700:
URL: https://github.com/apache/arrow-rs/pull/8700#discussion_r2693688957
##
arrow-cast/src/parse.rs:
##
@@ -2752,6 +2740,35 @@ mod tests {
let result = parse_decimal::(s, 76, scale);
assert_eq!(i, result.unwrap());
}
+
+let zero_scale_tests = [
+("0.123", 0, 3),
+("1.0", 1, 3),
+("1.2", 1, 3),
+("1.00", 1, 3),
+("1.23", 1, 3),
+("1.000", 1, 3),
+("1.123", 1, 3),
+("123.0", 123, 3),
+("123.4", 123, 3),
+("123.00", 123, 3),
+("123.45", 123, 3),
+("123.4", 123, 3),
+("0.123e2", 12, 3),
+("0.123e4", 1230, 10),
+("1.23e4", 12300, 10),
+("12.3e4", 123000, 10),
+("123e4", 123, 10),
+(
+"22.0",
+22,
+38,
+),
+];
+for (s, i, precision) in zero_scale_tests {
+let result_128 = parse_decimal::(s, precision,
0).unwrap();
+assert_eq!(i, result_128);
+}
Review Comment:
Thanks, these cases turned out to be very valuable.
For one thing `"."` case wasn't being correctly handled for zero scale
decimals, I fixed that now by factoring out various validation points
throughout the code into a common unsupported check at the start
```rust
if matches!(s, "" | "-" | "+" | ".") {
return Err(ArrowError::ParseError(format!(
"can't parse the string value {s} to decimal"
)));
}
```
I found this is a lot easier to grok, since previously these validations
were scattered throughout `parse_decimal` and deduced implicitly based on
side-effects (e.g. we finished parsing a decimal with a decimal point but the
digit count is zero). Let me know if you have performance concerns about this,
and I can resort to the old (implicit) manner on deducing unsupported strings.
For another thing, the test cases `"1e"`, `"1e+"` and `"1e-"` are curious.
Strictly speaking they aren't valid, but I wouldn't be surprised if some system
can parse/generate these. The question is what should `parse_decimal` output
for them. I tested these on both main and this branch:
```
| input | output(main)
| output(this branch) |
||---|-|
| parse_decimal::("1e", 5, 0)| Err(ParseError("can't
parse the string value 1e to decimal")) | Ok(1) |
| parse_decimal::("1e+", 5, 0) | Ok(1)
| Ok(1) |
| parse_decimal::("1e-", 5, 0) | Ok(1)
| Ok(1) |
| parse_decimal::("1e", 5, 2)| Err(ParseError("can't
parse the string value 1e to decimal")) | Ok(100) |
| parse_decimal::("1e+", 5, 2) | Ok(100)
| Ok(100) |
| parse_decimal::("1e-", 5, 2) | Ok(100)
| Ok(100) |
```
To make these consistent I think we should error out on all of them (that's
what Postgres does), at least until there's a worthwhile precedent to parse
some of these.
So _if_ the `matches!` approach from above is fine with you, and _if_ you
agree we should make these unsupported, then I can just append these patterns
there. Otherwise, let me know which ones would you think it'd make sense to
parse.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] fix: liberal parsing of zero scale decimals [arrow-rs]
gruuya commented on code in PR #8700: URL: https://github.com/apache/arrow-rs/pull/8700#discussion_r2693385982 ## arrow-cast/src/parse.rs: ## @@ -850,7 +835,9 @@ fn parse_e_notation( } /// Parse the string format decimal value to i128/i256 format and checking the precision and scale. -/// The result value can't be out of bounds. +/// Expected behavior: +/// - the result value can't be out of bounds. +/// - when parsing a decimal with scale 0, any digits pass the decimal point will be discarded Review Comment: Good point, let me re-word this. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] fix: liberal parsing of zero scale decimals [arrow-rs]
alamb commented on code in PR #8700:
URL: https://github.com/apache/arrow-rs/pull/8700#discussion_r2692299354
##
arrow-cast/src/parse.rs:
##
@@ -2752,6 +2740,35 @@ mod tests {
let result = parse_decimal::(s, 76, scale);
assert_eq!(i, result.unwrap());
}
+
+let zero_scale_tests = [
+("0.123", 0, 3),
+("1.0", 1, 3),
+("1.2", 1, 3),
+("1.00", 1, 3),
+("1.23", 1, 3),
+("1.000", 1, 3),
+("1.123", 1, 3),
+("123.0", 123, 3),
+("123.4", 123, 3),
+("123.00", 123, 3),
+("123.45", 123, 3),
+("123.4", 123, 3),
+("0.123e2", 12, 3),
+("0.123e4", 1230, 10),
+("1.23e4", 12300, 10),
+("12.3e4", 123000, 10),
+("123e4", 123, 10),
+(
+"22.0",
+22,
+38,
+),
+];
+for (s, i, precision) in zero_scale_tests {
+let result_128 = parse_decimal::(s, precision,
0).unwrap();
+assert_eq!(i, result_128);
+}
Review Comment:
Could you also add some error tests, for example
- `.`
- `blag`
Should both fail to parse
Also how about these values? Should they be valid?
"1e", "1e+", "1e-", ".", and ".123"
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] fix: liberal parsing of zero scale decimals [arrow-rs]
alamb commented on code in PR #8700:
URL: https://github.com/apache/arrow-rs/pull/8700#discussion_r2692296831
##
arrow-cast/src/parse.rs:
##
@@ -850,7 +835,9 @@ fn parse_e_notation(
}
/// Parse the string format decimal value to i128/i256 format and checking the
precision and scale.
-/// The result value can't be out of bounds.
+/// Expected behavior:
+/// - the result value can't be out of bounds.
+/// - when parsing a decimal with scale 0, any digits pass the decimal point
will be discarded
Review Comment:
I think for e-notation the digits after the decimal do affect the value
(e.g., "1.23e4" → 12300) as shown in the tests. Can we clarify this slightly?
##
arrow-cast/src/parse.rs:
##
@@ -2752,6 +2740,35 @@ mod tests {
let result = parse_decimal::(s, 76, scale);
assert_eq!(i, result.unwrap());
}
+
+let zero_scale_tests = [
+("0.123", 0, 3),
+("1.0", 1, 3),
+("1.2", 1, 3),
+("1.00", 1, 3),
+("1.23", 1, 3),
+("1.000", 1, 3),
+("1.123", 1, 3),
+("123.0", 123, 3),
+("123.4", 123, 3),
+("123.00", 123, 3),
+("123.45", 123, 3),
+("123.4", 123, 3),
+("0.123e2", 12, 3),
+("0.123e4", 1230, 10),
+("1.23e4", 12300, 10),
+("12.3e4", 123000, 10),
+("123e4", 123, 10),
+(
+"22.0",
+22,
+38,
+),
+];
+for (s, i, precision) in zero_scale_tests {
+let result_128 = parse_decimal::(s, precision,
0).unwrap();
+assert_eq!(i, result_128);
+}
Review Comment:
Could you also add some error tests, for example
- `.`
- `blag`
Should both fail to parse
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] fix: liberal parsing of zero scale decimals [arrow-rs]
gruuya commented on code in PR #8700:
URL: https://github.com/apache/arrow-rs/pull/8700#discussion_r2689449683
##
arrow-cast/src/parse.rs:
##
@@ -741,72 +741,53 @@ fn parse_e_notation(
let mut exp: i16 = 0;
let base = T::Native::usize_as(10);
-let mut exp_start: bool = false;
// e has a plus sign
let mut pos_shift_direction: bool = true;
-// skip to point or exponent index
-let mut bs;
-if fractionals > 0 {
-// it's a fraction, so the point index needs to be skipped, so +1
-bs = s.as_bytes().iter().skip(index + fractionals as usize + 1);
-} else {
-// it's actually an integer that is already written into the result,
so let's skip on to e
-bs = s.as_bytes().iter().skip(index);
-}
+// skip to the exponent index directly or just after any processed
fractionals
+let mut bs = s.as_bytes().iter().skip(index + fractionals as usize);
-while let Some(b) = bs.next() {
+// complete parsing of any unprocessed fractionals up to the exponent
Review Comment:
That is correct—this is indeed (more than) slightly confusing, and it stems
from unclear separation of concerns. We skip parsing any fractionals after
scale in `parse_decimal`, _unless_ we hit into the e-notation and realise we
may actually need those fractionals after all, in which case we parse them in
`parse_e_notation`.
This is also one more argument for going with the above suggestion:
> To fix this i think we need to s.split_once(['e', 'E']) first, then
optionally process the exponent, and only then move on to the mantissa, since
then we know exactly what numbers can be thrown away. This however goes beyond
the scope of this PR/issue I think, and probably requires more discussion
(especially around performance implications).
Besides fixing another category of edge cases, this would also simplify the
logic. I'm only worried about performance though, since that would change the
current one-pass into a two-pass algorithm.
For now I'll try to elaborate in the comment a bit more. I'll also open an
issue for a cleaner `parse_decimal`/`parse_e_notation` separation.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] fix: liberal parsing of zero scale decimals [arrow-rs]
gruuya commented on code in PR #8700:
URL: https://github.com/apache/arrow-rs/pull/8700#discussion_r2689730978
##
arrow-cast/src/parse.rs:
##
@@ -741,72 +741,53 @@ fn parse_e_notation(
let mut exp: i16 = 0;
let base = T::Native::usize_as(10);
-let mut exp_start: bool = false;
// e has a plus sign
let mut pos_shift_direction: bool = true;
-// skip to point or exponent index
-let mut bs;
-if fractionals > 0 {
-// it's a fraction, so the point index needs to be skipped, so +1
-bs = s.as_bytes().iter().skip(index + fractionals as usize + 1);
-} else {
-// it's actually an integer that is already written into the result,
so let's skip on to e
-bs = s.as_bytes().iter().skip(index);
-}
+// skip to the exponent index directly or just after any processed
fractionals
+let mut bs = s.as_bytes().iter().skip(index + fractionals as usize);
Review Comment:
That makes sense, though this leads to some cascading casting and type
changes, as we use `fractionals` to compute/compare with other variables,
notably with `scale` which is `i8`.
So we can then make `scale` an unsigned type too, since it looks like
negative scale is not supported right now.
However I'd rather retain the possibility of supporting negative scales at
some point, so I went with changing the `scale` type to `i16` instead. That way
`fractionals: u8` can be safely cast to `i16` for comparison/arithmetic (plus
`parse_e_notation` already expects `scale` to be `i16`).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] fix: liberal parsing of zero scale decimals [arrow-rs]
gruuya commented on code in PR #8700:
URL: https://github.com/apache/arrow-rs/pull/8700#discussion_r2689635369
##
arrow-cast/src/parse.rs:
##
@@ -741,72 +741,53 @@ fn parse_e_notation(
let mut exp: i16 = 0;
let base = T::Native::usize_as(10);
-let mut exp_start: bool = false;
// e has a plus sign
let mut pos_shift_direction: bool = true;
-// skip to point or exponent index
-let mut bs;
-if fractionals > 0 {
-// it's a fraction, so the point index needs to be skipped, so +1
-bs = s.as_bytes().iter().skip(index + fractionals as usize + 1);
-} else {
-// it's actually an integer that is already written into the result,
so let's skip on to e
-bs = s.as_bytes().iter().skip(index);
-}
+// skip to the exponent index directly or just after any processed
fractionals
+let mut bs = s.as_bytes().iter().skip(index + fractionals as usize);
-while let Some(b) = bs.next() {
+// complete parsing of any unprocessed fractionals up to the exponent
Review Comment:
Here's the issue capturing the broader problems of
`parse_decimal`/`parse_e_notation`:
https://github.com/apache/arrow-rs/issues/9170
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] fix: liberal parsing of zero scale decimals [arrow-rs]
gruuya commented on code in PR #8700:
URL: https://github.com/apache/arrow-rs/pull/8700#discussion_r2689827027
##
arrow-cast/src/parse.rs:
##
@@ -741,72 +741,53 @@ fn parse_e_notation(
let mut exp: i16 = 0;
let base = T::Native::usize_as(10);
-let mut exp_start: bool = false;
// e has a plus sign
let mut pos_shift_direction: bool = true;
-// skip to point or exponent index
-let mut bs;
-if fractionals > 0 {
-// it's a fraction, so the point index needs to be skipped, so +1
-bs = s.as_bytes().iter().skip(index + fractionals as usize + 1);
-} else {
-// it's actually an integer that is already written into the result,
so let's skip on to e
-bs = s.as_bytes().iter().skip(index);
-}
+// skip to the exponent index directly or just after any processed
fractionals
+let mut bs = s.as_bytes().iter().skip(index + fractionals as usize);
Review Comment:
Hmm, `scale` being `i8` is more widespread than I thought, I reverted the
commit that changes it.
I don't think we can seamlessly change `fractionals` to an unsigned type
then.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] fix: liberal parsing of zero scale decimals [arrow-rs]
alamb commented on PR #8700: URL: https://github.com/apache/arrow-rs/pull/8700#issuecomment-3732876090 I tested this change with the current datafusion tests (I used https://github.com/apache/datafusion/pull/19728#pullrequestreview-3646861445) and all the tests passed for what it is worth -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] fix: liberal parsing of zero scale decimals [arrow-rs]
Jefffrey commented on code in PR #8700:
URL: https://github.com/apache/arrow-rs/pull/8700#discussion_r2676341440
##
arrow-cast/src/parse.rs:
##
@@ -741,72 +741,53 @@ fn parse_e_notation(
let mut exp: i16 = 0;
let base = T::Native::usize_as(10);
-let mut exp_start: bool = false;
// e has a plus sign
let mut pos_shift_direction: bool = true;
-// skip to point or exponent index
-let mut bs;
-if fractionals > 0 {
-// it's a fraction, so the point index needs to be skipped, so +1
-bs = s.as_bytes().iter().skip(index + fractionals as usize + 1);
-} else {
-// it's actually an integer that is already written into the result,
so let's skip on to e
-bs = s.as_bytes().iter().skip(index);
-}
+// skip to the exponent index directly or just after any processed
fractionals
+let mut bs = s.as_bytes().iter().skip(index + fractionals as usize);
-while let Some(b) = bs.next() {
+// complete parsing of any unprocessed fractionals up to the exponent
Review Comment:
I must admit I'm confused by this line; above we state we skip to the
exponent or after any processed fractionals, but here we say we parse
unprocessed fractionals somehow?
After checking the code it seems it corresponds to this check:
https://github.com/splitgraph/arrow-rs/blob/3dbfe0d16447d4344ef1f6caeb2befddc0e91aed/arrow-cast/src/parse.rs#L900-L905
Where we previously skipped fractionals for exceeding the scale, but now we
include them since we parse the exponent. I feel we should capture this detail
here.
##
arrow-cast/src/parse.rs:
##
@@ -741,72 +741,53 @@ fn parse_e_notation(
let mut exp: i16 = 0;
let base = T::Native::usize_as(10);
-let mut exp_start: bool = false;
// e has a plus sign
let mut pos_shift_direction: bool = true;
-// skip to point or exponent index
-let mut bs;
-if fractionals > 0 {
-// it's a fraction, so the point index needs to be skipped, so +1
-bs = s.as_bytes().iter().skip(index + fractionals as usize + 1);
-} else {
-// it's actually an integer that is already written into the result,
so let's skip on to e
-bs = s.as_bytes().iter().skip(index);
-}
+// skip to the exponent index directly or just after any processed
fractionals
+let mut bs = s.as_bytes().iter().skip(index + fractionals as usize);
Review Comment:
If we're refactoring perhaps we should change the type of fractions to be
unsigned? It was a little confusing to have it be signed to me as I had to
verify that your refactor to collapse the if/else made sense since fractionals
was never negative (and the `+ 1` in the if block is accounted for by adjusting
`index` at the callsite)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] fix: liberal parsing of zero scale decimals [arrow-rs]
gruuya commented on PR #8700:
URL: https://github.com/apache/arrow-rs/pull/8700#issuecomment-3696913488
That said, the following (valid) example will lead to an overflow again
```rust
let result_128 =
parse_decimal::("22.0e-1",
38, 0).unwrap();
assert_eq!(result_128, 20);
assertion `left == right` failed
left: -1402823669209384634633746074317682114
right: 20
```
In fact this also happens with scale 1, and it happens on `main` as well (so
this PR isn't introducing it).
The main problem there is the way e-notation is parsed—by the time we hit
e-notation processing we've already applied all the significant digits to the
result without checking whether some of them are not needed (e.g. because of
the negative exponent moving the decimal point to the left). Hence we may have
overflowed by the point we want to do `div_wrapping`.
To fix this i think we need to `s.split_once(['e', 'E'])` first, then
optionally process the exponent, and only then move on to the mantissa, since
then we know exactly what numbers can be thrown away. This however goes beyond
the scope of this PR/issue I think, and probably requires more discussion
(especially around performance implications).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] fix: liberal parsing of zero scale decimals [arrow-rs]
gruuya commented on PR #8700:
URL: https://github.com/apache/arrow-rs/pull/8700#issuecomment-3696873750
> The current implementation has an overflow issue. You can test with the
following code:
>
> ```
> let result =
parse_decimal::("22", 38,
0).unwrap();
> assert_eq!(result, 22i128);
> let result =
parse_decimal::("22.0", 38,
0).unwrap();
> assert_eq!(result, 22i128);
> ```
>
> The first assert will pass. The second assert will fail:
>
> ```
> assertion `left == right` failed
> left: -14028236692093846346337460743176821143
> right: 22
> ```
>
> There is apparently an interger overflow.
Great catch, thanks!
I revised the implementation a bit, and tried a drive-by `parse_e_notation`
simplification. We won't hit the integer overflow in this case anymore (added a
test for it too), since we now avoid the redundant `result =
result.mul_wrapping(base)` unless we hit an e-notation (in which case we need
to do it since we don;t know ahead of time what are the actual non-fractional
digits then).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] fix: liberal parsing of zero scale decimals [arrow-rs]
gruuya commented on code in PR #8700:
URL: https://github.com/apache/arrow-rs/pull/8700#discussion_r2651216713
##
arrow-cast/src/parse.rs:
##
@@ -963,6 +963,14 @@ pub fn parse_decimal(
}
if !is_e_notation {
Review Comment:
Yup, added a couple of e-notation test cases now
```
("0.123e2", 12, 3),
("0.123e4", 1230, 10),
("1.23e4", 12300, 10),
("12.3e4", 123000, 10),
("123e4", 123, 10),
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] fix: liberal parsing of zero scale decimals [arrow-rs]
chenhao-db commented on PR #8700:
URL: https://github.com/apache/arrow-rs/pull/8700#issuecomment-369488
The current implementation has an overflow issue. You can test with the
following code:
```
let result =
parse_decimal::("22", 38,
0).unwrap();
assert_eq!(result, 22i128);
let result =
parse_decimal::("22.0", 38,
0).unwrap();
assert_eq!(result, 22i128);
```
The first assert will pass. The second assert will fail:
```
assertion `left == right` failed
left: -14028236692093846346337460743176821143
right: 22
```
There is apparently an interger overflow.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] fix: liberal parsing of zero scale decimals [arrow-rs]
Jefffrey commented on code in PR #8700:
URL: https://github.com/apache/arrow-rs/pull/8700#discussion_r2648759832
##
arrow-cast/src/parse.rs:
##
@@ -963,6 +963,14 @@ pub fn parse_decimal(
}
if !is_e_notation {
Review Comment:
Could we add a test case with e notation, for a sanity check?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] fix: liberal parsing of zero scale decimals [arrow-rs]
gruuya commented on code in PR #8700:
URL: https://github.com/apache/arrow-rs/pull/8700#discussion_r2460113096
##
arrow-cast/src/parse.rs:
##
@@ -963,6 +963,14 @@ pub fn parse_decimal(
}
if !is_e_notation {
+if scale == 0 && fractionals > 0 {
+// The input string contained some fractional digits after the
decimal point despite
+// the scale being zero. Eject all the fractional digits from the
number.
+result = result.div_wrapping(base.pow_wrapping(fractionals as _));
Review Comment:
I think it would, added another test case on that note
(`("123.4", 123)`).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] fix: liberal parsing of zero scale decimals [arrow-rs]
martin-g commented on code in PR #8700:
URL: https://github.com/apache/arrow-rs/pull/8700#discussion_r2459835602
##
arrow-cast/src/parse.rs:
##
@@ -2752,6 +2761,23 @@ mod tests {
let result = parse_decimal::(s, 76, scale);
assert_eq!(i, result.unwrap());
}
+
+let zero_scale_tests = [
Review Comment:
```suggestion
let zero_scale_tests = [
("0.123", 0),
```
##
arrow-cast/src/parse.rs:
##
@@ -963,6 +963,14 @@ pub fn parse_decimal(
}
if !is_e_notation {
+if scale == 0 && fractionals > 0 {
+// The input string contained some fractional digits after the
decimal point despite
+// the scale being zero. Eject all the fractional digits from the
number.
+result = result.div_wrapping(base.pow_wrapping(fractionals as _));
Review Comment:
Would this be correct if there are many fractional digits, e.g. 20+ ?!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
