docs(samples): add update table using dml query sample by stephaniewang526 · Pull Request #424 · googleapis/java-bigquery

stephaniewang526 · 2020-06-08T17:28:50Z

Fixes #413

kurtisvg · 2020-06-08T17:34:23Z

samples/snippets/src/test/java/com/example/bigquery/UpdateTableDMLIT.java

+  private ByteArrayOutputStream bout;
+  private PrintStream out;
+
+  private static final String BIGQUERY_DATASET_NAME = System.getenv("BIGQUERY_DATASET_NAME");


You are using getenv here, but also have requireEnvVar below. Maybe use requireEnvVar here but have it to return a value?

this is how we have been handling this in all the samples in BigQuery... is there a new standard?

You're right, this seems to be the same as being used in other places. However, I think maybe this pattern might be improved if we did something like this instead;

private static final String BIGQUERY_DATASET_NAME = requireEnvVar("BIGQUERY_DATASET_NAME"); private static string requireEnvVar(String varName) { String value = System.getenv(varName); assertNotNull( "Environment variable " + varName + " is required to perform these tests.", value); return value; }

This way we only have to specify "BIGQUERY_DATASET_NAME" in one pace.

(consider this one a nit)

okay makes sense - done

kurtisvg · 2020-06-08T17:34:59Z

samples/snippets/src/test/java/com/example/bigquery/UpdateTableDMLIT.java

+    String tableName = "UserSessions_TEST_" + UUID.randomUUID().toString().replace('-', '_');
+    Schema schema =
+        Schema.of(
+            Field.of("id", LegacySQLTypeName.STRING),
+            Field.of("user_id", LegacySQLTypeName.STRING),
+            Field.of("login_time", LegacySQLTypeName.STRING),
+            Field.of("logout_time", LegacySQLTypeName.STRING),
+            Field.of("ip_address", LegacySQLTypeName.STRING));
+
+    CreateTable.createTable(BIGQUERY_DATASET_NAME, tableName, schema);


Should this be set up?

We have always done this in the test method in all the other similar samples

So this one isn't a big deal, but logically it does make sense that "setUp" call since it's not directly related to a test.

(consider this one a nit)

kurtisvg · 2020-06-08T17:35:10Z

samples/snippets/src/test/java/com/example/bigquery/UpdateTableDMLIT.java

+    assertThat(bout.toString()).contains("Table updated successfully using DML");
+
+    // Clean up
+    DeleteTable.deleteTable(BIGQUERY_DATASET_NAME, tableName);


Should this be teardown?

this one also we have been doing this in the method itself...

This one is more of a concern - if any exception is thrown before this this step won't be executed. This means failures will leak resources.

codecov · 2020-06-08T17:36:14Z

Codecov Report

Merging #424 into master will not change coverage.
The diff coverage is n/a.

@@            Coverage Diff            @@
##             master     #424   +/-   ##
=========================================
  Coverage     81.36%   81.36%           
  Complexity     1225     1225           
=========================================
  Files            77       77           
  Lines          6218     6218           
  Branches        690      691    +1     
=========================================
  Hits           5059     5059           
  Misses          802      802           
  Partials        357      357

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 b45897a...6184779. Read the comment docs.

docs(samples): add update table using dml query sample

7e7537f

googlebot added the cla: yes This human has signed the Contributor License Agreement. label Jun 8, 2020

stephaniewang526 requested a review from kurtisvg June 8, 2020 17:29

kurtisvg suggested changes Jun 8, 2020

View reviewed changes

stephaniewang526 requested a review from kurtisvg June 8, 2020 17:39

stephaniewang526 added the automerge Merge the pull request once unit tests and other checks pass. label Jun 8, 2020

update based on comments

6184779

kurtisvg approved these changes Jun 8, 2020

View reviewed changes

gcf-merge-on-green bot merged commit 3902ba1 into googleapis:master Jun 8, 2020

Conversation

stephaniewang526 commented Jun 8, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Jun 8, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

codecov bot commented Jun 8, 2020 •

edited

Loading