You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "hadrian-reppas (via GitHub)" <gi...@apache.org> on 2025/07/01 15:34:13 UTC
[PR] GH-46629: [Python] Add options to DatasetFactory.inspect [arrow]
hadrian-reppas opened a new pull request, #46961:
URL: https://github.com/apache/arrow/pull/46961
### Rationale for this change
See https://github.com/apache/arrow/issues/46629.
### What changes are included in this PR?
This PR updates the `DatasetFactory.inspect` method so that it accepts new `promote_options` and `fragments` parameters.
### Are these changes tested?
I'm having issues importing `DatasetFactory` on main (see https://github.com/apache/arrow/issues/46629#issuecomment-3020425208). I tested it on version 20 and it works.
### Are there any user-facing changes?
This adds optional parameters to a public method. It changes the default behavior from checking one fragment to checking all fragments (the old documentation said it inspected "all data fragments" even though it didn't).
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-46629: [Python] Add options to DatasetFactory.inspect [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #46961:
URL: https://github.com/apache/arrow/pull/46961#discussion_r2192469359
##########
python/pyarrow/lib.pxd:
##########
@@ -678,6 +678,8 @@ cdef shared_ptr[const CKeyValueMetadata] pyarrow_unwrap_metadata(
cdef object pyarrow_wrap_metadata(
const shared_ptr[const CKeyValueMetadata]& meta)
+cdef CField.CMergeOptions _parse_field_merge_options(str promote_options, bint allow_none) except *
Review Comment:
Indeed, that might be simpler.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-46629: [Python] Add options to DatasetFactory.inspect [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #46961:
URL: https://github.com/apache/arrow/pull/46961#discussion_r2192297003
##########
python/pyarrow/lib.pxd:
##########
@@ -678,6 +678,8 @@ cdef shared_ptr[const CKeyValueMetadata] pyarrow_unwrap_metadata(
cdef object pyarrow_wrap_metadata(
const shared_ptr[const CKeyValueMetadata]& meta)
+cdef CField.CMergeOptions _parse_field_merge_options(str promote_options, bint allow_none) except *
Review Comment:
Please let's make the boolean parameter keyword-only (if supported by Cython).
```suggestion
cdef CField.CMergeOptions _parse_field_merge_options(str promote_options, *, bint allow_none) except *
```
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-46629: [Python] Add options to DatasetFactory.inspect [arrow]
Posted by "amoeba (via GitHub)" <gi...@apache.org>.
amoeba commented on code in PR #46961:
URL: https://github.com/apache/arrow/pull/46961#discussion_r2191267079
##########
python/pyarrow/_dataset.pyx:
##########
@@ -3151,9 +3151,19 @@ cdef class DatasetFactory(_Weakrefable):
schemas.append(pyarrow_wrap_schema(s))
return schemas
- def inspect(self):
+ def inspect(self, *, promote_options="default", fragments=None):
"""
- Inspect all data fragments and return a common Schema.
+ Inspect data fragments and return a common Schema.
+
+ Parameters
+ ----------
+ promote_options : str, default "default"
+ Control how to unify types. Accepts strings "default" and "permissive".
+ Default: types must match exactly, except nulls can be merged with other types.
+ Permissive: types are promoted when possible.
+ fragments : int, optional (default None)
+ How many fragments should be inspected to infer the unified schema.
+ Use None to inspect all fragments
Review Comment:
```suggestion
Use ``None`` to inspect all fragments.
```
Should be like this for it to show up as code in the generated docs. Plus add trailing period.
##########
python/pyarrow/_dataset.pyx:
##########
@@ -3151,9 +3151,19 @@ cdef class DatasetFactory(_Weakrefable):
schemas.append(pyarrow_wrap_schema(s))
return schemas
- def inspect(self):
+ def inspect(self, *, promote_options="default", fragments=None):
"""
- Inspect all data fragments and return a common Schema.
+ Inspect data fragments and return a common Schema.
+
+ Parameters
+ ----------
+ promote_options : str, default "default"
+ Control how to unify types. Accepts strings "default" and "permissive".
+ Default: types must match exactly, except nulls can be merged with other types.
+ Permissive: types are promoted when possible.
+ fragments : int, optional (default None)
Review Comment:
```suggestion
fragments : int, optional, default None
```
I think this is slightly more consistent with existing style. We try to follow numpydoc but I don't think there's a specific lint for this. https://numpydoc.readthedocs.io/en/latest/format.html#parameters is vague but I think this is closest to their intent.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-46629: [Python] Add options to DatasetFactory.inspect [arrow]
Posted by "hadrian-reppas (via GitHub)" <gi...@apache.org>.
hadrian-reppas commented on PR #46961:
URL: https://github.com/apache/arrow/pull/46961#issuecomment-3052383268
I made the changes and made sure to construct the `Table` in the test.
Please let me know what you think.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-46629: [Python] Add options to DatasetFactory.inspect [arrow]
Posted by "hadrian-reppas (via GitHub)" <gi...@apache.org>.
hadrian-reppas commented on PR #46961:
URL: https://github.com/apache/arrow/pull/46961#issuecomment-3025248656
Okay, defining `_parse_field_merge_options` actually wasn't so bad (and didn't need a new file).
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-46629: [Python] Add options to DatasetFactory.inspect [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #46961:
URL: https://github.com/apache/arrow/pull/46961#discussion_r2192293176
##########
python/pyarrow/_dataset.pyx:
##########
@@ -3151,9 +3151,19 @@ cdef class DatasetFactory(_Weakrefable):
schemas.append(pyarrow_wrap_schema(s))
return schemas
- def inspect(self):
+ def inspect(self, *, promote_options="default", fragments=None):
"""
- Inspect all data fragments and return a common Schema.
+ Inspect data fragments and return a common Schema.
+
+ Parameters
+ ----------
+ promote_options : str, default "default"
+ Control how to unify types. Accepts strings "default" and "permissive".
+ Default: types must match exactly, except nulls can be merged with other types.
+ Permissive: types are promoted when possible.
+ fragments : int, optional (default None)
Review Comment:
Judging by other docstrings, I think we would actually use
```suggestion
fragments : int, default None
```
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-46629: [Python] Add options to DatasetFactory.inspect [arrow]
Posted by "hadrian-reppas (via GitHub)" <gi...@apache.org>.
hadrian-reppas commented on PR #46961:
URL: https://github.com/apache/arrow/pull/46961#issuecomment-3073276167
Hi, just following up. Please let me know if/how I should check the un-unified schema in the test (see my response to your review).
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-46629: [Python] Add options to DatasetFactory.inspect [arrow]
Posted by "hadrian-reppas (via GitHub)" <gi...@apache.org>.
hadrian-reppas commented on code in PR #46961:
URL: https://github.com/apache/arrow/pull/46961#discussion_r2194901900
##########
python/pyarrow/tests/test_dataset.py:
##########
@@ -451,6 +487,67 @@ def test_dataset(dataset, dataset_reader):
assert_dataset_fragment_convenience_methods(dataset)
+def test_dataset_factory_inspect_schema_promotion(promotable_mockfs):
+ mockfs, path1, path2 = promotable_mockfs
+ factory = ds.FileSystemDatasetFactory(
+ mockfs, [path1, path2], ds.ParquetFileFormat()
+ )
+
+ with pytest.raises(
+ pa.ArrowTypeError,
+ match='Unable to merge: Field value has incompatible types: int8 vs uint16',
+ ):
+ factory.inspect()
+
+ schema = factory.inspect(promote_options='permissive')
+ expected_schema = pa.schema([
+ pa.field('value', pa.int32()),
+ pa.field('dictionary', pa.dictionary(pa.int16(), pa.string())),
+ ])
+ assert schema.equals(expected_schema)
+ dataset = factory.finish(schema)
+ table = dataset.to_table()
+ expected_table = pa.table({
+ 'value': pa.chunked_array([[1, 2], [3, 4]], type=pa.int32()),
+ 'dictionary': pa.chunked_array([
+ pa.DictionaryArray.from_arrays(
+ pa.array([0, 1], type=pa.int16()),
+ ['a', 'b']
+ ),
+ pa.DictionaryArray.from_arrays(
+ pa.array([1, 0], type=pa.int16()),
+ ['d', 'c']
+ )
+ ]),
+ })
+ assert table.equals(expected_table)
+
+ # Inspecting only one fragment should not promote the 'value' field
+ inspected_schema_one_frag = factory.inspect(
+ promote_options='permissive', fragments=1)
+ assert inspected_schema_one_frag.field('value').type != pa.int32()
Review Comment:
According to the `InspectOptions` docstring,
> The default value of `1` inspects the schema of the first (in no particular order) fragment only.
So we don't know which fragment is going to be inspected and which schema we'll get (even though in practice, I've only ever gotten the first one in the list of paths). So I'm just checking that we aren't getting the unified schema.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-46629: [Python] Add options to DatasetFactory.inspect [arrow]
Posted by "amoeba (via GitHub)" <gi...@apache.org>.
amoeba commented on code in PR #46961:
URL: https://github.com/apache/arrow/pull/46961#discussion_r2192846760
##########
python/pyarrow/_dataset.pyx:
##########
@@ -3151,9 +3151,19 @@ cdef class DatasetFactory(_Weakrefable):
schemas.append(pyarrow_wrap_schema(s))
return schemas
- def inspect(self):
+ def inspect(self, *, promote_options="default", fragments=None):
"""
- Inspect all data fragments and return a common Schema.
+ Inspect data fragments and return a common Schema.
+
+ Parameters
+ ----------
+ promote_options : str, default "default"
+ Control how to unify types. Accepts strings "default" and "permissive".
+ Default: types must match exactly, except nulls can be merged with other types.
+ Permissive: types are promoted when possible.
+ fragments : int, optional (default None)
Review Comment:
Makes sense.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-46629: [Python] Add options to DatasetFactory.inspect [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #46961:
URL: https://github.com/apache/arrow/pull/46961#discussion_r2177955895
##########
python/pyarrow/_dataset.pyx:
##########
@@ -3151,9 +3151,19 @@ cdef class DatasetFactory(_Weakrefable):
schemas.append(pyarrow_wrap_schema(s))
return schemas
- def inspect(self):
+ def inspect(self, promote_options="default", fragments=None):
Review Comment:
I think we should make these arguments kw-only, just like in `unify_schemas`.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-46629: [Python] Add options to DatasetFactory.inspect [arrow]
Posted by "AlenkaF (via GitHub)" <gi...@apache.org>.
AlenkaF commented on PR #46961:
URL: https://github.com/apache/arrow/pull/46961#issuecomment-3027514409
@hadrian-reppas I tried to reproduce the import failure on the latest main, and also on your PR, but everything works fine on my end. The first thing I’d suggest is doing a clean build: run `git clean -Xfd .` in your `arrow/` clone, then rebuild both C++ and PyArrow from scratch. Hopefully this clears out any stale build artifacts and resolves the import issue.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-46629: [Python] Add options to DatasetFactory.inspect [arrow]
Posted by "hadrian-reppas (via GitHub)" <gi...@apache.org>.
hadrian-reppas commented on PR #46961:
URL: https://github.com/apache/arrow/pull/46961#issuecomment-3028072893
That fixed it. Thanks @AlenkaF!
I just added some tests.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-46629: [Python] Add options to DatasetFactory.inspect [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #46961:
URL: https://github.com/apache/arrow/pull/46961#discussion_r2194852995
##########
python/pyarrow/tests/test_dataset.py:
##########
@@ -451,6 +487,67 @@ def test_dataset(dataset, dataset_reader):
assert_dataset_fragment_convenience_methods(dataset)
+def test_dataset_factory_inspect_schema_promotion(promotable_mockfs):
+ mockfs, path1, path2 = promotable_mockfs
+ factory = ds.FileSystemDatasetFactory(
+ mockfs, [path1, path2], ds.ParquetFileFormat()
+ )
+
+ with pytest.raises(
+ pa.ArrowTypeError,
+ match='Unable to merge: Field value has incompatible types: int8 vs uint16',
+ ):
+ factory.inspect()
+
+ schema = factory.inspect(promote_options='permissive')
+ expected_schema = pa.schema([
+ pa.field('value', pa.int32()),
+ pa.field('dictionary', pa.dictionary(pa.int16(), pa.string())),
+ ])
+ assert schema.equals(expected_schema)
+ dataset = factory.finish(schema)
+ table = dataset.to_table()
Review Comment:
Let's validate the table contents for correctness.
```suggestion
table = dataset.to_table()
table.validate(full=True)
```
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-46629: [Python] Add options to DatasetFactory.inspect [arrow]
Posted by "hadrian-reppas (via GitHub)" <gi...@apache.org>.
hadrian-reppas commented on code in PR #46961:
URL: https://github.com/apache/arrow/pull/46961#discussion_r2192465885
##########
python/pyarrow/lib.pxd:
##########
@@ -678,6 +678,8 @@ cdef shared_ptr[const CKeyValueMetadata] pyarrow_unwrap_metadata(
cdef object pyarrow_wrap_metadata(
const shared_ptr[const CKeyValueMetadata]& meta)
+cdef CField.CMergeOptions _parse_field_merge_options(str promote_options, bint allow_none) except *
Review Comment:
It might actually be nicer to remove the parameter completely. We only allow "none" in one place so for that call we could just do
```python
_parse_field_merge_options("default" if promote_options == "none" else promote_options)
```
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-46629: [Python] Add options to DatasetFactory.inspect [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #46961:
URL: https://github.com/apache/arrow/pull/46961#discussion_r2177958431
##########
python/pyarrow/_dataset.pyx:
##########
@@ -3162,6 +3172,20 @@ cdef class DatasetFactory(_Weakrefable):
cdef:
CInspectOptions options
CResult[shared_ptr[CSchema]] result
+
+ if promote_options == "permissive":
+ options.field_merge_options = CField.CMergeOptions.Permissive()
+ elif promote_options != "default":
+ raise ValueError(f"Invalid merge mode: {promote_options}")
Review Comment:
Nit: reflect actual parameter name?
```suggestion
raise ValueError(f"Invalid promote_options: {promote_options}")
```
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-46629: [Python] Add options to DatasetFactory.inspect [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #46961:
URL: https://github.com/apache/arrow/pull/46961#discussion_r2177964030
##########
python/pyarrow/_dataset.pyx:
##########
@@ -3162,6 +3172,20 @@ cdef class DatasetFactory(_Weakrefable):
cdef:
CInspectOptions options
CResult[shared_ptr[CSchema]] result
+
+ if promote_options == "permissive":
Review Comment:
Since this conversion code occurs several times across the codebase, how about making a helper function for it?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-46629: [Python] Add options to DatasetFactory.inspect [arrow]
Posted by "hadrian-reppas (via GitHub)" <gi...@apache.org>.
hadrian-reppas commented on code in PR #46961:
URL: https://github.com/apache/arrow/pull/46961#discussion_r2194901900
##########
python/pyarrow/tests/test_dataset.py:
##########
@@ -451,6 +487,67 @@ def test_dataset(dataset, dataset_reader):
assert_dataset_fragment_convenience_methods(dataset)
+def test_dataset_factory_inspect_schema_promotion(promotable_mockfs):
+ mockfs, path1, path2 = promotable_mockfs
+ factory = ds.FileSystemDatasetFactory(
+ mockfs, [path1, path2], ds.ParquetFileFormat()
+ )
+
+ with pytest.raises(
+ pa.ArrowTypeError,
+ match='Unable to merge: Field value has incompatible types: int8 vs uint16',
+ ):
+ factory.inspect()
+
+ schema = factory.inspect(promote_options='permissive')
+ expected_schema = pa.schema([
+ pa.field('value', pa.int32()),
+ pa.field('dictionary', pa.dictionary(pa.int16(), pa.string())),
+ ])
+ assert schema.equals(expected_schema)
+ dataset = factory.finish(schema)
+ table = dataset.to_table()
+ expected_table = pa.table({
+ 'value': pa.chunked_array([[1, 2], [3, 4]], type=pa.int32()),
+ 'dictionary': pa.chunked_array([
+ pa.DictionaryArray.from_arrays(
+ pa.array([0, 1], type=pa.int16()),
+ ['a', 'b']
+ ),
+ pa.DictionaryArray.from_arrays(
+ pa.array([1, 0], type=pa.int16()),
+ ['d', 'c']
+ )
+ ]),
+ })
+ assert table.equals(expected_table)
+
+ # Inspecting only one fragment should not promote the 'value' field
+ inspected_schema_one_frag = factory.inspect(
+ promote_options='permissive', fragments=1)
+ assert inspected_schema_one_frag.field('value').type != pa.int32()
Review Comment:
According to the `InspectOptions` docstring,
> The default value of `1` inspects the schema of the first (in no particular order) fragment only.
So we don't know which fragment is going to be inspected and which schema we'll get (even though in practice, it's always the first one in the list of paths). So I'm just checking that we aren't getting the unified schema.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-46629: [Python] Add options to DatasetFactory.inspect [arrow]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #46961:
URL: https://github.com/apache/arrow/pull/46961#issuecomment-3024544868
:warning: GitHub issue #46629 **has been automatically assigned in GitHub** to PR creator.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-46629: [Python] Add options to DatasetFactory.inspect [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #46961:
URL: https://github.com/apache/arrow/pull/46961#discussion_r2194852120
##########
python/pyarrow/tests/test_dataset.py:
##########
@@ -451,6 +487,67 @@ def test_dataset(dataset, dataset_reader):
assert_dataset_fragment_convenience_methods(dataset)
+def test_dataset_factory_inspect_schema_promotion(promotable_mockfs):
+ mockfs, path1, path2 = promotable_mockfs
+ factory = ds.FileSystemDatasetFactory(
+ mockfs, [path1, path2], ds.ParquetFileFormat()
+ )
+
+ with pytest.raises(
+ pa.ArrowTypeError,
+ match='Unable to merge: Field value has incompatible types: int8 vs uint16',
+ ):
+ factory.inspect()
+
+ schema = factory.inspect(promote_options='permissive')
+ expected_schema = pa.schema([
+ pa.field('value', pa.int32()),
+ pa.field('dictionary', pa.dictionary(pa.int16(), pa.string())),
Review Comment:
Perhaps add comments here to explain the promotions that are expected to occur? For example:
```suggestion
# Promoted from int8 and uint16
pa.field('value', pa.int32()),
# Dictionary index type promoted from int8 and int16
pa.field('dictionary', pa.dictionary(pa.int16(), pa.string())),
```
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-46629: [Python] Add options to DatasetFactory.inspect [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #46961:
URL: https://github.com/apache/arrow/pull/46961#discussion_r2192295606
##########
python/pyarrow/_dataset.pyx:
##########
@@ -3162,6 +3172,17 @@ cdef class DatasetFactory(_Weakrefable):
cdef:
CInspectOptions options
CResult[shared_ptr[CSchema]] result
+
+ options.field_merge_options = _parse_field_merge_options(promote_options, False)
Review Comment:
Please let's make the boolean parameter explicit:
```suggestion
options.field_merge_options = _parse_field_merge_options(promote_options, allow_none=False)
```
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-46629: [Python] Add options to DatasetFactory.inspect [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou merged PR #46961:
URL: https://github.com/apache/arrow/pull/46961
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-46629: [Python] Add options to DatasetFactory.inspect [arrow]
Posted by "amoeba (via GitHub)" <gi...@apache.org>.
amoeba commented on PR #46961:
URL: https://github.com/apache/arrow/pull/46961#issuecomment-3074877649
Thanks @hadrian-reppas.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-46629: [Python] Add options to DatasetFactory.inspect [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #46961:
URL: https://github.com/apache/arrow/pull/46961#discussion_r2194853798
##########
python/pyarrow/tests/test_dataset.py:
##########
@@ -451,6 +487,67 @@ def test_dataset(dataset, dataset_reader):
assert_dataset_fragment_convenience_methods(dataset)
+def test_dataset_factory_inspect_schema_promotion(promotable_mockfs):
+ mockfs, path1, path2 = promotable_mockfs
+ factory = ds.FileSystemDatasetFactory(
+ mockfs, [path1, path2], ds.ParquetFileFormat()
+ )
+
+ with pytest.raises(
+ pa.ArrowTypeError,
+ match='Unable to merge: Field value has incompatible types: int8 vs uint16',
+ ):
+ factory.inspect()
+
+ schema = factory.inspect(promote_options='permissive')
+ expected_schema = pa.schema([
+ pa.field('value', pa.int32()),
+ pa.field('dictionary', pa.dictionary(pa.int16(), pa.string())),
+ ])
+ assert schema.equals(expected_schema)
+ dataset = factory.finish(schema)
+ table = dataset.to_table()
+ expected_table = pa.table({
+ 'value': pa.chunked_array([[1, 2], [3, 4]], type=pa.int32()),
+ 'dictionary': pa.chunked_array([
+ pa.DictionaryArray.from_arrays(
+ pa.array([0, 1], type=pa.int16()),
+ ['a', 'b']
+ ),
+ pa.DictionaryArray.from_arrays(
+ pa.array([1, 0], type=pa.int16()),
+ ['d', 'c']
+ )
+ ]),
+ })
+ assert table.equals(expected_table)
+
+ # Inspecting only one fragment should not promote the 'value' field
+ inspected_schema_one_frag = factory.inspect(
+ promote_options='permissive', fragments=1)
+ assert inspected_schema_one_frag.field('value').type != pa.int32()
Review Comment:
Why not compare with the actual expected type?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-46629: [Python] Add options to DatasetFactory.inspect [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #46961:
URL: https://github.com/apache/arrow/pull/46961#discussion_r2207362933
##########
python/pyarrow/tests/test_dataset.py:
##########
@@ -451,6 +487,67 @@ def test_dataset(dataset, dataset_reader):
assert_dataset_fragment_convenience_methods(dataset)
+def test_dataset_factory_inspect_schema_promotion(promotable_mockfs):
+ mockfs, path1, path2 = promotable_mockfs
+ factory = ds.FileSystemDatasetFactory(
+ mockfs, [path1, path2], ds.ParquetFileFormat()
+ )
+
+ with pytest.raises(
+ pa.ArrowTypeError,
+ match='Unable to merge: Field value has incompatible types: int8 vs uint16',
+ ):
+ factory.inspect()
+
+ schema = factory.inspect(promote_options='permissive')
+ expected_schema = pa.schema([
+ pa.field('value', pa.int32()),
+ pa.field('dictionary', pa.dictionary(pa.int16(), pa.string())),
+ ])
+ assert schema.equals(expected_schema)
+ dataset = factory.finish(schema)
+ table = dataset.to_table()
+ expected_table = pa.table({
+ 'value': pa.chunked_array([[1, 2], [3, 4]], type=pa.int32()),
+ 'dictionary': pa.chunked_array([
+ pa.DictionaryArray.from_arrays(
+ pa.array([0, 1], type=pa.int16()),
+ ['a', 'b']
+ ),
+ pa.DictionaryArray.from_arrays(
+ pa.array([1, 0], type=pa.int16()),
+ ['d', 'c']
+ )
+ ]),
+ })
+ assert table.equals(expected_table)
+
+ # Inspecting only one fragment should not promote the 'value' field
+ inspected_schema_one_frag = factory.inspect(
+ promote_options='permissive', fragments=1)
+ assert inspected_schema_one_frag.field('value').type != pa.int32()
Review Comment:
I see, thanks.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-46629: [Python] Add options to DatasetFactory.inspect [arrow]
Posted by "hadrian-reppas (via GitHub)" <gi...@apache.org>.
hadrian-reppas commented on code in PR #46961:
URL: https://github.com/apache/arrow/pull/46961#discussion_r2178319698
##########
python/pyarrow/_dataset.pyx:
##########
@@ -3162,6 +3172,20 @@ cdef class DatasetFactory(_Weakrefable):
cdef:
CInspectOptions options
CResult[shared_ptr[CSchema]] result
+
+ if promote_options == "permissive":
Review Comment:
I've been fighting with Cython for a few hours. [This](https://github.com/hadrian-reppas/arrow/commit/534aa6148b8628229b77543e2e7fbfe1b686becd) is the closest I've gotten to making it work. Adding the definition of `_parse_field_merge_options` in other places I tried caused circular imports, so I put it in a different file. Even though I added _util to setup.py, I still get an import error:
```
>>> import pyarrow
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
File "/Users/hadrian/dev/arrow/python/pyarrow/__init__.py", line 61, in <module>
import pyarrow.lib as _lib
File "pyarrow/lib.pyx", line 1, in init pyarrow.lib
# Licensed to the Apache Software Foundation (ASF) under one
ModuleNotFoundError: No module named 'pyarrow._util'
```
I'll keep working on it.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org