Skip to content

GH-51042: [Python] Reject invalid Arrow wrapper types - #51163

Open
1fanwang wants to merge 3 commits into
apache:mainfrom
1fanwang:1fannnw/fix-pyarrow-type-segfaults
Open

1fanwang wants to merge 3 commits into
apache:mainfrom
1fanwang:1fannnw/fix-pyarrow-type-segfaults

Conversation

@1fanwang

@1fanwang 1fanwang commented Sep 5, 2026 •

Copy link
Copy Markdown
Contributor

Rationale for this change

Passing an invalid wrapper object to PyArrow can terminate Python. Acero sequences and explicit input lists abort while copying invalid native declarations.

Fixes #51042.

What changes are included in this PR?

Reject invalid wrapper objects before native calls in the affected Acero, Parquet, sparse tensor, compute option, and Substrait entry points.

Are these changes tested?

Built from source on macOS arm64 with Python 3.12.14, including Substrait. Invalid calls crash on upstream main and raise TypeError on the rebased branch. Valid plans, Parquet reads, sparse tensors, and Substrait round trips keep their results.

Raw logs
$ python -c "import pyarrow.acero as acero; acero.Declaration.from_sequence([0.0])"
Before:
libc++abi: terminating due to uncaught exception of type std::length_error: vector
returncode=-6
After:
TypeError: Expected a Declaration
returncode=1

$ python -m pytest pyarrow/tests/test_acero.py pyarrow/tests/test_sparse_tensor.py pyarrow/tests/test_substrait.py -q --tb=short
191 passed, 51 skipped, 1 warning in 0.22s

Are there any user-facing changes?

Invalid wrapper arguments raise TypeError. Valid declaration plans still execute.

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

Copilot AI lite review requested due to automatic review settings September 5, 2026 04:21
@github-actions

github-actions Bot commented Sep 5, 2026

Copy link
Copy Markdown

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

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.

🟢 Approval recommended

The changes directly address the reported segfaults by enforcing wrapper-type validation at the Cython boundary and add targeted regression tests for each affected API.

Pull request overview

This PR hardens several PyArrow entry points (Parquet read options, sparse tensor conversion, Substrait serialization, and compute IndexOptions) against invalid wrapper arguments that previously could propagate null native pointers into C++ and crash the interpreter; invalid inputs now raise TypeError instead.

Changes:

  • Add explicit wrapper-type validation for Parquet binary_type in both dataset and parquet reader paths.
  • Enforce Tensor, Schema, and Scalar wrapper types at key Cython boundaries (sparse from_tensor, Substrait serialization, and IndexOptions).
  • Add focused regression tests covering the previously-crashing invalid inputs.
File summaries
File Description
python/pyarrow/_parquet.pyx Reject non-DataType binary_type before unwrapping and native calls.
python/pyarrow/_dataset_parquet.pyx Reject non-DataType binary_type in ParquetReadOptions setter.
python/pyarrow/_compute.pyx Require Scalar for IndexOptions / _set_options to prevent invalid unwraps.
python/pyarrow/tensor.pxi Require Tensor for sparse from_tensor conversions across sparse tensor types.
python/pyarrow/_substrait.pyx Require Schema for Substrait serialize_schema / serialize_expressions.
python/pyarrow/tests/test_dataset.py Add regression test for ParquetReadOptions(binary_type=0) raising TypeError.
python/pyarrow/tests/parquet/test_parquet_file.py Add regression test for ParquetFile(..., binary_type=0) raising TypeError.
python/pyarrow/tests/test_compute.py Add regression test for IndexOptions(0) raising TypeError.
python/pyarrow/tests/test_sparse_tensor.py Add regression test ensuring sparse from_tensor(0) raises TypeError.
python/pyarrow/tests/test_substrait.py Add regression tests for Substrait serialization rejecting invalid schema inputs.
Review details
  • Files reviewed: 10/10 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.

@AlenkaF AlenkaF 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.

Thanks, the PR looks good to me.

Might you be willing to grep the Cython bindings more broadly for the same pattern? (also commented on #51161 (review)).

@github-actions github-actions Bot added awaiting committer review Awaiting committer review and removed awaiting review Awaiting review labels Sep 10, 2026
@1fanwang

1fanwang commented Sep 10, 2026 •

Copy link
Copy Markdown
Contributor Author

Thanks, the PR looks good to me.

Might you be willing to grep the Cython bindings more broadly for the same pattern? (also commented on #51161 (review)).

Thanks @AlenkaF

Swept the rest of the Cython bindings for the same pattern. Passing None to every public module-level callable across the main modules turned up one reachable crash, dataset.get_partition_keys, which killed the interpreter with a bus error. Filed as #51293 and fixed in #51294.

That PR also adds a guard test so the class stays covered: it passes None to each public callable in a child process and fails naming the offender if the interpreter dies by signal. Most typed parameters handle None deliberately, so it asserts that nothing crashes rather than requiring not None everywhere.

@AlenkaF

AlenkaF commented Sep 11, 2026

Copy link
Copy Markdown
Member

Thanks. I think the only missing thing from the connected issue is acero.Declaration.from_sequence([0.0]) case, then this can be merged.

Copilot AI review requested due to automatic review settings September 12, 2026 04:57
@github-actions

Copy link
Copy Markdown

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

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.

🟡 Changes recommended

_acero.pyx still lacks validation for Declaration.__init__ inputs, leaving an invalid-wrapper crash path unresolved.

Get a fresh assessment by requesting another Copilot review.

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

Comment thread python/pyarrow/_acero.pyx
@1fanwang

Copy link
Copy Markdown
Contributor Author

Done in d2cd124.

@github-actions

Copy link
Copy Markdown

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

Copilot AI review requested due to automatic review settings September 22, 2026 02:08

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.

Copilot review overview

🟢 Approval recommended

The changes consistently prevent the reported hard-crash cases by enforcing wrapper types before native unwraps, and the added tests cover the new rejection behavior across the affected APIs.

Review effort: Lite
Findings: None

Resolved since last review (1)

Signed-off-by: 1fanwang <1fannnw@gmail.com>
Signed-off-by: 1fanwang <1fannnw@gmail.com>
Signed-off-by: 1fanwang <1fannnw@gmail.com>
Copilot AI review requested due to automatic review settings September 24, 2026 06:34

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.

Copilot review overview

🟢 Approval recommended

No unresolved blocking issues were identified.

Review effort: Lite
Findings: None

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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Python] Several APIs segfault instead of rejecting arguments of the wrong type

3 participants