GH-54920: [C++][Compute] Support unsigned indices in inverse_permutation and scatter by zanmato1984 · Pull Request #55169 · apache/arrow

zanmato1984 · 2026-10-24T06:27:02Z

Rationale for this change

inverse_permutation and scatter currently accept only signed integer indices, requiring unnecessary casts for unsigned arrays. This PR adds unsigned support while reducing generated code size.

What changes are included in this PR?

Accept signed and unsigned integer indices in both functions.
Allow unsigned inverse-permutation output types, with the default still following the input type.
Share input handling by bit width and validity generation by output width, while retaining specialized output writes.
Reduce the swizzle object's code section from 432,480 to 193,520 bytes (about 55.3% smaller, Apple Clang/O2).
Preserve signed-index checks and uint64 bounds diagnostics, and update the C++ and Python-facing documentation.

Are these changes tested?

Added unit tests for unsigned types, index bounds, sliced/chunked inputs, and validity bitmap boundaries.

Are there any user-facing changes?

Unsigned integer indices are now accepted. For example, inverse permutation of uint8 [2, null, 0, 1] returns uint8 [2, 3, 0, null].

No breaking API changes.

Was AI used for this PR?

The changes were thoroughly reviewed by a human with AI assistance.

PR code and description written by:

Human
AI

Reviewed before submission by:

Human
AI
Not reviewed

GitHub Issue: [C++][Compute] inverse_permutation and scatter should accept unsigned integers #54920

github-actions · 2026-10-24T06:27:27Z

⚠️ GitHub issue #54920 has been automatically assigned in GitHub to PR creator.

Copilot

🟢 Approval recommended

Static review found no blocking issues in dispatch, bounds checks, or validity generation, with added tests covering key boundary cases.

No open issues 🎉

What changed in this PR

Adds unsigned integer support to C++ compute functions inverse_permutation and scatter, addressing GH-54920 while reducing generated code size.

Changes:

Accept signed and unsigned indices, and allow unsigned inverse-permutation outputs.
Share input processing and validity generation by integer width while preserving bounds checks.
Expand boundary, sliced-array, and chunked-array tests and update public documentation.

File	Description
`python/pyarrow/_compute.pyx`	Document unsigned inverse-permutation output support.
`docs/source/cpp/compute.rst`	List integer input and output support.
`cpp/src/arrow/compute/kernels/vector_swizzle.cc`	Add unsigned support and consolidate kernel implementations.
`cpp/src/arrow/compute/kernels/vector_swizzle_test.cc`	Expand integer-type and boundary-case coverage.
`cpp/src/arrow/compute/api_vector.h`	Document signed and unsigned integer support.

_{Review effort: Balanced}

pitrou · 2026-10-27T12:48:25Z

+  for (const auto& type : IntTypes()) {
+    auto indices = ArrayFromJSON(type, "[42, 2, null, 0, 1, 42]")->Slice(1, 4);
+    auto expected = ArrayFromJSON(type, "[2, 3, 0, null]");
+    AssertInversePermutation(indices, -1, type, expected, false);


Can we make function parameters more explicit?

Suggested change

AssertInversePermutation(indices, -1, type, expected, false);

AssertInversePermutation(indices, /*xxx=*/ -1, type, expected, /*yyy=*/ false);

Added the parameter-name comments to both calls, and checked the other new tests for similar cases.

pitrou · 2026-10-27T12:48:57Z

+          Datum(ChunkedArrayFromJSON(type, {"[]", "[" + invalid_index + "]"}))}) {
+      ASSERT_RAISES_WITH_MESSAGE(
+          IndexError, "Index error: Index out of bounds: " + invalid_index,
+          InversePermutation(indices, /*max_index=*/1, /*output_type=*/int64()));


Perhaps max_index=41 to make sure that boundary cases are treated properly?

Changed to max_index = 41, so index 42 is exactly one past the allowed maximum. The negative-index and large-uint64 checks are preserved.

pitrou · 2026-10-27T12:53:10Z

+// Keep it separate from the indexed writes, and generate whole bitmap bytes
+// instead of clearing one bit at a time for every missing destination.
+template <typename OutputCType>
+ARROW_NOINLINE Status InversePermutationValidity(KernelContext* ctx,


Return a Result<std::shared_ptr<Buffer>> here?

Changed to return Result<std::shared_ptr<Buffer>>, removing the output parameter.

pitrou · 2026-10-27T12:59:16Z

+  }
+  ARROW_ASSIGN_OR_RAISE(*validity_buf, ctx->AllocateBitmap(output_length));
+  auto* validity = (*validity_buf)->mutable_data();
+  bit_util::SetBitmap(validity, 0, offset);


AllocateBitmap should already zero-initialize the bitmap, so I don't think this is necessary.

This call sets the already-valid prefix [0, offset) to ones, rather than zeroing it. The 0 is the starting bit offset. I kept the call and added /*offset=*/0 and /*length=*/offset to make that distinction explicit.

pitrou · 2026-10-27T13:07:23Z

+    // Avoid per-block validity dispatch when every input index is valid.
+    if (!span.MayHaveNulls()) {


Instead of special-casing this, perhaps rewrite indices visiting using VisitBitRuns?

Switched to VisitBitRuns, removing the separate no-null loop. In isolated inverse-permutation benchmarks on Apple M4 with Clang/O2, the traversal change gave about 1.5–1.9× speedup with 50% random nulls, but about 2.7× runtime with strictly alternating nulls. The unified traversal therefore has a known performance trade-off.

pitrou · 2026-10-27T14:36:01Z

+      {InputType(match::Integer())}, OutputType(ResolveInversePermutationOutputType));
+  kernel.init = InversePermutationState::Init;
+  kernel.exec = [](KernelContext* ctx, const ExecSpan& span, ExecResult* result) {
+    const auto exec = GenerateInteger<UnsignedInversePermutation, void, ArrayKernelExec>(


Sounds almost like we would like a GenerateInteger that takes a transformation function for the input type?

That looks reasonable. I'll review #55291 and adapt this code to the shared generator once it lands.

Rebased onto main and switched both the array and chunked dispatch paths to GenerateKernelExec with TypeIdToUnsigned, removing the local adapters.

Copilot

🟢 Approval recommended

No active review issues block the changes adding unsigned-index support to inverse_permutation and scatter.

No open issues 🎉

_{Review effort: Balanced}

Copilot

🟢 Approval recommended

No active review issues are reported for the changes adding unsigned integer support to inverse_permutation and scatter.

No open issues 🎉

_{Review effort: Balanced}

zanmato1984

Thanks for the detailed review and helpful suggestions! I've addressed the comments below, and I'll review #55291 as well.

zanmato1984 · 2026-10-30T22:17:17Z

+  for (const auto& type : IntTypes()) {
+    auto indices = ArrayFromJSON(type, "[42, 2, null, 0, 1, 42]")->Slice(1, 4);
+    auto expected = ArrayFromJSON(type, "[2, 3, 0, null]");
+    AssertInversePermutation(indices, -1, type, expected, false);


Added the parameter-name comments to both calls, and checked the other new tests for similar cases.

zanmato1984 · 2026-10-30T22:17:19Z

+          Datum(ChunkedArrayFromJSON(type, {"[]", "[" + invalid_index + "]"}))}) {
+      ASSERT_RAISES_WITH_MESSAGE(
+          IndexError, "Index error: Index out of bounds: " + invalid_index,
+          InversePermutation(indices, /*max_index=*/1, /*output_type=*/int64()));


Changed to max_index = 41, so index 42 is exactly one past the allowed maximum. The negative-index and large-uint64 checks are preserved.

zanmato1984 · 2026-10-30T22:17:21Z

+// Keep it separate from the indexed writes, and generate whole bitmap bytes
+// instead of clearing one bit at a time for every missing destination.
+template <typename OutputCType>
+ARROW_NOINLINE Status InversePermutationValidity(KernelContext* ctx,


Changed to return Result<std::shared_ptr<Buffer>>, removing the output parameter.

zanmato1984 · 2026-10-30T22:17:22Z

+  }
+  ARROW_ASSIGN_OR_RAISE(*validity_buf, ctx->AllocateBitmap(output_length));
+  auto* validity = (*validity_buf)->mutable_data();
+  bit_util::SetBitmap(validity, 0, offset);


This call sets the already-valid prefix [0, offset) to ones, rather than zeroing it. The 0 is the starting bit offset. I kept the call and added /*offset=*/0 and /*length=*/offset to make that distinction explicit.

zanmato1984 · 2026-10-30T22:17:24Z

+    // Avoid per-block validity dispatch when every input index is valid.
+    if (!span.MayHaveNulls()) {


Switched to VisitBitRuns, removing the separate no-null loop. In isolated inverse-permutation benchmarks on Apple M4 with Clang/O2, the traversal change gave about 1.5–1.9× speedup with 50% random nulls, but about 2.7× runtime with strictly alternating nulls. The unified traversal therefore has a known performance trade-off.

zanmato1984 · 2026-10-30T22:17:25Z

+      {InputType(match::Integer())}, OutputType(ResolveInversePermutationOutputType));
+  kernel.init = InversePermutationState::Init;
+  kernel.exec = [](KernelContext* ctx, const ExecSpan& span, ExecResult* result) {
+    const auto exec = GenerateInteger<UnsignedInversePermutation, void, ArrayKernelExec>(


That looks reasonable. I'll review #55291 and adapt this code to the shared generator once it lands.

…atter Signed-off-by: Rossi Sun <zanmato1984@gmail.com>

Signed-off-by: Rossi Sun <zanmato1984@gmail.com>

Copilot

🟢 Approval recommended

No active review issues block the unsigned-integer support changes.

No open issues 🎉

_{Review effort: Balanced}

pitrou

LGTM, just two minor questions / comments

pitrou · 2026-11-02T20:09:03Z

This test is superseded by IndexBounds below, no?

Yes. I've consolidated these checks into IndexBounds, including positive out-of-bounds indices for both signed and unsigned types.

pitrou · 2026-11-02T20:13:32Z

+    // Keep output execution specialized so the loops remain inlineable. Sharing
+    // output signedness can cause aggressive outlining and regress throughput.


I'm not sure what the comment alludes to ("Sharing output signedness")?

This referred to an earlier experiment with sharing the output handling, which isn't part of this PR. I've removed the comment.

Copilot

🟢 Approval recommended

There are no active review issues for the unsigned-index support changes.

No open issues 🎉

_{Review effort: Balanced}

github-actions Bot added Component: C++ Component: Python Component: Documentation awaiting review Awaiting review labels Oct 24, 2026

zanmato1984 marked this pull request as ready for review October 24, 2026 06:38

zanmato1984 requested review from AlenkaF, pitrou, raulcd and rok as code owners October 24, 2026 06:38

Copilot AI balanced review requested due to automatic review settings October 24, 2026 06:38

Copilot started reviewing on behalf of zanmato1984 October 24, 2026 06:38

View session

Copilot AI reviewed Oct 24, 2026

View reviewed changes

pitrou reviewed Oct 27, 2026

View reviewed changes

github-actions Bot added awaiting committer review Awaiting committer review and removed awaiting review Awaiting review labels Oct 27, 2026

pitrou mentioned this pull request Oct 27, 2026

GH-55312: [C++][Compute] More flexible kernel exec generation #55291

Merged

2 of 5 tasks

Copilot AI balanced review requested due to automatic review settings October 30, 2026 21:27

Copilot started reviewing on behalf of zanmato1984 October 30, 2026 21:27

View session

Copilot AI reviewed Oct 30, 2026

View reviewed changes

Copilot AI balanced review requested due to automatic review settings October 30, 2026 22:16

Copilot started reviewing on behalf of zanmato1984 October 30, 2026 22:16

View session

Copilot AI reviewed Oct 30, 2026

View reviewed changes

zanmato1984 commented Oct 30, 2026

View reviewed changes

zanmato1984 added 3 commits November 2, 2026 10:45

[C++][Compute] Support unsigned indices in inverse permutation and sc…

ae2a704

…atter Signed-off-by: Rossi Sun <zanmato1984@gmail.com>

[C++][Compute] Use bitmap runs for inverse permutation traversal

3a5c01e

Signed-off-by: Rossi Sun <zanmato1984@gmail.com>

[C++][Compute] Refine inverse permutation helpers and tests

4580b18

Signed-off-by: Rossi Sun <zanmato1984@gmail.com>

zanmato1984 force-pushed the feat/unsigned-permutation-upstream branch from 0b0b62c to 4580b18 Compare November 2, 2026 19:04

Copilot AI balanced review requested due to automatic review settings November 2, 2026 19:04

Copilot started reviewing on behalf of zanmato1984 November 2, 2026 19:04

View session

Copilot AI reviewed Nov 2, 2026

View reviewed changes

pitrou reviewed Nov 2, 2026

View reviewed changes

[C++][Compute] Consolidate inverse permutation bounds tests

8b0a685

Copilot AI balanced review requested due to automatic review settings November 3, 2026 03:51

Copilot started reviewing on behalf of zanmato1984 November 3, 2026 03:52

View session

Copilot AI reviewed Nov 3, 2026

View reviewed changes

	AssertInversePermutation(indices, -1, type, expected, false);
	AssertInversePermutation(indices, /xxx=/ -1, type, expected, /yyy=/ false);

		// Avoid per-block validity dispatch when every input index is valid.
		if (!span.MayHaveNulls()) {

		// Keep output execution specialized so the loops remain inlineable. Sharing
		// output signedness can cause aggressive outlining and regress throughput.

Conversation

zanmato1984 commented Oct 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

Was AI used for this PR?

Uh oh!

github-actions Bot commented Oct 24, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟢 Approval recommended

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟢 Approval recommended

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟢 Approval recommended

Uh oh!

zanmato1984 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟢 Approval recommended

Uh oh!

pitrou left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

zanmato1984 commented Oct 24, 2026 •

edited

Loading