feat(bigquery): add helper classes for cloud-client testing (1/3) #10034

alarconesparza · 2025-02-27T20:58:06Z

Description

Part of internal b/394478489

First part of bigquery cloud-client (1/3).

Create[Dataset,Table,View] and Delete[Dataset,Table] are required for testing the following samples (samples will be added in part 2 and 3).

Samples

Checklist

OremGLG

LGTM

iennae · 2025-03-03T20:42:12Z

/gemini review

gemini-code-assist

Code Review

The pull request introduces helper classes for cloud-client testing in BigQuery, specifically focusing on creating and deleting datasets, tables, and views. The code includes the necessary classes and integration tests to validate the functionality. Overall, the code is well-structured and addresses the requirements outlined in the pull request description. However, there are a few areas that could be improved for better clarity and maintainability.

Summary of Findings

TODO comments: The code contains TODO comments that should be addressed before merging. These comments indicate areas where the developer needs to replace placeholder values with actual values.
Redundant cleanup in tearDown methods: In the tearDown methods of the integration tests, the dataset deletion is performed even if the table deletion fails. This could lead to unnecessary operations and potential errors. It's better to ensure that table deletion is successful before attempting to delete the dataset.
Inconsistent naming: In CreateTableIT.java, the schema fields are named stringField and booleanField, while in CreateViewIT.java they are named timestampField, stringField, and booleanField. This inconsistency should be addressed for better readability and maintainability.

Merge Readiness

The pull request is almost ready for merging. However, the TODO comments should be addressed, and the redundant cleanup in the tearDown methods should be fixed. Additionally, the naming inconsistency in the schema fields should be resolved. Once these issues are addressed, the pull request can be merged. I am unable to approve this pull request, and recommend that others review and approve this code before merging.

bigquery/cloud-client/snippets/src/main/java/com/example/bigquery/CreateDataset.java

bigquery/cloud-client/snippets/src/main/java/com/example/bigquery/CreateTable.java

bigquery/cloud-client/snippets/src/main/java/com/example/bigquery/CreateView.java

bigquery/cloud-client/snippets/src/main/java/com/example/bigquery/DeleteDataset.java

bigquery/cloud-client/snippets/src/main/java/com/example/bigquery/DeleteTable.java

gemini-code-assist · 2025-03-03T20:42:42Z

bigquery/cloud-client/snippets/src/test/java/com/example/bigquery/CreateTableIT.java

+    DeleteTable.deleteTable(GOOGLE_CLOUD_PROJECT, datasetName, tableName);
+    DeleteDataset.deleteDataset(GOOGLE_CLOUD_PROJECT, datasetName);


Consider adding a try-catch block around the DeleteTable.deleteTable call to handle potential exceptions during table deletion. If table deletion fails, log the error and proceed with dataset deletion. This ensures that the dataset is deleted even if the table deletion fails.

Suggested change

DeleteTable.deleteTable(GOOGLE_CLOUD_PROJECT, datasetName, tableName);

DeleteDataset.deleteDataset(GOOGLE_CLOUD_PROJECT, datasetName);

try {

DeleteTable.deleteTable(GOOGLE_CLOUD_PROJECT, datasetName, tableName);

} catch (Exception e) {

log.log(Level.SEVERE, "Error deleting table: " + e.getMessage(), e);

}

+1. I would also put the deletion of the table inside of a try-catch block.

gemini-code-assist · 2025-03-03T20:42:42Z

bigquery/cloud-client/snippets/src/test/java/com/example/bigquery/CreateViewIT.java

+    // Clean up.
+    DeleteTable.deleteTable(GOOGLE_CLOUD_PROJECT, datasetName, viewName);
+    DeleteTable.deleteTable(GOOGLE_CLOUD_PROJECT, datasetName, tableName);


Consider adding a try-catch block around the DeleteTable.deleteTable calls to handle potential exceptions during table deletion. If table deletion fails, log the error and proceed with dataset deletion. This ensures that the dataset is deleted even if the table deletion fails.

try { DeleteTable.deleteTable(GOOGLE_CLOUD_PROJECT, datasetName, viewName); } catch (Exception e) { log.log(Level.SEVERE, "Error deleting view: " + e.getMessage(), e); } try { DeleteTable.deleteTable(GOOGLE_CLOUD_PROJECT, datasetName, tableName); } catch (Exception e) { log.log(Level.SEVERE, "Error deleting table: " + e.getMessage(), e); }

gemini-code-assist · 2025-03-03T20:42:42Z

bigquery/cloud-client/snippets/src/test/java/com/example/bigquery/CreateViewIT.java

+            Field.of("timestampField", StandardSQLTypeName.TIMESTAMP),
+            Field.of("stringField", StandardSQLTypeName.STRING),
+            Field.of("booleanField", StandardSQLTypeName.BOOL));


In CreateTableIT.java, the schema fields are named stringField and booleanField, while here they are named timestampField, stringField, and booleanField. This inconsistency should be addressed for better readability and maintainability. Consider using consistent naming conventions across all test classes.

Schema schema = Schema.of( Field.of("stringField", StandardSQLTypeName.STRING), Field.of("booleanField", StandardSQLTypeName.BOOL));

telpirion · 2025-03-04T19:13:53Z

bigquery/cloud-client/snippets/src/main/java/com/example/bigquery/CreateDataset.java

+
+package com.example.bigquery;
+
+import com.google.cloud.bigquery.BigQuery;


issue: insert region tags. Make sure that you use the same region tags as used in the same sample (create dataset) in other languages. It looks like Go uses bigquery_create_dataset.

See https://googlecloudplatform.github.io/samples-style-guide/#region-tags.

Right now, those samples (with region tags) are in a different repo, but I need the classes to test the samples in the list. I'm planning to migrate the region tags and use the samples in this PR and repo once the PRs 10034, 10035 and 10036 are merged as that migration is not part of the current scope.

Is this a good way to go?

Okay, that makes sense. I assume that there is a work item or bug that tracks adding region tags to these files?

bigquery/cloud-client/snippets/src/main/java/com/example/bigquery/CreateDataset.java

telpirion · 2025-03-04T19:21:43Z

bigquery/cloud-client/snippets/src/test/java/com/example/bigquery/CreateTableIT.java

+  @After
+  public void tearDown() {
+    // Clean up
+    DeleteTable.deleteTable(GOOGLE_CLOUD_PROJECT, datasetName, tableName);


issue: Use the BQ client to delete table and dataset, not the corresponding samples.

Although it is much easier to use the samples, unfortunately using the samples for clean up has some unwanted potential side effects. Specifically, if one of these samples start failing, then that will cause THIS test to start failing as well.

telpirion · 2025-03-04T19:22:22Z

bigquery/cloud-client/snippets/src/test/java/com/example/bigquery/CreateViewIT.java

+  @After
+  public void tearDown() {
+    // Clean up.
+    DeleteTable.deleteTable(GOOGLE_CLOUD_PROJECT, datasetName, viewName);


See previous comment about tearDown().

telpirion · 2025-03-04T19:24:59Z

bigquery/cloud-client/snippets/src/test/java/com/example/bigquery/CreateViewIT.java

+    CreateDataset.createDataset(GOOGLE_CLOUD_PROJECT, datasetName);
+
+    // Create temporary table.
+    tableName = "table_test_" + UUID.randomUUID().toString().substring(0, 8);


nit: add a preface like bigquery_ before the "table_test_" string.

These strings are crucial for identifying failing tests in the logs. We have multiple products that involve entities called "tables" and "datasets." When I'm scanning through the logs to see why a test failed, it's helpful if the name of the resource that caused the failure helps me pinpoint which service or set of tests the failure originates from.

@alarconesparza as discussed internally, we could use a prefixer like Python does:
conftest.py#L22

Do we have an equivalent to the test_utils on the Java testing system? The prefix on Python looks similar to java-docs-samples-bigquery-cloud-client plus a timestamp.

telpirion · 2025-03-04T19:26:44Z

bigquery/cloud-client/snippets/src/test/java/com/example/bigquery/DeleteDatasetIT.java

+
+    // Create temporary dataset.
+    datasetName = RemoteBigQueryHelper.generateDatasetName();
+    CreateDataset.createDataset(GOOGLE_CLOUD_PROJECT, datasetName);


Same principle applies here as my previous comment about using samples for tearDown().

Using samples in test setUp() and tearDown() can cause a cascading set of test failures.

telpirion · 2025-03-04T19:27:15Z

bigquery/cloud-client/snippets/src/test/java/com/example/bigquery/DeleteTableIT.java

+
+    // Create temporary dataset.
+    datasetName = RemoteBigQueryHelper.generateDatasetName();
+    CreateDataset.createDataset(GOOGLE_CLOUD_PROJECT, datasetName);


Same comment as above about using samples in setUp() and tearDown().

telpirion · 2025-03-07T17:58:33Z

bigquery/cloud-client/snippets/src/main/java/com/example/bigquery/CreateDataset.java

+
+package com.example.bigquery;
+
+import com.google.cloud.bigquery.BigQuery;


Okay, that makes sense. I assume that there is a work item or bug that tracks adding region tags to these files?

telpirion · 2025-03-10T20:46:32Z

bigquery/cloud-client/snippets/src/test/java/com/example/bigquery/CreateTableIT.java

+    DeleteTable.deleteTable(GOOGLE_CLOUD_PROJECT, datasetName, tableName);
+    DeleteDataset.deleteDataset(GOOGLE_CLOUD_PROJECT, datasetName);


+1. I would also put the deletion of the table inside of a try-catch block.

feat(bigquery): add helper classes for cloud-client testing

843635d

product-auto-label bot added api: bigquery Issues related to the BigQuery API. samples Issues that are directly related to samples. labels Feb 27, 2025

alarconesparza marked this pull request as ready for review February 27, 2025 21:04

alarconesparza requested review from yoshi-approver and a team as code owners February 27, 2025 21:04

blunderbuss-gcf bot assigned kweinmeister Feb 27, 2025

alarconesparza assigned telpirion and unassigned kweinmeister Feb 27, 2025

alarconesparza changed the title ~~feat(bigquery): add helper classes for cloud-client testing~~ feat(bigquery): add helper classes for cloud-client testing (1/3) Feb 28, 2025

alarconesparza requested a review from telpirion February 28, 2025 18:31

OremGLG self-requested a review March 3, 2025 17:22

OremGLG reviewed Mar 3, 2025

View reviewed changes

OremGLG approved these changes Mar 3, 2025

View reviewed changes

gemini-code-assist bot suggested changes Mar 3, 2025

View reviewed changes

This was referenced Mar 3, 2025

feat(bigquery): add dataset samples to cloud-client (2/3) #10035

Open

feat(bigquery): add table/view samples to cloud-client (3/3) #10036

Open

feat(bigquery): create cloud-client samples #10033

Draft

use consistent names and fields

08ee8de

telpirion assigned alarconesparza and eapl-gemugami and unassigned alarconesparza Mar 4, 2025

telpirion reviewed Mar 4, 2025

View reviewed changes

telpirion requested changes Mar 4, 2025

View reviewed changes

add Util class for testing helpers

398b45c

telpirion approved these changes Mar 11, 2025

View reviewed changes

telpirion merged commit 1975c4c into GoogleCloudPlatform:main Mar 11, 2025
7 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(bigquery): add helper classes for cloud-client testing (1/3) #10034

feat(bigquery): add helper classes for cloud-client testing (1/3) #10034

alarconesparza commented Feb 27, 2025 •

edited

Loading

OremGLG left a comment

iennae commented Mar 3, 2025

gemini-code-assist bot left a comment

gemini-code-assist bot Mar 3, 2025

telpirion Mar 10, 2025

gemini-code-assist bot Mar 3, 2025

gemini-code-assist bot Mar 3, 2025

telpirion Mar 4, 2025

alarconesparza Mar 4, 2025

telpirion Mar 7, 2025

telpirion Mar 4, 2025

telpirion Mar 4, 2025

telpirion Mar 4, 2025

eapl-gemugami Mar 4, 2025 •

edited

Loading

telpirion Mar 4, 2025

telpirion Mar 4, 2025

telpirion Mar 7, 2025

telpirion Mar 10, 2025

		DeleteTable.deleteTable(GOOGLE_CLOUD_PROJECT, datasetName, tableName);
		DeleteDataset.deleteDataset(GOOGLE_CLOUD_PROJECT, datasetName);


		package com.example.bigquery;

		import com.google.cloud.bigquery.BigQuery;

feat(bigquery): add helper classes for cloud-client testing (1/3) #10034

feat(bigquery): add helper classes for cloud-client testing (1/3) #10034

Conversation

alarconesparza commented Feb 27, 2025 • edited Loading

Description

First part of bigquery cloud-client (1/3).

Samples

Checklist

OremGLG left a comment

Choose a reason for hiding this comment

iennae commented Mar 3, 2025

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Summary of Findings

Merge Readiness

gemini-code-assist bot Mar 3, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gemini-code-assist bot Mar 3, 2025

Choose a reason for hiding this comment

gemini-code-assist bot Mar 3, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eapl-gemugami Mar 4, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alarconesparza commented Feb 27, 2025 •

edited

Loading

eapl-gemugami Mar 4, 2025 •

edited

Loading