[FEAT] Add translation of IOConfig to PyArrow filesystem arguments #1592
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #1592 +/- ##
==========================================
- Coverage 85.29% 83.17% -2.12%
==========================================
Files 54 54
Lines 5161 5189 +28
==========================================
- Hits 4402 4316 -86
- Misses 759 873 +114
|
added 8 commits
November 10, 2023 10:58
…ethods in table_io
jaychia
force-pushed
the
jay/io-config-pyarrow
branch
from
836e043
to
4e54944
Compare
added 4 commits
November 10, 2023 14:51
…t to use_native_downloader=False
samster25
reviewed
Nov 11, 2023
clarkzinzow
approved these changes
Nov 13, 2023
Comment on lines
+219
to
+220
| _set_if_not_none(translated_kwargs, "access_key", s3_config.key_id) | ||
| _set_if_not_none(translated_kwargs, "secret_key", s3_config.access_key) |
There was a problem hiding this comment.
This naming asymmetry is a bit spooky to me, any particular reason for it?
There was a problem hiding this comment.
I think we just have it named differently :(
PyArrow:
$AWS_ACCESS_KEY_ID:access_key$AWS_SECRET_ACCESS_KEY:secret_key
Daft:
$AWS_ACCESS_KEY_ID:key_id$AWS_SECRET_ACCESS_KEY:access_key
Maybe @samster25 knows where we inherited our naming from?
There was a problem hiding this comment.
@jaychia Ah yeah, I based it off the aws sdk and boto3 suffix rather than what pyarrow did. What do you think we should use instead?
There was a problem hiding this comment.
Oh I wasn't proposing a change, just wondering where we got it from 😛
jaychia
added a commit
that referenced
this pull request
Nov 15, 2023
The change in #1592 introduced a bug where the inferred PyArrow S3FileSystem doesn't correctly infer the region of the bucket To work-around this, we can manually specify the `region_name` in our IOConfig. This problem should be less common once we move towards all-native reads. Co-authored-by: Jay Chia <jaychia94@gmail.com@users.noreply.github.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Closes: #1589
fsspecfilesystems inPythonStorageConfig. This was actually never used already since we deprecated the user-facing fsspec APIs in Daft 0.2.0filesystem.py:_resolve_paths_and_filesystemutility now takes in an optionalIOConfig_infer_filesystem) we now inmplement our own logic to translate IOConfig information into pyarrow filesystem kwargsio_configinto_resolve_paths_and_filesystemfor all cases where we are still usingPythonStorageConfigin our read APIs (i.e. when users opt to useuse_native_downloader=FalseIOConfigin ourwrite_*APIs as well