You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "EnricoMi (via GitHub)" <gi...@apache.org> on 2024/08/02 13:42:19 UTC
[PR] GH-36954: [Python] Add more FlightInfo / FlightEndpoint attributes [arrow]
EnricoMi opened a new pull request, #43537:
URL: https://github.com/apache/arrow/pull/43537
### 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-36954: [Python] Add more FlightInfo / FlightEndpoint attributes [arrow]
Posted by "EnricoMi (via GitHub)" <gi...@apache.org>.
EnricoMi commented on code in PR #43537:
URL: https://github.com/apache/arrow/pull/43537#discussion_r1765096687
##########
python/pyarrow/_flight.pyx:
##########
@@ -855,10 +895,14 @@ cdef class FlightInfo(_Weakrefable):
the descriptor for this flight.
endpoints : list of FlightEndpoint
a list of endpoints where this flight is available.
- total_records : int
- the total records in this flight, or -1 if unknown
- total_bytes : int
- the total bytes in this flight, or -1 if unknown
+ total_records : int optional, default None
+ the total records in this flight, -1 or None if unknown.
+ total_bytes : int optional, default None
+ the total bytes in this flight, -1 or None if unknown.
+ ordered : boolean optional, default False
+ Whether endpoints are in the same order as the data.
+ app_metadata : bytes or str optional, default ""
+ Application-defined opaque metadata.
Review Comment:
```suggestion
app_metadata : bytes or str, default ""
Application-defined opaque metadata.
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-36954: [Python] Add more FlightInfo / FlightEndpoint attributes [arrow]
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on code in PR #43537:
URL: https://github.com/apache/arrow/pull/43537#discussion_r1752964990
##########
python/pyarrow/_flight.pyx:
##########
@@ -736,16 +742,47 @@ 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 = TimePoint_from_ns(
+ expiration_time.cast(timestamp("ns")).value)
Review Comment:
SGTM
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-36954: [Python] Add more FlightInfo / FlightEndpoint attributes [arrow]
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on code in PR #43537:
URL: https://github.com/apache/arrow/pull/43537#discussion_r1764321234
##########
python/pyarrow/_flight.pyx:
##########
@@ -736,16 +742,48 @@ 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 = TimePoint_to_system_time(TimePoint_from_ns(
+ expiration_time.cast(timestamp("ns")).value))
+
+ self.endpoint.app_metadata = tobytes(app_metadata)
Review Comment:
sorry, does this handle None appropriately? (The docstring states 'optional')
##########
python/pyarrow/_flight.pyx:
##########
@@ -736,16 +742,48 @@ 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 = TimePoint_to_system_time(TimePoint_from_ns(
+ expiration_time.cast(timestamp("ns")).value))
+
+ self.endpoint.app_metadata = tobytes(app_metadata)
+
@property
def ticket(self):
"""Get the ticket in this endpoint."""
return Ticket(self.endpoint.ticket.ticket)
@property
def locations(self):
+ """Get locations where this flight is available."""
return [Location.wrap(location)
for location in self.endpoint.locations]
+ @property
+ def expiration_time(self):
+ """Get the Expiration time of this stream.
Review Comment:
```suggestion
"""Get the expiration time of this stream.
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-36954: [Python] Add more FlightInfo / FlightEndpoint attributes [arrow]
Posted by "EnricoMi (via GitHub)" <gi...@apache.org>.
EnricoMi commented on code in PR #43537:
URL: https://github.com/apache/arrow/pull/43537#discussion_r1771933017
##########
python/pyarrow/_flight.pyx:
##########
@@ -887,6 +933,23 @@ cdef class FlightInfo(_Weakrefable):
"""The size in bytes of the data in this flight, or -1 if unknown."""
return self.info.get().total_bytes()
+ @property
+ def ordered(self):
+ """Whether endpoints are in the same order as the data."""
+ return self.info.get().ordered()
+
+ @property
+ def app_metadata(self):
+ """
+ Application-defined opaque metadata.
+
+ There is no inherent or required relationship between this and the app_metadata fields in the FlightEndpoints
+ or resulting FlightData messages. Since this metadata is application-defined, a given application could define
+ there to be a relationship, but there is none required by the spec.
Review Comment:
```suggestion
There is no inherent or required relationship between this and the
app_metadata fields in the FlightEndpoints or resulting FlightData
messages. Since this metadata is application-defined, a given
application could define there to be a relationship, but there is
none required by the spec.
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-36954: [Python] Add more FlightInfo / FlightEndpoint attributes [arrow]
Posted by "EnricoMi (via GitHub)" <gi...@apache.org>.
EnricoMi commented on code in PR #43537:
URL: https://github.com/apache/arrow/pull/43537#discussion_r1771938028
##########
python/pyarrow/includes/libarrow.pxd:
##########
@@ -277,6 +277,7 @@ cdef extern from "arrow/api.h" namespace "arrow" nogil:
cdef cppclass CTimestampType" arrow::TimestampType"(CFixedWidthType):
CTimestampType(TimeUnit unit)
+ CTimestampType(TimeUnit unit, const c_string& timezone)
Review Comment:
```suggestion
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-36954: [Python] Add more FlightInfo / FlightEndpoint attributes [arrow]
Posted by "EnricoMi (via GitHub)" <gi...@apache.org>.
EnricoMi commented on code in PR #43537:
URL: https://github.com/apache/arrow/pull/43537#discussion_r1764796379
##########
python/pyarrow/_flight.pyx:
##########
@@ -736,16 +742,48 @@ 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 = TimePoint_to_system_time(TimePoint_from_ns(
+ expiration_time.cast(timestamp("ns")).value))
+
+ self.endpoint.app_metadata = tobytes(app_metadata)
Review Comment:
Optional in the sense that there is a default value for `app_metadata`. Documentation does not say `app_metadata` can be `None`, as in `Optional[str]`.
The `app_metadata` in C++ is non-nullable string, so default of "" is best to reflect this. I can make it None-safe, though:
```suggestion
self.endpoint.app_metadata = tobytes(app_metadata or "")
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-36954: [Python] Add more FlightInfo / FlightEndpoint attributes [arrow]
Posted by "EnricoMi (via GitHub)" <gi...@apache.org>.
EnricoMi commented on code in PR #43537:
URL: https://github.com/apache/arrow/pull/43537#discussion_r1751365590
##########
python/pyarrow/_flight.pyx:
##########
@@ -736,16 +742,47 @@ 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 = TimePoint_from_ns(
+ expiration_time.cast(timestamp("ns")).value)
Review Comment:
@adamreeve thanks for pinpointing the root cause for this. I can confirm changing Timestamp in `cpp/src/arrow/flight/types.h` fixes this:
```diff
-using Timestamp = std::chrono::system_clock::time_point;
+using Timestamp = std::chrono::time_point<std::chrono::system_clock, std::chrono::nanoseconds>;
```
84bbe5f68
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-36954: [Python] Add more FlightInfo / FlightEndpoint attributes [arrow]
Posted by "rok (via GitHub)" <gi...@apache.org>.
rok commented on code in PR #43537:
URL: https://github.com/apache/arrow/pull/43537#discussion_r1750286811
##########
python/pyarrow/_flight.pyx:
##########
@@ -736,16 +742,47 @@ 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 = TimePoint_from_ns(
+ expiration_time.cast(timestamp("ns")).value)
Review Comment:
Looking at how we do `optional` elsewhere, how about:
```suggestion
if expiration_time is not None:
self.endpoint.expiration_time = <optional[CTimePoint]>(
TimePoint_from_ns(expiration_time.value.cast(timestamp("ns"))))
```
Or maybe even:
```suggestion
self.endpoint.expiration_time = <optional[CTimePoint]>nullopt if expiration_time is None else
<optional[CTimePoint]>(TimePoint_from_ns(expiration_time.value.cast(timestamp("ns"))))
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-36954: [Python] Add more FlightInfo / FlightEndpoint attributes [arrow]
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou commented on code in PR #43537:
URL: https://github.com/apache/arrow/pull/43537#discussion_r1706239944
##########
python/pyarrow/_flight.pyx:
##########
@@ -855,10 +888,14 @@ cdef class FlightInfo(_Weakrefable):
the descriptor for this flight.
endpoints : list of FlightEndpoint
a list of endpoints where this flight is available.
- total_records : int
- the total records in this flight, or -1 if unknown
- total_bytes : int
- the total bytes in this flight, or -1 if unknown
+ 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.
Review Comment:
I agree with lidavidm.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-36954: [Python] Add more FlightInfo / FlightEndpoint attributes [arrow]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #43537:
URL: https://github.com/apache/arrow/pull/43537#issuecomment-2265433553
:warning: GitHub issue #36954 **has been automatically assigned in GitHub** to PR creator.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-36954: [Python] Add more FlightInfo / FlightEndpoint attributes [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #43537:
URL: https://github.com/apache/arrow/pull/43537#discussion_r1706567282
##########
python/pyarrow/_flight.pyx:
##########
@@ -855,10 +888,14 @@ cdef class FlightInfo(_Weakrefable):
the descriptor for this flight.
endpoints : list of FlightEndpoint
a list of endpoints where this flight is available.
- total_records : int
- the total records in this flight, or -1 if unknown
- total_bytes : int
- the total bytes in this flight, or -1 if unknown
+ 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.
Review Comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-36954: [Python] Add more FlightInfo / FlightEndpoint attributes [arrow]
Posted by "EnricoMi (via GitHub)" <gi...@apache.org>.
EnricoMi commented on code in PR #43537:
URL: https://github.com/apache/arrow/pull/43537#discussion_r1702857842
##########
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))
Review Comment:
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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-36954: [Python] Add more FlightInfo / FlightEndpoint attributes [arrow]
Posted by "EnricoMi (via GitHub)" <gi...@apache.org>.
EnricoMi commented on code in PR #43537:
URL: https://github.com/apache/arrow/pull/43537#discussion_r1705585503
##########
python/pyarrow/_flight.pyx:
##########
@@ -855,10 +888,14 @@ cdef class FlightInfo(_Weakrefable):
the descriptor for this flight.
endpoints : list of FlightEndpoint
a list of endpoints where this flight is available.
- total_records : int
- the total records in this flight, or -1 if unknown
- total_bytes : int
- the total bytes in this flight, or -1 if unknown
+ 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.
Review Comment:
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?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-36954: [Python] Add more FlightInfo / FlightEndpoint attributes [arrow]
Posted by "EnricoMi (via GitHub)" <gi...@apache.org>.
EnricoMi commented on PR #43537:
URL: https://github.com/apache/arrow/pull/43537#issuecomment-2464281078
Fixing the `Timestamp` type in C++ and reverting the workaround of this PR is done in #44681.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-36954: [Python] Add more FlightInfo / FlightEndpoint attributes [arrow]
Posted by "EnricoMi (via GitHub)" <gi...@apache.org>.
EnricoMi commented on code in PR #43537:
URL: https://github.com/apache/arrow/pull/43537#discussion_r1752235291
##########
python/pyarrow/_flight.pyx:
##########
@@ -736,16 +742,47 @@ 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 = TimePoint_from_ns(
+ expiration_time.cast(timestamp("ns")).value)
Review Comment:
> I wonder if you might be able to "trick" Cython with a declaration like
>
> ```
> CTimePoint CTimePointFromSystemTime"std::chrono::time_point_cast<TimePoint::duration>"(time_point)
> ```
How would I get this into the code? When I add it to `libarrow_flight.pxd` and use it in `_flight.pyx`:
```
self.endpoint.expiration_time = CTimePointToSystemTime(TimePoint_from_ns(expiration_time.cast(timestamp("ns")).value))
```
I get the usual:
```
pyarrow/_flight.pyx:748:84: Cannot convert 'CTimePoint' to Python object
```
That "trick" looks a lot like what https://github.com/adamreeve/arrow/commit/af9a437f7b23dcbca1307fbb3e4b8c9f28161ea4 does. Shall we go with that?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-36954: [Python] Add more FlightInfo / FlightEndpoint attributes [arrow]
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on code in PR #43537:
URL: https://github.com/apache/arrow/pull/43537#discussion_r1706680786
##########
python/pyarrow/_flight.pyx:
##########
@@ -855,10 +888,14 @@ cdef class FlightInfo(_Weakrefable):
the descriptor for this flight.
endpoints : list of FlightEndpoint
a list of endpoints where this flight is available.
- total_records : int
- the total records in this flight, or -1 if unknown
- total_bytes : int
- the total bytes in this flight, or -1 if unknown
+ 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.
Review Comment:
> 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).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-36954: [Python] Add more FlightInfo / FlightEndpoint attributes [arrow]
Posted by "EnricoMi (via GitHub)" <gi...@apache.org>.
EnricoMi commented on code in PR #43537:
URL: https://github.com/apache/arrow/pull/43537#discussion_r1706677572
##########
python/pyarrow/_flight.pyx:
##########
@@ -855,10 +888,14 @@ cdef class FlightInfo(_Weakrefable):
the descriptor for this flight.
endpoints : list of FlightEndpoint
a list of endpoints where this flight is available.
- total_records : int
- the total records in this flight, or -1 if unknown
- total_bytes : int
- the total bytes in this flight, or -1 if unknown
+ 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.
Review Comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-36954: [Python] Add more FlightInfo / FlightEndpoint attributes [arrow]
Posted by "adamreeve (via GitHub)" <gi...@apache.org>.
adamreeve commented on PR #43537:
URL: https://github.com/apache/arrow/pull/43537#issuecomment-2409148592
It looks like all the review comments have been addressed on this, are there any more changes required? @jorisvandenbossche?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-36954: [Python] Add more FlightInfo / FlightEndpoint attributes [arrow]
Posted by "adamreeve (via GitHub)" <gi...@apache.org>.
adamreeve commented on code in PR #43537:
URL: https://github.com/apache/arrow/pull/43537#discussion_r1751117367
##########
python/pyarrow/_flight.pyx:
##########
@@ -736,16 +742,47 @@ 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 = TimePoint_from_ns(
+ expiration_time.cast(timestamp("ns")).value)
Review Comment:
I tested this out and it doesn't actually fix the problem, as it isn't the handling of the `optional` that's the problem but the `duration` type parameter of the `time_point`.
I was getting confused trying to understand this error until I realised the type annotation for `CFlightEndpoint.expiration_time` is wrong, it is an `optional[CTimePoint]` in `libarrow_flight.pxd`, but in `cpp/src/arrow/flight/types.h`, it's actually a `std::optional<std::chrono::system_clock::time_point>` (see [types.h](https://github.com/apache/arrow/blob/f3dd298bd32f6dc38654680c49b3f1fbf97e3d5f/cpp/src/arrow/flight/types.h#L83)). `CTimePoint` is `std::chrono::time_point<std::chrono::system_clock, std::chrono::nanoseconds>` (from [python/datetime.h](https://github.com/apache/arrow/blob/f3dd298bd32f6dc38654680c49b3f1fbf97e3d5f/python/pyarrow/src/arrow/python/datetime.h#L88)) and always has nanosecond precision, but `std::chrono::system_clock::time_point` has microsecond precision on MacOS.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-36954: [Python] Add more FlightInfo / FlightEndpoint attributes [arrow]
Posted by "adamreeve (via GitHub)" <gi...@apache.org>.
adamreeve commented on code in PR #43537:
URL: https://github.com/apache/arrow/pull/43537#discussion_r1751117367
##########
python/pyarrow/_flight.pyx:
##########
@@ -736,16 +742,47 @@ 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 = TimePoint_from_ns(
+ expiration_time.cast(timestamp("ns")).value)
Review Comment:
I tested this out and it doesn't actually fix the problem, as it isn't the handling of the `optional` that's the problem but the `duration` type parameter of the `time_point`.
I was getting confused trying to understand this error until I realised the type annotation for `CFlightEndpoint.expiration_time` is wrong, it is an `optional[CTimePoint]` in [libarrow_flight.pxd](https://github.com/apache/arrow/blob/0844d3e493c77a2bbe20fb16e78ec4e7cb01acaf/python/pyarrow/includes/libarrow_flight.pxd#L138), but in the underyling C++ class, it's actually a `std::optional<std::chrono::system_clock::time_point>` instead, which isn't the same type (see [types.h](https://github.com/apache/arrow/blob/f3dd298bd32f6dc38654680c49b3f1fbf97e3d5f/cpp/src/arrow/flight/types.h#L83)). `CTimePoint` is `std::chrono::time_point<std::chrono::system_clock, std::chrono::nanoseconds>` (from [python/datetime.h](https://github.com/apache/arrow/blob/f3dd298bd32f6dc38654680c49b3f1fbf97e3d5f/python/pyarrow/src/arrow/python/datetime.h#L88)) and always has nanosecond precision, but `std::chrono::system_clock::time_point` has microsecond precision on MacOS.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-36954: [Python] Add more FlightInfo / FlightEndpoint attributes [arrow]
Posted by "adamreeve (via GitHub)" <gi...@apache.org>.
adamreeve commented on code in PR #43537:
URL: https://github.com/apache/arrow/pull/43537#discussion_r1751160819
##########
python/pyarrow/_flight.pyx:
##########
@@ -736,16 +742,47 @@ 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 = TimePoint_from_ns(
+ expiration_time.cast(timestamp("ns")).value)
Review Comment:
How about something like this change, which fixes the build: https://github.com/adamreeve/arrow/commit/af9a437f7b23dcbca1307fbb3e4b8c9f28161ea4
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-36954: [Python] Add more FlightInfo / FlightEndpoint attributes [arrow]
Posted by "EnricoMi (via GitHub)" <gi...@apache.org>.
EnricoMi commented on code in PR #43537:
URL: https://github.com/apache/arrow/pull/43537#discussion_r1705144191
##########
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))
Review Comment:
That did the trick, thanks for the pointer!
https://github.com/apache/arrow/blob/501d3a6d62d36e6d7c437d10c865535df44aeb33/python/pyarrow/_flight.pyx#L746-L748
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-36954: [Python] Add more FlightInfo / FlightEndpoint attributes [arrow]
Posted by "EnricoMi (via GitHub)" <gi...@apache.org>.
EnricoMi commented on code in PR #43537:
URL: https://github.com/apache/arrow/pull/43537#discussion_r1702867217
##########
python/pyarrow/_flight.pyx:
##########
@@ -855,10 +888,14 @@ cdef class FlightInfo(_Weakrefable):
the descriptor for this flight.
endpoints : list of FlightEndpoint
a list of endpoints where this flight is available.
- total_records : int
- the total records in this flight, or -1 if unknown
- total_bytes : int
- the total bytes in this flight, or -1 if unknown
+ 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.
Review Comment:
I will use `-1` though it is less pythonic, as it is more consistent with C++.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-36954: [Python] Add more FlightInfo / FlightEndpoint attributes [arrow]
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on code in PR #43537:
URL: https://github.com/apache/arrow/pull/43537#discussion_r1751323212
##########
cpp/src/arrow/flight/types.h:
##########
@@ -73,7 +73,7 @@ namespace flight {
/// > 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.
-using Timestamp = std::chrono::system_clock::time_point;
+using Timestamp = std::chrono::time_point<std::chrono::system_clock, std::chrono::nanoseconds>;
Review Comment:
Hmm, this is a breaking change, though - can we keep the fix on the bindings side?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-36954: [Python] Add more FlightInfo / FlightEndpoint attributes [arrow]
Posted by "EnricoMi (via GitHub)" <gi...@apache.org>.
EnricoMi commented on PR #43537:
URL: https://github.com/apache/arrow/pull/43537#issuecomment-2321415635
Any suggestion how to fix the macOS compile error?
I think I have to cast the `time_point` produced via `TimePoint_from_ns` to the system clock precision `std::chrono::system_clock::duration`, preferably via `std::chrono::time_point_cast<system_clock, system_clock::duration>`, but I cannot get this done in Cython words.
Any help appreciated.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-36954: [Python] Add more FlightInfo / FlightEndpoint attributes [arrow]
Posted by "EnricoMi (via GitHub)" <gi...@apache.org>.
EnricoMi commented on code in PR #43537:
URL: https://github.com/apache/arrow/pull/43537#discussion_r1738314366
##########
python/pyarrow/_flight.pyx:
##########
@@ -746,6 +759,24 @@ cdef class FlightEndpoint(_Weakrefable):
return [Location.wrap(location)
for location in self.endpoint.locations]
+ @property
+ def expiration_time(self):
+ cdef:
+ int64_t time_since_epoch
+ const char* UTC = "UTC"
+ shared_ptr[CTimestampType] time_type = make_shared[CTimestampType](TimeUnit.TimeUnit_NANO, UTC)
+ shared_ptr[CTimestampScalar] shared
+ if self.endpoint.expiration_time.has_value():
+ time_since_epoch = duration_cast[nanoseconds](
+ self.endpoint.expiration_time.value().time_since_epoch()).count()
Review Comment:
simplified constructor and `def expiration_time`, could removed `chrono.pxd` with that
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-36954: [Python] Add more FlightInfo / FlightEndpoint attributes [arrow]
Posted by "jorisvandenbossche (via GitHub)" <gi...@apache.org>.
jorisvandenbossche commented on code in PR #43537:
URL: https://github.com/apache/arrow/pull/43537#discussion_r1771296020
##########
python/pyarrow/_flight.pyx:
##########
@@ -704,7 +704,7 @@ cdef class FlightEndpoint(_Weakrefable):
cdef:
CFlightEndpoint endpoint
- def __init__(self, ticket, locations):
+ def __init__(self, ticket, locations, expiration_time=None, app_metadata=""):
Review Comment:
```suggestion
def __init__(self, ticket, locations, TimestampScalar expiration_time=None, app_metadata=""):
```
If we require such a scalar, I would either enforce that through cython in the signature, or otherwise add a check in the code below that ensures a proper values is passed. For example, right now if you pass a stdlib datetime, you get "'datetime.datetime' object has no attribute 'cast'"
##########
python/pyarrow/_flight.pyx:
##########
@@ -713,6 +713,12 @@ cdef class FlightEndpoint(_Weakrefable):
the ticket needed to access this flight
locations : list of string URIs
locations where this flight is available
+ expiration_time : TimestampScalar optional, default None
Review Comment:
```suggestion
expiration_time : TimestampScalar, default None
```
Let's put either `, optional` or either `, default None`, but not both
##########
python/pyarrow/_flight.pyx:
##########
@@ -887,6 +933,23 @@ cdef class FlightInfo(_Weakrefable):
"""The size in bytes of the data in this flight, or -1 if unknown."""
return self.info.get().total_bytes()
+ @property
+ def ordered(self):
+ """Whether endpoints are in the same order as the data."""
+ return self.info.get().ordered()
+
+ @property
+ def app_metadata(self):
+ """
+ Application-defined opaque metadata.
+
+ There is no inherent or required relationship between this and the app_metadata fields in the FlightEndpoints
+ or resulting FlightData messages. Since this metadata is application-defined, a given application could define
+ there to be a relationship, but there is none required by the spec.
Review Comment:
Can you wrap this text in max 80 chars wide?
##########
python/pyarrow/_flight.pyx:
##########
@@ -736,16 +742,48 @@ 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 = TimePoint_to_system_time(TimePoint_from_ns(
+ expiration_time.cast(timestamp("ns")).value))
+
+ self.endpoint.app_metadata = tobytes(app_metadata)
+
@property
def ticket(self):
"""Get the ticket in this endpoint."""
return Ticket(self.endpoint.ticket.ticket)
@property
def locations(self):
+ """Get locations where this flight is available."""
return [Location.wrap(location)
for location in self.endpoint.locations]
+ @property
+ def expiration_time(self):
+ """Get the expiration time of this stream.
+
+ If present, clients may assume they can retry DoGet requests.
+ Otherwise, clients should avoid retrying DoGet requests.
+
+ """
+ cdef:
+ int64_t time_since_epoch
+ const char* UTC = "UTC"
+ shared_ptr[CTimestampType] time_type = make_shared[CTimestampType](TimeUnit.TimeUnit_NANO, UTC)
+ shared_ptr[CTimestampScalar] shared
+ if self.endpoint.expiration_time.has_value():
+ time_since_epoch = TimePoint_to_ns(
+ TimePoint_from_system_time(self.endpoint.expiration_time.value()))
+ shared = make_shared[CTimestampScalar](time_since_epoch, time_type)
+ return Scalar.wrap(<shared_ptr[CScalar]> shared)
Review Comment:
```suggestion
return scalar(time_since_epoch, pa.timestamp("ns", "UTC"))
```
This is a bit simpler (I don't think the slight overhead is an issue for this API). This also avoids you have to define the `time_type` above
##########
python/pyarrow/src/arrow/python/datetime.h:
##########
@@ -144,6 +144,16 @@ inline TimePoint TimePoint_from_ns(int64_t val) {
return TimePoint(TimePoint::duration(val));
}
+ARROW_PYTHON_EXPORT
+inline std::chrono::system_clock::time_point TimePoint_to_system_time(TimePoint val) {
+ return std::chrono::time_point_cast<std::chrono::system_clock::duration>(val);
Review Comment:
Just to understand this code: what does this exactly do? Because my reading is that `TimePoint` type is also already an alias for some `time_point` ?
##########
python/pyarrow/includes/libarrow.pxd:
##########
@@ -277,6 +277,7 @@ cdef extern from "arrow/api.h" namespace "arrow" nogil:
cdef cppclass CTimestampType" arrow::TimestampType"(CFixedWidthType):
CTimestampType(TimeUnit unit)
+ CTimestampType(TimeUnit unit, const c_string& timezone)
Review Comment:
If you take my above suggestion, this is not needed anymore I think
##########
python/pyarrow/src/arrow/python/datetime.h:
##########
@@ -144,6 +144,16 @@ inline TimePoint TimePoint_from_ns(int64_t val) {
return TimePoint(TimePoint::duration(val));
}
+ARROW_PYTHON_EXPORT
+inline std::chrono::system_clock::time_point TimePoint_to_system_time(TimePoint val) {
+ return std::chrono::time_point_cast<std::chrono::system_clock::duration>(val);
Review Comment:
I see a comment in the tests about `system_clock::duration` being different depending on the platform, while `TimePoint` here is defined to always be nanoseconds?
So for the flight API we need to system-dependent resolution?
If that's the reason for adding those APIs, could you add a comment briefly explaining what those functions do and why we need them?
##########
python/pyarrow/_flight.pyx:
##########
@@ -855,10 +895,14 @@ cdef class FlightInfo(_Weakrefable):
the descriptor for this flight.
endpoints : list of FlightEndpoint
a list of endpoints where this flight is available.
- total_records : int
- the total records in this flight, or -1 if unknown
- total_bytes : int
- the total bytes in this flight, or -1 if unknown
+ total_records : int optional, default None
+ the total records in this flight, -1 or None if unknown.
+ total_bytes : int optional, default None
+ the total bytes in this flight, -1 or None if unknown.
+ ordered : boolean optional, default False
Review Comment:
```suggestion
ordered : boolean, default False
```
##########
python/pyarrow/_flight.pyx:
##########
@@ -713,6 +713,12 @@ cdef class FlightEndpoint(_Weakrefable):
the ticket needed to access this flight
locations : list of string URIs
locations where this flight is available
+ expiration_time : TimestampScalar optional, default None
+ Expiration time of this stream. If present, clients may assume
+ they can retry DoGet requests. Otherwise, clients should avoid
+ retrying DoGet requests.
+ app_metadata : bytes or str optional, default ""
Review Comment:
```suggestion
app_metadata : bytes or str, default ""
```
(to follow the same change as you did further down below)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-36954: [Python] Add more FlightInfo / FlightEndpoint attributes [arrow]
Posted by "EnricoMi (via GitHub)" <gi...@apache.org>.
EnricoMi commented on code in PR #43537:
URL: https://github.com/apache/arrow/pull/43537#discussion_r1737965889
##########
python/pyarrow/_flight.pyx:
##########
@@ -855,10 +888,14 @@ cdef class FlightInfo(_Weakrefable):
the descriptor for this flight.
endpoints : list of FlightEndpoint
a list of endpoints where this flight is available.
- total_records : int
- the total records in this flight, or -1 if unknown
- total_bytes : int
- the total bytes in this flight, or -1 if unknown
+ 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.
Review Comment:
To summarize, the constructor allows for `-1` and `None`, the properties in both cases return `-1`. Added test to highlight this.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-36954: [Python] Add more FlightInfo / FlightEndpoint attributes [arrow]
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on code in PR #43537:
URL: https://github.com/apache/arrow/pull/43537#discussion_r1751201774
##########
python/pyarrow/_flight.pyx:
##########
@@ -736,16 +742,47 @@ 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 = TimePoint_from_ns(
+ expiration_time.cast(timestamp("ns")).value)
Review Comment:
Thanks for tracking this down. Fixing the declaration sounds like the way to go.
I wonder if you might be able to "trick" Cython with a declaration like
```
CTimePoint CTimePointFromSystemTime"std::chrono::time_point_cast<TimePoint::duration>"(time_point)
```
but if not it's no big deal
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-36954: [Python] Add more FlightInfo / FlightEndpoint attributes [arrow]
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou commented on code in PR #43537:
URL: https://github.com/apache/arrow/pull/43537#discussion_r1702355452
##########
python/pyarrow/_flight.pyx:
##########
@@ -855,10 +888,14 @@ cdef class FlightInfo(_Weakrefable):
the descriptor for this flight.
endpoints : list of FlightEndpoint
a list of endpoints where this flight is available.
- total_records : int
- the total records in this flight, or -1 if unknown
- total_bytes : int
- the total bytes in this flight, or -1 if unknown
+ 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.
Review Comment:
How about using `-1` not `None` as the default value because the document says `-1 if unknown` not `None if unknown`?
##########
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'} "
Review Comment:
I think that we can use `True`/`False` in Python.
##########
python/pyarrow/_flight.pyx:
##########
@@ -770,7 +801,9 @@ cdef class FlightEndpoint(_Weakrefable):
def __repr__(self):
return (f"<pyarrow.flight.FlightEndpoint ticket={self.ticket!r} "
- f"locations={self.locations!r}>")
+ f"locations={self.locations!r} "
+ f"expiration_time={self.expiration_time} "
+ f"app_metadata='{self.app_metadata.hex()}'>")
Review Comment:
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?
##########
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))
Review Comment:
Is microseconds correct?
I think that we should use nanoseconds: https://github.com/apache/arrow/blob/8068554a5d18129121f824978c7ee4a9c430640b/cpp/src/arrow/flight/types.h#L76-L88
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-36954: [Python] Add more FlightInfo / FlightEndpoint attributes [arrow]
Posted by "EnricoMi (via GitHub)" <gi...@apache.org>.
EnricoMi commented on code in PR #43537:
URL: https://github.com/apache/arrow/pull/43537#discussion_r1706677572
##########
python/pyarrow/_flight.pyx:
##########
@@ -855,10 +888,14 @@ cdef class FlightInfo(_Weakrefable):
the descriptor for this flight.
endpoints : list of FlightEndpoint
a list of endpoints where this flight is available.
- total_records : int
- the total records in this flight, or -1 if unknown
- total_bytes : int
- the total bytes in this flight, or -1 if unknown
+ 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.
Review Comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-36954: [Python] Add more FlightInfo / FlightEndpoint attributes [arrow]
Posted by "EnricoMi (via GitHub)" <gi...@apache.org>.
EnricoMi commented on code in PR #43537:
URL: https://github.com/apache/arrow/pull/43537#discussion_r1764798611
##########
python/pyarrow/_flight.pyx:
##########
@@ -736,16 +742,48 @@ 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 = TimePoint_to_system_time(TimePoint_from_ns(
+ expiration_time.cast(timestamp("ns")).value))
+
+ self.endpoint.app_metadata = tobytes(app_metadata)
Review Comment:
```suggestion
if app_metadata is not None:
app_metadata = ""
self.endpoint.app_metadata = tobytes(app_metadata)
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-36954: [Python] Add more FlightInfo / FlightEndpoint attributes [arrow]
Posted by "EnricoMi (via GitHub)" <gi...@apache.org>.
EnricoMi commented on code in PR #43537:
URL: https://github.com/apache/arrow/pull/43537#discussion_r1764798611
##########
python/pyarrow/_flight.pyx:
##########
@@ -736,16 +742,48 @@ 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 = TimePoint_to_system_time(TimePoint_from_ns(
+ expiration_time.cast(timestamp("ns")).value))
+
+ self.endpoint.app_metadata = tobytes(app_metadata)
Review Comment:
```suggestion
if app_metadata is not None:
self.endpoint.app_metadata = tobytes(app_metadata)
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-36954: [Python] Add more FlightInfo / FlightEndpoint attributes [arrow]
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on code in PR #43537:
URL: https://github.com/apache/arrow/pull/43537#discussion_r1764905726
##########
python/pyarrow/_flight.pyx:
##########
@@ -736,16 +742,48 @@ 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 = TimePoint_to_system_time(TimePoint_from_ns(
+ expiration_time.cast(timestamp("ns")).value))
+
+ self.endpoint.app_metadata = tobytes(app_metadata)
Review Comment:
Maybe we can write `str, default ""` instead? To me `str, optional` is a little ambiguous in this case: I would have taken it to mean that `None` is allowed. I don't see a clear spec for this in numpydoc but I did find this Pandas docstring guide (albeit from 2018) with a similar assumption
https://python-sprints.github.io/pandas/guide/pandas_docstring.html#section-3-parameters
> In cases where the default value is None, meaning that the value will not be used. Instead of “str, default None”, it is preferred to write “str, optional”.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-36954: [Python] Add more FlightInfo / FlightEndpoint attributes [arrow]
Posted by "EnricoMi (via GitHub)" <gi...@apache.org>.
EnricoMi commented on PR #43537:
URL: https://github.com/apache/arrow/pull/43537#issuecomment-2462201114
@lidavidm thanks!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-36954: [Python] Add more FlightInfo / FlightEndpoint attributes [arrow]
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on PR #43537:
URL: https://github.com/apache/arrow/pull/43537#issuecomment-2461918792
@EnricoMi I don't think this has to be held up so much. Would you mind rebasing again to make sure CI hasn't bitrotted and then I think we can merge?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-36954: [Python] Add more FlightInfo / FlightEndpoint attributes [arrow]
Posted by "EnricoMi (via GitHub)" <gi...@apache.org>.
EnricoMi commented on code in PR #43537:
URL: https://github.com/apache/arrow/pull/43537#discussion_r1764796379
##########
python/pyarrow/_flight.pyx:
##########
@@ -736,16 +742,48 @@ 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 = TimePoint_to_system_time(TimePoint_from_ns(
+ expiration_time.cast(timestamp("ns")).value))
+
+ self.endpoint.app_metadata = tobytes(app_metadata)
Review Comment:
Optional in the sense that there is a default value for `app_metadata`. Documentation does not say `app_metadata` can be `None`, as in `Optional[str]`.
The `app_metadata` in C++ is non-nullable string, so default of "" is best to reflect this. I can make it None-safe, though:
```suggestion
if app_metadata is not None:
app_metadata = ""
self.endpoint.app_metadata = tobytes(app_metadata)
```
##########
python/pyarrow/_flight.pyx:
##########
@@ -736,16 +742,48 @@ 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 = TimePoint_to_system_time(TimePoint_from_ns(
+ expiration_time.cast(timestamp("ns")).value))
+
+ self.endpoint.app_metadata = tobytes(app_metadata)
Review Comment:
```suggestion
if app_metadata is not None:
app_metadata = ""
self.endpoint.app_metadata = tobytes(app_metadata)
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-36954: [Python] Add more FlightInfo / FlightEndpoint attributes [arrow]
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou commented on code in PR #43537:
URL: https://github.com/apache/arrow/pull/43537#discussion_r1702919305
##########
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))
Review Comment:
Can we use `std::chrono::duration_cast<Timestamp::duration>()` like https://github.com/apache/arrow/blob/1992cc64c74367579fb587f0431bf4340401c38a/cpp/src/arrow/flight/flight_internals_test.cc#L194-L195 with Cython?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-36954: [Python] Add more FlightInfo / FlightEndpoint attributes [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #43537:
URL: https://github.com/apache/arrow/pull/43537#discussion_r1705611942
##########
python/pyarrow/_flight.pyx:
##########
@@ -855,10 +888,14 @@ cdef class FlightInfo(_Weakrefable):
the descriptor for this flight.
endpoints : list of FlightEndpoint
a list of endpoints where this flight is available.
- total_records : int
- the total records in this flight, or -1 if unknown
- total_bytes : int
- the total bytes in this flight, or -1 if unknown
+ 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.
Review Comment:
The Python attributes can (and probably should?) return `None`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-36954: [Python] Add more FlightInfo / FlightEndpoint attributes [arrow]
Posted by "EnricoMi (via GitHub)" <gi...@apache.org>.
EnricoMi commented on code in PR #43537:
URL: https://github.com/apache/arrow/pull/43537#discussion_r1706006457
##########
python/pyarrow/_flight.pyx:
##########
@@ -855,10 +888,14 @@ cdef class FlightInfo(_Weakrefable):
the descriptor for this flight.
endpoints : list of FlightEndpoint
a list of endpoints where this flight is available.
- total_records : int
- the total records in this flight, or -1 if unknown
- total_bytes : int
- the total bytes in this flight, or -1 if unknown
+ 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.
Review Comment:
But that might be a breaking change.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-36954: [Python] Add more FlightInfo / FlightEndpoint attributes [arrow]
Posted by "EnricoMi (via GitHub)" <gi...@apache.org>.
EnricoMi commented on code in PR #43537:
URL: https://github.com/apache/arrow/pull/43537#discussion_r1706006457
##########
python/pyarrow/_flight.pyx:
##########
@@ -855,10 +888,14 @@ cdef class FlightInfo(_Weakrefable):
the descriptor for this flight.
endpoints : list of FlightEndpoint
a list of endpoints where this flight is available.
- total_records : int
- the total records in this flight, or -1 if unknown
- total_bytes : int
- the total bytes in this flight, or -1 if unknown
+ 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.
Review Comment:
I agree, that would be pythonic and consistent (with None default values). But that might be a breaking change.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-36954: [Python] Add more FlightInfo / FlightEndpoint attributes [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #43537:
URL: https://github.com/apache/arrow/pull/43537#discussion_r1705543355
##########
python/pyarrow/_flight.pyx:
##########
@@ -746,6 +759,24 @@ cdef class FlightEndpoint(_Weakrefable):
return [Location.wrap(location)
for location in self.endpoint.locations]
+ @property
+ def expiration_time(self):
Review Comment:
Can you add docstrings for these new properties?
##########
python/pyarrow/_flight.pyx:
##########
@@ -855,10 +888,14 @@ cdef class FlightInfo(_Weakrefable):
the descriptor for this flight.
endpoints : list of FlightEndpoint
a list of endpoints where this flight is available.
- total_records : int
- the total records in this flight, or -1 if unknown
- total_bytes : int
- the total bytes in this flight, or -1 if unknown
+ 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.
Review Comment:
`None` would be a preferrable default for a Python API, IMHO.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-36954: [Python] Add more FlightInfo / FlightEndpoint attributes [arrow]
Posted by "jorisvandenbossche (via GitHub)" <gi...@apache.org>.
jorisvandenbossche commented on code in PR #43537:
URL: https://github.com/apache/arrow/pull/43537#discussion_r1706693526
##########
python/pyarrow/_flight.pyx:
##########
@@ -746,6 +759,24 @@ cdef class FlightEndpoint(_Weakrefable):
return [Location.wrap(location)
for location in self.endpoint.locations]
+ @property
+ def expiration_time(self):
+ cdef:
+ int64_t time_since_epoch
+ const char* UTC = "UTC"
+ shared_ptr[CTimestampType] time_type = make_shared[CTimestampType](TimeUnit.TimeUnit_NANO, UTC)
+ shared_ptr[CTimestampScalar] shared
+ if self.endpoint.expiration_time.has_value():
+ time_since_epoch = duration_cast[nanoseconds](
+ self.endpoint.expiration_time.value().time_since_epoch()).count()
Review Comment:
We have some existing helpers defined for time point to nanoseconds conversion:
https://github.com/apache/arrow/blob/b852b720d43547136e39fbcd9773ba11fe625909/python/pyarrow/src/arrow/python/datetime.h#L135
(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)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-36954: [Python] Add more FlightInfo / FlightEndpoint attributes [arrow]
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on code in PR #43537:
URL: https://github.com/apache/arrow/pull/43537#discussion_r1706588994
##########
python/pyarrow/_flight.pyx:
##########
@@ -855,10 +888,14 @@ cdef class FlightInfo(_Weakrefable):
the descriptor for this flight.
endpoints : list of FlightEndpoint
a list of endpoints where this flight is available.
- total_records : int
- the total records in this flight, or -1 if unknown
- total_bytes : int
- the total bytes in this flight, or -1 if unknown
+ 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.
Review Comment:
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 is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-36954: [Python] Add more FlightInfo / FlightEndpoint attributes [arrow]
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on code in PR #43537:
URL: https://github.com/apache/arrow/pull/43537#discussion_r1706227669
##########
python/pyarrow/_flight.pyx:
##########
@@ -855,10 +888,14 @@ cdef class FlightInfo(_Weakrefable):
the descriptor for this flight.
endpoints : list of FlightEndpoint
a list of endpoints where this flight is available.
- total_records : int
- the total records in this flight, or -1 if unknown
- total_bytes : int
- the total bytes in this flight, or -1 if unknown
+ 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.
Review Comment:
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.)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-36954: [Python] Add more FlightInfo / FlightEndpoint attributes [arrow]
Posted by "EnricoMi (via GitHub)" <gi...@apache.org>.
EnricoMi commented on code in PR #43537:
URL: https://github.com/apache/arrow/pull/43537#discussion_r1706739834
##########
python/pyarrow/_flight.pyx:
##########
@@ -746,6 +759,24 @@ cdef class FlightEndpoint(_Weakrefable):
return [Location.wrap(location)
for location in self.endpoint.locations]
+ @property
+ def expiration_time(self):
+ cdef:
+ int64_t time_since_epoch
+ const char* UTC = "UTC"
+ shared_ptr[CTimestampType] time_type = make_shared[CTimestampType](TimeUnit.TimeUnit_NANO, UTC)
+ shared_ptr[CTimestampScalar] shared
+ if self.endpoint.expiration_time.has_value():
+ time_since_epoch = duration_cast[nanoseconds](
+ self.endpoint.expiration_time.value().time_since_epoch()).count()
Review Comment:
checking, thanks for the pointer!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-36954: [Python] Add more FlightInfo / FlightEndpoint attributes [arrow]
Posted by "EnricoMi (via GitHub)" <gi...@apache.org>.
EnricoMi commented on code in PR #43537:
URL: https://github.com/apache/arrow/pull/43537#discussion_r1751365590
##########
python/pyarrow/_flight.pyx:
##########
@@ -736,16 +742,47 @@ 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 = TimePoint_from_ns(
+ expiration_time.cast(timestamp("ns")).value)
Review Comment:
@adamreeve thanks for pinpointing the root cause for this. I can confirm changing Timestamp in `cpp/src/arrow/flight/types.h` fixes this:
```diff
-using Timestamp = std::chrono::system_clock::time_point;
+using Timestamp = std::chrono::time_point<std::chrono::system_clock, std::chrono::nanoseconds>;
```
b726b9a0259ac4f97f1cc7c76681b9ac7b80a0ce
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-36954: [Python] Add more FlightInfo / FlightEndpoint attributes [arrow]
Posted by "conbench-apache-arrow[bot] (via GitHub)" <gi...@apache.org>.
conbench-apache-arrow[bot] commented on PR #43537:
URL: https://github.com/apache/arrow/pull/43537#issuecomment-2463012765
After merging your PR, Conbench analyzed the 3 benchmarking runs that have been run so far on merge-commit b193c4f701afee4581c25c1489afa0e4be8f6a6a.
There were no benchmark performance regressions. 🎉
The [full Conbench report](https://github.com/apache/arrow/runs/32675741941) has more details. It also includes information about 4 possible false positives for unstable benchmarks that are known to sometimes produce them.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-36954: [Python] Add more FlightInfo / FlightEndpoint attributes [arrow]
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm merged PR #43537:
URL: https://github.com/apache/arrow/pull/43537
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-36954: [Python] Add more FlightInfo / FlightEndpoint attributes [arrow]
Posted by "EnricoMi (via GitHub)" <gi...@apache.org>.
EnricoMi commented on code in PR #43537:
URL: https://github.com/apache/arrow/pull/43537#discussion_r1771930698
##########
python/pyarrow/_flight.pyx:
##########
@@ -855,10 +895,14 @@ cdef class FlightInfo(_Weakrefable):
the descriptor for this flight.
endpoints : list of FlightEndpoint
a list of endpoints where this flight is available.
- total_records : int
- the total records in this flight, or -1 if unknown
- total_bytes : int
- the total bytes in this flight, or -1 if unknown
+ total_records : int optional, default None
+ the total records in this flight, -1 or None if unknown.
+ total_bytes : int optional, default None
+ the total bytes in this flight, -1 or None if unknown.
Review Comment:
```suggestion
total_records : int, default None
the total records in this flight, -1 or None if unknown.
total_bytes : int, default None
the total bytes in this flight, -1 or None if unknown.
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-36954: [Python] Add more FlightInfo / FlightEndpoint attributes [arrow]
Posted by "EnricoMi (via GitHub)" <gi...@apache.org>.
EnricoMi commented on code in PR #43537:
URL: https://github.com/apache/arrow/pull/43537#discussion_r1764796379
##########
python/pyarrow/_flight.pyx:
##########
@@ -736,16 +742,48 @@ 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 = TimePoint_to_system_time(TimePoint_from_ns(
+ expiration_time.cast(timestamp("ns")).value))
+
+ self.endpoint.app_metadata = tobytes(app_metadata)
Review Comment:
Optional in the sense that there is a default value for `app_metadata`. Documentation does not say `app_metadata` can be `None`, as in `Optional[str]`.
The `app_metadata` in C++ is non-nullable string, so default of `""` is best to reflect this in Python.
I can make it None-safe, though:
```suggestion
self.endpoint.app_metadata = tobytes(app_metadata or "")
```
##########
python/pyarrow/_flight.pyx:
##########
@@ -736,16 +742,48 @@ 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 = TimePoint_to_system_time(TimePoint_from_ns(
+ expiration_time.cast(timestamp("ns")).value))
+
+ self.endpoint.app_metadata = tobytes(app_metadata)
Review Comment:
Optional in the sense that there is a default value for `app_metadata`. Documentation does not say `app_metadata` can be `None`, as in `Optional[str]`.
The `app_metadata` in C++ is non-nullable string, so default of `""` is best to reflect this in Python.
I can make it `None`-safe, though:
```suggestion
self.endpoint.app_metadata = tobytes(app_metadata or "")
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-36954: [Python] Add more FlightInfo / FlightEndpoint attributes [arrow]
Posted by "EnricoMi (via GitHub)" <gi...@apache.org>.
EnricoMi commented on code in PR #43537:
URL: https://github.com/apache/arrow/pull/43537#discussion_r1764796379
##########
python/pyarrow/_flight.pyx:
##########
@@ -736,16 +742,48 @@ 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 = TimePoint_to_system_time(TimePoint_from_ns(
+ expiration_time.cast(timestamp("ns")).value))
+
+ self.endpoint.app_metadata = tobytes(app_metadata)
Review Comment:
Optional in the sense that there is a default value for `app_metadata`. Documentation does not say `app_metadata` can be `None`, as in `Optional[str]`.
The `app_metadata` in C++ is non-nullable string, so default of "" is best to reflect this.
I can make it None-safe, though:
```suggestion
self.endpoint.app_metadata = tobytes(app_metadata or "")
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-36954: [Python] Add more FlightInfo / FlightEndpoint attributes [arrow]
Posted by "EnricoMi (via GitHub)" <gi...@apache.org>.
EnricoMi commented on code in PR #43537:
URL: https://github.com/apache/arrow/pull/43537#discussion_r1764796379
##########
python/pyarrow/_flight.pyx:
##########
@@ -736,16 +742,48 @@ 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 = TimePoint_to_system_time(TimePoint_from_ns(
+ expiration_time.cast(timestamp("ns")).value))
+
+ self.endpoint.app_metadata = tobytes(app_metadata)
Review Comment:
Optional in the sense that there is a default value for `app_metadata`. Documentation does not say `app_metadata` can be `None`, as in `Optional[str]`.
The `app_metadata` in C++ is non-nullable string, so default of "" is best to reflect this. I can make it None-safe, though:
```suggestion
if app_metadata is None:
app_metadata = ""
self.endpoint.app_metadata = tobytes(app_metadata)
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-36954: [Python] Add more FlightInfo / FlightEndpoint attributes [arrow]
Posted by "EnricoMi (via GitHub)" <gi...@apache.org>.
EnricoMi commented on code in PR #43537:
URL: https://github.com/apache/arrow/pull/43537#discussion_r1764796379
##########
python/pyarrow/_flight.pyx:
##########
@@ -736,16 +742,48 @@ 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 = TimePoint_to_system_time(TimePoint_from_ns(
+ expiration_time.cast(timestamp("ns")).value))
+
+ self.endpoint.app_metadata = tobytes(app_metadata)
Review Comment:
Optional in the sense that there is a default value for `app_metadata`. Documentation does not say `app_metadata` can be `None`, as in `Optional[str]`.
I can make it None-safe, though.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-36954: [Python] Add more FlightInfo / FlightEndpoint attributes [arrow]
Posted by "EnricoMi (via GitHub)" <gi...@apache.org>.
EnricoMi commented on code in PR #43537:
URL: https://github.com/apache/arrow/pull/43537#discussion_r1765099010
##########
python/pyarrow/_flight.pyx:
##########
@@ -736,16 +742,48 @@ 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 = TimePoint_to_system_time(TimePoint_from_ns(
+ expiration_time.cast(timestamp("ns")).value))
+
+ self.endpoint.app_metadata = tobytes(app_metadata)
Review Comment:
Works for me: 8c241c6593fa1fefdaafe4f63c4c41b10ff54cde
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-36954: [Python] Add more FlightInfo / FlightEndpoint attributes [arrow]
Posted by "EnricoMi (via GitHub)" <gi...@apache.org>.
EnricoMi commented on code in PR #43537:
URL: https://github.com/apache/arrow/pull/43537#discussion_r1701878072
##########
python/pyarrow/_flight.pyx:
##########
@@ -770,7 +801,9 @@ cdef class FlightEndpoint(_Weakrefable):
def __repr__(self):
return (f"<pyarrow.flight.FlightEndpoint ticket={self.ticket!r} "
- f"locations={self.locations!r}>")
+ f"locations={self.locations!r} "
+ f"expiration_time={self.expiration_time} "
+ f"app_metadata='{self.app_metadata.hex()}'>")
Review Comment:
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()`.
##########
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'} "
Review Comment:
This matches the C++ implementation of `ToString`. In Python, you would expect `True` or `False` though. Happy to change this to
```suggestion
f"ordered={self.ordered} "
```
##########
python/pyarrow/tests/test_flight.py:
##########
@@ -877,14 +879,18 @@ def test_repr():
descriptor_repr = "<pyarrow.flight.FlightDescriptor cmd=b'foo'>"
endpoint_repr = ("<pyarrow.flight.FlightEndpoint "
"ticket=<pyarrow.flight.Ticket ticket=b'foo'> "
- "locations=[]>")
+ "locations=[] "
+ "expiration_time=2023-04-05 12:34:56+00:00 "
+ "app_metadata='656e64706f696e7420617070206d65746164617461'>")
info_repr = (
"<pyarrow.flight.FlightInfo "
"schema= "
"descriptor=<pyarrow.flight.FlightDescriptor path=[]> "
"endpoints=[] "
- "total_records=-1 "
- "total_bytes=-1>")
+ "total_records=1 "
+ "total_bytes=42 "
+ "ordered=true "
Review Comment:
In Python, you would expect
ordered=True
here.
##########
python/pyarrow/tests/test_flight.py:
##########
@@ -877,14 +879,18 @@ def test_repr():
descriptor_repr = "<pyarrow.flight.FlightDescriptor cmd=b'foo'>"
endpoint_repr = ("<pyarrow.flight.FlightEndpoint "
"ticket=<pyarrow.flight.Ticket ticket=b'foo'> "
- "locations=[]>")
+ "locations=[] "
+ "expiration_time=2023-04-05 12:34:56+00:00 "
+ "app_metadata='656e64706f696e7420617070206d65746164617461'>")
Review Comment:
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.
##########
python/pyarrow/_flight.pyx:
##########
@@ -855,10 +888,14 @@ cdef class FlightInfo(_Weakrefable):
the descriptor for this flight.
endpoints : list of FlightEndpoint
a list of endpoints where this flight is available.
- total_records : int
- the total records in this flight, or -1 if unknown
- total_bytes : int
- the total bytes in this flight, or -1 if unknown
+ 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.
Review Comment:
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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-36954: [Python] Add more FlightInfo / FlightEndpoint attributes [arrow]
Posted by "rustyconover (via GitHub)" <gi...@apache.org>.
rustyconover commented on PR #43537:
URL: https://github.com/apache/arrow/pull/43537#issuecomment-2267068987
+1 Looking forward for this being merged.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-36954: [Python] Add more FlightInfo / FlightEndpoint attributes [arrow]
Posted by "EnricoMi (via GitHub)" <gi...@apache.org>.
EnricoMi commented on code in PR #43537:
URL: https://github.com/apache/arrow/pull/43537#discussion_r1737965889
##########
python/pyarrow/_flight.pyx:
##########
@@ -855,10 +888,14 @@ cdef class FlightInfo(_Weakrefable):
the descriptor for this flight.
endpoints : list of FlightEndpoint
a list of endpoints where this flight is available.
- total_records : int
- the total records in this flight, or -1 if unknown
- total_bytes : int
- the total bytes in this flight, or -1 if unknown
+ 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.
Review Comment:
To summarize, the constructor allows for `-1` and `None`, the properties in both cases return `-1`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-36954: [Python] Add more FlightInfo / FlightEndpoint attributes [arrow]
Posted by "rok (via GitHub)" <gi...@apache.org>.
rok commented on PR #43537:
URL: https://github.com/apache/arrow/pull/43537#issuecomment-2340597215
Sorry @EnricoMi, I wanted to push to my fork but default pointed here. Reverted.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-36954: [Python] Add more FlightInfo / FlightEndpoint attributes [arrow]
Posted by "EnricoMi (via GitHub)" <gi...@apache.org>.
EnricoMi commented on code in PR #43537:
URL: https://github.com/apache/arrow/pull/43537#discussion_r1706677572
##########
python/pyarrow/_flight.pyx:
##########
@@ -855,10 +888,14 @@ cdef class FlightInfo(_Weakrefable):
the descriptor for this flight.
endpoints : list of FlightEndpoint
a list of endpoints where this flight is available.
- total_records : int
- the total records in this flight, or -1 if unknown
- total_bytes : int
- the total bytes in this flight, or -1 if unknown
+ 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.
Review Comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-36954: [Python] Add more FlightInfo / FlightEndpoint attributes [arrow]
Posted by "EnricoMi (via GitHub)" <gi...@apache.org>.
EnricoMi commented on code in PR #43537:
URL: https://github.com/apache/arrow/pull/43537#discussion_r1705585972
##########
python/pyarrow/_flight.pyx:
##########
@@ -746,6 +759,24 @@ cdef class FlightEndpoint(_Weakrefable):
return [Location.wrap(location)
for location in self.endpoint.locations]
+ @property
+ def expiration_time(self):
Review Comment:
sure
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org