-
Notifications
You must be signed in to change notification settings - Fork 4.3k
GH-51228: [Python] Raise instead of crashing on an invalid registry - #51247
GH-51228: [Python] Raise instead of crashing on an invalid registry #512471fanwang wants to merge 3 commits into
Conversation
Validate call_tabular_function registry inputs before casting them to the native FunctionRegistry pointer. Generated-by: GitHub Copilot CLI (Claude Opus 5) Signed-off-by: Stefan Wang <1fannnw@gmail.com>
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.
🟡 Changes recommended
The new subprocess-based test should be marked with @pytest.mark.processes so it can be skipped on platforms/configurations where process creation is disabled.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Fixes a crash in the PyArrow compute bindings by validating the optional func_registry argument passed to pyarrow.compute.call_tabular_function() before performing the Cython cast, so invalid types raise a catchable TypeError instead of segfaulting.
Changes:
- Add a runtime type check for
func_registryincall_tabular_function()and raiseTypeErroron invalid values. - Add a regression test that runs the call in a subprocess to assert the failure mode is a Python exception (not a process crash).
File summaries
| File | Description |
|---|---|
| python/pyarrow/_compute.pyx | Adds func_registry type validation before casting to a native registry pointer. |
| python/pyarrow/tests/test_compute.py | Adds a subprocess-based regression test to ensure invalid registries raise TypeError rather than crashing. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
The test spawns a subprocess, which Emscripten does not support. Without the marker the test runs there anyway and fails. Generated-by: GitHub Copilot CLI (Claude Opus 5) Signed-off-by: 1fanwang <1fannnw@gmail.com>
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.
🟢 Approval recommended
The fix is narrowly scoped, matches the stated user-facing behavior, and is covered by a regression test that would fail under the prior segfaulting implementation.
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 0 new
- Review effort level: Lite
@AlenkaF
AlenkaF
left a comment
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.
Would it make sense to also add a check in _register_user_defined_function as every register_* function uses that?
The subprocess wrapper guarded against the crash this change removes, so the in-process form reads better now. Signed-off-by: 1fanwang <1fannnw@gmail.com>
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.
🟡 Changes recommended
The added regression test should be executed in a subprocess (as described) to remain safe if the segfault regresses and to avoid crashing the test runner.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Lite
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.
Keeping it in-process per #51247 (comment); with the fix it raises rather than crashing.
AlenkaF
commented
Sep 11, 2026
Two things left:
- comment GH-51228: [Python] Raise instead of crashing on an invalid registry #51247 (review) is not yet addressed
- PR description needs an update on the testing section.
Uh oh!
There was an error while loading. Please reload this page.
Rationale for this change
pyarrow.compute.call_tabular_function()takes an optional registry argument. Passing the wrong Python type reaches an unchecked Cython cast to the native registry pointer, and the process dies with a SIGSEGV before Arrow can raise anything an application could catch.What changes are included in this PR?
The binding validates the registry argument before the cast. A value of the wrong type raises
TypeError("func_registry must be a FunctionRegistry").Are these changes tested?
Yes. The regression drives the call in a subprocess so a crash is observable rather than fatal to the test run.
Are there any user-facing changes?
Yes. An invalid registry argument now raises
TypeErrorinstead of terminating the process.This PR contains a "Critical Fix". It fixes a process crash reachable from ordinary Python input.
call_tabular_function()segfaults for some wrong typedfunc_registryvalues #51228