Skip to content

BigQuery: adds models API implementation. - #5021

Merged
shollyman merged 23 commits into
googleapis:masterfrom
shollyman:models
May 9, 2019
Merged

BigQuery: adds models API implementation.#5021
shollyman merged 23 commits into
googleapis:masterfrom
shollyman:models

Conversation

@shollyman

Copy link
Copy Markdown
Contributor

BigQuery: adds models API implementation.

This change adds support for BigQuery ML models as an API resource independent from the previous
ML-As-A-Table variant that existed during beta. It mirrors much of the interactions with Tables, so there's a corresponding ModelId for encapsulating the model reference, ModelInfo for the model metadata, and Model which has service awareness and can be invoked for operations like get/reload.

This initial change does NOT include the extended statistical information from models related to training runs, nor does it include ML feature and labelling column information. That will come in a subsequent change. This change exposes the model type (e.g. LINEAR_REGRESSION), and the ability to do operations like mutate the description/friendlyname/labels.

I've also marked one of the BigQuery methods deprecated due to ambiguous type signature.
boolean delete(String datasetId, String tableId);

When tables were the only kind of resource in a dataset, being able to specify the table via a string was sufficient. However, now that we have independent resource collection kinds within a dataset container, the signature for deletion needs to use more strongly qualified identifiers.

REVIEWER NOTE
I've overridden pom.xml temporarily to pin to an older version of google-api-services-bigquery which will allow testing. The BigQuery service is rolling out the changes necessary so that this no longer needs pinning, but this change is not appropriate for submission until that is complete.

@shollymanshollyman 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 Apr 25, 2019
@googlebotgooglebot added the cla: yes This human has signed the Contributor License Agreement. label Apr 25, 2019
@shollymanshollyman changed the title ModelsBigQuery: adds models API implementation.Apr 25, 2019
* ModelInfo modelInfo = beforeModel.toBuilder()
* .setDescription(newDescription)
* .build();
* Model afterModel = bigquery.update(moddelInfo);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Typo: moddel

Note: We have a little program at https://github.com/googleapis/google-cloud-java/blob/master/utilities/snippets.go for copying code samples into JavaDocs, if you want to try that out.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Fixed the typo. Inclined to add the snippets in a followup and redo the instrumentation then, unless you want this change to get even bigger.

break;
}
}
assertTrue(found);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

You sure you want to assert this since listing resources is eventually consistent? Could be flakey.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Good news, everyone! This should be strongly consistent, per conversation with one of the backend engineers who works on the metadata layer.

@tswast

Copy link
Copy Markdown
Contributor

I've also marked one of the BigQuery methods deprecated due to ambiguous type signature.
boolean delete(String datasetId, String tableId);

Thought: Should this be a separate PR so that it shows up nicely in the changelog?

LABELS("labels"),
LAST_MODIFIED_TIME("lastModifiedTime"),
LOCATION("location"),
MODEL_REFERENCE("modelReference"),

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I see model type in the Model class, but not here. At first I thought you were waiting for the enum from Apiary, but I guess not?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

I called it TYPE, but perhaps MODEL_TYPE is better for the enum?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Oh, you're right. TYPE seems fine. Less redundant.

@yoshi-automationyoshi-automation added the 🚨 critical P0 critical issue. Requires immediate fix label May 2, 2019
@shollyman
shollyman marked this pull request as ready for review May 8, 2019 18:06
@shollyman
shollyman requested a review from a teamMay 8, 2019 18:06
LABELS("labels"),
LAST_MODIFIED_TIME("lastModifiedTime"),
LOCATION("location"),
MODEL_REFERENCE("modelReference"),

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Oh, you're right. TYPE seems fine. Less redundant.

@codecov

codecovBot commented May 9, 2019

Copy link
Copy Markdown

Codecov Report

Merging #5021 into master will increase coverage by 0.01%.
The diff coverage is 65.09%.

Impacted file tree graph

@@ Coverage Diff @@## master #5021 +/- ##
============================================
+ Coverage 50.38% 50.4% +0.01% - Complexity 23736 23783 +47 
============================================
Files 2248 2251 +3 Lines 226471 226789 +318 Branches 24954 24968 +14 ============================================
+ Hits 114109 114314 +205 - Misses 103770 103870 +100 - Partials 8592 8605 +13
Impacted FilesCoverage ΔComplexity Δ
.../com/google/cloud/bigquery/spi/v2/BigQueryRpc.java77.77% <ø> (ø)0 <0> (ø)⬇️
.../google/cloud/bigquery/spi/v2/HttpBigQueryRpc.java6.1% <0%> (-0.83%)2 <0> (ø)
...e/cloud/bigquery/testing/RemoteBigQueryHelper.java62.5% <0%> (-1.14%)6 <0> (ø)
...n/java/com/google/cloud/bigquery/BigQueryImpl.java80.21% <60.75%> (-3.89%)58 <5> (+5)
...src/main/java/com/google/cloud/bigquery/Model.java75.51% <75.51%> (ø)12 <12> (?)
...main/java/com/google/cloud/bigquery/ModelInfo.java78.84% <78.84%> (ø)18 <18> (?)
...c/main/java/com/google/cloud/bigquery/ModelId.java79.16% <79.16%> (ø)13 <13> (?)
.../main/java/com/google/cloud/bigquery/BigQuery.java78.18% <80.76%> (+0.48%)0 <0> (ø)⬇️
...able/gaxx/reframing/ReframingResponseObserver.java88.99% <0%> (-1.84%)29% <0%> (-1%)
... and 2 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 20912a4...47957b6. Read the comment docs.

@shollyman

Copy link
Copy Markdown
ContributorAuthor

The failing kokoro OSX Java 8 test is related to datastore, not BQ. Will proceed with submit.

@shollyman
shollyman merged commit 57ac06a into googleapis:masterMay 9, 2019
@shollymanshollyman removed 🚨 critical P0 critical issue. Requires immediate fix do not merge Indicates a pull request not ready for merge, due to either quality or timing. labels May 9, 2019
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

api: bigqueryIssues related to the BigQuery API.cla: yesThis human has signed the Contributor License Agreement.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@shollyman@tswast@googlebot@yoshi-automation