You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by GitBox <gi...@apache.org> on 2020/10/01 12:42:24 UTC
[GitHub] [arrow] bkietz opened a new pull request #8317: ARROW-10134: [Python][Dataset] Add ParquetFileFragment.num_row_groups
bkietz opened a new pull request #8317:
URL: https://github.com/apache/arrow/pull/8317
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] jorisvandenbossche commented on a change in pull request #8317: ARROW-10134: [Python][Dataset] Add ParquetFileFragment.num_row_groups
Posted by GitBox <gi...@apache.org>.
jorisvandenbossche commented on a change in pull request #8317:
URL: https://github.com/apache/arrow/pull/8317#discussion_r501109018
##########
File path: cpp/src/arrow/dataset/file_parquet.cc
##########
@@ -538,17 +556,17 @@ Status ParquetFileFragment::EnsureCompleteMetadata(parquet::arrow::FileReader* r
physical_schema_ = std::move(schema);
std::shared_ptr<parquet::FileMetaData> metadata = reader->parquet_reader()->metadata();
- int num_row_groups = metadata->num_row_groups();
Review comment:
I think we still might need to keep this. Below, it was used for checking that the ids are not out of bounds. But using `num_row_groups_`, which can be smaller than the max number of row groups in the file, is the wrong number to check this, I 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] github-actions[bot] commented on pull request #8317: ARROW-10134: [Python][Dataset] Add ParquetFileFragment.num_row_groups
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #8317:
URL: https://github.com/apache/arrow/pull/8317#issuecomment-702109871
https://issues.apache.org/jira/browse/ARROW-10134
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] bkietz commented on a change in pull request #8317: ARROW-10134: [Python][Dataset] Add ParquetFileFragment.num_row_groups
Posted by GitBox <gi...@apache.org>.
bkietz commented on a change in pull request #8317:
URL: https://github.com/apache/arrow/pull/8317#discussion_r499617235
##########
File path: python/pyarrow/_dataset.pyx
##########
@@ -962,6 +962,10 @@ cdef class ParquetFileFragment(FileFragment):
return None
return [RowGroupInfo.wrap(row_group) for row_group in c_row_groups]
+ @property
+ def num_row_groups(self):
+ return None if self.row_groups is None else len(self.row_groups)
Review comment:
I'm adding a method to load just the number of row groups, leaving statistics unparsed
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] jorisvandenbossche commented on pull request #8317: ARROW-10134: [Python][Dataset] Add ParquetFileFragment.num_row_groups
Posted by GitBox <gi...@apache.org>.
jorisvandenbossche commented on pull request #8317:
URL: https://github.com/apache/arrow/pull/8317#issuecomment-702823129
I think the main reason such a property would be interesting for dask's use case is to get the number of row groups in a case all statistics are not available / not yet parsed. So the way that this PR returns `None` in that case is not super useful, I think.
I think ideally if the number of row groups is not know (the row_groups are not set), it would retrieve this information from the FileMetaData.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] jorisvandenbossche commented on pull request #8317: ARROW-10134: [Python][Dataset] Add ParquetFileFragment.num_row_groups
Posted by GitBox <gi...@apache.org>.
jorisvandenbossche commented on pull request #8317:
URL: https://github.com/apache/arrow/pull/8317#issuecomment-705025495
Could you add test for the case I commented about? I think this should do it (didn't run the code though):
```python
@pytest.mark.parquet
def test_parquet_fragment_num_row_groups(tempdir):
import pyarrow.parquet as pq
table = pa.table({'a': range(8)})
pq.write_table(table, tempdir / "test.parquet", row_group_size=2)
dataset = ds.dataset(tempdir / "test.parquet", format="parquet")
original_fragment = list(dataset.get_fragments())[0]
# create fragment with subset of row groups
fragment = original_fragment.format.make_fragment(
original_fragment.path, original_fragment.filesystem,
row_groups=[1, 3])
assert fragment.num_row_groups == 2
# ensure that parsing metadata preserves correct number of row groups
fragment.ensure_complete_metadata()
assert fragment.num_row_groups == 2
assert len(fragment.row_groups) == 2
```
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] bkietz commented on pull request #8317: ARROW-10134: [Python][Dataset] Add ParquetFileFragment.num_row_groups
Posted by GitBox <gi...@apache.org>.
bkietz commented on pull request #8317:
URL: https://github.com/apache/arrow/pull/8317#issuecomment-703876867
@jorisvandenbossche PTAL
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou commented on a change in pull request #8317: ARROW-10134: [Python][Dataset] Add ParquetFileFragment.num_row_groups
Posted by GitBox <gi...@apache.org>.
pitrou commented on a change in pull request #8317:
URL: https://github.com/apache/arrow/pull/8317#discussion_r499611181
##########
File path: python/pyarrow/_dataset.pyx
##########
@@ -962,6 +962,10 @@ cdef class ParquetFileFragment(FileFragment):
return None
return [RowGroupInfo.wrap(row_group) for row_group in c_row_groups]
+ @property
+ def num_row_groups(self):
+ return None if self.row_groups is None else len(self.row_groups)
Review comment:
Is there a way to do this without reifying `self.row_groups`? Or is it cheap enough?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] bkietz commented on pull request #8317: ARROW-10134: [Python][Dataset] Add ParquetFileFragment.num_row_groups
Posted by GitBox <gi...@apache.org>.
bkietz commented on pull request #8317:
URL: https://github.com/apache/arrow/pull/8317#issuecomment-702862461
@jorisvandenbossche just confirming: you want `f.num_row_groups` to potentially perform IO?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] jorisvandenbossche commented on pull request #8317: ARROW-10134: [Python][Dataset] Add ParquetFileFragment.num_row_groups
Posted by GitBox <gi...@apache.org>.
jorisvandenbossche commented on pull request #8317:
URL: https://github.com/apache/arrow/pull/8317#issuecomment-704808851
> you want f.num_row_groups to potentially perform IO?
Yes, that's indeed the consequence for now (if the metadata was not yet parsed before). Long term I would like us to cache the metadata, though, without the need to necessarily directly parse all statistics etc (https://issues.apache.org/jira/browse/ARROW-10131-
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] jorisvandenbossche commented on a change in pull request #8317: ARROW-10134: [Python][Dataset] Add ParquetFileFragment.num_row_groups
Posted by GitBox <gi...@apache.org>.
jorisvandenbossche commented on a change in pull request #8317:
URL: https://github.com/apache/arrow/pull/8317#discussion_r500896637
##########
File path: cpp/src/arrow/dataset/file_parquet.cc
##########
@@ -530,17 +548,17 @@ Status ParquetFileFragment::EnsureCompleteMetadata(parquet::arrow::FileReader* r
physical_schema_ = std::move(schema);
std::shared_ptr<parquet::FileMetaData> metadata = reader->parquet_reader()->metadata();
- int num_row_groups = metadata->num_row_groups();
+ num_row_groups_ = metadata->num_row_groups();
Review comment:
I don't think we should set the class property here? Because that will override a potential subselection of row groups?
Small example:
```python
import pyarrow.parquet as pq
table = pa.table({'a': [1, 2, 3, 4]})
pq.write_table(table, "test_num_row_groups.parquet", row_group_size=2)
import pyarrow.dataset as ds
dataset = ds.dataset("test_num_row_groups.parquet")
fragment = list(dataset.get_fragments())[0]
# make fragment viewing the first row group
In [14]: fragment0 = fragment.format.make_fragment(
...: fragment.path, fragment.filesystem, row_groups=[0])
In [15]: fragment0.num_row_groups
Out[15]: 1
In [16]: fragment0.row_groups
Out[16]: [<pyarrow._dataset.RowGroupInfo at 0x7f633bbc93f8>]
In [17]: fragment0.row_groups[0].statistics
# complete the metadata -> still has a single fragment, but property returns wrong length
In [18]: fragment0.ensure_complete_metadata()
In [19]: fragment0.num_row_groups
Out[19]: 2
In [20]: fragment0.row_groups
Out[20]: [<pyarrow._dataset.RowGroupInfo at 0x7f633bbcc3a0>]
In [21]: fragment0.row_groups[0].statistics
Out[21]: {'a': {'min': 1, 'max': 2}}
```
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] bkietz commented on pull request #8317: ARROW-10134: [Python][Dataset] Add ParquetFileFragment.num_row_groups
Posted by GitBox <gi...@apache.org>.
bkietz commented on pull request #8317:
URL: https://github.com/apache/arrow/pull/8317#issuecomment-705261226
CI failure is unrelated. Merging
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] bkietz closed pull request #8317: ARROW-10134: [Python][Dataset] Add ParquetFileFragment.num_row_groups
Posted by GitBox <gi...@apache.org>.
bkietz closed pull request #8317:
URL: https://github.com/apache/arrow/pull/8317
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] bkietz commented on a change in pull request #8317: ARROW-10134: [Python][Dataset] Add ParquetFileFragment.num_row_groups
Posted by GitBox <gi...@apache.org>.
bkietz commented on a change in pull request #8317:
URL: https://github.com/apache/arrow/pull/8317#discussion_r501080829
##########
File path: cpp/src/arrow/dataset/file_parquet.cc
##########
@@ -530,17 +548,17 @@ Status ParquetFileFragment::EnsureCompleteMetadata(parquet::arrow::FileReader* r
physical_schema_ = std::move(schema);
std::shared_ptr<parquet::FileMetaData> metadata = reader->parquet_reader()->metadata();
- int num_row_groups = metadata->num_row_groups();
+ num_row_groups_ = metadata->num_row_groups();
Review comment:
Ah, you're right. I'll check for an explicit subselection first
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org