Update lexical-core requirement from 0.8 to 1.0 (to resolve RUSTSEC-2023-0086) #6402

dariocurr · 2024-09-16T15:50:44Z

Which issue does this PR close?

Closes #6397

Rationale for this change

It solves RUSTSEC-2023-0086

What changes are included in this PR?

Just update the Cargo.tomls and remove unnecessary unsafe block according to:

https://github.com/Alexhuszagh/rust-lexical/blob/fd3baac52d87b3253bd46669a498140bf2886833/CHANGELOG#L48

Are there any user-facing changes?

No

alamb · 2024-09-16T16:07:09Z

Thank you @dariocurr 🙏

Also @crepererum 's analysis #6401 (review) is that this will not be a breaking API and thus we'll be able to release this in the next minor release #6340

alamb · 2024-09-16T16:08:08Z

This PR looks good to me -- I am just going to run the cast kernel benchmarks to make sure there are no hidden performance implications

dariocurr · 2024-09-16T16:15:49Z

Thank you for this project, I'm glad to help!

dariocurr · 2024-09-16T16:52:01Z

Sorry, I was making other little improvements and mistakenly pushed on master 😢

alamb · 2024-09-16T17:01:48Z

I ran with this branch and saw no performance difference (as expected)

++ critcmp master master
group                                                master
-----                                                ------
cast binary view to string                           1.00    110.1±0.43µs        ? ?/sec
cast binary view to string view                      1.00    112.7±0.20µs        ? ?/sec
cast binary view to wide string                      1.00    114.9±0.33µs        ? ?/sec
cast date32 to date64 512                            1.00    401.5±0.98ns        ? ?/sec
cast date64 to date32 512                            1.00    609.6±1.19ns        ? ?/sec
cast decimal128 to decimal128 512                    1.00      2.9±0.01µs        ? ?/sec
cast decimal128 to decimal128 512 with same scale    1.00     74.9±0.18ns        ? ?/sec
cast decimal128 to decimal256 512                    1.00      9.8±0.39µs        ? ?/sec
cast decimal256 to decimal128 512                    1.00     46.4±0.13µs        ? ?/sec
cast decimal256 to decimal256 512                    1.00      9.8±0.02µs        ? ?/sec
cast decimal256 to decimal256 512 with same scale    1.00     75.1±0.15ns        ? ?/sec
cast dict to string view                             1.00     51.3±0.17µs        ? ?/sec
cast f32 to string 512                               1.00     22.4±0.11µs        ? ?/sec
cast f64 to string 512                               1.00     25.3±0.04µs        ? ?/sec
cast float32 to int32 512                            1.00   1565.7±2.27ns        ? ?/sec
cast float64 to float32 512                          1.00   1563.9±1.74ns        ? ?/sec
cast float64 to uint64 512                           1.00      2.1±0.27µs        ? ?/sec
cast i64 to string 512                               1.00     18.2±0.06µs        ? ?/sec
cast int32 to float32 512                            1.00   1453.1±1.31ns        ? ?/sec
cast int32 to float64 512                            1.00   1597.6±2.62ns        ? ?/sec
cast int32 to int32 512                              1.00    226.3±0.38ns        ? ?/sec
cast int32 to int64 512                              1.00   1651.7±1.74ns        ? ?/sec
cast int32 to uint32 512                             1.00   1652.3±9.38ns        ? ?/sec
cast int64 to int32 512                              1.00   1699.4±2.37ns        ? ?/sec
cast string to binary view 512                       1.00      3.6±0.01µs        ? ?/sec
cast string view to binary view                      1.00     95.9±0.24ns        ? ?/sec
cast string view to dict                             1.00    217.9±0.67µs        ? ?/sec
cast string view to string                           1.00    194.5±0.47µs        ? ?/sec
cast string view to wide string                      1.00    199.9±0.42µs        ? ?/sec
cast time32s to time32ms 512                         1.00    327.9±1.28ns        ? ?/sec
cast time32s to time64us 512                         1.00   410.9±51.93ns        ? ?/sec
cast time64ns to time32s 512                         1.00    612.8±1.39ns        ? ?/sec
cast timestamp_ms to i64 512                         1.00    514.7±2.73ns        ? ?/sec
cast timestamp_ms to timestamp_ns 512                1.00      2.6±0.01µs        ? ?/sec
cast timestamp_ns to timestamp_s 512                 1.00    223.4±0.45ns        ? ?/sec
cast utf8 to date32 512                              1.00     11.4±0.04µs        ? ?/sec
cast utf8 to date64 512                              1.00     40.2±0.13µs        ? ?/sec
cast utf8 to f32                                     1.00     15.4±0.02µs        ? ?/sec
cast wide string to binary view 512                  1.00      5.9±0.01µs        ? ?/sec

alamb

Thank you @dariocurr

Dandandan · 2024-09-16T17:06:01Z

I ran with this branch and saw no performance difference (as expected)

++ critcmp master master
group                                                master
-----                                                ------
cast binary view to string                           1.00    110.1±0.43µs        ? ?/sec
cast binary view to string view                      1.00    112.7±0.20µs        ? ?/sec
cast binary view to wide string                      1.00    114.9±0.33µs        ? ?/sec
cast date32 to date64 512                            1.00    401.5±0.98ns        ? ?/sec
cast date64 to date32 512                            1.00    609.6±1.19ns        ? ?/sec
cast decimal128 to decimal128 512                    1.00      2.9±0.01µs        ? ?/sec
cast decimal128 to decimal128 512 with same scale    1.00     74.9±0.18ns        ? ?/sec
cast decimal128 to decimal256 512                    1.00      9.8±0.39µs        ? ?/sec
cast decimal256 to decimal128 512                    1.00     46.4±0.13µs        ? ?/sec
cast decimal256 to decimal256 512                    1.00      9.8±0.02µs        ? ?/sec
cast decimal256 to decimal256 512 with same scale    1.00     75.1±0.15ns        ? ?/sec
cast dict to string view                             1.00     51.3±0.17µs        ? ?/sec
cast f32 to string 512                               1.00     22.4±0.11µs        ? ?/sec
cast f64 to string 512                               1.00     25.3±0.04µs        ? ?/sec
cast float32 to int32 512                            1.00   1565.7±2.27ns        ? ?/sec
cast float64 to float32 512                          1.00   1563.9±1.74ns        ? ?/sec
cast float64 to uint64 512                           1.00      2.1±0.27µs        ? ?/sec
cast i64 to string 512                               1.00     18.2±0.06µs        ? ?/sec
cast int32 to float32 512                            1.00   1453.1±1.31ns        ? ?/sec
cast int32 to float64 512                            1.00   1597.6±2.62ns        ? ?/sec
cast int32 to int32 512                              1.00    226.3±0.38ns        ? ?/sec
cast int32 to int64 512                              1.00   1651.7±1.74ns        ? ?/sec
cast int32 to uint32 512                             1.00   1652.3±9.38ns        ? ?/sec
cast int64 to int32 512                              1.00   1699.4±2.37ns        ? ?/sec
cast string to binary view 512                       1.00      3.6±0.01µs        ? ?/sec
cast string view to binary view                      1.00     95.9±0.24ns        ? ?/sec
cast string view to dict                             1.00    217.9±0.67µs        ? ?/sec
cast string view to string                           1.00    194.5±0.47µs        ? ?/sec
cast string view to wide string                      1.00    199.9±0.42µs        ? ?/sec
cast time32s to time32ms 512                         1.00    327.9±1.28ns        ? ?/sec
cast time32s to time64us 512                         1.00   410.9±51.93ns        ? ?/sec
cast time64ns to time32s 512                         1.00    612.8±1.39ns        ? ?/sec
cast timestamp_ms to i64 512                         1.00    514.7±2.73ns        ? ?/sec
cast timestamp_ms to timestamp_ns 512                1.00      2.6±0.01µs        ? ?/sec
cast timestamp_ns to timestamp_s 512                 1.00    223.4±0.45ns        ? ?/sec
cast utf8 to date32 512                              1.00     11.4±0.04µs        ? ?/sec
cast utf8 to date64 512                              1.00     40.2±0.13µs        ? ?/sec
cast utf8 to f32                                     1.00     15.4±0.02µs        ? ?/sec
cast wide string to binary view 512                  1.00      5.9±0.01µs        ? ?/sec

This compares master with master ?

alamb · 2024-09-16T17:09:39Z

This compares master with master ?

Sorry that is my script's fault -- both branches are named master -- the one in arrow as well as the one in @dariocurr 's fork: https://github.com/dariocurr/arrow-rs/tree/master

So it is comparing master with master but the branches are in different repositories

alamb · 2024-09-16T19:46:58Z

I'll plan to merge this tomorrow unless there are other comments

cc @Jefffrey

Dandandan · 2024-09-16T19:54:24Z

This compares master with master ?

Sorry that is my script's fault -- both branches are named master -- the one in arrow as well as the one in @dariocurr 's fork: https://github.com/dariocurr/arrow-rs/tree/master

So it is comparing master with master but the branches are in different repositories

Ah okay, makes sense.

Dandandan · 2024-09-16T19:55:07Z

arrow-cast/src/display.rs

@@ -423,7 +423,7 @@ macro_rules! primitive_display {
                let mut buffer = [0u8; <$t as ArrowPrimitiveType>::Native::FORMATTED_SIZE];
                // SAFETY:
                // buffer is T::FORMATTED_SIZE
-                let b = unsafe { lexical_core::write_unchecked(value, &mut buffer) };
+                let b = lexical_core::write(value, &mut buffer);


Safety comment above this can be removed

Done with 481883d

Jefffrey

Looks good, thanks.

Ran example test from #5422 under this PR branch, now panics instead of parsing incorrectly:

arrow-rs$ cargo test -p arrow-json reader::tests::test_basic -- --nocapture --exact
    Blocking waiting for file lock on build directory
   Compiling arrow-json v53.0.0 (/home/jeffrey/Code/arrow-rs/arrow-json)
    Finished `test` profile [unoptimized + debuginfo] target(s) in 9.76s
     Running unittests src/lib.rs (/media/jeffrey/1tb_860evo_ssd/.cargo_target_cache/debug/deps/arrow_json-8aa17cf2e84b5131)

running 1 test
thread 'reader::tests::test_basic' panicked at arrow-json/src/reader/mod.rs:742:18:
called `Result::unwrap()` on an `Err` value: JsonError("whilst decoding field 'a': failed to parse 999 as UInt8")
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
test reader::tests::test_basic ... FAILED

failures:

failures:
    reader::tests::test_basic

test result: FAILED. 0 passed; 1 failed; 0 ignored; 0 measured; 78 filtered out; finished in 0.00s

error: test failed, to rerun pass `-p arrow-json --lib`

…023-0086) (apache#6402) * Update lexical-core requirement from 0.8 to 1.0 * Remove safety comment

Update lexical-core requirement from 0.8 to 1.0

a46cdf2

github-actions bot added the arrow Changes to the arrow crate label Sep 16, 2024

alamb mentioned this pull request Sep 16, 2024

Update lexical-core requirement from ^0.8 to ^1.0 #6401

Closed

alamb changed the title ~~Update lexical-core requirement from 0.8 to 1.0~~ Update lexical-core requirement from 0.8 to 1.0 (to resolve RUSTSEC-2023-0086) Sep 16, 2024

alamb approved these changes Sep 16, 2024

View reviewed changes

Dandandan approved these changes Sep 16, 2024

View reviewed changes

Dandandan reviewed Sep 16, 2024

View reviewed changes

Remove safety comment

481883d

Jefffrey approved these changes Sep 17, 2024

View reviewed changes

tustvold merged commit 3490639 into apache:master Sep 17, 2024
27 checks passed

Jefffrey mentioned this pull request Sep 18, 2024

Remove usages of lexical_core for parsing integers #5422

Closed

wpikulik pushed a commit to wpikulik/arrow-rs that referenced this pull request Sep 19, 2024

Update lexical-core requirement from 0.8 to 1.0 (to resolve RUSTSEC-2…

2d0d0a1

…023-0086) (apache#6402) * Update lexical-core requirement from 0.8 to 1.0 * Remove safety comment

jspaezp mentioned this pull request Sep 26, 2024

chore: update dep versions MannLabs/timsrust#30

Closed

alamb mentioned this pull request Oct 2, 2024

Bump lexical-core to 1.0 #6397

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update lexical-core requirement from 0.8 to 1.0 (to resolve RUSTSEC-2023-0086) #6402

Update lexical-core requirement from 0.8 to 1.0 (to resolve RUSTSEC-2023-0086) #6402

dariocurr commented Sep 16, 2024 •

edited

Loading

alamb commented Sep 16, 2024 •

edited

Loading

alamb commented Sep 16, 2024

dariocurr commented Sep 16, 2024

dariocurr commented Sep 16, 2024 •

edited

Loading

alamb commented Sep 16, 2024

alamb left a comment

Dandandan commented Sep 16, 2024 •

edited

Loading

alamb commented Sep 16, 2024 •

edited

Loading

alamb commented Sep 16, 2024

Dandandan commented Sep 16, 2024

Dandandan Sep 16, 2024 •

edited

Loading

dariocurr Sep 16, 2024

Jefffrey left a comment

Update lexical-core requirement from 0.8 to 1.0 (to resolve RUSTSEC-2023-0086) #6402

Update lexical-core requirement from 0.8 to 1.0 (to resolve RUSTSEC-2023-0086) #6402

Conversation

dariocurr commented Sep 16, 2024 • edited Loading

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are there any user-facing changes?

alamb commented Sep 16, 2024 • edited Loading

alamb commented Sep 16, 2024

dariocurr commented Sep 16, 2024

dariocurr commented Sep 16, 2024 • edited Loading

alamb commented Sep 16, 2024

alamb left a comment

Choose a reason for hiding this comment

Dandandan commented Sep 16, 2024 • edited Loading

alamb commented Sep 16, 2024 • edited Loading

alamb commented Sep 16, 2024

Dandandan commented Sep 16, 2024

Dandandan Sep 16, 2024 • edited Loading

Choose a reason for hiding this comment

dariocurr Sep 16, 2024

Choose a reason for hiding this comment

Jefffrey left a comment

Choose a reason for hiding this comment

dariocurr commented Sep 16, 2024 •

edited

Loading

alamb commented Sep 16, 2024 •

edited

Loading

dariocurr commented Sep 16, 2024 •

edited

Loading

Dandandan commented Sep 16, 2024 •

edited

Loading

alamb commented Sep 16, 2024 •

edited

Loading

Dandandan Sep 16, 2024 •

edited

Loading