GH-48167: [Python][C++][Compute] Add python bindings for scatter, inverse_permutation #48267

tadeja · 2025-11-26T15:15:32Z

Rationale for this change

To close or discuss #48167
inverse_permutation and scatter functions got implemented via #44393, PR #44394.

What changes are included in this PR?

Python tests for scatter, inverse_permutation kernels and bindings for InversePermutationOptions and ScatterOptions.

Are these changes tested?

Yes, tests added in test_compute.py.

Are there any user-facing changes?

Bindings for InversePermutationOptions and ScatterOptions are added.

This PR includes breaking changes to public APIs.

Options InversePermutationOptions changed from accepting parameter
std::shared_ptr<DataType> output_type = NULLPTR
to
std::optional<std::shared_ptr<DataType>> output_type = std::nullopt

GitHub Issue: [Python][Compute] Add python wrappers for scatter, inverse_permutation #48167

github-actions · 2025-11-26T15:15:58Z

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

tadeja · 2025-11-26T15:23:59Z

The background of inverse_permutation and scatter functions is nicely explained under this umbrella issue

Github issue [C++][Compute] Support special form in expression system #47374

Does it make sense to add Python wrappers/bindings for InversePermutationOptions and ScatterOptions already at this point in time? (Or at all?) What would you say, @felipecrv, @zanmato1984 ?

Currently both InversePermutationOptions and ScatterOptions do not seem to get accepted by inverse_permutation and scatter as demonstrated in added tests of test_compute.py.

github-actions · 2025-11-26T15:28:01Z

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

github-actions · 2025-11-26T18:57:16Z

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

zanmato1984 · 2025-11-26T19:17:14Z

Hi @tadeja , thanks for working on this. Appreciate it. I'm glad to see we are adding python bindings for scatter and inverse_permutation. To your questions:

Does it make sense to add Python wrappers/bindings for InversePermutationOptions and ScatterOptions already at this point in time? (Or at all?) What would you say, @felipecrv, @zanmato1984 ?

I think it does make sense to have them.

Currently both InversePermutationOptions and ScatterOptions do not seem to get accepted by inverse_permutation and scatter as demonstrated in added tests of test_compute.py.

I'm not sure what you mean. The underlying C++ APIs will accept those options. So I would expect no problem for their python bindings. Are you saying that they are not working?

tadeja · 2025-11-28T14:23:28Z

Thank you for the input, @zanmato1984, that helped.

I'm not sure what you mean. The underlying C++ APIs will accept those options. So I would expect no problem for their python bindings. Are you saying that they are not working?

It looks like Python bindings got fixed by adding option class names to FunctionDoc in vector_swizzle.cc.

Now test_option_class_equality is failing by default with
pyarrow.lib.ArrowInvalid: Could not serialize field output_type of options type InversePermutationOptions: shared_ptr<DataType> is nullptr
unless I pass output_type there, e.g. pc.InversePermutationOptions(output_type=pa.int32()).

If output_type is NULLPTR in C++ (which happens if output_type is None in Python) then InversePermutationOptions can't be serialized.
Any advice on the approach for fixing this? (Should Python users always specify output_type, or perhaps output_type be optional in C++? Or is it fine to leave it as it is?)

See example below:

import pyarrow.compute as pc
pc.InversePermutationOptions()

>>> InversePermutationOptions(max_index=-1, output_type=<NULLPTR>)

import pyarrow.compute as pc
pc.InversePermutationOptions().serialize()

>>> Traceback (most recent call last):
  File "<python-input-3>", line 1, in <module>
    pc.InversePermutationOptions().serialize()
    ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^^
  File "pyarrow/_compute.pyx", line 623, in pyarrow._compute.FunctionOptions.serialize
    shared_ptr[CBuffer] c_buf = GetResultValue(res)
  File "pyarrow/error.pxi", line 155, in pyarrow.lib.pyarrow_internal_check_status
    return check_status(status)
  File "pyarrow/error.pxi", line 92, in pyarrow.lib.check_status
    raise convert_status(status)
pyarrow.lib.ArrowInvalid: Could not serialize field output_type of options type InversePermutationOptions: shared_ptr<DataType> is nullptr

cc @pitrou

zanmato1984 · 2025-12-03T01:28:24Z

Thanks @tadeja for the further explanation! I now see the problem. Let me check the code and get back to you soon.

zanmato1984 · 2025-12-04T02:45:04Z

It looks like Python bindings got fixed by adding option class names to FunctionDoc in vector_swizzle.cc.

Nice catch! I forgot to declare the option type in function doc when I was adding these functions, and didn't realize it affects python binding until you fixed it. Really neat finding, thank you!

If output_type is NULLPTR in C++ (which happens if output_type is None in Python) then InversePermutationOptions can't be serialized. Any advice on the approach for fixing this? (Should Python users always specify output_type, or perhaps output_type be optional in C++? Or is it fine to leave it as it is?)

I want to suggest the following changes to allow std::shared_ptr<DataType> being null, when doing function options serde:

arrow/cpp/src/arrow/compute/function_internal.h

Lines 347 to 353 in e90bacd

    
           static inline Result<std::shared_ptr<Scalar>> GenericToScalar( 
        
               const std::shared_ptr<DataType>& value) { 
        
             if (!value) { 
        
               return Status::Invalid("shared_ptr<DataType> is nullptr"); 
        
             } 
        
             return MakeNullScalar(value); 
        
           }

static inline Result<std::shared_ptr<Scalar>> GenericToScalar(
    const std::shared_ptr<DataType>& value) {
  if (!value) {
-    return Status::Invalid("shared_ptr<DataType> is nullptr");
+    return std::make_shared<NullScalar>();
  }
  return MakeNullScalar(value);
}

and

arrow/cpp/src/arrow/compute/function_internal.h

Lines 448 to 452 in e90bacd

    
           template <typename T> 
        
           static inline enable_if_same_result<T, std::shared_ptr<DataType>> GenericFromScalar( 
        
               const std::shared_ptr<Scalar>& value) { 
        
             return value->type; 
        
           }

template <typename T>
static inline enable_if_same_result<T, std::shared_ptr<DataType>> GenericFromScalar(
    const std::shared_ptr<Scalar>& value) {
+  if (value->type->id() == Type::NA) {
+    return std::shared_ptr<NullType>();
+  }
  return value->type;
}

But I'll need to consult @pitrou if this is appropriate.

tadeja · 2025-12-04T14:29:46Z

@zanmato1984 I've included your suggested changes - 208fd24
Checks are passing so far. Now waiting for feedback from @pitrou

zanmato1984 · 2025-12-08T05:59:44Z

Hi @pitrou , do you think the changes to data type pointer serde make sense? Thanks.

pitrou · 2025-12-08T08:43:18Z

Hi @pitrou , do you think the changes to data type pointer serde make sense? Thanks.

That will have annoying side-effects, for example an explicit null DataType will be deserialized as a null pointer.

We should try to find another way of serializing a null pointer as a scalar, though I'm not sure how.

pitrou · 2025-12-08T08:50:41Z

How about this instead:

diff --git a/cpp/src/arrow/compute/function_internal.h b/cpp/src/arrow/compute/function_internal.h
index 9d8928466b..96bdbacf04 100644
--- a/cpp/src/arrow/compute/function_internal.h
+++ b/cpp/src/arrow/compute/function_internal.h
@@ -347,7 +347,8 @@ static inline Result<std::shared_ptr<Scalar>> GenericToScalar(
 static inline Result<std::shared_ptr<Scalar>> GenericToScalar(
     const std::shared_ptr<DataType>& value) {
   if (!value) {
-    return Status::Invalid("shared_ptr<DataType> is nullptr");
+    // An omitted DataType is serialized as boolean true
+    return MakeScalar(boolean(), true);
   }
   return MakeNullScalar(value);
 }
@@ -448,6 +449,10 @@ static inline enable_if_same_result<T, SortKey> GenericFromScalar(
 template <typename T>
 static inline enable_if_same_result<T, std::shared_ptr<DataType>> GenericFromScalar(
     const std::shared_ptr<Scalar>& value) {
+  if (value->type->id() == Type::BOOL && value->is_valid) {
+    // Boolean true represents an omitted DataType (nullptr)
+    return nullptr;
+  }
   return value->type;
 }

Also, should add a test in expression_test.cc (see SerializationRoundTrips)

zanmato1984 · 2025-12-08T08:56:54Z

Hi @pitrou , do you think the changes to data type pointer serde make sense? Thanks.

That will have annoying side-effects, for example an explicit null DataType will be deserialized as a null pointer.

We should try to find another way of serializing a null pointer as a scalar, though I'm not sure how.

Some alternative approaches in mind, both changing the C++ code:

Changing the type of output_type from std::shared_ptr<DataType> to std::optional<std::shared_ptr<DataType>>;
Or if we think std::optional<std::shared_ptr<DataType>> is problematic, we can in turn change output_type to std::optional<Type::type>.

@pitrou what do you think?

zanmato1984 · 2025-12-08T08:57:45Z

How about this instead:

diff --git a/cpp/src/arrow/compute/function_internal.h b/cpp/src/arrow/compute/function_internal.h
index 9d8928466b..96bdbacf04 100644
--- a/cpp/src/arrow/compute/function_internal.h
+++ b/cpp/src/arrow/compute/function_internal.h
@@ -347,7 +347,8 @@ static inline Result<std::shared_ptr<Scalar>> GenericToScalar(
 static inline Result<std::shared_ptr<Scalar>> GenericToScalar(
     const std::shared_ptr<DataType>& value) {
   if (!value) {
-    return Status::Invalid("shared_ptr<DataType> is nullptr");
+    // An omitted DataType is serialized as boolean true
+    return MakeScalar(boolean(), true);
   }
   return MakeNullScalar(value);
 }
@@ -448,6 +449,10 @@ static inline enable_if_same_result<T, SortKey> GenericFromScalar(
 template <typename T>
 static inline enable_if_same_result<T, std::shared_ptr<DataType>> GenericFromScalar(
     const std::shared_ptr<Scalar>& value) {
+  if (value->type->id() == Type::BOOL && value->is_valid) {
+    // Boolean true represents an omitted DataType (nullptr)
+    return nullptr;
+  }
   return value->type;
 }

Also, should add a test in expression_test.cc (see SerializationRoundTrips)

Feels a bit hacky imho.

pitrou · 2025-12-08T09:10:57Z

Feels a bit hacky imho.

Definitely (though less fragile than the solution in the current PR :-)).

pitrou · 2025-12-08T09:12:13Z

Some alternative approaches in mind, both changing the C++ code:

Changing the type of output_type from std::shared_ptr<DataType> to std::optional<std::shared_ptr<DataType>>;

Or if we think std::optional<std::shared_ptr<DataType>> is problematic, we can in turn change output_type to std::optional<Type::type>.

I would be ok with either, but that's an API breakage in both cases.

I think @bkietz contributed the original serialization code, perhaps we would like to chime in?

tadeja · 2025-12-18T22:24:26Z

See my attempt here as per one of the mentioned alternative approaches:

Changing the type of output_type from std::shared_ptr<DataType> to std::optional<std::shared_ptr<DataType>>

Please check, @zanmato1984, @pitrou, what do you say?

cpp/src/arrow/compute/function_internal.h

zanmato1984 · 2025-12-19T03:30:00Z

See my attempt here as per one of the mentioned alternative approaches:

Changing the type of output_type from std::shared_ptr<DataType> to std::optional<std::shared_ptr<DataType>>

Please check, @zanmato1984, @pitrou, what do you say?

I'm OK with this approach. Since @bkietz is not joining us, shall we move on with this approach, @pitrou ?

pitrou · 2025-12-19T09:03:19Z

I'm OK with this approach. Since @bkietz is not joining us, shall we move on with this approach, @pitrou ?

Let's do it!

cpp/src/arrow/compute/function_internal.h

zanmato1984

+1

zanmato1984 · 2025-12-23T09:23:02Z

I've approved this PR. Let's wait for some while to hear from other reviewers. Then I'll merge it.

Thanks a lot @tadeja for working on this!

zanmato1984 · 2025-12-29T05:24:36Z

@github-actions crossbow submit -g cpp -g python

github-actions · 2025-12-29T05:27:24Z

Revision: 45ba764

Submitted crossbow builds: ursacomputing/crossbow @ actions-fa19036110

Task	Status
example-cpp-minimal-build-static
example-cpp-minimal-build-static-system-dependency
example-cpp-tutorial
example-python-minimal-build-fedora-conda
example-python-minimal-build-ubuntu-venv
test-build-cpp-fuzz
test-conda-cpp
test-conda-cpp-valgrind
test-conda-python-3.10
test-conda-python-3.10-hdfs-2.9.2
test-conda-python-3.10-hdfs-3.2.1
test-conda-python-3.10-pandas-1.3.4-numpy-1.21.2
test-conda-python-3.11
test-conda-python-3.11-dask-latest
test-conda-python-3.11-dask-upstream_devel
test-conda-python-3.11-hypothesis
test-conda-python-3.11-pandas-latest-numpy-latest
test-conda-python-3.11-spark-master
test-conda-python-3.12
test-conda-python-3.12-cpython-debug
test-conda-python-3.12-pandas-latest-numpy-1.26
test-conda-python-3.12-pandas-latest-numpy-latest
test-conda-python-3.13
test-conda-python-3.13-pandas-nightly-numpy-nightly
test-conda-python-3.13-pandas-upstream_devel-numpy-nightly
test-conda-python-3.14
test-conda-python-emscripten
test-cuda-cpp-ubuntu-22.04-cuda-11.7.1
test-cuda-python-ubuntu-22.04-cuda-11.7.1
test-debian-12-cpp-amd64
test-debian-12-cpp-i386
test-debian-12-python-3-amd64
test-debian-12-python-3-i386
test-fedora-42-cpp
test-fedora-42-python-3
test-ubuntu-22.04-cpp
test-ubuntu-22.04-cpp-20
test-ubuntu-22.04-cpp-bundled
test-ubuntu-22.04-cpp-emscripten
test-ubuntu-22.04-cpp-no-threading
test-ubuntu-22.04-python-3
test-ubuntu-22.04-python-313-freethreading
test-ubuntu-24.04-cpp
test-ubuntu-24.04-cpp-bundled-offline
test-ubuntu-24.04-cpp-gcc-13-bundled
test-ubuntu-24.04-cpp-gcc-14
test-ubuntu-24.04-cpp-minimal-with-formats
test-ubuntu-24.04-cpp-thread-sanitizer
test-ubuntu-24.04-python-3

zanmato1984 · 2025-12-29T07:14:08Z

I've made a trivial commit to merely adjust the code order.

Now CI is good. The failures are unrelated. I'm merging now.

Initial commit

fd8f27a

github-actions bot added Component: Python Component: Documentation awaiting review Awaiting review labels Nov 26, 2025

github-actions bot added the Component: C++ label Nov 27, 2025

Adding option class in FunctionDocs of vector_swizzle.cc

47e3e8c

tadeja force-pushed the bindings_for_inverse_permutation_and_scatter branch from a3b6fb1 to 47e3e8c Compare November 27, 2025 22:05

tadeja marked this pull request as ready for review November 28, 2025 17:31

tadeja requested review from AlenkaF, raulcd and rok as code owners November 28, 2025 17:31

tadeja added 2 commits December 4, 2025 13:55

Including suggested changes to function_internal.h

208fd24

Lint

89896d5

Alternative approach with output_type as optional in cpp.

4cd1e67

tadeja added 4 commits December 18, 2025 16:36

Update test.

375f61b

Lint format.

3f09582

Improve bindings

0b10a90

Update docstring

640f40f

zanmato1984 reviewed Dec 19, 2025

View reviewed changes

cpp/src/arrow/compute/function_internal.h Outdated Show resolved Hide resolved

github-actions bot added awaiting committer review Awaiting committer review and removed awaiting review Awaiting review labels Dec 19, 2025

tadeja and others added 2 commits December 19, 2025 15:18

Update function_internal.h

e2ba6aa

Update python docstring

93ab9e3

tadeja commented Dec 19, 2025

View reviewed changes

cpp/src/arrow/compute/function_internal.h Show resolved Hide resolved

zanmato1984 reviewed Dec 23, 2025

View reviewed changes

zanmato1984 approved these changes Dec 23, 2025

View reviewed changes

tadeja changed the title ~~GH-48167: [Python][Compute] Add python bindings for scatter, inverse_permutation~~ GH-48167: [Python][C++][Compute] Add python bindings for scatter, inverse_permutation Dec 23, 2025

Adjust order

45ba764

zanmato1984 merged commit 7de2f61 into apache:main Dec 29, 2025
46 checks passed

zanmato1984 removed the awaiting committer review Awaiting committer review label Dec 29, 2025

zanmato1984 mentioned this pull request Dec 29, 2025

[Python][Compute] Add python wrappers for scatter, inverse_permutation #48167

Closed

GH-48167: [Python][C++][Compute] Add python bindings for scatter, inverse_permutation #48267

GH-48167: [Python][C++][Compute] Add python bindings for scatter, inverse_permutation #48267

Conversation

tadeja commented Nov 26, 2025 • 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?

This PR includes breaking changes to public APIs.

Uh oh!

github-actions bot commented Nov 26, 2025

Uh oh!

tadeja commented Nov 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Nov 26, 2025

Uh oh!

github-actions bot commented Nov 26, 2025

Uh oh!

zanmato1984 commented Nov 26, 2025

Uh oh!

tadeja commented Nov 28, 2025

Uh oh!

zanmato1984 commented Dec 3, 2025

Uh oh!

zanmato1984 commented Dec 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tadeja commented Dec 4, 2025

Uh oh!

zanmato1984 commented Dec 8, 2025

Uh oh!

pitrou commented Dec 8, 2025

Uh oh!

pitrou commented Dec 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zanmato1984 commented Dec 8, 2025

Uh oh!

zanmato1984 commented Dec 8, 2025

Uh oh!

pitrou commented Dec 8, 2025

Uh oh!

pitrou commented Dec 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tadeja commented Dec 18, 2025

Uh oh!

Uh oh!

zanmato1984 commented Dec 19, 2025

Uh oh!

pitrou commented Dec 19, 2025

Uh oh!

Uh oh!

zanmato1984 left a comment

Choose a reason for hiding this comment

Uh oh!

zanmato1984 commented Dec 23, 2025

Uh oh!

zanmato1984 commented Dec 29, 2025

Uh oh!

github-actions bot commented Dec 29, 2025

Uh oh!

zanmato1984 commented Dec 29, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

tadeja commented Nov 26, 2025 •

edited

Loading

tadeja commented Nov 26, 2025 •

edited

Loading

zanmato1984 commented Dec 4, 2025 •

edited

Loading

pitrou commented Dec 8, 2025 •

edited

Loading

pitrou commented Dec 8, 2025 •

edited

Loading