Skip to content

[PF-2195] Switch default BQ resolve delimiter and add a FULL_PATH_SQL option. #351

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.

Sign up for GitHub

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

Open
gjuggler wants to merge 3 commits into
base: main
Choose a base branch
from
Open

[PF-2195] Switch default BQ resolve delimiter and add a FULL_PATH_SQL option. #351

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
9e205a5
Switch default behavior and add a FULL_PATH_SQL option.
Nov 6, 2022
4867145
Fix tests.
Nov 6, 2022
d752dfa
Add controlled tests and remove unnecessary util method calls.
Nov 6, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 3 additions & 8 deletions src/main/java/bio/terra/cli/businessobject/resource/BqDataset.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,13 +23,6 @@
*/
public class BqDataset extends Resource {
private static final Logger logger = LoggerFactory.getLogger(BqDataset.class);
/**
* Delimiter between the project id and dataset id for a BigQuery dataset.
*
* <p>The choice is somewhat arbitrary. BigQuery Datatsets do not have true URIs. The '.'
* delimiter allows the path to be used directly in SQL calls with a BigQuery extension.
*/
private static final char BQ_PROJECT_DATASET_DELIMITER = '.';

private String projectId;
private String datasetId;
Expand Down Expand Up @@ -164,7 +157,9 @@ public String resolve() {
public String resolve(BqResolvedOptions resolveOption) {
switch (resolveOption) {
case FULL_PATH:
return projectId + BQ_PROJECT_DATASET_DELIMITER + datasetId;
return projectId + BqResolvedOptions.BQ_PROJECT_DELIMITER + datasetId;
case FULL_PATH_SQL:
return projectId + BqResolvedOptions.BQ_PROJECT_DELIMITER_SQL + datasetId;
case DATASET_ID_ONLY:
return datasetId;
case PROJECT_ID_ONLY:
Expand Down
20 changes: 14 additions & 6 deletions src/main/java/bio/terra/cli/businessobject/resource/BqResolvedOptions.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,16 +2,24 @@

/** This enum specifies the possible ways to resolve a BigQuery resource. */
public enum BqResolvedOptions {
FULL_PATH, // For data table: [project id].[dataset id].[data table id if applicable]
FULL_PATH, // For data table: [project id]:[dataset id].[data table id if applicable]
Copy link
Contributor

@melissachang melissachang Nov 8, 2022

Choose a reason for hiding this comment

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

Suggest FULL_PATH -> FULL_PATH_BQ and some comment changes

// project:dataset.table, used for bq CLI
FULL_PATH_BQ,
// project.dataset.table, used for SQL
FULL_PATH_SQL,

(I left out if applicable for brevity..)

FULL_PATH_SQL, // For data table: [project id].[dataset id].[data table id if applicable]
TABLE_ID_ONLY, // [data table id]
DATASET_ID_ONLY, // [dataset id]
PROJECT_ID_ONLY; // [project id]

/**
* Delimiter between the project id, dataset id and/or data table id.
*
* <p>The choice is somewhat arbitrary. BigQuery datasets or tables do not have true URIs. The '.'
* delimiter allows the path to be used directly in SQL calls with a BigQuery extension.
* Delimiter between the project id and dataset id when using the command-line compatible
* identifier format.
*/
public static final char BQ_PROJECT_DATA_TABLE_DELIMITER = '.';
public static final char BQ_PROJECT_DELIMITER = ':';

/**
* Delimiter between the project id and dataset id when using the SQL-compatible identifier
* format.
*/
public static final char BQ_PROJECT_DELIMITER_SQL = '.';

/** Delimiter between the dataset ID and data table. */
public static final char BQ_TABLE_DELIMITER = '.';
}
12 changes: 8 additions & 4 deletions src/main/java/bio/terra/cli/businessobject/resource/BqTable.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,5 @@
package bio.terra.cli.businessobject.resource;

import static bio.terra.cli.businessobject.resource.BqResolvedOptions.BQ_PROJECT_DATA_TABLE_DELIMITER;

import bio.terra.cli.businessobject.Context;
import bio.terra.cli.businessobject.Resource;
import bio.terra.cli.serialization.persisted.resource.PDBqTable;
Expand Down Expand Up @@ -133,9 +131,15 @@ public String resolve(BqResolvedOptions resolveOption) {
switch (resolveOption) {
case FULL_PATH:
return projectId
+ BQ_PROJECT_DATA_TABLE_DELIMITER
+ BqResolvedOptions.BQ_PROJECT_DELIMITER
+ datasetId
+ BqResolvedOptions.BQ_TABLE_DELIMITER
+ dataTableId;
case FULL_PATH_SQL:
return projectId
+ BqResolvedOptions.BQ_PROJECT_DELIMITER_SQL
+ datasetId
+ BQ_PROJECT_DATA_TABLE_DELIMITER
+ BqResolvedOptions.BQ_TABLE_DELIMITER
+ dataTableId;
case TABLE_ID_ONLY:
return dataTableId;
Expand Down
13 changes: 12 additions & 1 deletion src/test/java/harness/utils/ExternalBQDatasets.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -217,14 +217,25 @@ public static BigQuery getBQClient(GoogleCredentials credentials) {
.getService();
}

/** Gets cloud identifier for Dataset in full-path: [project id].[dataset id] */
/** Gets cloud identifier for Dataset in full-path: [project id]:[dataset id] */
public static String getDatasetFullPath(String projectId, String datasetId) {
Copy link
Contributor

@melissachang melissachang Nov 8, 2022

Choose a reason for hiding this comment

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

getDatasetFullPathBq ?

return projectId + ":" + datasetId;
}

/** Gets cloud identifier for Dataset in full-path SQL: [project id].[dataset id] */
public static String getDatasetFullPathSql(String projectId, String datasetId) {
return projectId + "." + datasetId;
}

/** Gets cloud identifier for data table in full-path: [project id].[dataset id].[table id] */
public static String getDataTableFullPath(
String projectId, String datasetId, String dataTableId) {
return projectId + ":" + datasetId + "." + dataTableId;
}

/** Gets cloud identifier for data table in full-path: [project id].[dataset id].[table id] */
public static String getDataTableFullPathSql(
String projectId, String datasetId, String dataTableId) {
return projectId + "." + datasetId + "." + dataTableId;
}

Expand Down
13 changes: 11 additions & 2 deletions src/test/java/unit/BqDatasetControlled.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -221,9 +221,18 @@ void resolve() throws IOException {
JSONObject resolved =
TestCommand.runAndGetJsonObjectExpectSuccess("resource", "resolve", "--name=" + name);
assertEquals(
workspace.googleProjectId + "." + datasetId,
workspace.googleProjectId + ":" + datasetId,
resolved.get(name),
"default resolve includes [project id].[dataset id]");
"default resolve includes [project id]:[dataset id]");

// `terra resource resolve --name=$name --format=json --bq-path=FULL_PATH_SQL`
JSONObject resolvedFullSql =
TestCommand.runAndGetJsonObjectExpectSuccess(
"resource", "resolve", "--name=" + name, "--bq-path=FULL_PATH_SQL");
assertEquals(
workspace.googleProjectId + "." + datasetId,
resolvedFullSql.get(name),
"sql-compatible path includes [project id].[dataset id]");

// `terra resource resolve --name=$name --bq-path=PROJECT_ID_ONLY --format=json`
JSONObject resolvedProjectIdOnly =
Expand Down
14 changes: 11 additions & 3 deletions src/test/java/unit/BqDatasetReferenced.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -196,10 +196,18 @@ void resolve() throws IOException {
JSONObject resolved =
TestCommand.runAndGetJsonObjectExpectSuccess("resource", "resolve", "--name=" + name);
assertEquals(
ExternalBQDatasets.getDatasetFullPath(
externalDataset.getProjectId(), externalDataset.getDatasetId()),
externalDataset.getProjectId() + ":" + externalDataset.getDatasetId(),
resolved.get(name),
"default resolve includes [project id].[dataset id]");
"default resolve includes [project id]:[dataset id]");

// `terra resource resolve --name=$name --bq-path=PROJECT_ID_ONLY --format=json`
JSONObject resolvedFullPathSql =
TestCommand.runAndGetJsonObjectExpectSuccess(
"resource", "resolve", "--name=" + name, "--bq-path=FULL_PATH_SQL");
assertEquals(
externalDataset.getProjectId() + "." + externalDataset.getDatasetId(),
resolvedFullPathSql.get(name),
"resolve with FULL_PATH_SQL includes [project id].[dataset id]");

// `terra resource resolve --name=$name --bq-path=PROJECT_ID_ONLY --format=json`
JSONObject resolvedProjectIdOnly =
Expand Down
22 changes: 19 additions & 3 deletions src/test/java/unit/BqTableReferenced.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -260,10 +260,26 @@ void resolve() throws IOException {
JSONObject resolved =
TestCommand.runAndGetJsonObjectExpectSuccess("resource", "resolve", "--name=" + name);
assertEquals(
ExternalBQDatasets.getDataTableFullPath(
externalDataset.getProjectId(), externalDataset.getDatasetId(), externalDataTableName),
externalDataset.getProjectId()
+ ":"
+ externalDataset.getDatasetId()
+ "."
+ externalDataTableName,
resolved.get(name),
"default resolve include full path");
"default resolve includes full path");

// `terra resource resolve --name=$name --format=json --bq-path=FULL_PATH_SQL`
JSONObject resolvedFullPathSql =
TestCommand.runAndGetJsonObjectExpectSuccess(
"resource", "resolve", "--name=" + name, "--bq-path=FULL_PATH_SQL");
assertEquals(
externalDataset.getProjectId()
+ "."
+ externalDataset.getDatasetId()
+ "."
+ externalDataTableName,
resolvedFullPathSql.get(name),
"sql-compatible resolve includes SQL delimeter");

// `terra resource resolve --name=$name --bq-path=PROJECT_ID_ONLY --format=json`
JSONObject resolvedProjectIdOnly =
Expand Down