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

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

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
@googlebotgooglebot added the cla: yes This human has signed the Contributor License Agreement. label Jul 12, 2019
@shollyman
shollyman requested a review from tswastJuly 12, 2019 18:27
@codecov

codecovBot commented Jul 12, 2019

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 FilesCoverage ΔComplexity Δ
...src/main/java/com/google/cloud/bigquery/Model.java67.27% <ø> (ø)12 <0> (ø)⬇️
...main/java/com/google/cloud/bigquery/ModelInfo.java75.53% <81.81%> (-0.58%)21 <0> (ø)
...m/google/cloud/vision/v1/ImageAnnotatorClient.java58.69% <0%> (-3.31%)15% <0%> (-4%)
...e/cloud/vision/v1p4beta1/ImageAnnotatorClient.java58.69% <0%> (-3.31%)15% <0%> (-4%)
...e/cloud/vision/v1p3beta1/ImageAnnotatorClient.java47.05% <0%> (-2.95%)9% <0%> (-2%)
...e/cloud/vision/v1p2beta1/ImageAnnotatorClient.java47.05% <0%> (-2.95%)9% <0%> (-2%)
.../java/com/google/cloud/speech/v1/SpeechClient.java48.57% <0%> (-2.78%)10% <0%> (-2%)
...om/google/cloud/speech/v1p1beta1/SpeechClient.java48.57% <0%> (-2.78%)10% <0%> (-2%)
...d/webrisk/v1beta1/WebRiskServiceV1Beta1Client.java63.41% <0%> (-2.5%)12% <0%> (-3%)
...ogle/cloud/texttospeech/v1/TextToSpeechClient.java55.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.

@tswasttswast 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

Copy link
Copy Markdown
ContributorAuthor

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

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

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.

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.

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.

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 reviewJuly 18, 2019 17:43

comment addressed

@shollyman
shollyman merged commit 187fd3d into googleapis:masterJul 18, 2019
@shollyman
shollyman deleted the modelschange branch July 18, 2019 18:24
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

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@kolea2@googlebot