mbutrovich opened a new pull request, #11829: URL: https://github.com/apache/arrow-rs/pull/11829
# Which issue does this PR close? - Closes #11825. # Rationale for this change #10945 is not in a release yet, so this fix should go in 60.1.0 (#10525). # What changes are included in this PR? - `take_record_batch` checks the indices once against the row count with `check_bounds`, then takes every column unchecked. It no longer relies on a checked take of the first column, which reads no index for some types. - When indices have nulls, `check_bounds` now runs its vectorized loop over all values first. It falls back to the null-aware loop only if that finds a value out of bounds. The null-aware loop costs about 30x the vectorized one, so without this change the new `check_bounds` call costs 7-10% on `take_record_batch 7 mixed cols null values null indices 1024`. Null slots that hold out-of-bounds values pay for both loops, and `take check bounds i32 null indices out of bounds 1024` measures that case. - Two benchmarks cover `take` with `check_bounds: true` and null indices. They are in their own commit, and the `check_bounds` change is in the last commit, so the bot can compare each step. # Are these changes tested? `take_record_batch` had no unit tests. The new `test_take_record_batch*` tests in `arrow-select/src/take.rs` cover columns whose take reads no index, out-of-bounds and negative indices, null index slots holding out-of-bounds values, empty batches, and batches with no columns. Local results on an Apple M5 Max with thin LTO (as the bot builds), comparing `main` to this PR in one session. Times are criterion's point estimates, and lower is faster. | benchmark | `main` | this PR | |---|---|---| | `take check bounds i32 null indices 1024` | 1.05 us | 0.34 us | | `take_record_batch 5 primitive cols no nulls 1024` | 1.26 us | 1.27 us | | `take_record_batch 6 string/binary cols no nulls 1024` | 18.5 us | 18.8 us | | `take_record_batch 7 mixed cols null values 1024` | 9.11 us | 8.90 us | | `take_record_batch 7 mixed cols null values null indices 1024` | 9.40 us | 9.40 us | A second run with every function and block aligned to the cache line (`-C llvm-args=-align-all-functions=7 -C llvm-args=-align-all-nofallthru-blocks=7`) gave the same results, so they do not come from code layout. The first row comes from a temporary benchmark built the same way as the new one. Identical code differed by up to 8% between builds in the same session, so only the first row is outside the noise. # Are there any significant user-facing changes? `take_record_batch` returns an error for an out-of-bounds index. 60.0.0 panicked for most column types, and accepted the index when no column read it, for example in a batch of only `Null` columns. -- 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]
