Conversation
|
|
There was a problem hiding this comment.
🟢 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_typein both dataset and parquet reader paths. - Enforce
Tensor,Schema, andScalarwrapper types at key Cython boundaries (sparsefrom_tensor, Substrait serialization, andIndexOptions). - 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
left a comment
There was a problem hiding this comment.
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 That PR also adds a guard test so the class stays covered: it passes |
|
Thanks. I think the only missing thing from the connected issue is |
|
|
There was a problem hiding this comment.
🟡 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
|
Done in d2cd124. |
|
|
There was a problem hiding this comment.
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>
f317332 to
a1c5a89
Compare

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
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:
Reviewed before submission by: