Skip to content

BigQuery: update signature for ML model access to label/feature columns. - #5732

Merged
shollyman merged 1 commit into
googleapis:masterfrom
shollyman:modelschange
Jul 18, 2019
Merged

BigQuery: update signature for ML model access to label/feature columns.#5732
shollyman merged 1 commit into
googleapis:masterfrom
shollyman:modelschange

Conversation

@shollyman

@shollyman shollyman commented Jul 12, 2019

Copy link
Copy Markdown
Contributor

This is a breaking change to a @BetaApi within BigQuery.

It swaps in a new veneer StandardSQLField type in place of the underlying
discovery type for getting information about the feature and label columns
from ML models, which was functionality introduced in:
#5214

@shollyman
BigQuery: update signature for ML model access to label/feature columns.
4d5b911 
This is a breaking change to a @BetaApi within BigQuery.

It swaps in a new veneer StandardSQLField type in place of the underlying
discovery type for getting information about the feature and label columns
from ML models, which was functionality introduced in:
googleapis#5214
@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Jul 12, 2019
@shollyman
shollyman requested a review from tswast July 12, 2019 18:27
@codecov

codecov Bot commented Jul 12, 2019 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

Merging #5732 into master will decrease coverage by 0.38%.
The diff coverage is 81.81%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #5732      +/-   ##
============================================
- Coverage     47.09%   46.71%   -0.39%     
+ Complexity    25063    23629    -1434     
============================================
  Files          2389     2389              
  Lines        259743   254614    -5129     
  Branches      29404    29257     -147     
============================================
- Hits         122320   118934    -3386     
+ Misses       128462   127626     -836     
+ Partials       8961     8054     -907
Impacted Files Coverage Δ Complexity Δ
...src/main/java/com/google/cloud/bigquery/Model.java 67.27% <ø> (ø) 12 <0> (ø) ⬇️
...main/java/com/google/cloud/bigquery/ModelInfo.java 75.53% <81.81%> (-0.58%) 21 <0> (ø)
...m/google/cloud/vision/v1/ImageAnnotatorClient.java 58.69% <0%> (-3.31%) 15% <0%> (-4%)
...e/cloud/vision/v1p4beta1/ImageAnnotatorClient.java 58.69% <0%> (-3.31%) 15% <0%> (-4%)
...e/cloud/vision/v1p3beta1/ImageAnnotatorClient.java 47.05% <0%> (-2.95%) 9% <0%> (-2%)
...e/cloud/vision/v1p2beta1/ImageAnnotatorClient.java 47.05% <0%> (-2.95%) 9% <0%> (-2%)
.../java/com/google/cloud/speech/v1/SpeechClient.java 48.57% <0%> (-2.78%) 10% <0%> (-2%)
...om/google/cloud/speech/v1p1beta1/SpeechClient.java 48.57% <0%> (-2.78%) 10% <0%> (-2%)
...d/webrisk/v1beta1/WebRiskServiceV1Beta1Client.java 63.41% <0%> (-2.5%) 12% <0%> (-3%)
...ogle/cloud/texttospeech/v1/TextToSpeechClient.java 55.88% <0%> (-2.46%) 9% <0%> (-2%)
... and 624 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 d66c22b...4d5b911. Read the comment docs.

tswast
tswast approved these changes Jul 17, 2019
View reviewed changes

@tswast tswast left a comment

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.

LGTM. No tests to update? I guess I didn't really check for all properties on the Models API stats, either.

@shollyman

shollyman commented Jul 17, 2019

Copy link
Copy Markdown
Contributor Author

LGTM. No tests to update? I guess I didn't really check for all properties on the Models API stats, either.

The tests accessed the data transitively without declaring the intermediate types. The types changed but the getter names didn't. Ex:

 assertEquals(model.getFeatureColumns().get(0).getName(), "f1");
 assertEquals(model.getLabelColumns().get(0).getName(), "predicted_label");

kolea2
kolea2 previously requested changes Jul 18, 2019
View reviewed changes
Comment thread google-cloud-clients/google-cloud-bigquery/src/main/java/com/google/cloud/bigquery/Model.java

@Override
Builder setLabelColumns(List<StandardSqlField> labelColumnList) {
Builder setLabelColumns(List<StandardSQLField> labelColumnList) {

@kolea2 kolea2 Jul 18, 2019

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.

Where is this annotated with @BetaApi? Just want to make sure we don't introduce a breaking change to something that wasn't labeled as such.

@shollyman shollyman Jul 18, 2019

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The feature represents readonly properties provided by the BQ backend, so I marked it as BetaApi on the public getters in ModelInfo. The internal builder was not annotated this way.

@kolea2 kolea2 Jul 18, 2019

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.

Ah, sorry didn't realize this wasn't a public method. SGTM.

@kolea2
kolea2 dismissed their stale review July 18, 2019 17:43

comment addressed

@shollyman
shollyman merged commit 187fd3d into googleapis:master Jul 18, 2019
@shollyman
shollyman deleted the modelschange branch July 18, 2019 18:24
@chingor13 chingor13 mentioned this pull request Jul 24, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@tswast tswast tswast approved these changes
+1 more reviewer
@kolea2 kolea2 kolea2 left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

cla: yes This human has signed the Contributor License Agreement.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@shollyman @tswast @kolea2 @googlebot