arrow icon indicating copy to clipboard operation
arrow copied to clipboard

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

Open EnricoMi opened this issue 1 year ago • 5 comments

Rationale for this change

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

What changes are included in this PR?

Make the following attributes available in Python:

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

Also makes existing attributes optional in constructor:

  • FlightInfo.total_records
  • FlightInfo.total_bytes

Are these changes tested?

Existing tests that test existing attributes are extended.

Are there any user-facing changes?

Yes, changes are backward compatible.

  • GitHub Issue: #36954

EnricoMi avatar Aug 02 '24 13:08 EnricoMi

:warning: GitHub issue #36954 has been automatically assigned in GitHub to PR creator.

github-actions[bot] avatar Aug 02 '24 13:08 github-actions[bot]

+1 Looking forward for this being merged.

rustyconover avatar Aug 03 '24 17:08 rustyconover

Any suggestion how to fix the macOS compile error? https://github.com/apache/arrow/actions/runs/10634219298/job/29481084040?pr=43537#step:8:3165

I think I have to cast the time_point produced via TimePoint_from_ns to the system clock precision std::chrono::system_clock::duration before assigning to endpoint.expiration_time (which wraps it into an std::optional), preferably via std::chrono::time_point_cast<system_clock, system_clock::duration>, but I cannot get this done in Cython words.

Any help appreciated.

EnricoMi avatar Aug 30 '24 14:08 EnricoMi

Sorry @EnricoMi, I wanted to push to my fork but default pointed here. Reverted.

rok avatar Sep 10 '24 12:09 rok

It looks like all the review comments have been addressed on this, are there any more changes required? @jorisvandenbossche?

adamreeve avatar Oct 13 '24 21:10 adamreeve

@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?

lidavidm avatar Nov 07 '24 10:11 lidavidm

@lidavidm thanks!

EnricoMi avatar Nov 07 '24 13:11 EnricoMi

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 has more details. It also includes information about 4 possible false positives for unstable benchmarks that are known to sometimes produce them.

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

EnricoMi avatar Nov 08 '24 09:11 EnricoMi