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

[1fanwang]

Rationale for this change

A null expression passed to partition-key extraction or expression equality can terminate Python rather than raise an exception. Applications cannot catch that failure.

Fixes #51293.

What changes are included in this PR?

The two expression parameters reject null at the Cython boundary. Direct regression cases cover partition-key extraction and equality. The existing subprocess tests exercise public functions and methods.

Are these changes tested?

Both null-expression calls were reproduced in separate processes with 20-second timeouts, then run against rebuilt native extensions. Valid equality and partition-key extraction retained their results. The broader callable sweeps were not rerun.

Raw logs

$ python -c 'import pyarrow.dataset as ds; ds.get_partition_keys(None)'
Before, native Arrow 26.0.0-SNAPSHOT:
returncode=-11
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, native Arrow 26.0.0-SNAPSHOT:
returncode=-10
After:
TypeError: Argument 'other' has incorrect type (expected pyarrow._compute.Expression, got NoneType)
returncode=1

$ cd python
$ python -m pytest pyarrow/tests/test_compute.py -k 'expression_construction or expression_boolean_operators or expression_call_function' -q
...                                                                      [100%]
3 passed, 605 deselected, 1 warning in 0.41s

Are there any user-facing changes?

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

New Contributor's Guide |
Contributing Overview |
AI-generated Code Guidance

• GitHub Issue: [Python] get_partition_keys segfaults when the partition expression is None #51293

[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