This is an automated email from the ASF dual-hosted git repository.
pitrou pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/arrow.git
The following commit(s) were added to refs/heads/main by this push:
new 0973268900 GH-50601: [C++] Refactor TranslateTo for clearer semantics
(#50602)
0973268900 is described below
commit 0973268900da96c5825dd1bb6245dafc901d1ecc
Author: Alexander Taepper <[email protected]>
AuthorDate: Wed Jul 22 14:50:21 2026 +0200
GH-50601: [C++] Refactor TranslateTo for clearer semantics (#50602)
### Rationale for this change
Follow-up to #50247
`GenericNullLikePartition::TranslateTo` translates a partition result from
a source range to a target range. This includes pointer arithmetic and does not
allow bounds checking for the source and target range, as only the range starts
are given. Ideally, we want to pass the entire source and target range to the
function, which will allow us to both validate sizes, guarantee that the
correct source range is passed, and the resulting `GenericNullLikePartition`
will point into the target range.
### What changes are included in this PR?
Additional refactor for `GenericNullLikePartition::TranslateTo`, that now
allows more `DCHECK`s. Each caller already had a `std::span` object for both
source and target, which is now passed in directly.
### Are these changes tested?
Re-ran existing test-suite.
### Are there any user-facing changes?
No.
* GitHub Issue: #50601
Authored-by: Alexander Taepper <[email protected]>
Signed-off-by: Antoine Pitrou <[email protected]>
---
cpp/src/arrow/compute/kernels/vector_sort.cc | 6 ++--
.../arrow/compute/kernels/vector_sort_internal.h | 33 ++++++++++++++--------
2 files changed, 25 insertions(+), 14 deletions(-)
diff --git a/cpp/src/arrow/compute/kernels/vector_sort.cc
b/cpp/src/arrow/compute/kernels/vector_sort.cc
index de5b7e1bca..4a12a04aee 100644
--- a/cpp/src/arrow/compute/kernels/vector_sort.cc
+++ b/cpp/src/arrow/compute/kernels/vector_sort.cc
@@ -115,7 +115,7 @@ class ChunkedArraySorter : public TypeVisitor {
std::vector<ChunkedNullLikePartition> chunk_sorted(num_chunks);
for (int i = 0; i < num_chunks; ++i) {
- chunk_sorted[i] = sorted[i].TranslateTo(indices_.data(),
chunked_indices.data());
+ chunk_sorted[i] = sorted[i].TranslateTo(indices_, chunked_indices);
}
// merge function for merging ranges where the first sort key is equal
@@ -155,7 +155,7 @@ class ChunkedArraySorter : public TypeVisitor {
// Reverse everything
sorted.resize(1);
- sorted[0] = chunk_sorted[0].TranslateTo(chunked_indices.data(),
indices_.data());
+ sorted[0] = chunk_sorted[0].TranslateTo(chunked_indices, indices_);
RETURN_NOT_OK(chunked_mapper.PhysicalToLogical());
}
@@ -681,7 +681,7 @@ class TableSorter {
std::vector<ChunkedNullLikePartition> chunk_sorted(num_batches);
for (int64_t i = 0; i < num_batches; ++i) {
- chunk_sorted[i] = sorted[i].TranslateTo(indices_.data(),
chunked_indices.data());
+ chunk_sorted[i] = sorted[i].TranslateTo(indices_, chunked_indices);
}
struct Visitor {
diff --git a/cpp/src/arrow/compute/kernels/vector_sort_internal.h
b/cpp/src/arrow/compute/kernels/vector_sort_internal.h
index 38f4ab4899..c19acc582c 100644
--- a/cpp/src/arrow/compute/kernels/vector_sort_internal.h
+++ b/cpp/src/arrow/compute/kernels/vector_sort_internal.h
@@ -135,19 +135,30 @@ struct GenericNullLikePartition {
return std::max(non_null_like_end(), null_end());
}
- // Note that "_begin" is not actually the begin of the stored ranges, but
can be much
- // smaller. I.e. this function can be and is used when the Partition object
is pointing
- // into a larger buffer and translate it into another larger buffer at the
same offset
+ // Re-express this partition's ranges as offsets into `target`, at the same
+ // positions they occupy within `source`. `source` and `target` are the
(possibly
+ // larger) buffers the partition points into: this partition must lie within
`source`,
+ // and the translated partition lies within `target` at the same offsets.
template <typename TargetIndexType>
GenericNullLikePartition<TargetIndexType> TranslateTo(
- IndexType* indices_begin, TargetIndexType* target_indices_begin) const {
- size_t non_null_offset = non_null_like_range.data() - indices_begin;
- size_t nan_offset = nan_range.data() - indices_begin;
- size_t null_offset = null_range.data() - indices_begin;
- return {.non_null_like_range = {target_indices_begin + non_null_offset,
- non_null_like_range.size()},
- .nan_range = {target_indices_begin + nan_offset, nan_range.size()},
- .null_range = {target_indices_begin + null_offset,
null_range.size()}};
+ std::span<IndexType> source, std::span<TargetIndexType> target) const {
+ ARROW_DCHECK_EQ(source.size(), target.size());
+ // This partition must lie within `source`.
+ ARROW_DCHECK_GE(overall_begin(), source.data());
+ ARROW_DCHECK_LE(overall_end(), source.data() + source.size());
+
+ size_t non_null_offset = non_null_like_range.data() - source.data();
+ size_t nan_offset = nan_range.data() - source.data();
+ size_t null_offset = null_range.data() - source.data();
+ GenericNullLikePartition<TargetIndexType> result{
+ .non_null_like_range = {target.data() + non_null_offset,
+ non_null_like_range.size()},
+ .nan_range = {target.data() + nan_offset, nan_range.size()},
+ .null_range = {target.data() + null_offset, null_range.size()}};
+ // The translated partition must lie within `target`.
+ ARROW_DCHECK_GE(result.overall_begin(), target.data());
+ ARROW_DCHECK_LE(result.overall_end(), target.data() + target.size());
+ return result;
}
static GenericNullLikePartition FromCounts(std::span<IndexType> indices,
