Skip to content
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

Add BigQuery Routine #4707

Merged
merged 25 commits into from
Feb 11, 2020
Merged

Add BigQuery Routine #4707

merged 25 commits into from
Feb 11, 2020

Conversation

quartzmo
Copy link
Member

@quartzmo quartzmo commented Jan 21, 2020

See googleapis/google-cloud-python#8491 for an example.

closes: #4430

@googlebot
Copy link

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

@googlebot googlebot added the cla: no This human has *not* signed the Contributor License Agreement. label Jan 21, 2020
@quartzmo quartzmo added api: bigquery Issues related to the BigQuery API. do not merge Indicates a pull request not ready for merge, due to either quality or timing. labels Jan 21, 2020
@quartzmo quartzmo self-assigned this Jan 21, 2020
@quartzmo quartzmo requested review from a team and shollyman January 21, 2020 17:53
@dazuma dazuma added cla: yes This human has signed the Contributor License Agreement. and removed cla: no This human has *not* signed the Contributor License Agreement. labels Jan 21, 2020
@googlebot
Copy link

A Googler has manually verified that the CLAs look good.

(Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.)

ℹ️ Googlers: Go here for more info.

@googlebot
Copy link

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

@googlebot googlebot added cla: no This human has *not* signed the Contributor License Agreement. and removed cla: yes This human has signed the Contributor License Agreement. labels Jan 23, 2020
@dazuma dazuma added cla: yes This human has signed the Contributor License Agreement. and removed cla: no This human has *not* signed the Contributor License Agreement. labels Jan 28, 2020
@googlebot
Copy link

A Googler has manually verified that the CLAs look good.

(Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.)

ℹ️ Googlers: Go here for more info.

@googlebot
Copy link

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

@googlebot googlebot added cla: no This human has *not* signed the Contributor License Agreement. and removed cla: yes This human has signed the Contributor License Agreement. labels Jan 29, 2020
Copy link
Contributor

@shollyman shollyman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for putting this together. Added comments, reviewing this helped me realize we need to revisit filter / readmask for the initial implementations where missing.

class Routine
##
# Routine::List is a special case Array with additional values.
class List < DelegateClass(::Array)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like this is missing support for supported list params. Added a comment to the impl guide because I realized the docs weren't properly exposing their presence.


routines.count.must_equal 6
routines.each { |ds| ds.must_be_kind_of Google::Cloud::Bigquery::Routine }
end
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

possibly want an list test in this test set that leverages filter once it is available.

@quartzmo
Copy link
Member Author

quartzmo commented Feb 5, 2020

@shollyman:

we need to revisit filter / readmask for the initial implementations where missing.

I'll start work now on adding the filter property to restrict what types of entities are returned by the routines.list method (e.g. only list UDFs, or stored procedures).

Is the readmask also appropriate/desired in the ruby client? You probably noticed that this client has a somewhat elaborate mechanism for transitioning an object from a partial to a full projection. I'm not sure how this will interact with a readmask for routines.list since the detection of the partial projection state depends on the presence of a field that is missing in the default partial project from the list but required in the full projection.

@quartzmo quartzmo removed the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Feb 6, 2020
@quartzmo
Copy link
Member Author

quartzmo commented Feb 7, 2020

@shollyman I added filter to Dataset#routines (Routine::List) as well as QueryJob#ddl_target_routine (statistics).

@quartzmo
Copy link
Member Author

@shollyman PTAL, let me know if you want anything else in this PR, or if it's ready for @dazuma to review. Thanks!

@dazuma dazuma added cla: yes This human has signed the Contributor License Agreement. and removed cla: no This human has *not* signed the Contributor License Agreement. labels Feb 10, 2020
@googlebot
Copy link

A Googler has manually verified that the CLAs look good.

(Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.)

ℹ️ Googlers: Go here for more info.

@shollyman
Copy link
Contributor

@shollyman PTAL, let me know if you want anything else in this PR, or if it's ready for @dazuma to review. Thanks!

Thanks, it looks good from my perspective. I concur with the read mask being of lesser value here given you've got a much more stateful implementation to deal with here.

@quartzmo quartzmo requested a review from dazuma February 11, 2020 00:53
@blowmage
Copy link
Contributor

@googlebot I consent.

@googlebot
Copy link

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

@quartzmo quartzmo merged commit a5b2c0d into googleapis:master Feb 11, 2020
@quartzmo quartzmo deleted the bigquery-routine branch February 11, 2020 22:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: bigquery Issues related to the BigQuery API. cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

BigQuery: implement Routines API support
5 participants