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

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

Merged
merged 2 commits into from Jun 8, 2020

Conversation

stephaniewang526
Copy link
Member

@stephaniewang526 stephaniewang526 commented Jun 8, 2020

Fixes #413

@googlebot googlebot added the cla: yes label Jun 8, 2020
@stephaniewang526 stephaniewang526 requested a review from kurtisvg Jun 8, 2020
private ByteArrayOutputStream bout;
private PrintStream out;

private static final String BIGQUERY_DATASET_NAME = System.getenv("BIGQUERY_DATASET_NAME");
Copy link
Contributor

@kurtisvg kurtisvg Jun 8, 2020

Choose a reason for hiding this comment

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

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

Copy link
Member Author

@stephaniewang526 stephaniewang526 Jun 8, 2020

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@kurtisvg kurtisvg Jun 8, 2020

Choose a reason for hiding this comment

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

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)

Copy link
Member Author

@stephaniewang526 stephaniewang526 Jun 8, 2020

Choose a reason for hiding this comment

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

okay makes sense - done

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);
Copy link
Contributor

@kurtisvg kurtisvg Jun 8, 2020

Choose a reason for hiding this comment

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

Should this be set up?

Copy link
Member Author

@stephaniewang526 stephaniewang526 Jun 8, 2020

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@kurtisvg kurtisvg Jun 8, 2020

Choose a reason for hiding this comment

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

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)

Copy link
Member Author

@stephaniewang526 stephaniewang526 Jun 8, 2020

Choose a reason for hiding this comment

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

done

assertThat(bout.toString()).contains("Table updated successfully using DML");

// Clean up
DeleteTable.deleteTable(BIGQUERY_DATASET_NAME, tableName);
Copy link
Contributor

@kurtisvg kurtisvg Jun 8, 2020

Choose a reason for hiding this comment

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

Should this be teardown?

Copy link
Member Author

@stephaniewang526 stephaniewang526 Jun 8, 2020

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@kurtisvg kurtisvg Jun 8, 2020

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

@stephaniewang526 stephaniewang526 Jun 8, 2020

Choose a reason for hiding this comment

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

done

@codecov
Copy link

@codecov codecov bot commented Jun 8, 2020

Codecov Report

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

Impacted file tree graph

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

@stephaniewang526 stephaniewang526 requested a review from kurtisvg Jun 8, 2020
@stephaniewang526 stephaniewang526 added the automerge label Jun 8, 2020
@gcf-merge-on-green gcf-merge-on-green bot merged commit 3902ba1 into googleapis:master Jun 8, 2020
15 checks passed
gcf-merge-on-green bot pushed a commit that referenced this issue Jun 9, 2020
🤖 I have created a release \*beep\* \*boop\* 
---
### [1.116.2](https://www.github.com/googleapis/java-bigquery/compare/v1.116.1...v1.116.2) (2020-06-09)


### Documentation

* **samples:** add load CSV from GCS sample ([#426](https://www.github.com/googleapis/java-bigquery/issues/426)) ([3810366](https://www.github.com/googleapis/java-bigquery/commit/3810366451097a7f14db11504103865540ac242a))
* **samples:** add load CSV from GCS to overwrite table sample ([#428](https://www.github.com/googleapis/java-bigquery/issues/428)) ([21a3606](https://www.github.com/googleapis/java-bigquery/commit/21a3606f5fb65287f808b12a6fef65817c8a8ba6))
* **samples:** add update table using dml query sample ([#424](https://www.github.com/googleapis/java-bigquery/issues/424)) ([3902ba1](https://www.github.com/googleapis/java-bigquery/commit/3902ba1cf0d8a88d3e6f30b6606067503487c77d)), closes [#413](https://www.github.com/googleapis/java-bigquery/issues/413)
* **samples:** added copy table and accompanying test ([#414](https://www.github.com/googleapis/java-bigquery/issues/414)) ([de0d97f](https://www.github.com/googleapis/java-bigquery/commit/de0d97f2f940c9cf507d19c5595e1a0e819ef19c))
* **samples:** added extract to json and accompanying test ([#416](https://www.github.com/googleapis/java-bigquery/issues/416)) ([16a956d](https://www.github.com/googleapis/java-bigquery/commit/16a956db0aa545df84f7885ffb4425460cf55a16))
* **samples:** adding browse table sample and test ([#422](https://www.github.com/googleapis/java-bigquery/issues/422)) ([dff4e5f](https://www.github.com/googleapis/java-bigquery/commit/dff4e5f86764b1c779c2ef131182483e2ffa1c1b))
* **samples:** adding destination query sample and test ([#418](https://www.github.com/googleapis/java-bigquery/issues/418)) ([0f50961](https://www.github.com/googleapis/java-bigquery/commit/0f50961aaf1092f3ecc4e02fa9cebb50f6d45e90))
* **samples:** adding simple query example for completeness ([#417](https://www.github.com/googleapis/java-bigquery/issues/417)) ([59426df](https://www.github.com/googleapis/java-bigquery/commit/59426df912f743b7927deb562366b625aba6f087))
* **samples:** rename extract table json to extract table csv ([#415](https://www.github.com/googleapis/java-bigquery/issues/415)) ([c1f21e6](https://www.github.com/googleapis/java-bigquery/commit/c1f21e6c16df40bb3c71610f9d5b4fb4855b28fb))


### Dependencies

* update dependency com.google.apis:google-api-services-bigquery to v2-rev20200523-1.30.9 ([#409](https://www.github.com/googleapis/java-bigquery/issues/409)) ([d30c823](https://www.github.com/googleapis/java-bigquery/commit/d30c823c5a604b195f17d8ac33894107cdee967e))
---


This PR was generated with [Release Please](https://github.com/googleapis/release-please).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge cla: yes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants