Skip to content

fix: Change panic to error intake kernel for StringArrary/BinaryArray on overflow #7793

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

Jump to bottom
Merged
merged 1 commit into from
Jul 11, 2025
Merged

fix: Change panic to error intake kernel for StringArrary/BinaryArray on overflow #7793

merged 1 commit into from
Jul 11, 2025

Conversation

@chenkovsky
Copy link
Contributor

@chenkovsky chenkovsky commented Jun 26, 2025 •
edited by alamb
Loading

Which issue does this PR close?

Rationale for this change

arrow will panic if offset overflows.

What changes are included in this PR?

return an error instead of panic.

Are these changes tested?

UT

Are there any user-facing changes?

No

@github-actions github-actions bot added the arrow Changes to the arrow crate label Jun 26, 2025
alamb
alamb reviewed Jun 26, 2025
View reviewed changes
Copy link
Contributor

@alamb alamb left a comment

Choose a reason for hiding this comment

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

Thanks @chenkovsky -- the idea looks good to me but I worry about performance implications. Let's see what the benchmarks say

arrow-schema/src/error.rs
DictionaryKeyOverflowError,
/// Error when the run end index in a REE array is bigger than the array length
RunEndIndexOverflowError,
/// Error when the offset overflows.
Copy link
Contributor

@alamb alamb Jun 26, 2025

Choose a reason for hiding this comment

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

this is a breaking API change but probably a reasonable one given we are about to open main for development of 0.56.0

arrow-select/src/take.rs

let (offsets, values) = if array.null_count() == 0 && indices.null_count() == 0 {
offsets.extend(indices.values().iter().map(|index| {
offsets.reserve(indices.len());
Copy link
Contributor

@alamb alamb Jun 26, 2025

Choose a reason for hiding this comment

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

I am worried this change will cause a non trivial performance regression. I will run some benchmarks

@alamb
Copy link
Contributor

alamb commented Jun 26, 2025

🤖 ./gh_compare_arrow.sh Benchmark Script Running
Linux aal-dev 6.11.0-1015-gcp #15~24.04.1-Ubuntu SMP Thu Apr 24 20:41:05 UTC 2025 x86_64 x86_64 x86_64 GNU/Linux
Comparing fix/offset_overflow (ea990bb) to 6deefb7 diff
BENCH_NAME=take_kernels
BENCH_COMMAND=cargo bench --features=arrow,async,test_common,experimental --bench take_kernels
BENCH_FILTER=
BENCH_BRANCH_NAME=fix_offset_overflow
Results will be posted here when complete

@alamb
Copy link
Contributor

alamb commented Jun 26, 2025

🤖: Benchmark completed

Details 

group                                                                     fix_offset_overflow                    main
-----                                                                     -------------------                    ----
take bool 1024                                                            1.02   1413.1±2.22ns        ? ?/sec    1.00   1388.1±1.97ns        ? ?/sec
take bool 512                                                             1.01    728.6±1.79ns        ? ?/sec    1.00    721.6±1.60ns        ? ?/sec
take bool null indices 1024                                               1.00   1681.0±9.45ns        ? ?/sec    1.04   1755.6±5.48ns        ? ?/sec
take bool null values 1024                                                1.03      3.0±0.00µs        ? ?/sec    1.00      2.9±0.00µs        ? ?/sec
take bool null values null indices 1024                                   1.00      3.5±0.02µs        ? ?/sec    1.00      3.5±0.05µs        ? ?/sec
take check bounds i32 1024                                                1.00   1207.7±7.89ns        ? ?/sec    1.07  1289.4±34.75ns        ? ?/sec
take check bounds i32 512                                                 1.00    708.5±2.48ns        ? ?/sec    1.03    726.9±2.48ns        ? ?/sec
take i32 1024                                                             1.01    694.1±0.80ns        ? ?/sec    1.00    689.9±3.63ns        ? ?/sec
take i32 512                                                              1.01    436.0±0.81ns        ? ?/sec    1.00    432.1±1.31ns        ? ?/sec
take i32 null indices 1024                                                1.00    794.1±1.76ns        ? ?/sec    1.00    797.3±0.96ns        ? ?/sec
take i32 null values 1024                                                 1.04      2.1±0.00µs        ? ?/sec    1.00      2.1±0.00µs        ? ?/sec
take i32 null values null indices 1024                                    1.01      2.5±0.01µs        ? ?/sec    1.00      2.4±0.01µs        ? ?/sec
take primitive fsb value len: 12, indices: 1024                           1.00      8.0±0.01µs        ? ?/sec    1.05      8.4±0.02µs        ? ?/sec
take primitive fsb value len: 12, null values, indices: 1024              1.15      9.9±0.11µs        ? ?/sec    1.00      8.6±0.07µs        ? ?/sec
take primitive run logical len: 1024, physical len: 512, indices: 1024    1.01     21.0±0.28µs        ? ?/sec    1.00     20.9±0.07µs        ? ?/sec
take str 1024                                                             1.04     11.2±0.11µs        ? ?/sec    1.00     10.7±0.02µs        ? ?/sec
take str 512                                                              1.06      5.5±0.02µs        ? ?/sec    1.00      5.2±0.03µs        ? ?/sec
take str null indices 1024                                                1.00      7.8±0.06µs        ? ?/sec    1.45     11.4±0.03µs        ? ?/sec
take str null indices 512                                                 1.00      3.8±0.01µs        ? ?/sec    1.34      5.1±0.03µs        ? ?/sec
take str null values 1024                                                 1.00      8.9±0.14µs        ? ?/sec    1.18     10.5±0.04µs        ? ?/sec
take str null values null indices 1024                                    1.11      8.5±0.08µs        ? ?/sec    1.00      7.6±0.04µs        ? ?/sec
take stringview 1024                                                      1.08    858.4±2.21ns        ? ?/sec    1.00    792.1±1.73ns        ? ?/sec
take stringview 512                                                       1.00    473.5±1.72ns        ? ?/sec    1.00    474.0±3.32ns        ? ?/sec
take stringview null indices 1024                                         1.05  1479.1±20.08ns        ? ?/sec    1.00  1403.8±20.90ns        ? ?/sec
take stringview null indices 512                                          1.10    764.5±1.87ns        ? ?/sec    1.00    696.4±1.75ns        ? ?/sec
take stringview null values 1024                                          1.05      2.2±0.00µs        ? ?/sec    1.00      2.1±0.00µs        ? ?/sec
take stringview null values null indices 1024                             1.01      2.9±0.03µs        ? ?/sec    1.00      2.9±0.02µs        ? ?/sec

@alamb
Copy link
Contributor

alamb commented Jun 27, 2025

The performance runs show mixed results. I'll rerun them to see if they are reproduceable

@alamb alamb changed the title fix: Throw an error when there is an overflow instead of panicking. fix: Error in take kernel for strings instead of panicking on overflow Jun 27, 2025
@alamb alamb changed the title fix: Error in take kernel for strings instead of panicking on overflow fix: Change panic to error intake kernel for String/BinaryArray on overflow Jun 27, 2025
@alamb alamb changed the title fix: Change panic to error intake kernel for String/BinaryArray on overflow fix: Change panic to error intake kernel for StringArrary/BinaryArray on overflow Jun 27, 2025
@alamb
Copy link
Contributor

alamb commented Jun 27, 2025

🤖 ./gh_compare_arrow.sh Benchmark Script Running
Linux aal-dev 6.11.0-1015-gcp #15~24.04.1-Ubuntu SMP Thu Apr 24 20:41:05 UTC 2025 x86_64 x86_64 x86_64 GNU/Linux
Comparing fix/offset_overflow (ea990bb) to 6deefb7 diff
BENCH_NAME=take_kernels
BENCH_COMMAND=cargo bench --features=arrow,async,test_common,experimental --bench take_kernels
BENCH_FILTER=
BENCH_BRANCH_NAME=fix_offset_overflow
Results will be posted here when complete

@alamb
Copy link
Contributor

alamb commented Jun 27, 2025

🤖: Benchmark completed

Details 

group                                                                     fix_offset_overflow                    main
-----                                                                     -------------------                    ----
take bool 1024                                                            1.01   1410.6±1.67ns        ? ?/sec    1.00   1393.4±2.77ns        ? ?/sec
take bool 512                                                             1.00    725.3±1.23ns        ? ?/sec    1.00    724.4±1.23ns        ? ?/sec
take bool null indices 1024                                               1.00  1679.6±12.54ns        ? ?/sec    1.00   1674.8±6.51ns        ? ?/sec
take bool null values 1024                                                1.08      3.0±0.00µs        ? ?/sec    1.00      2.8±0.00µs        ? ?/sec
take bool null values null indices 1024                                   1.00      3.4±0.02µs        ? ?/sec    1.01      3.5±0.04µs        ? ?/sec
take check bounds i32 1024                                                1.00   1218.0±9.76ns        ? ?/sec    1.05   1278.7±7.19ns        ? ?/sec
take check bounds i32 512                                                 1.00    705.7±2.98ns        ? ?/sec    1.02    721.0±3.60ns        ? ?/sec
take i32 1024                                                             1.01    694.1±2.96ns        ? ?/sec    1.00    687.8±3.53ns        ? ?/sec
take i32 512                                                              1.00    435.5±0.70ns        ? ?/sec    1.00    433.7±4.48ns        ? ?/sec
take i32 null indices 1024                                                1.00    785.5±1.23ns        ? ?/sec    1.01    793.2±1.13ns        ? ?/sec
take i32 null values 1024                                                 1.04      2.1±0.01µs        ? ?/sec    1.00      2.1±0.00µs        ? ?/sec
take i32 null values null indices 1024                                    1.00      2.5±0.01µs        ? ?/sec    1.00      2.5±0.01µs        ? ?/sec
take primitive fsb value len: 12, indices: 1024                           1.00      8.0±0.02µs        ? ?/sec    1.04      8.3±0.02µs        ? ?/sec
take primitive fsb value len: 12, null values, indices: 1024              1.16      9.9±0.12µs        ? ?/sec    1.00      8.5±0.06µs        ? ?/sec
take primitive run logical len: 1024, physical len: 512, indices: 1024    1.00     20.8±0.17µs        ? ?/sec    1.02     21.3±0.06µs        ? ?/sec
take str 1024                                                             1.05     11.2±0.02µs        ? ?/sec    1.00     10.7±0.03µs        ? ?/sec
take str 512                                                              1.04      5.5±0.02µs        ? ?/sec    1.00      5.3±0.02µs        ? ?/sec
take str null indices 1024                                                1.00      7.8±0.04µs        ? ?/sec    1.45     11.4±0.04µs        ? ?/sec
take str null indices 512                                                 1.00      3.9±0.01µs        ? ?/sec    1.30      5.0±0.03µs        ? ?/sec
take str null values 1024                                                 1.00      9.0±0.08µs        ? ?/sec    1.14     10.3±0.04µs        ? ?/sec
take str null values null indices 1024                                    1.13      8.7±0.09µs        ? ?/sec    1.00      7.6±0.07µs        ? ?/sec
take stringview 1024                                                      1.09    852.1±0.88ns        ? ?/sec    1.00    779.7±1.54ns        ? ?/sec
take stringview 512                                                       1.00    472.3±0.81ns        ? ?/sec    1.01    477.1±1.44ns        ? ?/sec
take stringview null indices 1024                                         1.04  1446.9±20.52ns        ? ?/sec    1.00  1389.4±21.13ns        ? ?/sec
take stringview null indices 512                                          1.09    758.4±2.07ns        ? ?/sec    1.00    696.5±1.29ns        ? ?/sec
take stringview null values 1024                                          1.01      2.1±0.00µs        ? ?/sec    1.00      2.1±0.01µs        ? ?/sec
take stringview null values null indices 1024                             1.01      2.9±0.02µs        ? ?/sec    1.00      2.9±0.02µs        ? ?/sec

alamb
alamb approved these changes Jul 10, 2025
View reviewed changes
Copy link
Contributor

@alamb alamb left a comment

Choose a reason for hiding this comment

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

Thank you @chenkovsky -- this looks good to me

@alamb alamb merged commit 0269382 into apache:main Jul 11, 2025
29 of 30 checks passed
@andygrove andygrove mentioned this pull request Aug 12, 2025
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@alamb alamb alamb approved these changes

Assignees

No one assigned

Labels

arrow Changes to the arrow crate

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@chenkovsky @alamb