GH-51293: [Python] Reject a null Expression in two public APIs

[1fanwang]

Rationale for this change

Passing None to partition-key extraction or expression equality terminates Python instead of raising an exception. Applications cannot catch that failure.

Fixes #51293.

What changes are included in this PR?

Both expression parameters reject None at the Cython boundary. Direct and subprocess regressions cover these two entry points.

Are these changes tested?

Built from source on macOS arm64 with Python 3.12.14. Both calls crash on upstream main and raise TypeError on the rebased branch. Valid equality and partition-key extraction retain their results.

Raw logs

$ python -c 'import pyarrow.dataset as ds; ds.get_partition_keys(None)'
Before:
returncode=-10
After:
TypeError: Argument 'partition_expression' has incorrect type (expected pyarrow._compute.Expression, got NoneType)
returncode=1

$ python -c "import pyarrow.compute as pc; pc.field('a').equals(None)"
Before:
returncode=-10
After:
TypeError: Argument 'other' has incorrect type (expected pyarrow._compute.Expression, got NoneType)
returncode=1

$ python -m pytest pyarrow/tests/test_compute.py pyarrow/tests/test_dataset.py pyarrow/tests/test_misc.py pyarrow/tests/test_pandas.py::test_threaded_pandas_import -q --tb=short --basetemp=build/pytest-51294
1035 passed, 67 skipped, 7 warnings in 7.28s

Are there any user-facing changes?

Null expressions raise TypeError instead of terminating Python. Valid calls keep their existing behavior.

Was AI used for this PR?

In accordance to the AI generation guidelines, please disclose below whether and how AI was used in this PR.

PR code and description written by:

• Human
• AI

Reviewed before submission by:

• Human
• AI
• Not reviewed

[github-actions]

⚠️ GitHub issue #51293 has been automatically assigned in GitHub to PR creator.

[Copilot]

🟢 Approval recommended

The change is minimal, directly addresses a crash, and includes a focused regression test validating the new behavior.

Pull request overview

This PR fixes a hard crash in the PyArrow Dataset Python API by rejecting a None partition expression at the Cython boundary, converting an interpreter “bus error” into a normal Python TypeError and adding a regression test.

Changes:

• Mark get_partition_keys’s partition_expression parameter as not None in python/pyarrow/_dataset.pyx so None is rejected before dereferencing.
• Add a regression test ensuring ds.get_partition_keys(None) raises TypeError.

File summaries

File	Description
python/pyarrow/_dataset.pyx	Adds not None to the typed Expression parameter to prevent null from reaching Cython internals.
python/pyarrow/tests/test_dataset.py	Adds a regression test asserting TypeError is raised for None input.

Review details

• Files reviewed: 2/2 changed files
• Comments generated: 0
• Review effort level: Lite

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

🟡 Changes recommended

The new subprocess-based sweep test can hang indefinitely and may miss post-“DONE” crashes unless it asserts returncode == 0 and uses a timeout.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

• Files reviewed: 3/3 changed files
• Comments generated: 2
• Review effort level: Lite

[Copilot]

🔵 Needs a closer look

The new subprocess-based sweep tests need a timeout (to avoid hanging CI) and a couple of small documentation/comment fixes for correctness and debuggability.

Review details

Suppressed comments (4)

Previously missed (1) — in code that hasn't changed since the last review.

python/pyarrow/tests/test_misc.py:319

• This subprocess sweep has no timeout, so a single blocking callable can hang the entire test run. Consider adding a reasonable timeout (and include stderr in the failure) to make CI failures deterministic and easier to debug.

This issue also appears on line 377 of the same file.

python/pyarrow/tests/test_misc.py:280

• The comment says passing None to any public callable must produce a Python exception, but the test intentionally allows callables to accept None (it only asserts the subprocess doesn't crash). Please update the comment to match the actual contract (no crash).

    # GH-51293: a typed Cython parameter that is not declared "not None"
    # lets None reach code that dereferences it, killing the interpreter
    # instead of raising. Passing None to any public callable must produce
    # a Python exception, never a fatal signal.

python/pyarrow/tests/test_misc.py:381

• Same issue as the callable sweep above: this subprocess.run has no timeout and the failure message drops stderr, which can lead to hung CI jobs and harder debugging when something goes wrong.

    res = subprocess.run([sys.executable, "-c", code],
                         capture_output=True, text=True)
    lines = res.stdout.splitlines()
    if not lines or lines[-1] != "DONE":
        culprit = lines[-1] if lines else "<no output>"

python/pyarrow/_compute.pyx:2687

• The equals() docstring parameter type references pyarrow.dataset.Expression, but this is pyarrow.compute.Expression (and the method signature enforces that). This mismatch can confuse users reading the docs/help().

        Parameters
        ----------
        other : pyarrow.dataset.Expression

• Files reviewed: 4/4 changed files
• Comments generated: 0 new
• Review effort level: Lite

[Copilot]

🟢 Approval recommended

The functional changes directly address the reported interpreter-crash bug and are covered by targeted and guard-style tests; remaining feedback is limited to minor doc/test robustness improvements.

Review details

Suppressed comments (2)

Previously missed (2) — in code that hasn't changed since the last review.

python/pyarrow/_compute.pyx:2687

• The Expression.equals docstring references pyarrow.dataset.Expression, but this type lives in pyarrow.compute (and this method is defined in pyarrow/_compute.pyx). Updating the docstring avoids confusing API users.

    def equals(self, Expression other not None):
        """
        Parameters
        ----------
        other : pyarrow.dataset.Expression

python/pyarrow/tests/test_misc.py:363

• The subprocess method sweep uses inspect._empty, which is a private inspect implementation detail. Prefer the public inspect.Parameter.empty to reduce risk of breakage across Python versions.

• Files reviewed: 4/4 changed files
• Comments generated: 0 new
• Review effort level: Lite

[AlenkaF]

Thanks for the PR.

Since a hand-audit goes stale, this adds two guard tests that pass None to public
callables in a child process and fail naming the offender if the interpreter dies.
One covers module-level functions, the other methods reached from a live object. The
method sweep found Expression.equals after the function sweep came back clean.

I am not sure this is needed. @raul what do you think?

I would rather see a missing test for the Expression.equals(None) to be added.

[1fanwang]

Done in 4415d41. This adds the focused Expression.equals(None) regression.

[Copilot]

🟡 Changes recommended

The subprocess guard can exercise unrelated crashing methods and may skip the intended Cython methods, so it does not reliably validate the fix.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (1)

python/pyarrow/tests/test_misc.py:360

• This method guard is effectively vacuous for the Cython methods it is meant to cover. The child process uses stdlib inspect, but this repository's Cython build does not reliably expose __text_signature__; dev/archery/archery/lang/python.py:169-184 adds a docstring parser specifically because inspect.signature fails. This except therefore skips Expression.equals and similar methods, so the test can pass without exercising the regression; use the repository's Cython signature parser or explicitly include the methods under test.

                    sig = inspect.signature(attr)
                except BaseException:
                    continue

• Files reviewed: 5/5 changed files
• Comments generated: 1
• Review effort level: Lite

[Copilot]

Copilot review overview

🟢 Approval recommended

All reviewed changes address the null-expression crash with regression coverage.

Review effort: Lite
Findings: None

Resolved since last review (1)

• This selector makes the new child-process test exercise many existing typed methods beyond the two…

[Copilot]

Copilot review overview

🟢 Approval recommended

The reviewed changes address null-expression handling with regression coverage and have no blocking issues.

Review effort: Lite
Findings: None