Conversation
|
|
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.
|
|
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
AlenkaF
left a comment
There was a problem hiding this comment.
Are there any other instances of this pattern anywhere else in the Cython bindings? They might produce other errors (like AttributeError) but would still benefit from similar change.
Ah, ok, there is another PR aiming at other files in PyArrow: #51163 For future work we could try keeping the number of PRs down and tackle similar issues in one. |
|
|
1 similar comment
|
|
Signed-off-by: 1fanwang <1fannnw@gmail.com>
Signed-off-by: 1fanwang <1fannnw@gmail.com>
Signed-off-by: 1fanwang <1fannnw@gmail.com>
…tors Raise TypeError instead of aborting the interpreter when ScanNodeOptions, ProjectNodeOptions, ParquetSchema, ColumnSchema, or SubTreeFileSystem receive None for a required argument. Signed-off-by: 1fanwang <1fannnw@gmail.com>
846a3ac to
bb913b6
Compare
Rationale for this change
Passing None for a required Arrow object can abort the Python process or raise an unrelated AttributeError. Later comments on #51043 added ScanNodeOptions, ParquetSchema, SubTreeFileSystem, ProjectNodeOptions and ColumnSchema.
Fixes #51043.
get_partition_keys(None) is in #51294.
What changes are included in this PR?
Required schema, format, type, dictionary, fragment, filesystem, and child-array arguments reject None. ScanNodeOptions, ParquetSchema, ColumnSchema, SubTreeFileSystem, and ProjectNodeOptions do the same. Array buffer constructors require a data type. String constructors keep existing buffer validation.
Are these changes tested?
A native Arrow 26.0.0-SNAPSHOT build reproduced the failures. The same commands then raised TypeError.
ProjectNodeOptions([None]) and SubTreeFileSystem("", None) aborted with SIGBUS. ColumnSchema(None, 0) aborted on .name with SIGSEGV. All three now raise TypeError.
Are there any user-facing changes?
Missing required objects raise TypeError. Valid null buffers keep their existing behavior.
New Contributor's Guide |
Contributing Overview |
AI-generated Code Guidance
Was AI used for this PR?
AI assisted with the code, tests, and description.
PR code and description written by:
Reviewed before submission by:
None#51043