perf(arrow-select) : rename generic bound & avoid bounds check in take_primitive - #10944
Open
Rich-T-kid wants to merge 4 commits into
Open
Conversation
Contributor
Author
|
run benchmark take_kernel |
|
🤖 Arrow criterion benchmark running (GKE) | trigger CPU Details (lscpu)Comparing rich-T-kid/thread-unsafe-checks-against-const-generic (f2d6a4c) to 27a6a40 (merge-base) diff Run configurationrun benchmark take_kernelBENCH_COMMAND=cargo bench --features=arrow,async,test_common,experimental,object_store --bench take_kernel File an issue against this benchmark runner |
|
Benchmark for this request failed before finishing (Kubernetes reason: Benchmarks requested: Kubernetes messageFile an issue against this benchmark runner |
Contributor
Author
|
benchmarks shows a minor 3-4% improvement when I ran this locally with array sizes |
Rich-T-kid
changed the title
Contributor
Author
|
run benchmark take_kernel |
|
🤖 Arrow criterion benchmark running (GKE) | trigger CPU Details (lscpu)Comparing rich-T-kid/thread-unsafe-checks-against-const-generic (f2d6a4c) to 27a6a40 (merge-base) diff Run configurationrun benchmark take_kernelBENCH_COMMAND=cargo bench --features=arrow,async,test_common,experimental,object_store --bench take_kernel File an issue against this benchmark runner |
|
Benchmark for this request failed before finishing (Kubernetes reason: Benchmarks requested: Kubernetes messageFile an issue against this benchmark runner |
Contributor
are we sure this isnt noise? from what i see in this PR, the generic isnt actually being threaded in to be also side note, i do wonder the impact of this on codegen. i know we introduced it in previous PRs, but @alamb did raise a recent epic (in datafusion) about binary size, and this is likely to contribute to a larger binary now if we thread in just something to think about 🤔 |
Rich-T-kid
force-pushed
the
rich-T-kid/thread-unsafe-checks-against-const-generic
branch
from
September 2, 2026 03:04
f2d6a4c to
0dbc0aa
Compare
github-actions
Bot
added
parquet
github-actions
Bot
removed
parquet-variant
Contributor
Author
this is true, it could be noise. when I ran the benchmarks locally I did switch out the true for false before/after running the benchmarks to compare
I will keep this in mind. I think seeing the performance difference between the two variants will give us the best guide on weather or not its worth introducing the extra code. #10945 should have this once I can get the benchmarks bot working |
Contributor
Author
|
run benchmark take_kernel |
|
🤖 Arrow criterion benchmark running (GKE) | trigger CPU Details (lscpu)Comparing rich-T-kid/thread-unsafe-checks-against-const-generic (c50e65c) to 8c49c71 (merge-base) diff Run configurationrun benchmark take_kernel
env:
BENCH_FILTER: "primitive"BENCH_COMMAND=cargo bench --features=arrow,async,test_common,experimental,object_store --bench take_kernel File an issue against this benchmark runner |
|
Benchmark for this request failed before finishing (Kubernetes reason: Benchmarks requested: Kubernetes messageFile an issue against this benchmark runner |
Contributor
Author
|
run benchmark take_kernels |
|
🤖 Arrow criterion benchmark running (GKE) | trigger CPU Details (lscpu)Comparing rich-T-kid/thread-unsafe-checks-against-const-generic (2a59992) to f12c504 (merge-base) diff Run configurationrun benchmark take_kernels
env:
BENCH_FILTER: "primitive"BENCH_COMMAND=cargo bench --features=arrow,async,test_common,experimental,object_store --bench take_kernels File an issue against this benchmark runner |
|
🤖 Arrow criterion benchmark completed (GKE) | trigger Instance: Comparing rich-T-kid/thread-unsafe-checks-against-const-generic (2a59992) to f12c504 (merge-base) diff Run configurationrun benchmark take_kernels
env:
BENCH_FILTER: "primitive"CPU Details (lscpu)Details
Resource Usagebase (merge-base)
branch
File an issue against this benchmark runner |
Contributor
Author
|
run benchmark take_kernels |
|
🤖 Arrow criterion benchmark running (GKE) | trigger CPU Details (lscpu)Comparing rich-T-kid/thread-unsafe-checks-against-const-generic (2a59992) to f12c504 (merge-base) diff Run configurationrun benchmark take_kernelsBENCH_COMMAND=cargo bench --features=arrow,async,test_common,experimental,object_store --bench take_kernels File an issue against this benchmark runner |
|
🤖 Arrow criterion benchmark completed (GKE) | trigger Instance: Comparing rich-T-kid/thread-unsafe-checks-against-const-generic (2a59992) to f12c504 (merge-base) diff Run configurationrun benchmark take_kernelsCPU Details (lscpu)Details
Resource Usagebase (merge-base)
branch
File an issue against this benchmark runner |
Contributor
Author
within noise |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
#10945
#10944 <- here
Which issue does this PR close?
takekernels #8879.Rationale for this change
The take kernel uses a const generic
VALIDATE_INDICES: bool(previously CHECKED,5c8afaf) to control whether index bounds are validated. Threading this through internal helpers liketake_nativeandtake_union_type_idsallows callers who have already validated their indices to skip redundant bounds checks, and enables a new take_record_batch_unchecked API (seperate PR,).What changes are included in this PR?
CHECKEDtoVALIDATE_INDICESacross all internal take functions for clarityVALIDATE_INDICESintotake_nativeandtake_union_type_ids, with the false branch using get_unchecked in the no-null path.Are these changes tested?
Yes, existing tests in arrow-select cover the take kernel. The unchecked path shares the same logic as the checked path
Are there any user-facing changes?
nope