GH-47358: [Python] IPC and Flight options representation

[rmnskb]

Rationale for this change

Please see #47358

What changes are included in this PR?

Added __repr__ dunder methods to ipc.IpcWriteOptions, ipc.IpcReadOptions and flight.FlightCallOptions

Are these changes tested?

Yes

Are there any user-facing changes?

Nicer representations of the classes mentioned above

• GitHub Issue: [Python] IPC and Flight options lack a nice repr/str #47358

[rmnskb]

hey @pitrou, I hope you're doing well :)
I've started implementing the __repr__ dunders for the classes that you've mentioned in #47358, however, now I stumbled upon FlightCallOptions's arguments not being convertable back to Python objects after they are set as C objects inside FlightCallOptions.options.
Could you please provide some guidance on what's the better way to tackle this? I've gone through the implementations of underlying objects in both APIs, and couldn't find any methods that would already do that? Thank you :)

[pitrou]

I've started implementing the __repr__ dunders for the classes that you've mentioned in #47358, however, now I stumbled upon FlightCallOptions's arguments not being convertable back to Python objects after they are set as C objects inside FlightCallOptions.options.

I'm not sure what you mean, because if you define the __repr__ in Cython you should be able to access the C-level attributes. You may need to add the necessary API declarations for C++ Flight types.

For example, this change works for me:

diff --git a/python/pyarrow/_flight.pyx b/python/pyarrow/_flight.pyx
index fe2e1b3d67..ca842f35bb 100644
--- a/python/pyarrow/_flight.pyx
+++ b/python/pyarrow/_flight.pyx
@@ -142,6 +142,10 @@ cdef class FlightCallOptions(_Weakrefable):
             return &((<FlightCallOptions> obj).options)
         raise TypeError(f"Expected a FlightCallOptions object, not '{type(obj)}'")
 
+    def __repr__(self):
+        # Draft implementation, should be improved
+        return f'FlightCallOptions(timeout={self.options.timeout.count()})'
+
 
 _CertKeyPair = collections.namedtuple('_CertKeyPair', ['cert', 'key'])
 
diff --git a/python/pyarrow/includes/libarrow_flight.pxd b/python/pyarrow/includes/libarrow_flight.pxd
index b1af6bcb4f..f2b15fb706 100644
--- a/python/pyarrow/includes/libarrow_flight.pxd
+++ b/python/pyarrow/includes/libarrow_flight.pxd
@@ -272,6 +272,7 @@ cdef extern from "arrow/flight/api.h" namespace "arrow" nogil:
 
     cdef cppclass CTimeoutDuration" arrow::flight::TimeoutDuration":
         CTimeoutDuration(double)
+        double count()
 
     cdef cppclass CFlightCallOptions" arrow::flight::FlightCallOptions":
         CFlightCallOptions()

[rmnskb]

I've started implementing the __repr__ dunders for the classes that you've mentioned in #47358, however, now I stumbled upon FlightCallOptions's arguments not being convertable back to Python objects after they are set as C objects inside FlightCallOptions.options.

I'm not sure what you mean, because if you define the __repr__ in Cython you should be able to access the C-level attributes. You may need to add the necessary API declarations for C++ Flight types.

For example, this change works for me:

diff --git a/python/pyarrow/_flight.pyx b/python/pyarrow/_flight.pyx
index fe2e1b3d67..ca842f35bb 100644
--- a/python/pyarrow/_flight.pyx
+++ b/python/pyarrow/_flight.pyx
@@ -142,6 +142,10 @@ cdef class FlightCallOptions(_Weakrefable):
             return &((<FlightCallOptions> obj).options)
         raise TypeError(f"Expected a FlightCallOptions object, not '{type(obj)}'")
 
+    def __repr__(self):
+        # Draft implementation, should be improved
+        return f'FlightCallOptions(timeout={self.options.timeout.count()})'
+
 
 _CertKeyPair = collections.namedtuple('_CertKeyPair', ['cert', 'key'])
 
diff --git a/python/pyarrow/includes/libarrow_flight.pxd b/python/pyarrow/includes/libarrow_flight.pxd
index b1af6bcb4f..f2b15fb706 100644
--- a/python/pyarrow/includes/libarrow_flight.pxd
+++ b/python/pyarrow/includes/libarrow_flight.pxd
@@ -272,6 +272,7 @@ cdef extern from "arrow/flight/api.h" namespace "arrow" nogil:
 
     cdef cppclass CTimeoutDuration" arrow::flight::TimeoutDuration":
         CTimeoutDuration(double)
+        double count()
 
     cdef cppclass CFlightCallOptions" arrow::flight::FlightCallOptions":
         CFlightCallOptions()

Hm, that's interesting, since the same changes have segfaulted for me 🤔
Ok, thanks for the confirmation, I will look into that further.

[rmnskb]

hi @pitrou,

I have implemented __repr__ methods for the aforementioned objects as discussed. For flight.FlightCallOptions, I've decided to go with the straightforward approach by assigning the constructor arguments to private attributes, and then accessing them later. Please let me know what you think :)

[AlenkaF]

Thank you for another contribution @rmnskb !

The overall approach looks good to me. I like that you're only appending the read and write options when they're defined. I think we don’t currently have wrap_options functionality to reconstruct the Python Ipc*Options from self.options.read/write_options, so holding on to the original Python objects makes sense.

One request from me — it would be great to add __repr__ tests, though there aren’t many currently.

One very minor nit: stylistically, I’d consider adding the parentheses inside the class name in the __repr__, like this:

    def __repr__(self):
        return (f"<pyarrow.ipc.IpcReadOptions ("
                ...
                f"included_fields={self.included_fields})>")

[rmnskb]

Hey @AlenkaF ! Thank you for the comments :)

Regarding the formatting: I wanted to stay consistent with other __repr__ dunders that were already present at the moment of implementing the new ones, hence this style.

Other than that, I've covered the implementation with tests, as you've suggested.
Please let me know if you have any other suggestions :)

[AlenkaF]

Thank you for the changes, the PR is looking great!
I only have one small comment and then that is all from my side.

@pitrou mind giving one more look?

[AlenkaF]

It also looks like the linter needs a change:

diff --git a/python/pyarrow/ipc.pxi b/python/pyarrow/ipc.pxi
index c25b2dcab2..565fe3d8fe 100644
--- a/python/pyarrow/ipc.pxi
+++ b/python/pyarrow/ipc.pxi
@@ -335,7 +335,7 @@ cdef class IpcWriteOptions(_Weakrefable):
 
     def __repr__(self):
         compression_repr = f"compression=\"{self.compression}\" " \
-                if self.compression is not None else ""
+            if self.compression is not None else ""
 
         metadata_version = MetadataVersion(self.metadata_version).name

[rmnskb]

Hey @AlenkaF @pitrou @raulcd , thank you for the review!
Regarding the read_options and write_options: I decided to go ahead with the wrapper functions implementations as @raulcd has suggested. Thank you for pointing in the right direction :)
I've also added the docstrings for the FlightCallOptions getters.
Please let me know if there is anything else that I can enhance on this PR :)

[raulcd]

Looks great, just a nit on a test which I don't think it's covered.

[AlenkaF]

@github-actions crossbow submit -g python

[github-actions]

Revision: de04f9b

Submitted crossbow builds: ursacomputing/crossbow @ actions-71c612bccb

Task	Status
example-python-minimal-build-fedora-conda
example-python-minimal-build-ubuntu-venv
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-emscripten
test-cuda-python-ubuntu-22.04-cuda-11.7.1
test-debian-12-python-3-amd64
test-debian-12-python-3-i386
test-fedora-42-python-3
test-ubuntu-22.04-python-3
test-ubuntu-22.04-python-313-freethreading
test-ubuntu-24.04-python-3

[AlenkaF]

Thanks for the reviews and the responsive updates! The PR looks much better now.
While checking the minimal extended builds, I noticed we’ll need a pytest zstd mark (see this example).

The test-conda-python-emscripten failure seems unrelated.

[AlenkaF]

@github-actions crossbow submit example-python-minimal-build-*

[AlenkaF]

Rebase would probably remove the AppVeyor build (failure).

[github-actions]

Revision: 593a0a8

Submitted crossbow builds: ursacomputing/crossbow @ actions-b9c2408cdb

Task	Status
example-python-minimal-build-fedora-conda
example-python-minimal-build-ubuntu-venv

[raulcd]

Rebase would probably remove the AppVeyor build (failure).

Not really, we are waiting for INFRA to remove the integration because the job was migrated so there's no AppVeyor configuration on the repo anymore, see:
https://issues.apache.org/jira/browse/INFRA-27239

[AlenkaF]

Aha, thanks for the info @raulcd !

[AlenkaF]

There has been some great work done here, thanks!
I think this PR is ready. Will wait for another approval before we merge. cc @pitrou @raulcd