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 2022/11/02 15:08:41 UTC
[GitHub] [arrow] milesgranger opened a new pull request, #14574: ARROW-18225: Write metadata fully use kwargs
milesgranger opened a new pull request, #14574:
URL: https://github.com/apache/arrow/pull/14574
WIP
Will fix [ARROW-18225](https://issues.apache.org/jira/browse/ARROW-18225)
--
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
[GitHub] [arrow] milesgranger commented on a diff in pull request #14574: ARROW-18225: [Python] Fully use filesystem in parquet.write_metadata
Posted by GitBox <gi...@apache.org>.
milesgranger commented on code in PR #14574:
URL: https://github.com/apache/arrow/pull/14574#discussion_r1020278846
##########
python/pyarrow/parquet/core.py:
##########
@@ -3423,16 +3427,22 @@ def write_metadata(schema, where, metadata_collector=None, **kwargs):
... table.schema, 'dataset_metadata/_metadata',
... metadata_collector=metadata_collector)
"""
- writer = ParquetWriter(where, schema, **kwargs)
+ filesystem, where = _resolve_filesystem_and_path(where, filesystem)
+
+ writer = ParquetWriter(where, schema, filesystem, **kwargs)
writer.close()
if metadata_collector is not None:
# ParquetWriter doesn't expose the metadata until it's written. Write
# it and read it again.
- metadata = read_metadata(where)
+ metadata = read_metadata(where, filesystem=filesystem)
for m in metadata_collector:
metadata.append_row_groups(m)
- metadata.write_metadata_file(where)
+ file_ctx = nullcontext()
+ if filesystem is not None:
+ file_ctx = where = filesystem.open_output_stream(where)
+ with file_ctx:
+ metadata.write_metadata_file(where)
Review Comment:
Didn't work out of the box with file-like, but some small modifications for setting cursor back after reading before re-writing seems to work okay. https://github.com/apache/arrow/pull/14574/commits/54dd6415189ae8fe475e8965f949130b904065d1
--
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
[GitHub] [arrow] milesgranger commented on a diff in pull request #14574: ARROW-18225: [Python] Fully use filesystem in parquet.write_metadata
Posted by GitBox <gi...@apache.org>.
milesgranger commented on code in PR #14574:
URL: https://github.com/apache/arrow/pull/14574#discussion_r1021200067
##########
python/pyarrow/parquet/core.py:
##########
@@ -3423,13 +3427,15 @@ def write_metadata(schema, where, metadata_collector=None, **kwargs):
... table.schema, 'dataset_metadata/_metadata',
... metadata_collector=metadata_collector)
"""
- writer = ParquetWriter(where, schema, **kwargs)
+ filesystem, where = _resolve_filesystem_and_path(where, filesystem)
+
+ writer = ParquetWriter(where, schema, filesystem, **kwargs)
writer.close()
if metadata_collector is not None:
# ParquetWriter doesn't expose the metadata until it's written. Write
# it and read it again.
- metadata = read_metadata(where)
+ metadata = read_metadata(where, filesystem=filesystem)
for m in metadata_collector:
metadata.append_row_groups(m)
metadata.write_metadata_file(where)
Review Comment:
Not sure if https://github.com/apache/arrow/pull/14574/commits/b8b9ceacd7adb020ee7c82426c4109c121f33769 is what you had it mind, let me know should anything be adjusted there. :)
--
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
[GitHub] [arrow] jorisvandenbossche commented on a diff in pull request #14574: ARROW-18225: [Python] Fully use filesystem in parquet.write_metadata
Posted by GitBox <gi...@apache.org>.
jorisvandenbossche commented on code in PR #14574:
URL: https://github.com/apache/arrow/pull/14574#discussion_r1016812065
##########
python/pyarrow/parquet/core.py:
##########
@@ -3430,10 +3430,14 @@ def write_metadata(schema, where, metadata_collector=None, **kwargs):
if metadata_collector is not None:
# ParquetWriter doesn't expose the metadata until it's written. Write
# it and read it again.
- metadata = read_metadata(where)
+ metadata = read_metadata(where, filesystem=kwargs.get("filesystem"))
Review Comment:
I think we can still explicitly add a filesystem keyword to the `write_metadata` signature instead through kwargs; that makes it more discoverable for the user (and then also document it in the docstring), and is consistent with other functions in this module that support this keyword.
--
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
[GitHub] [arrow] milesgranger commented on a diff in pull request #14574: ARROW-18225: [Python] Fully use filesystem in parquet.write_metadata
Posted by GitBox <gi...@apache.org>.
milesgranger commented on code in PR #14574:
URL: https://github.com/apache/arrow/pull/14574#discussion_r1020254060
##########
python/pyarrow/parquet/core.py:
##########
@@ -3423,16 +3427,22 @@ def write_metadata(schema, where, metadata_collector=None, **kwargs):
... table.schema, 'dataset_metadata/_metadata',
... metadata_collector=metadata_collector)
"""
- writer = ParquetWriter(where, schema, **kwargs)
+ filesystem, where = _resolve_filesystem_and_path(where, filesystem)
+
+ writer = ParquetWriter(where, schema, filesystem, **kwargs)
writer.close()
if metadata_collector is not None:
# ParquetWriter doesn't expose the metadata until it's written. Write
# it and read it again.
- metadata = read_metadata(where)
+ metadata = read_metadata(where, filesystem=filesystem)
for m in metadata_collector:
metadata.append_row_groups(m)
- metadata.write_metadata_file(where)
+ file_ctx = nullcontext()
+ if filesystem is not None:
+ file_ctx = where = filesystem.open_output_stream(where)
+ with file_ctx:
+ metadata.write_metadata_file(where)
Review Comment:
> Just a nit, but to see the diff, this is just as many lines of code, and even less characters, while trying to be less "smart" (and IMO easier to read)
Ya, I sort of agree, but saw read_metadata used that pattern and thought maybe that was preferred.
--
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
[GitHub] [arrow] milesgranger commented on pull request #14574: ARROW-18225: [Python] Fully use filesystem in parquet.write_metadata
Posted by GitBox <gi...@apache.org>.
milesgranger commented on PR #14574:
URL: https://github.com/apache/arrow/pull/14574#issuecomment-1322612312
Ping for another review / more comments. :pray:
--
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
[GitHub] [arrow] milesgranger commented on a diff in pull request #14574: ARROW-18225: [Python] Fully use filesystem in parquet.write_metadata
Posted by GitBox <gi...@apache.org>.
milesgranger commented on code in PR #14574:
URL: https://github.com/apache/arrow/pull/14574#discussion_r1017416364
##########
python/pyarrow/parquet/core.py:
##########
@@ -3430,10 +3430,14 @@ def write_metadata(schema, where, metadata_collector=None, **kwargs):
if metadata_collector is not None:
# ParquetWriter doesn't expose the metadata until it's written. Write
# it and read it again.
- metadata = read_metadata(where)
+ metadata = read_metadata(where, filesystem=kwargs.get("filesystem"))
Review Comment:
Oi, I misinterpreted this one. Updated now. :) https://github.com/apache/arrow/pull/14574/commits/e120e9ada309d83df674be22e2106e8e056cab44
--
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
[GitHub] [arrow] milesgranger commented on a diff in pull request #14574: ARROW-18225: [Python] Fully use filesystem in parquet.write_metadata
Posted by GitBox <gi...@apache.org>.
milesgranger commented on code in PR #14574:
URL: https://github.com/apache/arrow/pull/14574#discussion_r1019245324
##########
python/pyarrow/parquet/core.py:
##########
@@ -3423,13 +3427,15 @@ def write_metadata(schema, where, metadata_collector=None, **kwargs):
... table.schema, 'dataset_metadata/_metadata',
... metadata_collector=metadata_collector)
"""
- writer = ParquetWriter(where, schema, **kwargs)
+ filesystem, where = _resolve_filesystem_and_path(where, filesystem)
+
+ writer = ParquetWriter(where, schema, filesystem, **kwargs)
writer.close()
if metadata_collector is not None:
# ParquetWriter doesn't expose the metadata until it's written. Write
# it and read it again.
- metadata = read_metadata(where)
+ metadata = read_metadata(where, filesystem=filesystem)
for m in metadata_collector:
metadata.append_row_groups(m)
metadata.write_metadata_file(where)
Review Comment:
Removing the `if filesystem is not None` at the top of this function then resolved the previous error for the file URI when `write_metadata_file` was not able to open file "file:///...". But suppose here, you're right. For that point, I've added back that condition and updated the test. I can go about adding the s3 variant as well.
--
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
[GitHub] [arrow] jorisvandenbossche commented on a diff in pull request #14574: ARROW-18225: [Python] Fully use filesystem in parquet.write_metadata
Posted by GitBox <gi...@apache.org>.
jorisvandenbossche commented on code in PR #14574:
URL: https://github.com/apache/arrow/pull/14574#discussion_r1018163232
##########
python/pyarrow/parquet/core.py:
##########
@@ -3423,15 +3427,17 @@ def write_metadata(schema, where, metadata_collector=None, **kwargs):
... table.schema, 'dataset_metadata/_metadata',
... metadata_collector=metadata_collector)
"""
- writer = ParquetWriter(where, schema, **kwargs)
+ writer = ParquetWriter(where, schema, filesystem, **kwargs)
writer.close()
if metadata_collector is not None:
# ParquetWriter doesn't expose the metadata until it's written. Write
# it and read it again.
- metadata = read_metadata(where)
+ metadata = read_metadata(where, filesystem=filesystem)
for m in metadata_collector:
metadata.append_row_groups(m)
+ if filesystem is not None:
+ filesystem, where = _resolve_filesystem_and_path(where, filesystem)
Review Comment:
That's would indeed be cleaner, I think (it can go to the first line of the function).
@milesgranger can you add a test using a URI? That should also cover this new ability (see my suggestion at https://github.com/apache/arrow/pull/14574#discussion_r1017707397, you can create a `meta3` and pass `meta3.as_uri()` as path)
--
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
[GitHub] [arrow] jorisvandenbossche commented on pull request #14574: ARROW-18225: [Python] Fully use kwargs in parquet.write_metadata
Posted by GitBox <gi...@apache.org>.
jorisvandenbossche commented on PR #14574:
URL: https://github.com/apache/arrow/pull/14574#issuecomment-1300723803
The generic utility to get matching kwargs _might_ be useful for other keywords, but for this specific case I think it is fine to explicitly add `filesystem` as a keyword to `write_metadata` (we also recently added that to `read_metadata`)
--
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
[GitHub] [arrow] ursabot commented on pull request #14574: ARROW-18225: [Python] Fully support filesystem in parquet.write_metadata
Posted by GitBox <gi...@apache.org>.
ursabot commented on PR #14574:
URL: https://github.com/apache/arrow/pull/14574#issuecomment-1325606718
Benchmark runs are scheduled for baseline = cf66f4882d083262cd183c3198e4bca2e6ea0d16 and contender = 57b81cac8a5d9dfa56c8a224cb2bc9b9046fb807. 57b81cac8a5d9dfa56c8a224cb2bc9b9046fb807 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Finished :arrow_down:0.0% :arrow_up:0.0%] [ec2-t3-xlarge-us-east-2](https://conbench.ursa.dev/compare/runs/fc523fe0bed849ab91bcdf6ffc510dc5...bfbf7ab23f724faca7282345988f2aad/)
[Failed :arrow_down:0.1% :arrow_up:0.0%] [test-mac-arm](https://conbench.ursa.dev/compare/runs/5b0e0c8e880441388aa75105bdd88c35...58eba1b5253a499da5f3c4adf7288c82/)
[Finished :arrow_down:0.0% :arrow_up:0.0%] [ursa-i9-9960x](https://conbench.ursa.dev/compare/runs/b2b7587c00da4f6699724e6e761dcc66...23965cf1b957433cbadf6a29bf182dc3/)
[Finished :arrow_down:0.21% :arrow_up:0.03%] [ursa-thinkcentre-m75q](https://conbench.ursa.dev/compare/runs/969bb248418d467fb5954c3ecdf15547...60738ae62f61490d814a5a77270e6d68/)
Buildkite builds:
[Finished] [`57b81cac` ec2-t3-xlarge-us-east-2](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ec2-t3-xlarge-us-east-2/builds/1904)
[Finished] [`57b81cac` test-mac-arm](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-test-mac-arm/builds/1926)
[Finished] [`57b81cac` ursa-i9-9960x](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-i9-9960x/builds/1895)
[Finished] [`57b81cac` ursa-thinkcentre-m75q](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-thinkcentre-m75q/builds/1918)
[Finished] [`cf66f488` ec2-t3-xlarge-us-east-2](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ec2-t3-xlarge-us-east-2/builds/1903)
[Failed] [`cf66f488` test-mac-arm](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-test-mac-arm/builds/1925)
[Finished] [`cf66f488` ursa-i9-9960x](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-i9-9960x/builds/1894)
[Finished] [`cf66f488` ursa-thinkcentre-m75q](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-thinkcentre-m75q/builds/1917)
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java
--
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
[GitHub] [arrow] milesgranger commented on a diff in pull request #14574: ARROW-18225: [Python] Fully use filesystem in parquet.write_metadata
Posted by GitBox <gi...@apache.org>.
milesgranger commented on code in PR #14574:
URL: https://github.com/apache/arrow/pull/14574#discussion_r1020254060
##########
python/pyarrow/parquet/core.py:
##########
@@ -3423,16 +3427,22 @@ def write_metadata(schema, where, metadata_collector=None, **kwargs):
... table.schema, 'dataset_metadata/_metadata',
... metadata_collector=metadata_collector)
"""
- writer = ParquetWriter(where, schema, **kwargs)
+ filesystem, where = _resolve_filesystem_and_path(where, filesystem)
+
+ writer = ParquetWriter(where, schema, filesystem, **kwargs)
writer.close()
if metadata_collector is not None:
# ParquetWriter doesn't expose the metadata until it's written. Write
# it and read it again.
- metadata = read_metadata(where)
+ metadata = read_metadata(where, filesystem=filesystem)
for m in metadata_collector:
metadata.append_row_groups(m)
- metadata.write_metadata_file(where)
+ file_ctx = nullcontext()
+ if filesystem is not None:
+ file_ctx = where = filesystem.open_output_stream(where)
+ with file_ctx:
+ metadata.write_metadata_file(where)
Review Comment:
> Just a nit, but to see the diff, this is just as many lines of code, and even less characters, while trying to be less "smart" (and IMO easier to read)
Ya, I sort of agree, but saw read_metadata used that pattern and thought maybe that was preferred. Dropped the commit.
--
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
[GitHub] [arrow] jorisvandenbossche commented on a diff in pull request #14574: ARROW-18225: [Python] Fully use filesystem in parquet.write_metadata
Posted by GitBox <gi...@apache.org>.
jorisvandenbossche commented on code in PR #14574:
URL: https://github.com/apache/arrow/pull/14574#discussion_r1017705626
##########
python/pyarrow/parquet/core.py:
##########
@@ -3424,16 +3426,20 @@ def write_metadata(schema, where, metadata_collector=None, **kwargs):
... table.schema, 'dataset_metadata/_metadata',
... metadata_collector=metadata_collector)
"""
- writer = ParquetWriter(where, schema, **kwargs)
+ writer = ParquetWriter(where, schema, filesystem, **kwargs)
writer.close()
if metadata_collector is not None:
# ParquetWriter doesn't expose the metadata until it's written. Write
# it and read it again.
- metadata = read_metadata(where)
+ metadata = read_metadata(where, filesystem=filesystem)
for m in metadata_collector:
metadata.append_row_groups(m)
- metadata.write_metadata_file(where)
+ if filesystem is not None and not hasattr(where, "write"):
Review Comment:
```suggestion
if filesystem is not None:
```
I would think we already ran into an error with `ParquetWriter` / `read_metadata` if the user would pass a filesystem in combination with a file-like object.
##########
python/pyarrow/tests/parquet/test_dataset.py:
##########
@@ -1300,6 +1301,32 @@ def _test_write_to_dataset_with_partitions(base_path,
tm.assert_frame_equal(output_df, input_df)
+def test_write_metadata_with_without_filesystem(tempdir):
Review Comment:
A `write_metadata` specific test is in `parquet/test_metadata.py`, so maybe move this there?
##########
python/pyarrow/parquet/core.py:
##########
@@ -3424,16 +3426,20 @@ def write_metadata(schema, where, metadata_collector=None, **kwargs):
... table.schema, 'dataset_metadata/_metadata',
... metadata_collector=metadata_collector)
"""
- writer = ParquetWriter(where, schema, **kwargs)
+ writer = ParquetWriter(where, schema, filesystem, **kwargs)
writer.close()
if metadata_collector is not None:
# ParquetWriter doesn't expose the metadata until it's written. Write
# it and read it again.
- metadata = read_metadata(where)
+ metadata = read_metadata(where, filesystem=filesystem)
for m in metadata_collector:
metadata.append_row_groups(m)
- metadata.write_metadata_file(where)
+ if filesystem is not None and not hasattr(where, "write"):
Review Comment:
But so actually, `read_metadata` does this by calling `filesystem, where = _resolve_filesystem_and_path(where, filesystem)`
We should probably do that here as well, so that the capabilities are consistent. For example that also supports writing to a URI string (you can test this with a local filesystem using `Path.as_uri()` to get a files:// URI)
##########
python/pyarrow/parquet/core.py:
##########
@@ -3392,6 +3392,8 @@ def write_metadata(schema, where, metadata_collector=None, **kwargs):
where : string or pyarrow.NativeFile
metadata_collector : list
where to collect metadata information.
+ filesystem : FileSystem, default None
+ If passed, will be used in subsequent calls accepting a filesystem.
Review Comment:
That it is being passed to subsequent calls is a bit of an implementation detail, I would say. ParquetWriter has this:
```
filesystem : FileSystem, default None
If nothing passed, will be inferred from `where` if path-like, else
`where` is already a file-like object so no filesystem is needed.
```
so maybe let's copy that?
--
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
[GitHub] [arrow] milesgranger commented on a diff in pull request #14574: ARROW-18225: [Python] Fully use filesystem in parquet.write_metadata
Posted by GitBox <gi...@apache.org>.
milesgranger commented on code in PR #14574:
URL: https://github.com/apache/arrow/pull/14574#discussion_r1017803009
##########
python/pyarrow/parquet/core.py:
##########
@@ -3424,16 +3426,20 @@ def write_metadata(schema, where, metadata_collector=None, **kwargs):
... table.schema, 'dataset_metadata/_metadata',
... metadata_collector=metadata_collector)
"""
- writer = ParquetWriter(where, schema, **kwargs)
+ writer = ParquetWriter(where, schema, filesystem, **kwargs)
writer.close()
if metadata_collector is not None:
# ParquetWriter doesn't expose the metadata until it's written. Write
# it and read it again.
- metadata = read_metadata(where)
+ metadata = read_metadata(where, filesystem=filesystem)
for m in metadata_collector:
metadata.append_row_groups(m)
- metadata.write_metadata_file(where)
+ if filesystem is not None and not hasattr(where, "write"):
Review Comment:
That's a lot better. :+1:
--
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
[GitHub] [arrow] github-actions[bot] commented on pull request #14574: ARROW-18225: [Python] Fully use kwargs in parquet.write_metadata
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #14574:
URL: https://github.com/apache/arrow/pull/14574#issuecomment-1301018082
https://issues.apache.org/jira/browse/ARROW-18225
--
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
[GitHub] [arrow] jorisvandenbossche commented on a diff in pull request #14574: ARROW-18225: [Python] Fully use filesystem in parquet.write_metadata
Posted by GitBox <gi...@apache.org>.
jorisvandenbossche commented on code in PR #14574:
URL: https://github.com/apache/arrow/pull/14574#discussion_r1019985446
##########
python/pyarrow/parquet/core.py:
##########
@@ -3423,16 +3427,22 @@ def write_metadata(schema, where, metadata_collector=None, **kwargs):
... table.schema, 'dataset_metadata/_metadata',
... metadata_collector=metadata_collector)
"""
- writer = ParquetWriter(where, schema, **kwargs)
+ filesystem, where = _resolve_filesystem_and_path(where, filesystem)
+
+ writer = ParquetWriter(where, schema, filesystem, **kwargs)
writer.close()
if metadata_collector is not None:
# ParquetWriter doesn't expose the metadata until it's written. Write
# it and read it again.
- metadata = read_metadata(where)
+ metadata = read_metadata(where, filesystem=filesystem)
for m in metadata_collector:
metadata.append_row_groups(m)
- metadata.write_metadata_file(where)
+ file_ctx = nullcontext()
+ if filesystem is not None:
+ file_ctx = where = filesystem.open_output_stream(where)
+ with file_ctx:
+ metadata.write_metadata_file(where)
Review Comment:
Actually, one more thing that I realize now: with using `_resolve_filesystem_and_path`, the only case when `filesystem` will still be `None`, is when `where` is a file-like object. But in that case, I am not sure the code to handle `metadata_collector` would work anyway? (like writing, reading and writing again to the same file like object, will that work?) It might be good to test that, so we either have coverage for it (if it works), or either we can simplify the above code (since we can assume `filesystem` to be always 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
[GitHub] [arrow] milesgranger commented on a diff in pull request #14574: ARROW-18225: [Python] Fully use filesystem in parquet.write_metadata
Posted by GitBox <gi...@apache.org>.
milesgranger commented on code in PR #14574:
URL: https://github.com/apache/arrow/pull/14574#discussion_r1020254060
##########
python/pyarrow/parquet/core.py:
##########
@@ -3423,16 +3427,22 @@ def write_metadata(schema, where, metadata_collector=None, **kwargs):
... table.schema, 'dataset_metadata/_metadata',
... metadata_collector=metadata_collector)
"""
- writer = ParquetWriter(where, schema, **kwargs)
+ filesystem, where = _resolve_filesystem_and_path(where, filesystem)
+
+ writer = ParquetWriter(where, schema, filesystem, **kwargs)
writer.close()
if metadata_collector is not None:
# ParquetWriter doesn't expose the metadata until it's written. Write
# it and read it again.
- metadata = read_metadata(where)
+ metadata = read_metadata(where, filesystem=filesystem)
for m in metadata_collector:
metadata.append_row_groups(m)
- metadata.write_metadata_file(where)
+ file_ctx = nullcontext()
+ if filesystem is not None:
+ file_ctx = where = filesystem.open_output_stream(where)
+ with file_ctx:
+ metadata.write_metadata_file(where)
Review Comment:
> Just a nit, but to see the diff, this is just as many lines of code, and even less characters, while trying to be less "smart" (and IMO easier to read)
Ya, I sort of agree, but saw read_meta used that pattern and thought maybe that was preferred.
--
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
[GitHub] [arrow] jorisvandenbossche commented on a diff in pull request #14574: ARROW-18225: [Python] Fully use filesystem in parquet.write_metadata
Posted by GitBox <gi...@apache.org>.
jorisvandenbossche commented on code in PR #14574:
URL: https://github.com/apache/arrow/pull/14574#discussion_r1019977209
##########
python/pyarrow/parquet/core.py:
##########
@@ -3423,16 +3427,22 @@ def write_metadata(schema, where, metadata_collector=None, **kwargs):
... table.schema, 'dataset_metadata/_metadata',
... metadata_collector=metadata_collector)
"""
- writer = ParquetWriter(where, schema, **kwargs)
+ filesystem, where = _resolve_filesystem_and_path(where, filesystem)
+
+ writer = ParquetWriter(where, schema, filesystem, **kwargs)
writer.close()
if metadata_collector is not None:
# ParquetWriter doesn't expose the metadata until it's written. Write
# it and read it again.
- metadata = read_metadata(where)
+ metadata = read_metadata(where, filesystem=filesystem)
for m in metadata_collector:
metadata.append_row_groups(m)
- metadata.write_metadata_file(where)
+ file_ctx = nullcontext()
+ if filesystem is not None:
+ file_ctx = where = filesystem.open_output_stream(where)
+ with file_ctx:
+ metadata.write_metadata_file(where)
Review Comment:
```suggestion
if filesystem is not None:
with filesystem.open_output_stream(where) as dest:
metadata.write_metadata_file(dest)
else:
metadata.write_metadata_file(where)
```
Just a nit, but to see the diff, this is just as many lines of code, and even less characters, while trying to be less "smart" (and IMO easier to read)
--
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
[GitHub] [arrow] wjones127 commented on a diff in pull request #14574: ARROW-18225: [Python] Fully use filesystem in parquet.write_metadata
Posted by GitBox <gi...@apache.org>.
wjones127 commented on code in PR #14574:
URL: https://github.com/apache/arrow/pull/14574#discussion_r1018100867
##########
python/pyarrow/parquet/core.py:
##########
@@ -3423,15 +3427,17 @@ def write_metadata(schema, where, metadata_collector=None, **kwargs):
... table.schema, 'dataset_metadata/_metadata',
... metadata_collector=metadata_collector)
"""
- writer = ParquetWriter(where, schema, **kwargs)
+ writer = ParquetWriter(where, schema, filesystem, **kwargs)
writer.close()
if metadata_collector is not None:
# ParquetWriter doesn't expose the metadata until it's written. Write
# it and read it again.
- metadata = read_metadata(where)
+ metadata = read_metadata(where, filesystem=filesystem)
for m in metadata_collector:
metadata.append_row_groups(m)
+ if filesystem is not None:
+ filesystem, where = _resolve_filesystem_and_path(where, filesystem)
Review Comment:
Shouldn't this go up higher, if you want to support inferring the filesystem from the path? (Even if those other function also do the inference, it seems like it would be nice if the inference was only performed once.
--
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
[GitHub] [arrow] jorisvandenbossche merged pull request #14574: ARROW-18225: [Python] Fully support filesystem in parquet.write_metadata
Posted by GitBox <gi...@apache.org>.
jorisvandenbossche merged PR #14574:
URL: https://github.com/apache/arrow/pull/14574
--
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