-
Notifications
You must be signed in to change notification settings - Fork 57
feat!: add allow_large_results
option to read_gbq_query
, aligning with bpd.options.compute.allow_large_results
option
#1935
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
base: main
Are you sure you want to change the base?
Conversation
allow_large_results
option to read_gbq_query
allow_large_results
option to read_gbq_query
. Set to False
to enable faster queries
bigframes/pandas/io/api.py
Outdated
@@ -215,6 +217,7 @@ def read_gbq( | |||
use_cache: Optional[bool] = None, | |||
col_order: Iterable[str] = (), | |||
dry_run: bool = False, | |||
allow_large_results: bool = True, |
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.
Should allow_large_results
also default to None
? This would allow it to inherit its value from ComputeOptions.allow_large_results
.
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.
If we do so, it'd technicaly be a breaking change. Users with query results > 10 GB would have to set this option to True
.
Might be worth it though for the consistency with other places, though?
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.
done.
Doctest looks like a real failure:
It seems I'm not setting the index correctly. |
pytest --doctest-modules bigframes/session/__init__.py::bigframes.session.Session.read_gbq_query
This has been fixed. I've confirmed the doctest passes locally and have added two tests for the columns and index_col arguments. |
Looks like I have some more failing tests to address:
Getting started on that now. |
allow_large_results
option to read_gbq_query
. Set to False
to enable faster queriesallow_large_results
option to read_gbq_query
. Set to False
to enable faster queries
A lot more failures now.
I'll take a look at this when I get back I guess. |
I think this is a real failure. We aren't sorting by the |
allow_large_results
option to read_gbq_query
. Set to False
to enable faster queriesallow_large_results
option to read_gbq_query
, aligning with bpd.options.compute.allow_large_results
option
Thank you for opening a Pull Request! Before submitting your PR, there are a few things you can do to make sure it goes smoothly:
Fixes #<issue_number_goes_here> 🦕