Copilot commented on code in PR #51161:
URL: https://github.com/apache/arrow/pull/51161#discussion_r4528697808
##########
python/pyarrow/tests/test_misc.py:
##########
@@ -290,3 +290,32 @@ def test_extension_type_constructor_errors(klass):
msg = f"Do not call {klass.__name__}'s constructor directly, use .*
instead."
with pytest.raises(TypeError, match=msg):
klass()
+
+
[email protected]
[email protected](("module", "call", "message"), [
+ pytest.param(
+ "acero", "ScanNodeOptions(None)", "Argument 'dataset'",
+ marks=[pytest.mark.acero, pytest.mark.dataset], id="scan"),
+ pytest.param(
+ "acero", "ProjectNodeOptions([None])", "Expression must not be None",
+ marks=pytest.mark.acero, id="project"),
+ pytest.param(
+ "parquet", "ParquetSchema(None)", "Argument 'container'",
+ marks=pytest.mark.parquet, id="parquet-schema"),
+ pytest.param(
+ "parquet", "ColumnSchema(None, 0)", "Argument 'schema'",
+ marks=pytest.mark.parquet, id="column-schema"),
+ pytest.param(
+ "fs", 'SubTreeFileSystem("", None)', "Argument 'base_fs'",
+ id="subtree"),
+])
Review Comment:
The `fs`/`SubTreeFileSystem` subprocess case isn’t marked with a feature
marker (while `acero`, `dataset`, and `parquet` are). If filesystem support can
be disabled in some build/test configurations, this param can fail with an
`ImportError` rather than exercising the intended regression. Consider adding
the appropriate filesystem-related pytest mark used elsewhere in the suite (or
otherwise conditionally skipping) so the test only runs when
`pyarrow.fs.SubTreeFileSystem` is available.
##########
python/pyarrow/tests/test_dataset.py:
##########
@@ -396,6 +396,12 @@ def test_filesystem_dataset(mockfs):
# validation of required arguments
with pytest.raises(TypeError, match="incorrect type"):
ds.FileSystemDataset(fragments, file_format, schema)
+ with pytest.raises(TypeError, match="Fragment must not be None"):
+ ds.FileSystemDataset([None], schema=schema, format=file_format)
+ with pytest.raises(TypeError, match="Argument 'schema' has incorrect
type"):
+ ds.FileSystemDataset(fragments, schema=None, format=file_format)
+ with pytest.raises(TypeError, match="Argument 'format' has incorrect
type"):
Review Comment:
These tests match exact Cython-generated `TypeError` phrasing (e.g., \"has
incorrect type\"). That wording can vary across Cython/Python versions and
build configurations, making the test brittle. Consider loosening the regex to
assert the stable parts (argument name + `NoneType` / `got NoneType`) rather
than the exact sentence form, while still ensuring the error is a `TypeError`.
##########
python/pyarrow/_dataset.pyx:
##########
@@ -1138,6 +1139,8 @@ cdef class FileSystemDataset(Dataset):
)
for fragment in fragments:
+ if fragment is None:
+ raise TypeError("Fragment must not be None")
Review Comment:
The new `TypeError` is correct, but it’s not very actionable when
`fragments` is long (it doesn’t indicate which element was `None`). Consider
including the index (and optionally the fragments length) in the error message
(e.g., \"Fragment at index i must not be None\") to make debugging easier.
Similar index-specific messaging would also help for the new `None` checks in
expression/children validation.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
