You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "viirya (via GitHub)" <gi...@apache.org> on 2024/02/13 19:28:06 UTC
[PR] Return null for overflow when casting string to integer under safe option enabled [arrow-rs]
viirya opened a new pull request, #5398:
URL: https://github.com/apache/arrow-rs/pull/5398
# Which issue does this PR close?
<!--
We generally require a GitHub issue to be filed for all bug fixes and enhancements and this helps us generate change logs for our releases. You can link an issue to this PR using the GitHub syntax. For example `Closes #123` indicates that this PR will close issue #123.
-->
Closes #5397.
# Rationale for this change
<!--
Why are you proposing this change? If this is already explained clearly in the issue then this section is not needed.
Explaining clearly why changes are proposed helps reviewers understand your changes and offer better suggestions for fixes.
-->
# What changes are included in this PR?
<!--
There is no need to duplicate the description in the issue here but it is sometimes worth providing a summary of the individual changes in this PR.
-->
# Are there any user-facing changes?
<!--
If there are user-facing changes then we may require documentation to be updated before approving the PR.
-->
<!---
If there are any breaking changes to public APIs, please add the `breaking change` label.
-->
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Return null for overflow when casting string to integer under safe option enabled [arrow-rs]
Posted by "viirya (via GitHub)" <gi...@apache.org>.
viirya commented on code in PR #5398:
URL: https://github.com/apache/arrow-rs/pull/5398#discussion_r1488476246
##########
arrow-cast/src/parse.rs:
##########
@@ -438,7 +438,7 @@ macro_rules! parser_primitive {
($t:ty) => {
impl Parser for $t {
fn parse(string: &str) -> Option<Self::Native> {
- lexical_core::parse::<Self::Native>(string.as_bytes()).ok()
+ string.parse::<Self::Native>().ok()
Review Comment:
Not sure if floating parser also has same issue.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Return null for overflow when casting string to integer under safe option enabled [arrow-rs]
Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold merged PR #5398:
URL: https://github.com/apache/arrow-rs/pull/5398
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Return null for overflow when casting string to integer under safe option enabled [arrow-rs]
Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold commented on code in PR #5398:
URL: https://github.com/apache/arrow-rs/pull/5398#discussion_r1489959084
##########
arrow-cast/Cargo.toml:
##########
@@ -49,6 +49,7 @@ chrono = { workspace = true }
half = { version = "2.1", default-features = false }
num = { version = "0.4", default-features = false, features = ["std"] }
lexical-core = { version = "^0.8", default-features = false, features = ["write-integers", "write-floats", "parse-integers", "parse-floats"] }
+atoi_simd = "0.15.6"
Review Comment:
I'm a little concerned that this crate does not seem to have a very large community around it.
How does it compare to atoi?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Return null for overflow when casting string to integer under safe option enabled [arrow-rs]
Posted by "viirya (via GitHub)" <gi...@apache.org>.
viirya commented on code in PR #5398:
URL: https://github.com/apache/arrow-rs/pull/5398#discussion_r1490069029
##########
arrow-cast/Cargo.toml:
##########
@@ -49,6 +49,7 @@ chrono = { workspace = true }
half = { version = "2.1", default-features = false }
num = { version = "0.4", default-features = false, features = ["std"] }
lexical-core = { version = "^0.8", default-features = false, features = ["write-integers", "write-floats", "parse-integers", "parse-floats"] }
+atoi = "2.0.0"
Review Comment:
Hmm, atoi seems not working as expected. For an invalid string like `9.1`, it will simply parse `9` and ignore string after it.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Return null for overflow when casting string to integer under safe option enabled [arrow-rs]
Posted by "viirya (via GitHub)" <gi...@apache.org>.
viirya commented on PR #5398:
URL: https://github.com/apache/arrow-rs/pull/5398#issuecomment-1944573742
`atoi` encounters more regression as expected, although looks like it is not as worse as `str.parse`:
```
4096 u64(0) - 128 time: [202.56 µs 203.03 µs 203.43 µs]
change: [+15.998% +16.532% +17.063%] (p = 0.00 < 0.05)
Performance has regressed.
4096 u64(0) - 1024 time: [181.58 µs 181.95 µs 182.37 µs]
change: [+16.615% +17.517% +18.295%] (p = 0.00 < 0.05)
Performance has regressed.
Found 9 outliers among 100 measurements (9.00%)
5 (5.00%) high mild
4 (4.00%) high severe
4096 u64(0) - 4096 time: [197.34 µs 198.98 µs 201.23 µs]
change: [+25.579% +28.243% +31.146%] (p = 0.00 < 0.05)
Performance has regressed.
Found 21 outliers among 100 measurements (21.00%)
4 (4.00%) low severe
2 (2.00%) low mild
3 (3.00%) high mild
12 (12.00%) high severe
4096 i64_small(0) - 128 time: [120.70 µs 121.20 µs 121.76 µs]
change: [+3.6602% +4.1249% +4.5580%] (p = 0.00 < 0.05)
Performance has regressed.
4096 i64_small(0) - 1024
time: [103.90 µs 105.02 µs 106.38 µs]
change: [+9.7972% +12.351% +15.643%] (p = 0.00 < 0.05)
Performance has regressed.
Found 12 outliers among 100 measurements (12.00%)
4 (4.00%) high mild
8 (8.00%) high severe
```
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Return null for overflow when casting string to integer under safe option enabled [arrow-rs]
Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold commented on code in PR #5398:
URL: https://github.com/apache/arrow-rs/pull/5398#discussion_r1489959084
##########
arrow-cast/Cargo.toml:
##########
@@ -49,6 +49,7 @@ chrono = { workspace = true }
half = { version = "2.1", default-features = false }
num = { version = "0.4", default-features = false, features = ["std"] }
lexical-core = { version = "^0.8", default-features = false, features = ["write-integers", "write-floats", "parse-integers", "parse-floats"] }
+atoi_simd = "0.15.6"
Review Comment:
I'm a little concerned that this crate does not seem to have a very large community around it.
How does performance compare to atoi?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Return null for overflow when casting string to integer under safe option enabled [arrow-rs]
Posted by "viirya (via GitHub)" <gi...@apache.org>.
viirya commented on PR #5398:
URL: https://github.com/apache/arrow-rs/pull/5398#issuecomment-1944764248
Due to `atoi` not work as expected, and the concern around `atoi_simd`, I changed it back to `str.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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Return null for overflow when casting string to integer under safe option enabled [arrow-rs]
Posted by "viirya (via GitHub)" <gi...@apache.org>.
viirya commented on PR #5398:
URL: https://github.com/apache/arrow-rs/pull/5398#issuecomment-1944377437
I switched to use `atoi_simd`. I will run bench to check performance diff.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Return null for overflow when casting string to integer under safe option enabled [arrow-rs]
Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold commented on code in PR #5398:
URL: https://github.com/apache/arrow-rs/pull/5398#discussion_r1489967558
##########
arrow-cast/Cargo.toml:
##########
@@ -49,6 +49,7 @@ chrono = { workspace = true }
half = { version = "2.1", default-features = false }
num = { version = "0.4", default-features = false, features = ["std"] }
lexical-core = { version = "^0.8", default-features = false, features = ["write-integers", "write-floats", "parse-integers", "parse-floats"] }
+atoi_simd = "0.15.6"
Review Comment:
> Community size seems not a big concern to me
Given the motivating factor for switching is a bug not getting attention, I am concerned. I'd be willing to sacrifice performance, for a better long-term maintenance story.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Return null for overflow when casting string to integer under safe option enabled [arrow-rs]
Posted by "viirya (via GitHub)" <gi...@apache.org>.
viirya commented on code in PR #5398:
URL: https://github.com/apache/arrow-rs/pull/5398#discussion_r1489965238
##########
arrow-cast/Cargo.toml:
##########
@@ -49,6 +49,7 @@ chrono = { workspace = true }
half = { version = "2.1", default-features = false }
num = { version = "0.4", default-features = false, features = ["std"] }
lexical-core = { version = "^0.8", default-features = false, features = ["write-integers", "write-floats", "parse-integers", "parse-floats"] }
+atoi_simd = "0.15.6"
Review Comment:
I will run benchmark with `atoi` to compare it.
As this is a parser for integers, I guess we can easily switch to other similar crate (e.g., atoi) if we want. Community size seems not a big concern to me.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Return null for overflow when casting string to integer under safe option enabled [arrow-rs]
Posted by "viirya (via GitHub)" <gi...@apache.org>.
viirya commented on PR #5398:
URL: https://github.com/apache/arrow-rs/pull/5398#issuecomment-1944372414
Hmm, polars uses `atoi_simd::parse`, maybe it is a good choice for both correctness and performance.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Return null for overflow when casting string to integer under safe option enabled [arrow-rs]
Posted by "viirya (via GitHub)" <gi...@apache.org>.
viirya commented on PR #5398:
URL: https://github.com/apache/arrow-rs/pull/5398#issuecomment-1944421038
The benchmark is mixed with improvement and a little regression:
```
4096 u64_small(0) - 128 time: [70.724 µs 70.777 µs 70.867 µs]
change: [-12.211% -12.012% -11.792%] (p = 0.00 < 0.05)
Performance has improved.
Found 1 outliers among 100 measurements (1.00%)
1 (1.00%) high severe
4096 u64_small(0) - 1024
time: [55.353 µs 55.382 µs 55.417 µs]
change: [-12.536% -12.217% -11.812%] (p = 0.00 < 0.05)
Performance has improved.
Found 13 outliers among 100 measurements (13.00%)
1 (1.00%) low mild
3 (3.00%) high mild
9 (9.00%) high severe
4096 u64_small(0) - 4096
time: [54.867 µs 55.030 µs 55.169 µs]
change: [-11.483% -11.257% -10.999%] (p = 0.00 < 0.05)
Performance has improved.
Found 1 outliers among 100 measurements (1.00%)
1 (1.00%) high mild
4096 i64(0) - 128 time: [114.86 µs 114.93 µs 115.04 µs]
change: [-23.963% -23.561% -23.122%] (p = 0.00 < 0.05)
Performance has improved.
Found 9 outliers among 100 measurements (9.00%)
2 (2.00%) high mild
7 (7.00%) high severe
4096 i64(0) - 1024 time: [99.625 µs 100.80 µs 101.90 µs]
change: [-27.344% -26.984% -26.517%] (p = 0.00 < 0.05)
Performance has improved.
Found 17 outliers among 100 measurements (17.00%)
3 (3.00%) high mild
14 (14.00%) high severe
4096 i64(0) - 4096 time: [108.51 µs 108.94 µs 109.50 µs]
change: [-18.432% -17.622% -16.805%] (p = 0.00 < 0.05)
Performance has improved.
```
```
4096 u64(0) - 1024 time: [115.03 µs 115.18 µs 115.41 µs]
change: [+4.2654% +4.4768% +4.6911%] (p = 0.00 < 0.05)
Performance has regressed.
Found 2 outliers among 100 measurements (2.00%)
1 (1.00%) high mild
1 (1.00%) high severe
4096 u64(0) - 4096 time: [121.25 µs 121.51 µs 121.87 µs]
change: [+5.9620% +6.6001% +7.2883%] (p = 0.00 < 0.05)
Performance has regressed.
```
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Return null for overflow when casting string to integer under safe option enabled [arrow-rs]
Posted by "viirya (via GitHub)" <gi...@apache.org>.
viirya commented on PR #5398:
URL: https://github.com/apache/arrow-rs/pull/5398#issuecomment-1944482342
Hmm, that’s good point. Although I also think maintenance is not highly
related to community size?
Anyway, I will try atoi once I return to my laptop later.
On Wed, Feb 14, 2024 at 11:34 AM Raphael Taylor-Davies <
***@***.***> wrote:
> ***@***.**** commented on this pull request.
> ------------------------------
>
> In arrow-cast/Cargo.toml
> <https://github.com/apache/arrow-rs/pull/5398#discussion_r1489967558>:
>
> > @@ -49,6 +49,7 @@ chrono = { workspace = true }
> half = { version = "2.1", default-features = false }
> num = { version = "0.4", default-features = false, features = ["std"] }
> lexical-core = { version = "^0.8", default-features = false, features = ["write-integers", "write-floats", "parse-integers", "parse-floats"] }
> +atoi_simd = "0.15.6"
>
> Community size seems not a big concern to me
>
> Given the motivating factor for switching is a bug not getting attention,
> I am concerned. I'd be willing to sacrifice performance, for a better
> long-term maintenance story.
>
> —
> Reply to this email directly, view it on GitHub
> <https://github.com/apache/arrow-rs/pull/5398#discussion_r1489967558>, or
> unsubscribe
> <https://github.com/notifications/unsubscribe-auth/AAAQZ5362DXWCNDEYLWPSXLYTUGT7AVCNFSM6AAAAABDHB72B2VHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTQOBRGA4TKNZZG4>
> .
> You are receiving this because you authored the thread.Message ID:
> ***@***.***>
>
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Return null for overflow when casting string to integer under safe option enabled [arrow-rs]
Posted by "viirya (via GitHub)" <gi...@apache.org>.
viirya commented on code in PR #5398:
URL: https://github.com/apache/arrow-rs/pull/5398#discussion_r1490069632
##########
arrow-cast/Cargo.toml:
##########
@@ -49,6 +49,7 @@ chrono = { workspace = true }
half = { version = "2.1", default-features = false }
num = { version = "0.4", default-features = false, features = ["std"] }
lexical-core = { version = "^0.8", default-features = false, features = ["write-integers", "write-floats", "parse-integers", "parse-floats"] }
+atoi = "2.0.0"
Review Comment:
@tustvold What do you think? Should we go back to `str.parse` if you worry about `atoi_simd`?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Return null for overflow when casting string to integer under safe option enabled [arrow-rs]
Posted by "viirya (via GitHub)" <gi...@apache.org>.
viirya commented on PR #5398:
URL: https://github.com/apache/arrow-rs/pull/5398#issuecomment-1946576034
Thank you @tustvold @Jefffrey
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Return null for overflow when casting string to integer under safe option enabled [arrow-rs]
Posted by "viirya (via GitHub)" <gi...@apache.org>.
viirya commented on code in PR #5398:
URL: https://github.com/apache/arrow-rs/pull/5398#discussion_r1488476246
##########
arrow-cast/src/parse.rs:
##########
@@ -438,7 +438,7 @@ macro_rules! parser_primitive {
($t:ty) => {
impl Parser for $t {
fn parse(string: &str) -> Option<Self::Native> {
- lexical_core::parse::<Self::Native>(string.as_bytes()).ok()
+ string.parse::<Self::Native>().ok()
Review Comment:
Not sure if floating parser also has same issue. I'm not sure about if there is floating type overflow 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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Return null for overflow when casting string to integer under safe option enabled [arrow-rs]
Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold commented on PR #5398:
URL: https://github.com/apache/arrow-rs/pull/5398#issuecomment-1944881703
By default `atoi` allows for trailing content, the following ensures there is none.
```
macro_rules! parser_primitive {
($t:ty) => {
impl Parser for $t {
fn parse(string: &str) -> Option<Self::Native> {
match atoi::FromRadix10SignedChecked::from_radix_10_signed_checked(string.as_bytes()) {
(Some(n), x) if x == string.len() => Some(n),
_ => None,
}
}
}
};
}
```
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Return null for overflow when casting string to integer under safe option enabled [arrow-rs]
Posted by "viirya (via GitHub)" <gi...@apache.org>.
viirya commented on code in PR #5398:
URL: https://github.com/apache/arrow-rs/pull/5398#discussion_r1488478065
##########
arrow-cast/src/parse.rs:
##########
@@ -438,7 +438,7 @@ macro_rules! parser_primitive {
($t:ty) => {
impl Parser for $t {
fn parse(string: &str) -> Option<Self::Native> {
- lexical_core::parse::<Self::Native>(string.as_bytes()).ok()
+ string.parse::<Self::Native>().ok()
Review Comment:
We can wait for a fix at the upstream crate. But the ticket is open for more than 6 months, and no progress so far. I think we may need to fix here directly.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Return null for overflow when casting string to integer under safe option enabled [arrow-rs]
Posted by "viirya (via GitHub)" <gi...@apache.org>.
viirya commented on code in PR #5398:
URL: https://github.com/apache/arrow-rs/pull/5398#discussion_r1488475472
##########
arrow-cast/src/parse.rs:
##########
@@ -438,7 +438,7 @@ macro_rules! parser_primitive {
($t:ty) => {
impl Parser for $t {
fn parse(string: &str) -> Option<Self::Native> {
- lexical_core::parse::<Self::Native>(string.as_bytes()).ok()
+ string.parse::<Self::Native>().ok()
Review Comment:
Only touched integer parser.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Return null for overflow when casting string to integer under safe option enabled [arrow-rs]
Posted by "viirya (via GitHub)" <gi...@apache.org>.
viirya commented on code in PR #5398:
URL: https://github.com/apache/arrow-rs/pull/5398#discussion_r1488476246
##########
arrow-cast/src/parse.rs:
##########
@@ -438,7 +438,7 @@ macro_rules! parser_primitive {
($t:ty) => {
impl Parser for $t {
fn parse(string: &str) -> Option<Self::Native> {
- lexical_core::parse::<Self::Native>(string.as_bytes()).ok()
+ string.parse::<Self::Native>().ok()
Review Comment:
Not sure if floating parser also has same issue. I'm not sure about floating type overflow 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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org