You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "AlenkaF (via GitHub)" <gi...@apache.org> on 2023/09/18 14:33:27 UTC
[GitHub] [arrow] AlenkaF opened a new pull request, #37768: GH-36983: [Python] Different get_file_info behaviour between pyarrow.fs.S3FileSystem and s3fs
AlenkaF opened a new pull request, #37768:
URL: https://github.com/apache/arrow/pull/37768
### What changes are included in this PR?
Add test comparing `get_file_info` with `FileSelector` for `S3FileSystem` and `s3fs` filesystem when mapped into a `PyFileSystem`. The tests checks that both are ignoring the base directory.
--
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-36983: [Python] Different get_file_info behaviour between pyarrow.fs.S3FileSystem and s3fs [arrow]
Posted by "jorisvandenbossche (via GitHub)" <gi...@apache.org>.
jorisvandenbossche merged PR #37768:
URL: https://github.com/apache/arrow/pull/37768
--
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-36983: [Python] Different get_file_info behaviour between pyarrow.fs.S3FileSystem and s3fs [arrow]
Posted by "jorisvandenbossche (via GitHub)" <gi...@apache.org>.
jorisvandenbossche commented on code in PR #37768:
URL: https://github.com/apache/arrow/pull/37768#discussion_r1347240273
##########
python/pyarrow/tests/test_fs.py:
##########
@@ -699,12 +699,12 @@ def test_get_file_info_with_selector(fs, pathfn):
infos = fs.get_file_info(selector)
if fs.type_name == "py::fsspec+s3":
- # s3fs only lists directories if they are not empty, but depending
- # on the s3fs/fsspec version combo, it includes the base_dir
+ # s3fs only lists directories if they are not empty
# (https://github.com/dask/s3fs/issues/393)
Review Comment:
You can remove this issue link as well, because it is about the base dir issue (not the empty directories)
##########
python/pyarrow/tests/test_fs.py:
##########
@@ -730,9 +726,10 @@ def test_get_file_info_with_selector(fs, pathfn):
# + for s3fs 0.5.2 all directories are dropped because of buggy
# side-effect of previous find() call
# (https://github.com/dask/s3fs/issues/410)
Review Comment:
This comment is for old s3fs version we also don't test anymore, so let's remove it altogether
```suggestion
```
##########
python/pyarrow/tests/test_fs.py:
##########
@@ -699,12 +699,12 @@ def test_get_file_info_with_selector(fs, pathfn):
infos = fs.get_file_info(selector)
if fs.type_name == "py::fsspec+s3":
- # s3fs only lists directories if they are not empty, but depending
- # on the s3fs/fsspec version combo, it includes the base_dir
+ # s3fs only lists directories if they are not empty
# (https://github.com/dask/s3fs/issues/393)
- assert (len(infos) == 4) or (len(infos) == 5)
+ len(infos) == 4
else:
assert len(infos) == 5
+ assert base_dir not in infos
Review Comment:
I am not sure this would ever give false (because `infos` is a list of FileInfo objects, and `base_dir` is a path. So you would have to do something like `base_dir not in [info.path for info in infos]`, but in hindsight that's actually ocvered already by the loop below (by removing the check for the base "selector-dir", we are now actually already ensuring that it is not in the list of `infos`.
--
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-36983: [Python] Different get_file_info behaviour between pyarrow.fs.S3FileSystem and s3fs [arrow]
Posted by "jorisvandenbossche (via GitHub)" <gi...@apache.org>.
jorisvandenbossche commented on code in PR #37768:
URL: https://github.com/apache/arrow/pull/37768#discussion_r1352275415
##########
python/pyarrow/tests/test_fs.py:
##########
@@ -699,10 +699,7 @@ def test_get_file_info_with_selector(fs, pathfn):
infos = fs.get_file_info(selector)
if fs.type_name == "py::fsspec+s3":
- # s3fs only lists directories if they are not empty, but depending
- # on the s3fs/fsspec version combo, it includes the base_dir
- # (https://github.com/dask/s3fs/issues/393)
- assert (len(infos) == 4) or (len(infos) == 5)
+ len(infos) == 4
Review Comment:
```suggestion
# s3fs only lists directories if they are not empty
len(infos) == 4
```
(that first part is still valid)
##########
python/pyarrow/tests/test_fs.py:
##########
@@ -726,11 +719,7 @@ def test_get_file_info_with_selector(fs, pathfn):
infos = fs.get_file_info(selector)
if fs.type_name == "py::fsspec+s3":
- # s3fs only lists directories if they are not empty
- # + for s3fs 0.5.2 all directories are dropped because of buggy
- # side-effect of previous find() call
- # (https://github.com/dask/s3fs/issues/410)
- assert (len(infos) == 3) or (len(infos) == 2)
+ assert len(infos) == 3
Review Comment:
```suggestion
# s3fs only lists directories if they are not empty
assert len(infos) == 3
```
--
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-36983: [Python] Different get_file_info behaviour between pyarrow.fs.S3FileSystem and s3fs [arrow]
Posted by "AlenkaF (via GitHub)" <gi...@apache.org>.
AlenkaF commented on code in PR #37768:
URL: https://github.com/apache/arrow/pull/37768#discussion_r1347259958
##########
python/pyarrow/tests/test_fs.py:
##########
@@ -699,12 +699,12 @@ def test_get_file_info_with_selector(fs, pathfn):
infos = fs.get_file_info(selector)
if fs.type_name == "py::fsspec+s3":
- # s3fs only lists directories if they are not empty, but depending
- # on the s3fs/fsspec version combo, it includes the base_dir
+ # s3fs only lists directories if they are not empty
# (https://github.com/dask/s3fs/issues/393)
- assert (len(infos) == 4) or (len(infos) == 5)
+ len(infos) == 4
else:
assert len(infos) == 5
+ assert base_dir not in infos
Review Comment:
You are correct. Will remove this lines.
(I am having trouble testing it locally as the test is failing on current master for me. Can't figure out why)
--
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-36983: [Python] Different get_file_info behaviour between pyarrow.fs.S3FileSystem and s3fs [arrow]
Posted by "AlenkaF (via GitHub)" <gi...@apache.org>.
AlenkaF commented on code in PR #37768:
URL: https://github.com/apache/arrow/pull/37768#discussion_r1352277408
##########
python/pyarrow/tests/test_fs.py:
##########
@@ -699,10 +699,7 @@ def test_get_file_info_with_selector(fs, pathfn):
infos = fs.get_file_info(selector)
if fs.type_name == "py::fsspec+s3":
- # s3fs only lists directories if they are not empty, but depending
- # on the s3fs/fsspec version combo, it includes the base_dir
- # (https://github.com/dask/s3fs/issues/393)
- assert (len(infos) == 4) or (len(infos) == 5)
+ len(infos) == 4
Review Comment:
😬 🤦♀️ Thanks!
##########
python/pyarrow/tests/test_fs.py:
##########
@@ -699,10 +699,7 @@ def test_get_file_info_with_selector(fs, pathfn):
infos = fs.get_file_info(selector)
if fs.type_name == "py::fsspec+s3":
- # s3fs only lists directories if they are not empty, but depending
- # on the s3fs/fsspec version combo, it includes the base_dir
- # (https://github.com/dask/s3fs/issues/393)
- assert (len(infos) == 4) or (len(infos) == 5)
+ len(infos) == 4
Review Comment:
😬 🤦♀️ 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-36983: [Python] Different get_file_info behaviour between pyarrow.fs.S3FileSystem and s3fs [arrow]
Posted by "conbench-apache-arrow[bot] (via GitHub)" <gi...@apache.org>.
conbench-apache-arrow[bot] commented on PR #37768:
URL: https://github.com/apache/arrow/pull/37768#issuecomment-1759065157
After merging your PR, Conbench analyzed the 6 benchmarking runs that have been run so far on merge-commit 94b7258c7e1dc500c53f873b17d97457752f95c7.
There were no benchmark performance regressions. 🎉
The [full Conbench report](https://github.com/apache/arrow/runs/17631585989) has more details. It also includes information about 2 possible false positives for unstable benchmarks that are known to sometimes produce them.
--
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