-
Notifications
You must be signed in to change notification settings - Fork 306
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
feat: add Client.query_and_wait
which directly returns a RowIterator
of results
#1722
Conversation
timeout: Optional[float], | ||
job_retry: retries.Retry, | ||
) -> table.RowIterator: | ||
"""Initiate a query using jobs.query with jobCreationMode=JOB_CREATION_REQUIRED. |
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 this be JOB_CREATION_OPTIONAL?
It seems that way based on the descriptions above.
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.
You're correct. I'll update.
…_is_completely_cached
…_is_completely_cached
…or` of results Set the `QUERY_PREVIEW_ENABLED=TRUE` environment variable to use this with the new JOB_CREATION_OPTIONAL mode (currently in preview).
93d47e7
to
c16e4be
Compare
elif page_size is not None or max_results is not None: | ||
request_body["maxResults"] = page_size or max_results | ||
|
||
if os.getenv("QUERY_PREVIEW_ENABLED", "").casefold() == "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.
@tswast , in our previous discussion, I only suggested for this environment variable to use a list of project IDs for a more fine-grained control. We have discussed it with @shollyman , and for the Java client we decided to go with a boolean. I am fine either way, staying consistent with a boolean or doing a more fine-grained project id list (if required by python client usage).
This was being reset in some cases when the rows were all available in the first page of results.
if default_job_config is None: | ||
return job_config | ||
|
||
# anything that's not defined on the incoming |
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 comment is a bit unclear. Can we clean this up a bit?
# should be filled in with the default | ||
# the incoming therefore has precedence | ||
# | ||
# Note that _fill_from_default doesn't mutate the receiver |
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.
For my own edification, what do we mean by "receiver"?
Co-authored-by: Anthonios Partheniou <[email protected]>
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. Please address minor comments.
Set the
QUERY_PREVIEW_ENABLED=TRUE
environment variable to use this with the new JOB_CREATION_OPTIONAL mode (currently in preview).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:
Note: Currently in DRAFT mode as I realized that some of the necessary work to speed things up overlaps with the
query(api_method="QUERY")
work (#1723), so I'll work on modifying that first before moving on to the next step of a "jobless" API.Towards #589 🦕