Skip to content

Panic on out-of-bounds take and Interleave indices - #10949

Closed
connortsui20 wants to merge 2 commits into
ct/decimal-interleavefrom
ct/interleave-take-panic
Closed

connortsui20 wants to merge 2 commits into
ct/decimal-interleavefrom
ct/interleave-take-panic

Conversation

@connortsui20

Copy link
Copy Markdown
Member

Summary

An out-of-bounds take index panicked or returned an error depending on the encoding. Fixed-width take (and so Dict execution) panics, while RunEnd take, Chunked take and Interleave execution returned an error. An index past the end is a contract violation by the caller, not invalid input, so all of them now panic.

Changes

Interleave cannot be serialized, so its selectors always come from code in the same process. Its bool and fixed-width kernels now panic on a selector that names a missing value or row, which also matches Interleave::scalar_at. RunEnd and Chunked take panic with the same message as the fixed-width kernel.

Take indices can still come from a file, for example the codes of a dict layout. A corrupt file with an out-of-bounds code now panics for RunEnd and Chunked values, as it already does for primitive values.

Follow-up: FixedSizeList, Null and Filter take, and the fixed-width take of a PiecewiseSequence, still return an error.

API Changes

take on a RunEnd or Chunked array, and execution of an Interleave array, panic on an out-of-bounds index where they used to return an error.

`Interleave` cannot be serialized, so its selectors always come from code in the same process. A selector that names a missing value or row is a bug in that code, not bad input. The bool and fixed-width kernels now panic on it, which matches `Interleave::scalar_at` and the fixed-width take kernel that `Dict` execution uses.

Signed-off-by: Connor Tsui <connor.tsui20@gmail.com>
The fixed-width take kernel already panics on an index past the end of the array. `RunEnd` and `Chunked` take returned an error instead, so the result of a bad index depended on the encoding of the array. Both now panic with the message of the fixed-width kernel.

Signed-off-by: Connor Tsui <connor.tsui20@gmail.com>
@connortsui20 connortsui20 added the changelog/break A breaking API change label Oct 28, 2026
@connortsui20

Copy link
Copy Markdown
Member Author

Closing for now. It is not decided whether an out-of-bounds take index must panic or return an error. The branch ct/interleave-take-panic stays for reference.

@codspeed

codspeed Bot commented Oct 28, 2026

Copy link
Copy Markdown

Merging this PR will degrade performance by 16.32%

3 warnings

⚠️ Walltime results may be unreliable

Some benchmarks used the Walltime instrument outside CodSpeed Macro Runners, which can make the results inconsistent.

Learn more about Macro Runners

See affected benchmarks

⚠️ 1 benchmark is dominated by system calls

System calls cannot be simulated consistently and are left out of the measure. The reported value understates the real cost. Switch to the Walltime instrument to measure them.

Learn more about system calls

See affected benchmarks

⚠️ Base and head ran on different runner types

Some benchmarks with significant performance changes were compared across different runner types, so the changes might not come from the code. Run base and head on the same runner type, or use CodSpeed Macro Runners for more stable results.

Learn more about Macro Runners

Open the report in CodSpeed to investigate

⚡ 3 improved benchmarks
❌ 12 regressed benchmarks
✅ 2259 untouched benchmarks
⏩ 370 skipped benchmarks1
🗄️ 30 archived benchmarks run2

Warning

Please fix the performance issues or acknowledge them on CodSpeed.

Performance Changes

Mode Benchmark BASE HEAD Efficiency
❌ CPU Simulation filter_powerlaw_by_random[10000] 670 ns 1,901 ns -64.76%
❌ Walltime mul_u64_nonnull_neon 20.1 µs 39.2 µs -48.82%
❌ CPU Simulation bench_compare_varbin[(50000, 2)] 133.8 µs 162.2 µs -17.49%
❌ CPU Simulation bench_compare_varbin[(50000, 64)] 135.6 µs 163.8 µs -17.25%
❌ CPU Simulation vortex[round_robin/n2/nonnull] 246.7 µs 283.7 µs -13.04%
❌ CPU Simulation vortex[random/n2/nonnull] 247.5 µs 284.4 µs -12.99%
❌ CPU Simulation vortex[round_robin/n2/null] 251.2 µs 288.1 µs -12.8%
❌ CPU Simulation vortex[random/n2/null] 252.2 µs 289.1 µs -12.76%
❌ CPU Simulation bench_compare_sliced_dict_varbinview[(7500, 10000)] 200.8 µs 227.7 µs -11.81%
❌ CPU Simulation bench_compare_sliced_dict_varbinview[(10000, 10000)] 207 µs 234 µs -11.53%
❌ CPU Simulation bench_compare_sliced_dict_varbinview[(1000, 10000)] 79.9 µs 89 µs -10.19%
❌ CPU Simulation interleave[canonical] 1.2 ms 1.3 ms -10.01%
⚡ CPU Simulation random_i8[0.01] 123.3 µs 98.7 µs +24.95%
⚡ CPU Simulation max_f64 315.2 µs 282.1 µs +11.73%
⚡ Walltime compare_u64_avx2 3.6 µs 3.3 µs +10.67%

Tip

Investigate this regression by commenting @codspeedbot fix this regression on this PR, or directly use the CodSpeed MCP with your agent.


Comparing ct/interleave-take-panic (b0c7c8f) with ct/decimal-interleave (c0eb004)

Open in CodSpeed

Footnotes

  1. 370 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩

  2. 30 benchmarks were run, but are now archived. If they were deleted in another branch, consider rebasing to remove them from the report. Instead if they were added back, click here to restore them. ↩

@connortsui20
connortsui20 deleted the ct/interleave-take-panic branch October 29, 2026 02:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

changelog/break A breaking API change

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant