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 QueryJob#num_dml_affected_rows and DDL/DML docs #2530

Merged
merged 5 commits into from
Oct 24, 2018

Conversation

quartzmo
Copy link
Member

[closes #2141]

@quartzmo quartzmo added the api: bigquery Issues related to the BigQuery API. label Oct 18, 2018
@quartzmo quartzmo self-assigned this Oct 18, 2018
@quartzmo quartzmo requested a review from blowmage October 18, 2018 21:10
@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Oct 18, 2018
@blowmage
Copy link
Contributor

I would like to get @tswast's review on this.

Copy link
Contributor

@blowmage blowmage left a comment

Choose a reason for hiding this comment

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

LGTM, but I'd defer to @tswast.

@quartzmo
Copy link
Member Author

quartzmo commented Oct 18, 2018

@tswast Do you think we should add wrapper methods similar to #query for DDL and DML operations? These new methods would wait for the job to complete, then return the table reference (DDL) or the number of rows affected (DML).

@tswast
Copy link
Contributor

tswast commented Oct 18, 2018

In Python we return a job for the query helper (but then allow iterating over it for getting the results). Ideally one should be able to use the same helper to call DDL and DML queries, though it's tough when the helper returns "results".

We suffered a similar problem in Java, where my solution was to special-case an "empty results" object for DDL queries. googleapis/google-cloud-java#3469

Copy link
Contributor

@tswast tswast left a comment

Choose a reason for hiding this comment

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

Property LGTM. I think the docs to recommend the job method are good for DDL / DML, but ideally one should be able to call the regular query method if they don't care about the detailed results.

@quartzmo
Copy link
Member Author

@tswast Good suggestion, I've been working today on adding DDL/DML response info to the array type returned by #query. (Which would be an empty or lazy array for these usages.)

Add job_gapi to Data.
Update QueryJob#data to conditionally return empty Data.
@quartzmo
Copy link
Member Author

In Python we return a job for the query helper (but then allow iterating over it for getting the results).

@tswast @blowmage PTAL at the most recent commit:

  • Add DDL/DML attrs to Data, such as the table reference (DDL) or the number of rows affected (DML).
  • Update #query to conditionally return empty Data populated with DDL/DML attrs.

end

def ddl?
%w[CREATE_TABLE CREATE_TABLE_AS_SELECT DROP_TABLE CREATE_VIEW \

This comment was marked as spam.

end

def dml?
%w[INSERT UPDATE DELETE].include? statement_type

This comment was marked as spam.

end

def dml?
%w[INSERT UPDATE DELETE].include? statement_type

This comment was marked as spam.

@@ -217,6 +217,15 @@ def statement_type
@gapi.statistics.query.statement_type
end

def ddl?
%w[CREATE_TABLE CREATE_TABLE_AS_SELECT DROP_TABLE CREATE_VIEW \

This comment was marked as spam.

@@ -490,15 +511,18 @@ def wait_until_done!
#
def data token: nil, max: nil, start: nil
return nil unless done?

if ddl? || dml?

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

@quartzmo
Copy link
Member Author

@tswast I updated totalRows to be nil for DDL/DML, and completed docs and unit tests. I believe this PR is ready for final review.

@quartzmo quartzmo merged commit 343fbe4 into googleapis:master Oct 24, 2018
@quartzmo quartzmo deleted the bigquery-dml branch October 24, 2018 16:13
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.

Don't return query results from BigQuery DML/DDL statements
4 participants