Repository navigation
Conversation
copy_rows is a safe trait method with no documented precondition, but the primitive and byte view implementations used get_unchecked on offset..offset + len, relying on callers for memory safety. Use ordinary bounds-checked slicing instead (one check per call, not per element). Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…llers append_values_by_indices, append_values_by_slices and append_views_by_filter write through raw pointers using indices taken from a FilterPredicate, relying on the predicate matching the source array. Mark them unsafe fn with documented preconditions, and have the safe copy_rows_by_filter callers verify filter_len <= source len before calling them, returning an error otherwise. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
alamb
changed the title
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
This reverts commit 604de3c.
Contributor
Author
|
run benchmark coalesce_kernels |
|
🤖 Benchmark starting (GKE) | trigger Target: coalesce_kernels Sharding factor: 1 (1 workers). Comparing alamb/coalesce-copy-rows-safe (209188f) to 01a9917 (merge-base) diff Run configurationrun benchmark coalesce_kernels
shards: 1
Results will be posted when all workers finish. File an issue against this benchmark runner |
github-actions
Bot
added
the
arrow-kernels
alamb
changed the title
mbutrovich
reviewed
Oct 21, 2026
Contributor
There was a problem hiding this comment.
Thanks @alamb for cleaning this up.
One more spot outside the diff has the pattern this PR removes. append_views_and_copy_strings_inner is a safe function that calls get_unchecked on buffers at the buffer index and offset read from each view (byte_view.rs L352-L357). It's sound only when the caller passes views that are valid for buffers, which its one caller chain from copy_rows does today (L433).
Comment on lines
+130
to
131
| unsafe fn append_views_by_filter(&mut self, views: &[u128], filter: &FilterPredicate) { | ||
| let selected_count = filter.count(); |
Contributor
There was a problem hiding this comment.
Should this check its precondition with a debug_assert?
Suggested change
| unsafe fn append_views_by_filter(&mut self, views: &[u128], filter: &FilterPredicate) { | |
| let selected_count = filter.count(); | |
| unsafe fn append_views_by_filter(&mut self, views: &[u128], filter: &FilterPredicate) { | |
| debug_assert!(filter.filter_len() <= views.len()); | |
| let selected_count = filter.count(); |
I don't see this as a pattern in the repo currently (enforcing unsafe invariants with debug_assert) so more of a question than a suggestion.
Comment on lines
+71
to
+72
| /// additional elements | ||
| unsafe fn append_values_by_indices( |
Contributor
There was a problem hiding this comment.
What do you think about having append_values_by_indices and append_values_by_slices call current.reserve(selected_count) themselves, the way append_views_by_filter does (byte_view.rs L133)? That drops the capacity precondition from both # Safety sections, and the callers at L200 and L220 no longer need their own reserve.
| }; | ||
| Ok(()) | ||
| } | ||
| // Other selection shapes reuse the generic copy_rows path. |
Contributor
There was a problem hiding this comment.
Should check_filter_len run once before the match? An all-true filter longer than the source takes this fallback to copy_rows. That was UB on main (L169), and this PR makes it a panic (L187). The other two arms and the byte view impl return the new error. A case for this shape in test_copy_rows_by_filter_rejects_filter_longer_than_source would cover it.
|
🤖 Benchmark failed or incomplete (GKE) | trigger Target: coalesce_kernels Comparing alamb/coalesce-copy-rows-safe (209188f) to 01a9917 (merge-base) diff Run configurationrun benchmark coalesce_kernels
shards: 1
Errors / missing shardsPer-runner informationcoalesce_kernels — shard 1/1Node: gk3-benchmark-cluster-nap-15xf2ybh-e962690e-6s4t Instance: c4a-highmem-16 (12 vCPU / 65 GiB) uname: BENCH_COMMAND: cargo bench --features=arrow,async,test_common,experimental,object_store --bench coalesce_kernelsCPU Details (lscpu)Resource Usagebase (merge-base)
File an issue against this benchmark runner |
Contributor
|
run benchmark coalesce_kernels |
|
🤖 Benchmark starting (GKE) | trigger Target: coalesce_kernels Sharding factor: 1 (1 workers). Comparing alamb/coalesce-copy-rows-safe (209188f) to 01a9917 (merge-base) diff Run configurationrun benchmark coalesce_kernels
env:
BENCH_FILTER: filter
shards: 1
Results will be posted when all workers finish. File an issue against this benchmark runner |
|
🤖 Benchmark completed (GKE) | trigger Target: coalesce_kernels Comparing alamb/coalesce-copy-rows-safe (209188f) to 01a9917 (merge-base) diff Run configurationrun benchmark coalesce_kernels
env:
BENCH_FILTER: filter
shards: 1
DetailsPer-runner informationcoalesce_kernels — shard 1/1Node: gk3-benchmark-cluster-nap-vaa38lox-de4a402e-cxsz Instance: c4a-highmem-16 (12 vCPU / 65 GiB) uname: BENCH_COMMAND: cargo bench --features=arrow,async,test_common,experimental,object_store --bench coalesce_kernelsCPU Details (lscpu)Resource Usagebase (merge-base)
branch
File an issue against this benchmark runner |
This branch has not been deployed
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
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.
Which issue does this PR close?
Rationale for this change
arrow-rsfollows the Rust Soundness Pledge: code must not be able to trigger undefined behavior using safe APIs.However, the specialized
InProgressArrayimplementations added in this PR have several safe functions whose memory safety depends on their callers:BatchCoalescer::push_batch_with_filterfor low selectivity filters and inlined Utf8View/BinaryView #9755A caller passing an out of range
offset/len, or a filter longer than the source array, would read out of bounds through a safe API.This came up while reviewing #11174, where the
copy_rowspattern was cited as precedent. This PR removes the precedent.What changes are included in this PR?
Are these changes tested?
CI
Are there any significant user-facing changes?
No.