-
Notifications
You must be signed in to change notification settings - Fork 3.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
GH-36954: [Python] Add more FlightInfo / FlightEndpoint attributes #43537
Changes from all commits
47a3902
bfca244
bd53f7f
5555bad
1e87b19
911e991
9ab0624
e54872b
eb98843
2382b0e
7501924
4355408
4c09c13
60ec782
336b3e4
176347a
29ad932
d30bc6c
428162d
86e3fad
08b1a58
9dc0e75
989f7a2
08d6b48
985b115
d264ce1
bf62c5d
818660f
c88fdd5
828070e
d2d7cab
94a40b2
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,23 @@ | ||
# Licensed to the Apache Software Foundation (ASF) under one | ||
# or more contributor license agreements. See the NOTICE file | ||
# distributed with this work for additional information | ||
# regarding copyright ownership. The ASF licenses this file | ||
# to you under the Apache License, Version 2.0 (the | ||
# "License"); you may not use this file except in compliance | ||
# with the License. You may obtain a copy of the License at | ||
# | ||
# http://www.apache.org/licenses/LICENSE-2.0 | ||
# | ||
# Unless required by applicable law or agreed to in writing, | ||
# software distributed under the License is distributed on an | ||
# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY | ||
# KIND, either express or implied. See the License for the | ||
# specific language governing permissions and limitations | ||
# under the License. | ||
|
||
# distutils: language = c++ | ||
|
||
|
||
cdef extern from "<chrono>" namespace "std::chrono::system_clock": | ||
cdef cppclass time_point: | ||
pass |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -144,6 +144,20 @@ inline TimePoint TimePoint_from_ns(int64_t val) { | |
return TimePoint(TimePoint::duration(val)); | ||
} | ||
|
||
ARROW_PYTHON_EXPORT | ||
// Note: Needed by FlightEndpoint.expiration_time, which is an OS-dependent | ||
// std::chrono::system_clock::time_point | ||
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); | ||
Comment on lines
+150
to
+151
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just to understand this code: what does this exactly do? Because my reading is that There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I see a comment in the tests about 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The underlying issue is
The latter is the desired type. The right thing to do is to fix See #43537 (comment). I have added more comments to the workaround (0b49a09) and will raise a PR (EnricoMi#3) to fix the type and revert the workaround to manage that breaking change separately. |
||
} | ||
|
||
ARROW_PYTHON_EXPORT | ||
// Note: Needed by FlightEndpoint.expiration_time, which is an OS-dependent | ||
// std::chrono::system_clock::time_point | ||
inline TimePoint TimePoint_from_system_time(std::chrono::system_clock::time_point val) { | ||
return std::chrono::time_point_cast<TimePoint::duration>(val); | ||
} | ||
|
||
ARROW_PYTHON_EXPORT | ||
inline int64_t PyDelta_to_s(PyDateTime_Delta* pytimedelta) { | ||
return (PyDateTime_DELTA_GET_DAYS(pytimedelta) * 86400LL + | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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'"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added type checks in 415aca7. Note none of the other arguments where checked, so I added that for all arguments. This includes tests.