Implement missing features and fix found bugs for BigQuery Client Library by vam-google · Pull Request #2446 · googleapis/google-cloud-java

vam-google · 2017-09-15T23:16:01Z

Access query results row cells by name. Now row.get(1) and get("firstColumnName") are supported (currently supported only for results returned by BigQuery.query() and not supported by results returned BigQuery.listTableData() because there is no schema and it would require additional request to the server to get schema (but still can be added in the future easily with cost of an extra network call)). Same syntax is supported for record-type fields (i.e. nested fields can be accessed by name too).
Refactor BigQuery.query() to use jobs.insert/jobs.getQueryResults combination instead of jobs.query. QueryRequest class was removed, now query() method directly accepts QueryJobConfiguration instead. The remaining properties of removed QueryRequest class now are passed in a form of QueryOption vararg argument. QueryOption combines query and waiting options (waiting options are necessary because query() is now waiting for query to complete).
Make BigQuery.query() call synchronous (waits for query completion before return result), with jittered exponential backoff for polling (based on gax.retrying package)
Add client side job id generation (JobId.of())
Replace WaitForOption with RetryOption (based on RetrySettings and is consistent with the rest of the codebase). This affected compute and spanner clients too.
Rewrite Job.wait() to use jittered exponentiall backoff polling (based ongax.retrying package). Polling is performed differently depending on the type of job: for query jobs on each poll getQueryResults() is called, for all other job types getJob() is used instead.
Use standard SQL as default for all queries
Fix wrong query results iteration samples code used all over the place in documentation.
Various smaller changes/refactorings

…rary: 0) Access query results row cells by name. Now both row.get(1) and get("firstColumnName") are supported (currently supported only for results returned by BigQuery.query() and not supported by results returned BigQuery.listTableData(), because there is not schema and it would require additional request to the server to get schema (still can be added in the future easily)). 1) Rewrite BigQuery.query() to use jobs.insert/jobs.getQueryResults combination instead of jobs.query 2) BigQuery.query() call is synchronous, with jittered exponential backoff for polling (based on gax.retrying package) 3) Client side job id generation (JobId.of()) 4) Replace WaitForOption with RetryOption (based on RetrySettings and is consistent with the rest of the codebase) 5) Rewrite Job.wait() to use jittered exponentiall backoff polling (based on gax.retrying package) 6) Use standard SQL as default for all queries 7) Various smaller changes/refactorings

vam-google · 2017-09-15T23:17:08Z

@vkedia please review spanner's Operation.waitFor(), as it was rewrittent as part of this PR.

pongad

This is a little big for me to properly review, but I don't see anything wrong apart from a couple of nits noted.

google-cloud-bigquery/src/main/java/com/google/cloud/bigquery/BigQuery.java

+      return option instanceof RetryOption ? (RetryOption) option : null;
+    }
+
+    public static QueryResultsOption[] filterQueryResutlsOptions(QueryOption... options) {


google-cloud-bigquery/src/main/java/com/google/cloud/bigquery/BigQueryImpl.java

      String cursor = result.x();
      return new PageImpl<>(new TableDataPageFetcher(tableId, serviceOptions, cursor, optionsMap),
-          cursor, transformTableData(result.y()));
+          cursor, transformTableData(result.y(), null));


...oud-examples/src/main/java/com/google/cloud/examples/bigquery/snippets/BigQuerySnippets.java

-      response = bigquery.getQueryResults(response.getJobId());
-    }
+    QueryJobConfiguration queryConfig =
+        QueryJobConfiguration.newBuilder(query).setUseLegacySql(true).build();


...oud-examples/src/main/java/com/google/cloud/examples/bigquery/snippets/BigQuerySnippets.java

    // [START runQueryWithParameters]
-    QueryRequest request = QueryRequest.newBuilder(query)
+    QueryJobConfiguration queryConfig = QueryJobConfiguration.newBuilder(query)
        .setUseLegacySql(false) // standard SQL is required to use query parameters


...mples/src/main/java/com/google/cloud/examples/bigquery/snippets/InsertDataAndQueryTable.java

-            .setPageSize(1000L)
-            .build();
+    QueryJobConfiguration queryConfig =
+        QueryJobConfiguration.newBuilder("SELECT * FROM my_dataset_id.my_table_id").build();


google-cloud-spanner/src/main/java/com/google/cloud/spanner/Operation.java

+                if (!(prevThrowable instanceof SpannerException)) {
+                  return ((BaseServiceException) prevThrowable).isRetryable();
+                }
+                return ((SpannerException) prevThrowable).getErrorCode() != ErrorCode.NOT_FOUND;


google-cloud-bigquery/src/main/java/com/google/cloud/bigquery/Field.java

-      this.type = checkNotNull(type);
-      return this;
+    public Builder setType(LegacySQLTypeName type, Field... subFields) {
+      return setType(type, subFields.length > 0 ? Fields.of(subFields) : null);


google-cloud-bigquery/src/main/java/com/google/cloud/bigquery/Fields.java

+    ImmutableMap.Builder<String, Integer> nameIndexBuilder = ImmutableMap.builder();
+    int index = 0;
+    for (Field field : fields) {
+      nameIndexBuilder.put(field.getName(), index++);


google-cloud-bigquery/src/test/java/com/google/cloud/bigquery/FieldValueTest.java

  @Test
  public void testFromPb() {
-    FieldValue value = FieldValue.fromPb(BOOLEAN_FIELD);
+    FieldValue value = FieldValue.fromPb(BOOLEAN_FIELD, null);


google-cloud-spanner/src/main/java/com/google/cloud/spanner/Operation.java

+                return !prevResponse.getDone();
+              }
+              if (prevThrowable instanceof BaseServiceException) {
+                if (!(prevThrowable instanceof SpannerException)) {


The logic was simplified and modified to match exactly the original one. Note, this does not allow to retry on any retriable BaseServiceExceptions, if they are not SpannerException (on practice here each BaseServiceException is always also a SpannerException).

vam-google · 2017-09-30T00:01:26Z

Addressed review feedback. PTAL.

@vkedia I fixed the retry logic (sorry for for breaking it). Hopefully now it is correct (matches exactly the original one in functionality). Please note that it theoretically may refuse to retry a retriable BaseServiceException, which is retriable but is not a SpannerException. This is only a theoretical concern, since all BaseServiceExceptions in this context will always be SpanneExceptions too.
Also, please note that with this change all unknown unchecked exceptions will also be wrapped in SpannerException (different from the original code). I think it makes sense here. Please let me know if that should be changed too.

google-cloud-bigquery/src/main/java/com/google/cloud/bigquery/BigQuery.java

+      return option instanceof RetryOption ? (RetryOption) option : null;
+    }
+
+    public static QueryResultsOption[] filterQueryResutlsOptions(QueryOption... options) {


google-cloud-bigquery/src/main/java/com/google/cloud/bigquery/BigQuery.java

+   * String query = "SELECT distinct(corpus) FROM `bigquery-public-data.samples.shakespeare`";
+   * QueryJobConfiguration queryConfig = QueryJobConfiguration.of(query);
+   *
+   * // To run the legacy syntax queries use the following code instead:


google-cloud-bigquery/src/main/java/com/google/cloud/bigquery/FieldValues.java

+ * may or may not contain related schema. If schema is not provided, the individual cells of the row
+ * will still be accessible by index but not by name.
+ */
+public class FieldValues extends AbstractList<FieldValue> implements Serializable {


google-cloud-bigquery/src/main/java/com/google/cloud/bigquery/Fields.java

+ * Google BigQuery Table schema fields (columns). Each field has a unique name and index. Fields
+ * with duplicate names are not allowed in BigQuery schema.
+ */
+public final class Fields extends AbstractList<Field> implements Serializable {


garrettjonesgoogle · 2017-10-03T21:51:55Z

LGTM

vam-google requested review from garrettjonesgoogle, pongad, tswast and vkedia as code owners September 15, 2017 23:16

googlebot added the cla: yes This human has signed the Contributor License Agreement. label Sep 15, 2017

Fix minor issues found by codacy-bot

0647bea

pongad reviewed Sep 18, 2017

View reviewed changes

tswast reviewed Sep 18, 2017

View reviewed changes

vkedia reviewed Sep 21, 2017

View reviewed changes

garrettjonesgoogle reviewed Sep 27, 2017

View reviewed changes

vkedia reviewed Sep 29, 2017

View reviewed changes

vam-google added 3 commits September 29, 2017 15:48

Address review feedback

9253994

Merge remote-tracking branch 'upstream/master'

8bd2cb2

Fix spanner retry logic.

d171824

The logic was simplified and modified to match exactly the original one. Note, this does not allow to retry on any retriable BaseServiceExceptions, if they are not SpannerException (on practice here each BaseServiceException is always also a SpannerException).

garrettjonesgoogle reviewed Oct 2, 2017

View reviewed changes

vkedia approved these changes Oct 3, 2017

View reviewed changes

vam-google added 2 commits October 3, 2017 14:36

Address review feedback

ba1954f

Address review feedback

522c5d7

vam-google merged commit 1fefc4c into googleapis:master Oct 3, 2017

Conversation

vam-google commented Sep 15, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vam-google commented Sep 15, 2017

Uh oh!

pongad left a comment

Choose a reason for hiding this comment

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

vam-google commented Sep 30, 2017

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

garrettjonesgoogle commented Oct 3, 2017

Uh oh!

Reviewers

Assignees

Labels

vam-google commented Sep 15, 2017 •

edited

Loading