Skip to content

GH-50431: [Python] Make FlightError a non-cdef class for abi3 wheel precursor#50427

Merged
lidavidm merged 1 commit into
apache:mainfrom
mroeschke:ref/abi3/flight_exception
Jul 20, 2026
Merged

GH-50431: [Python] Make FlightError a non-cdef class for abi3 wheel precursor#50427
lidavidm merged 1 commit into
apache:mainfrom
mroeschke:ref/abi3/flight_exception

Conversation

@mroeschke

@mroeschke mroeschke commented Jul 8, 2026 •
edited by github-actions Bot
Loading

Copy link
Copy Markdown
Contributor

Rationale for this change

Towards #50398

closes #50431

What changes are included in this PR?

This is an agent generated PR that makes FlightError a non-cdef subclass of Exception as Cython does not support building with the Limited API and cdef classes that inherit from Python classes.

It appears before a cdef CStatus to_status was defined on each subclass to return a MakeFlightError per flight error flavor. This was pull out to a dedicated _flight_error_to_status function instead.

Are these changes tested?

Yes (via existing tests?)

Are there any user-facing changes?

No

@mroeschke
mroeschke requested a review from lidavidm as a code owner July 8, 2026 20:31
Copilot AI review requested due to automatic review settings July 8, 2026 20:31
@mroeschke mroeschke changed the title GH-50398 [Python] Use more CPython Limited APIs in python/pyarrowMake FlightError a non-cdef class for abi3 wheel precursor GH-50398 [Python] Make FlightError a non-cdef class for abi3 wheel precursor Jul 8, 2026
@github-actions github-actions Bot added the awaiting review Awaiting review label Jul 8, 2026
Copilot AI reviewed Jul 8, 2026
View reviewed changes

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR adjusts the Python Flight exception hierarchy to support building toward abi3 wheels by removing FlightError (and its key subclasses) as Cython cdef class types, since Cython limited-API builds can’t support cdef classes inheriting from Python classes like Exception.

Changes:

  • Convert FlightError and its main subclasses from cdef class to regular Python class.
  • Replace per-subclass to_status() Cython methods with a centralized _flight_error_to_status() helper that maps FlightError instances to the appropriate arrow::flight::MakeFlightError(...) status.
  • Update all server/callback exception translation sites to use _flight_error_to_status(...) instead of casting and calling to_status().

lidavidm
lidavidm reviewed Jul 8, 2026
View reviewed changes

@lidavidm lidavidm left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems reasonable.

Is this the only change needed for the linked issue? It may be better to file this as a subissue?

@github-actions github-actions Bot added awaiting changes Awaiting changes and removed awaiting review Awaiting review labels Jul 8, 2026
@mroeschke

mroeschke commented Jul 9, 2026

Copy link
Copy Markdown
Contributor Author

It seems reasonable.

Is this the only change needed for the linked issue? It may be better to file this as a subissue?

Sure opened #50431 and linked it in the OP. (I don't think I have permissions to create subissues from the other linked issue)

@lidavidm lidavidm changed the title GH-50398 [Python] Make FlightError a non-cdef class for abi3 wheel precursor GH-50431: [Python] Make FlightError a non-cdef class for abi3 wheel precursor Jul 9, 2026
@github-actions github-actions Bot added awaiting merge Awaiting merge and removed awaiting changes Awaiting changes labels Jul 9, 2026
@lidavidm

lidavidm commented Jul 20, 2026

Copy link
Copy Markdown
Member

I tried to merge this but GitHub seems to be borked. I'll try again later.

Traceback (most recent call last):
  File "/home/lidavidm/Code/arrow/dev/merge_arrow_pr.py", line 134, in __init__
    self.issue = self.github_api.get_issue_data(github_id)
                 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^^^^^^^^^^^
  File "/home/lidavidm/Code/arrow/dev/merge_arrow_pr.py", line 286, in get_issue_data
    return get_json("%s/issues/%s" % (self.github_api, number),
                    headers=self.headers)
  File "/home/lidavidm/Code/arrow/dev/merge_arrow_pr.py", line 65, in get_json
    raise ValueError(response.json())
ValueError: {'message': 'No server is currently available to service your request. Sorry about that. Please try resubmitting your request and contact us if the problem persists.'}

@lidavidm
lidavidm merged commit 38efbf7 into apache:main Jul 20, 2026
41 of 43 checks passed
@lidavidm lidavidm removed the awaiting merge Awaiting merge label Jul 20, 2026
@lidavidm lidavidm mentioned this pull request Jul 20, 2026
Closed
@conbench-apache-arrow

conbench-apache-arrow Bot commented Jul 20, 2026

Copy link
Copy Markdown

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

There were 4 benchmark results indicating a performance regression:

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Copilot code review Copilot Copilot left review comments
@lidavidm lidavidm lidavidm approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[Python] Make FlightError a non-cdef class

3 participants

@mroeschke @lidavidm