GH-36954: [Python] Add more FlightInfo / FlightEndpoint attributes #43537

EnricoMi · 2024-08-02T13:42:16Z

Rationale for this change

The C++ classes FlightInfo and FlightEndpoint have attributes that are not available via the Python API.

What changes are included in this PR?

Make the following attributes available in Python:

FlightInfo.ordered
FlightInfo.app_metadata
FlightEndpoint.expiration_time
FlightEndpoint.app_metadata

Also makes existing attributes optional in constructor:

FlightInfo.total_records
FlightInfo.total_bytes

Are these changes tested?

Existing tests that test existing attributes are extended.

Are there any user-facing changes?

Yes, changes are backward compatible.

GitHub Issue: [FlightRPC][Python] Expose new Flight features in Python #36954

github-actions · 2024-08-02T13:42:42Z

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

EnricoMi · 2024-08-02T13:50:11Z

python/pyarrow/_flight.pyx

-                f"locations={self.locations!r}>")
+                f"locations={self.locations!r} "
+                f"expiration_time={self.expiration_time} "
+                f"app_metadata='{self.app_metadata.hex()}'>")


This matches the C++ implementation of ToString. In Python, we could easily use self.app_metadata, which represents the metadata bytes in ASCII with automatic escaping of non-printable characters, e.g. b'abc\xe2\x80\xa6', which is more readable in the absence of non-printable characters than .hex().

I think that we don't need to use the same style as C++ in Python.
So we can remove .hex().

@jorisvandenbossche What do you think about this?

EnricoMi · 2024-08-02T13:51:40Z

python/pyarrow/_flight.pyx

+        total_records : int optional, default None
+            the total records in this flight, or -1 if unknown.
+        total_bytes : int optional, default None
+            the total bytes in this flight, or -1 if unknown.


While adding new attributes ordered and app_metadata as optional arguments, which simplifies user and test code, making existing total_records and total_bytes optional sounds like a good idea

How about using -1 not None as the default value because the document says -1 if unknown not None if unknown?

I will use -1 though it is less pythonic, as it is more consistent with C++.

EnricoMi · 2024-08-02T14:00:16Z

python/pyarrow/_flight.pyx

@@ -950,7 +1005,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'} "


This matches the C++ implementation of ToString. In Python, you would expect True or False though. Happy to change this to

Suggested change

f"ordered={'true' if self.ordered else 'false'} "

f"ordered={self.ordered} "

I think that we can use True/False in Python.

EnricoMi · 2024-08-02T14:02:38Z

python/pyarrow/tests/test_flight.py

-                     "locations=[]>")
+                     "locations=[] "
+                     "expiration_time=2023-04-05 12:34:56+00:00 "
+                     "app_metadata='656e64706f696e7420617070206d65746164617461'>")


This could read (which is more readable)

app_metadata=b'endpoint app metadata'

if we wouldn't stick to the C++ implementation of ToString here.

EnricoMi · 2024-08-02T14:03:07Z

python/pyarrow/tests/test_flight.py

-        "total_bytes=-1>")
+        "total_records=1 "
+        "total_bytes=42 "
+        "ordered=true "


In Python, you would expect

ordered=True

here.

kou · 2024-08-02T21:52:49Z

python/pyarrow/_flight.pyx

-                f"locations={self.locations!r}>")
+                f"locations={self.locations!r} "
+                f"expiration_time={self.expiration_time} "
+                f"app_metadata='{self.app_metadata.hex()}'>")


I think that we don't need to use the same style as C++ in Python.
So we can remove .hex().

@jorisvandenbossche What do you think about this?

kou · 2024-08-02T21:53:25Z

python/pyarrow/_flight.pyx

+        total_records : int optional, default None
+            the total records in this flight, or -1 if unknown.
+        total_bytes : int optional, default None
+            the total bytes in this flight, or -1 if unknown.


How about using -1 not None as the default value because the document says -1 if unknown not None if unknown?

kou · 2024-08-02T21:54:14Z

python/pyarrow/_flight.pyx

@@ -950,7 +1005,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'} "


I think that we can use True/False in Python.

kou · 2024-08-02T21:56:52Z

python/pyarrow/_flight.pyx

@@ -736,6 +743,12 @@ cdef class FlightEndpoint(_Weakrefable):
                    CLocation.Parse(tobytes(location)).Value(&c_location))
            self.endpoint.locations.push_back(c_location)

+        if expiration_time is not None:
+            self.endpoint.expiration_time = time_point(
+                microseconds(expiration_time.cast(timestamp("us")).value))


Is microseconds correct?
I think that we should use nanoseconds:

arrow/cpp/src/arrow/flight/types.h

Lines 76 to 88 in 8068554

/// \brief A timestamp compatible with Protocol Buffer's

/// google.protobuf.Timestamp:

///

/// https://protobuf.dev/reference/protobuf/google.protobuf/#timestamp

///

/// > A Timestamp represents a point in time independent of any time

/// > zone or calendar, represented as seconds and fractions of

/// > seconds at nanosecond resolution in UTC Epoch time. It is

/// > encoded using the Proleptic Gregorian Calendar which extends the

/// > Gregorian calendar backwards to year one. It is encoded assuming

/// > all minutes are 60 seconds long, i.e. leap seconds are "smeared"

/// > so that no leap second table is needed for interpretation. Range

/// > is from 0001-01-01T00:00:00Z to 9999-12-31T23:59:59.999999999Z.

I'd prefer nanoseconds as well, but that did not work on macOS:

/Users/runner/work/arrow/arrow/build/python/build/temp.macosx-10.9-universal2-cpython-311/_flight.cpp:31893:102: error: no matching conversion for functional-style cast from 'std::chrono::nanoseconds' (aka 'duration<long long, ratio<1LL, 1000000000LL>>') to 'std::chrono::system_clock::time_point' (aka 'time_point<std::chrono::system_clock>') __pyx_v_self->endpoint.expiration_time = ((std::optional<std::chrono::system_clock::time_point> )std::chrono::system_clock::time_point(std::chrono::nanoseconds(__pyx_t_12))); ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

https://github.com/EnricoMi/arrow/actions/runs/10083176212/job/27879168275#step:8:3720

Can we use std::chrono::duration_cast<Timestamp::duration>() like

arrow/cpp/src/arrow/flight/flight_internals_test.cc

Lines 194 to 195 in 1992cc6

Timestamp expiration_time(

std::chrono::duration_cast<Timestamp::duration>(expiration_time_duration));

with Cython?

That did the trick, thanks for the pointer!

arrow/python/pyarrow/_flight.pyx

Lines 746 to 748 in 501d3a6

if expiration_time is not None:

self.endpoint.expiration_time = time_point(duration_cast[time_point.duration](

nanoseconds(expiration_time.cast(timestamp("ns")).value)))

rustyconover · 2024-08-03T17:08:29Z

+1 Looking forward for this being merged.

lidavidm

No additional comments from me.

pitrou

LGTM, just some minor comments

python/pyarrow/_flight.pyx

pitrou · 2024-08-06T13:30:13Z

python/pyarrow/_flight.pyx

+        total_records : int optional, default -1
+            the total records in this flight, or -1 if unknown.
+        total_bytes : int optional, default -1
+            the total bytes in this flight, or -1 if unknown.


None would be a preferrable default for a Python API, IMHO.

Me too, but the attributes will have to return -1 (because of the C++ backend), so that would be a bit inconsistent. @kou your thoughts?

The Python attributes can (and probably should?) return None.

I agree, that would be pythonic and consistent (with None default values). But that might be a breaking change.

None isn't a real value for these. (It's also inconsistent with the underlying protobuf, which doesn't really have optional values, and would default to 0 for a truly unspecified value.)

I'm not sure what a "real value" is? None is the standard placeholder in Python for missing or unknown data. I agree that it would be a slight compatibility break to start returning None instead of -1, but that's for the better IMHO.

The underlying structure does not admit a null value. It is Protobuf, there are no true null values (*this has changed slightly in recent versions but we do not use that codegen option for this structure).

Let's be clear here, though. Is this for input, or output? I would be -1 to ever returning None for this attribute. If it's merely to accept it as an input on construction that might be OK, but it would be misleading to people when it doesn't round trip.

This does not affect the underlying structure. The Python FlightEnpoint can take None values and translate to -1 for the underlying C++ class. The getter methods can translate those -1 back to None, so this Python class feels pythonic.

Using -1 in constructor would continue to work while getters would return None.

User code like if endpoint.total_records == -1 would break. This would have to be migrated to if endpoint.total_records is None, which is for the better, as @pitrou phrased it.

The getter methods can translate those -1 back to None, so this Python class feels pythonic.

This is what I would object to.

The Python FlightEnpoint can take None values and translate to -1 for the underlying C++ class.

I don't mind this though it should be clearly documented (as the underlying Protobuf does not take None).

To summarize, the constructor allows for -1 and None, the properties in both cases return -1. Added test to highlight this.

jorisvandenbossche · 2024-08-07T09:31:25Z

python/pyarrow/_flight.pyx

+        if self.endpoint.expiration_time.has_value():
+            time_since_epoch = duration_cast[nanoseconds](
+                self.endpoint.expiration_time.value().time_since_epoch()).count()


We have some existing helpers defined for time point to nanoseconds conversion:

arrow/python/pyarrow/src/arrow/python/datetime.h

Line 135 in b852b72

inline int64_t TimePoint_to_ns(TimePoint val) { return val.time_since_epoch().count(); }

(and similar for the other way around, TimePoint_from_ns)

That could maybe be used here? (didn't check in detail if it applies here, but they are already declared in libarrow_python.pxd, and then the new chrono.pxd might not be needed)

checking, thanks for the pointer!

simplified constructor and def expiration_time, could removed chrono.pxd with that

This reverts commit 84bbe5f.

Co-authored-by: David Li <li.davidm96@gmail.com>

Co-authored-by: Joris Van den Bossche <jorisvandenbossche@gmail.com>

EnricoMi · 2024-11-07T13:09:51Z

@lidavidm thanks!

conbench-apache-arrow · 2024-11-07T19:06:57Z

After merging your PR, Conbench analyzed the 3 benchmarking runs that have been run so far on merge-commit b193c4f.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details. It also includes information about 4 possible false positives for unstable benchmarks that are known to sometimes produce them.

EnricoMi · 2024-11-08T09:57:09Z

Fixing the Timestamp type in C++ and reverting the workaround of this PR is done in #44681.

…ound from #43537 (#44681) ### What changes are included in this PR? Make `arrow.flight.Timestamp` nanoseconds precision and OS-independent. Use the `CTimePoint` for `FlightEndpoint.expiration_time` to have OS-independent nanoseconds precision. https://github.com/apache/arrow/blob/093655c60783321c786a8e69632f185d37520f4d/python/pyarrow/includes/libarrow_flight.pxd#L133-L139 This reverts workaround for `expiration_time` from #43537. ### Are these changes tested? Existing unit tests are adjusted to the new precision. ### Are there any user-facing changes? **This PR includes breaking changes to public APIs.** The values of `FlightEndpoint.expiration_time` now have nanoseconds precision on any operating system. * GitHub Issue: #44679 Authored-by: Enrico Minack <github@enrico.minack.dev> Signed-off-by: David Li <li.davidm96@gmail.com>

EnricoMi requested a review from lidavidm as a code owner August 2, 2024 13:42

github-actions bot added Component: Python Component: FlightRPC awaiting review Awaiting review labels Aug 2, 2024

EnricoMi commented Aug 2, 2024

View reviewed changes

github-actions bot added awaiting committer review Awaiting committer review and removed awaiting review Awaiting review labels Aug 2, 2024

This was referenced Aug 2, 2024

Add ordered and app_metadata attributes to PyArrow FlightInfo EnricoMi/arrow#1

Closed

More FlightEndpoint attributes EnricoMi/arrow#2

Closed

kou reviewed Aug 2, 2024

View reviewed changes

github-actions bot added awaiting changes Awaiting changes and removed awaiting committer review Awaiting committer review labels Aug 2, 2024

github-actions bot added awaiting change review Awaiting change review awaiting changes Awaiting changes and removed awaiting changes Awaiting changes awaiting change review Awaiting change review labels Aug 3, 2024

lidavidm reviewed Aug 5, 2024

View reviewed changes

github-actions bot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Aug 5, 2024

EnricoMi force-pushed the more-flightendpoint-attributes branch 3 times, most recently from 0506ead to e5ece4b Compare August 6, 2024 11:12

pitrou reviewed Aug 6, 2024

View reviewed changes

github-actions bot added awaiting changes Awaiting changes and removed awaiting change review Awaiting change review labels Aug 7, 2024

jorisvandenbossche reviewed Aug 7, 2024

View reviewed changes

github-actions bot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Aug 30, 2024

EnricoMi and others added 12 commits November 7, 2024 12:00

Remove chrono.pxd, use existing CTimePoint instead

08b1a58

Fix Flight Timestamp definition to OS-agnostic nanoseconds

9dc0e75

Revert "Fix Flight Timestamp definition to OS-agnostic nanoseconds"

989f7a2

This reverts commit 84bbe5f.

Fix conversion to and from system time

08d6b48

Fix lint error

985b115

Fix docstring typo

d264ce1

Co-authored-by: David Li <li.davidm96@gmail.com>

Emphasize app_metadata non-nullability in docstring

bf62c5d

Apply suggestions from code review

818660f

Co-authored-by: Joris Van den Bossche <jorisvandenbossche@gmail.com>

Apply review suggestion around CTimestampType with timezone

c88fdd5

Co-authored-by: Joris Van den Bossche <jorisvandenbossche@gmail.com>

Cleanup after committing suggestions

828070e

Check argument types in FlightEndpoint()

d2d7cab

Add comments on timestamp cast needs

94a40b2

EnricoMi force-pushed the more-flightendpoint-attributes branch from 0b49a09 to 94a40b2 Compare November 7, 2024 11:01

lidavidm approved these changes Nov 7, 2024

View reviewed changes

github-actions bot added awaiting merge Awaiting merge and removed awaiting change review Awaiting change review labels Nov 7, 2024

lidavidm merged commit b193c4f into apache:main Nov 7, 2024
13 checks passed

lidavidm removed the awaiting merge Awaiting merge label Nov 7, 2024

lidavidm mentioned this pull request Nov 7, 2024

[FlightRPC][Python] Expose new Flight features in Python #36954

Closed

7 tasks

EnricoMi added a commit to EnricoMi/arrow that referenced this pull request Nov 7, 2024

Fix Flight Timestamp type, revert workaround from apache#43537

1729d5a

EnricoMi added a commit to EnricoMi/arrow that referenced this pull request Nov 7, 2024

Fix Flight Timestamp type, revert workaround from apache#43537

a61175d

EnricoMi added a commit to EnricoMi/arrow that referenced this pull request Nov 7, 2024

Fix Flight Timestamp type, revert workaround from apache#43537

481732e

EnricoMi deleted the more-flightendpoint-attributes branch November 8, 2024 08:10

EnricoMi added a commit to EnricoMi/arrow that referenced this pull request Nov 8, 2024

Fix Flight Timestamp type, revert workaround from apache#43537

13630cd

EnricoMi mentioned this pull request Nov 8, 2024

GH-44679: [C++][Python] Fix Flight Timestamp precision, revert workaround from #43537 #44681

Merged

EnricoMi added a commit to EnricoMi/arrow that referenced this pull request Nov 8, 2024

Fix Flight Timestamp type, revert workaround from apache#43537

40c9ea0

	f"ordered={'true' if self.ordered else 'false'} "
	f"ordered={self.ordered} "

	/// \brief A timestamp compatible with Protocol Buffer's
	/// google.protobuf.Timestamp:
	///
	/// https://protobuf.dev/reference/protobuf/google.protobuf/#timestamp
	///
	/// > A Timestamp represents a point in time independent of any time
	/// > zone or calendar, represented as seconds and fractions of
	/// > seconds at nanosecond resolution in UTC Epoch time. It is
	/// > encoded using the Proleptic Gregorian Calendar which extends the
	/// > Gregorian calendar backwards to year one. It is encoded assuming
	/// > all minutes are 60 seconds long, i.e. leap seconds are "smeared"
	/// > so that no leap second table is needed for interpretation. Range
	/// > is from 0001-01-01T00:00:00Z to 9999-12-31T23:59:59.999999999Z.

	Timestamp expiration_time(
	std::chrono::duration_cast<Timestamp::duration>(expiration_time_duration));

	if expiration_time is not None:
	self.endpoint.expiration_time = time_point(duration_cast[time_point.duration](
	nanoseconds(expiration_time.cast(timestamp("ns")).value)))

GH-36954: [Python] Add more FlightInfo / FlightEndpoint attributes #43537

GH-36954: [Python] Add more FlightInfo / FlightEndpoint attributes #43537

Conversation

EnricoMi commented Aug 2, 2024 • edited by github-actions bot Loading

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

github-actions bot commented Aug 2, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rustyconover commented Aug 3, 2024

lidavidm left a comment

Choose a reason for hiding this comment

pitrou left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

EnricoMi Aug 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

EnricoMi Aug 7, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

EnricoMi Aug 30, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

EnricoMi commented Nov 7, 2024

conbench-apache-arrow bot commented Nov 7, 2024

EnricoMi commented Nov 8, 2024

EnricoMi commented Aug 2, 2024 •

edited by github-actions bot

Loading

EnricoMi Aug 6, 2024 •

edited

Loading

EnricoMi Aug 7, 2024 •

edited

Loading

EnricoMi Aug 30, 2024 •

edited

Loading