-
Notifications
You must be signed in to change notification settings - Fork 651
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
FIX-#4756: Correctly propagate storage_options
in read_parquet
#4764
Conversation
…arquet Signed-off-by: Karthik Velayutham <vkarthik@ponder.io>
7c90d3d
to
f91864c
Compare
Codecov Report
@@ Coverage Diff @@
## master #4764 +/- ##
==========================================
+ Coverage 83.32% 89.88% +6.55%
==========================================
Files 259 260 +1
Lines 19339 19635 +296
==========================================
+ Hits 16114 17648 +1534
+ Misses 3225 1987 -1238
📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more |
718d7fe
to
5baadbd
Compare
@pyrito, do the changes address the issue? |
@YarShev yes they do address the issue. I just need to add a dataset to one of the S3 buckets hosted by the Intel folks with some storage options set so that I know that things are being passed through properly. The existing tests didn't catch this issue. |
@pyrito, cool. I left a comment, otherwise LGTM. Please let me know when the changes are ready to be merged. |
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.
LGTM, just a minor suggestion
@pyrito, please rebase on actual master to make Dask tests passed |
Co-authored-by: Alexey Prutskov <lehaprutskov@gmail.com>
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.
@pyrito, thanks!
Signed-off-by: Karthik Velayutham vkarthik@ponder.io
What do these changes do?
This PR addresses a bug where we do not propagate
storage_options
to theread_table
call inbuild_index
. This wasn't caught by any of our CI tests since we didn't stringently check for the options pass in forread_parquet
. I'm working on getting access to a bucket so we can make sure that this works accordingly.flake8 modin/ asv_bench/benchmarks scripts/doc_checker.py
black --check modin/ asv_bench/benchmarks scripts/doc_checker.py
git commit -s
docs/development/architecture.rst
is up-to-date