GH-51043: [Python] Reject null required Arrow objects - #51161
Conversation
Generated-by: GitHub Copilot CLI (GPT-5.6 Sol) Signed-off-by: 1fanwang <1fannnw@gmail.com>
|
|
There was a problem hiding this comment.
🟡 Changes recommended
DictionaryArray.from_buffers() still allows dictionary=None while dereferencing it unconditionally, so a None argument can still trigger a crash.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR hardens several PyArrow Cython entry points by rejecting None for required Arrow extension objects, preventing null C++ pointer dereferences that can segfault the Python interpreter (GH-51043).
Changes:
- Mark required
Schema,FileFormat, andDataTypeparameters as non-nullable (not None) at the Cython boundary to raiseTypeErrorinstead of crashing. - Add an explicit
Nonecheck forFileSystemDatasetfragments before unwrapping. - Add focused regression tests covering the newly rejected
Nonearguments.
File summaries
| File | Description |
|---|---|
| python/pyarrow/_dataset.pyx | Reject None fragments and make schema/format non-nullable in FileSystemDataset. |
| python/pyarrow/_parquet.pyx | Make SortingColumn conversion helpers reject schema=None at the boundary. |
| python/pyarrow/array.pxi | Make DictionaryArray.from_buffers reject type=None at the boundary. |
| python/pyarrow/tests/test_dataset.py | Add regression assertions for FileSystemDataset(..., schema=None/format=None) and [None] fragments. |
| python/pyarrow/tests/parquet/test_metadata.py | Add regression assertions for SortingColumn.* with schema=None. |
| python/pyarrow/tests/test_array.py | Add regression assertion for DictionaryArray.from_buffers(type=None, ...). |
Review details
- Files reviewed: 6/6 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.
| def from_buffers(DataType type not None, int64_t length, buffers, | ||
| Array dictionary, int64_t null_count=-1, | ||
| int64_t offset=0): |
Generated-by: GitHub Copilot CLI (GPT-5.6 Sol) Signed-off-by: 1fanwang <1fannnw@gmail.com>
|
|
There was a problem hiding this comment.
🟢 Approval recommended
The changes directly address the null-dereference crash at the Cython boundary and are covered by targeted regression tests for the reported cases.
Review details
- Files reviewed: 6/6 changed files
- Comments generated: 0 new
- Review effort level: Lite
Rationale for this change
Passing null values to required PyArrow object arguments can dereference a null C++ pointer and terminate the Python process. These calls should reject invalid input with a type error, consistent with other typed PyArrow APIs.
Fixes #51043.
What changes are included in this PR?
Required schema, file-format, data-type, and dictionary array arguments now reject null at the Cython boundary.
FileSystemDatasetalso rejects a null fragment before unwrapping it.Are these changes tested?
The reported calls were run in separate Python processes against PyArrow 25.0.1, then covered by focused tests against the patched source.
Raw logs
Are there any user-facing changes?
Yes. Invalid null arguments now raise a type error instead of crashing the interpreter or failing with an unrelated attribute error.
None#51043