-
Notifications
You must be signed in to change notification settings - Fork 49
fix: include location in Session-based temporary storage manager DDL queries #1780
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
Tested manually locally and so far it seems to fix the issue on HEAD.
|
@@ -63,28 +62,6 @@ def _assert_bq_execution_location( | |||
expected_result, result.to_pandas(), check_dtype=False, check_index_type=False | |||
) | |||
|
|||
# Ensure BQ Storage Read client operation succceeds | |||
table = result.query_job.destination |
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.
This table and query_job might not actually exist / refer to an older object since to_pandas()
will likely end up in the "jobless" API path.
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 think this code is testing that the result of a BigFrames operation are stored in that location itself, which has value in asserting the location behavior, especially with regional endpoints. Rather than deleting, can we not do one of the following:
- Force a query job via
allow_large_results=True
in theSession
creation in all tests - Move this test code to a special test which enforces
allow_large_results=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.
I don't think this would verify the location though. You'd have to check that the dataset is in the correct location. Otherwise, this would read from whereever the table is.
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 have forced a job and validate the location of the stored results in 15fa728.
@Genesis929 @shobsi Please review.
New e2e failure is New notebook failures is @Genesis929 please review again. I think the setting to dismiss stale reviews has been turned on for security reasons. |
…queries
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> 🦕