pitrou commented on PR #50248:
URL: https://github.com/apache/arrow/pull/50248#issuecomment-5045463100
Yes, that looks clearer and safer too. Can you open an issue and PR?
Le 22 juillet 2026 13:36:29 GMT+02:00, Alexander Taepper ***@***.***> a
écrit :
>taepper left a comment (apache/arrow#50248)
>
>During lunch I realized that the `TranslateTo` method could have been made
clearer like this:
>
>
>```
>commit 669e9cbf14c7d4534d433c107a6ce8614da14be4
>Author: Alexander Taepper ***@***.***>
>Date: Wed Jul 22 13:16:07 2026 +0200
>
> refactor TranslateTo for clearer semantics
>
>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`
> 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,
>```
>
>What do you think? Do we want this addition? (if so, do I create issue ->
PR, or would a direct PR also be acceptable practice?)
>
>--
>Reply to this email directly or view it on GitHub:
>https://github.com/apache/arrow/pull/50248#issuecomment-5045299479
>You are receiving this because you modified the open/close state.
>
>Message ID: ***@***.***>
--
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]
