Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Use pyarrow S3 file system at read time for arrow parquet engine #9669

Closed
wants to merge 3 commits into from
Closed

Use pyarrow S3 file system at read time for arrow parquet engine #9669

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
5aa0160
implement basic logic to use S3FileSystem for pyarrow parquet engine
rjzamora Nov 16, 2022
f3cf5bd
Merge remote-tracking branch 'upstream/main' into pyarrow-native-open…
rjzamora Nov 16, 2022
83bb174
minor revisions
rjzamora Nov 17, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
26 changes: 26 additions & 0 deletions dask/dataframe/io/parquet/arrow.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -335,9 +335,13 @@ def read_metadata(
ignore_metadata_file=False,
metadata_task_size=0,
parquet_file_extension=None,
storage_options=None,
**kwargs,
):

# Optimize open_file_func for remote filesystems
cls._update_open_file_func(fs, storage_options, kwargs)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we make this something more like:

kwargs = cls._update_open_file_func(fs, storage_options, kwargs)

or (psuedocode below):

if "open_file_options" not in kwargs:
    open_file_options = cls._get_open_file_options(fs, storage_options, kwargs)
    if open_file_options:
        kwargs["open_file_options"] = open_file_options

Where possible I'd like to avoid methods that mutate things inplace as it can make it more difficult to reason about the logic at play. Being more explicit about where things are set is, I think, a good thing.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where possible I'd like to avoid methods that mutate things inplace as it can make it more difficult to reason about the logic at play. Being more explicit about where things are set is, I think, a good thing.

I completely agree that code should be functional whenever possible. Sorry, you were so quick to review that I didn't get a chance to clean this up


# Stage 1: Collect general dataset information
dataset_info = cls._collect_dataset_info(
paths,
Expand Down Expand Up @@ -762,6 +766,28 @@ def write_metadata(cls, parts, meta, fs, path, append=False, **kwargs):
# Private Class Methods
#

@classmethod
def _update_open_file_func(cls, fs, storage_options, kwargs):
"""Update kwargs if we can use a native pyarrow filesystem

Currently supports ``s3fs`` -> ``pyarrow.fs.S3FileSystem``.
"""
is_s3fs = type(fs).__module__.split(".")[0] == "s3fs"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As is, I think we could use dask.utils.typename(...) here to make things more succinct. That said, I wonder if we could use a more direct check using isinstance(fs, fsspec.AbstractFileSystem) and fs.protocol (or something along these lines)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, this line is pretty gross. If we cant boil this down to a direct instance check, then I like the idea of using a well-defined utility.

if is_s3fs and not kwargs.get("open_file_options", {}):
pa_option_map = {"anon": "anonymous"}
try:
from pyarrow import fs as pa_fs

pa_options = {
pa_option_map.get(k): v for k, v in storage_options.items()
}
_fs = pa_fs.S3FileSystem(**pa_options)
kwargs["open_file_options"] = {"open_file_func": _fs.open_input_file}
except KeyError:
# Could not map one or more ``storage_options``
# keys to ``S3FileSystem`` options
pass

@classmethod
def _collect_dataset_info(
cls,
Expand Down
1 change: 1 addition & 0 deletions dask/dataframe/io/parquet/core.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -483,6 +483,7 @@ def read_parquet(
ignore_metadata_file=ignore_metadata_file,
metadata_task_size=metadata_task_size,
parquet_file_extension=parquet_file_extension,
storage_options=storage_options,
**kwargs,
)

Expand Down
1 change: 1 addition & 0 deletions dask/dataframe/io/parquet/fastparquet.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -828,6 +828,7 @@ def read_metadata(
ignore_metadata_file=False,
metadata_task_size=None,
parquet_file_extension=None,
storage_options=None,
**kwargs,
):

Expand Down