Add ordered and app_metadata attributes to PyArrow FlightInfo #1
Conversation
EnricoMi
force-pushed
the
more-flightinfo-attributes
branch
from
54f8151
to
73ef3c0
Compare
python/pyarrow/_flight.pyx
Outdated
| @@ -844,7 +844,7 @@ cdef class FlightInfo(_Weakrefable): | |||
| return obj | |||
|
|
|||
| def __init__(self, Schema schema, FlightDescriptor descriptor, endpoints, | |||
| total_records, total_bytes): | |||
| total_records, total_bytes, ordered, app_metadata): | |||
There was a problem hiding this comment.
| total_records, total_bytes, ordered, app_metadata): | |
| total_records, total_bytes, ordered=False, app_metadata=b''): |
These new parameters should be optional for backwards compatibility
There was a problem hiding this comment.
Tried that, this is not supported by Cython 😞.
There was a problem hiding this comment.
That's odd, there are other cdef classes in this same file that have optional parameters in their __init__, eg. FlightError also has an optional bytes parameter. What is the error you get? (Disclaimer: I'm not very familiar with Cython)
There was a problem hiding this comment.
Looks like it works this time. 🥳
python/pyarrow/_flight.pyx
Outdated
| the total bytes in this flight, or -1 if unknown. | ||
| ordered : boolean | ||
| Whether endpoints are in the same order as the data. | ||
| app_metadata : str |
There was a problem hiding this comment.
| app_metadata : str | |
| app_metadata : bytes, optional |
There was a problem hiding this comment.
I guess if we wanted to be a bit more flexible we could allow bytes or str, and call tobytes on this.
| @@ -950,7 +972,9 @@ cdef class FlightInfo(_Weakrefable): | |||
| f"descriptor={self.descriptor} " | |||
| f"endpoints={self.endpoints} " | |||
| f"total_records={self.total_records} " | |||
| f"total_bytes={self.total_bytes}>") | |||
| f"total_bytes={self.total_bytes} " | |||
| f"ordered={'true' if self.ordered else 'false'} " | |||
There was a problem hiding this comment.
Nitpicking, but I'd expect to have True and False in a Python repr, as the builtin bool constants are capitalized.
There was a problem hiding this comment.
Makes sense, I'll flag this with the arrow devs.
| "total_records=1 " | ||
| "total_bytes=42 " | ||
| "ordered=true " | ||
| "app_metadata='7465737420617070206d65746164617461'>") |
There was a problem hiding this comment.
This behaviour of using the hex representation might be a little confusing. It might be best to stick with the default bytes repr, ie. app_metadata=b'test app metadata'. This will still handle non-ascii bytes ok by displaying them like \x00.
Although I'm also a bit concerned that this field could be quite large, maybe we just want something like app_metadata=<bytes with length 17>
There was a problem hiding this comment.
This is what the c++ implementation does. I guess they use hex because it is opaque metadata. I'd leave this to / flag this with the arrow devs.
There was a problem hiding this comment.
OK yeah matching the C++ behaviour makes sense then.
EnricoMi
force-pushed
the
more-flightinfo-attributes
branch
2 times, most recently
from
3d69078
to
2c8bfa3
Compare
EnricoMi
force-pushed
the
more-flightinfo-attributes
branch
from
2c8bfa3
to
cf01691
Compare
|
Thanks for the code review, raised upstream: apache#43537. |
Rationale for this change
What changes are included in this PR?
Are these changes tested?
Are there any user-facing changes?