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

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 issue #48167 has been automatically assigned in GitHub to PR creator.

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?

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

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

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:

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

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.

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

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

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.

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)

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?

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.

Feels a bit hacky imho.

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

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?

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?

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 ?

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

Let's do it!

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!

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

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

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

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