-
Notifications
You must be signed in to change notification settings - Fork 4k
ARROW-10433 [Python] Swopped the conditions for checking for fsspec filesystems #8557
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
195c8da to
2681356
Compare
Since s3fs.S3FileSystem is an fsspec filesystem moving this check will ensure that it is correctly detected as an fsspec filesystem and not wrapped with S3FSWrapper.
2681356 to
114d859
Compare
|
Rebased this, and running the tests that I added in #8573 with latest s3fs locally (we currently still get s3fs 0.4 on CI) |
|
So what use is the |
|
might be useful still for s3fs<0.4 ? |
|
I was going to propose to actually completely remove the This class if from before my time, but as far as I understand, it was initially used to wrap s3fs filesystems (so they could be used in ParquetDataset). Later on, the fsspec filesystems (and thus s3fs as well) became actual pyarrow.filesystem.FileSystem subclasses, so they would be used directly in ParquetDataset. So in practice, this S3FSWrapper has not been in use for the last s3fs / fsspec releases. But this change to inherit from pyarrow is already more than 2 years old (eg fsspec/filesystem_spec@f461317), so I think in practice nobody is (should be) using such an old fsspec version. |
jorisvandenbossche
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I checked locally that the relevant parquet tests are now passing with this patch and s3fs master (released s3fs 0.5.1 still has some regressions causing failures in reading partitioned parquet datasets)
As mentioned above, I think we can remove the S3FSWrapper call here alltogether, but will deal with that in a separate PR directly deprecating the class as well.
|
I've just hit |
Since s3fs.S3FileSystem is an fsspec filesystem moving this check will
ensure that it is correctly detected as an fsspec filesystem and not
wrapped with S3FSWrapper.