Skip to content

DT-1701: Improve Snapshot by Query error message #1974

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
wants to merge 9 commits into
base: develop
Choose a base branch
from
Open

DT-1701: Improve Snapshot by Query error message #1974

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
946acb7
Add exception message if column or table names are not correctly qual…
snf2ye May 24, 2025
547e7a4
Add error message if user does not correctly qualify the column and t…
snf2ye May 27, 2025
7879be1
Table names that are not qualified are caught at the parse step, not …
snf2ye May 29, 2025
c6d7c24
Add visible for testing flag
snf2ye May 29, 2025
d282056
invert if/else
snf2ye May 29, 2025
1fb8927
Update src/main/java/bio/terra/grammar/google/BigQueryVisitor.java
snf2ye May 29, 2025
2717fc1
Add column name in question to error message
snf2ye May 29, 2025
24d109e
Merge branch 'sh/add-error-handling-by-query' of https://github.com/D…
snf2ye May 29, 2025
cfbc2e9
update references to static method
snf2ye May 29, 2025
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
17 changes: 14 additions & 3 deletions src/main/java/bio/terra/grammar/google/BigQueryVisitor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,11 @@
import bio.terra.common.PdaoConstant;
import bio.terra.grammar.DatasetAwareVisitor;
import bio.terra.grammar.SQLParser;
import bio.terra.grammar.exception.InvalidQueryException;
import bio.terra.model.DatasetModel;
import bio.terra.model.SnapshotModel;
import bio.terra.service.snapshotbuilder.query.TableNameGenerator;
import com.google.common.annotations.VisibleForTesting;
import java.util.Map;
import java.util.Objects;

Expand All @@ -15,7 +17,8 @@ public BigQueryVisitor(Map<String, DatasetModel> datasetMap) {
super(datasetMap);
}

public String generateAlias(String datasetName, String tableName) {
@VisibleForTesting
public static String generateAlias(String datasetName, String tableName) {
return "alias" + Math.abs(Objects.hash(datasetName, tableName));
}

Expand All @@ -37,9 +40,17 @@ private static String generateTableName(String dataProject, String name, String

@Override
public String visitColumn_expr(SQLParser.Column_exprContext ctx) {
String bqDatasetName = PdaoConstant.PDAO_PREFIX + getNameFromContext(ctx.dataset_name());
String datasetName = getNameFromContext(ctx.dataset_name());
String tableName = getNameFromContext(ctx.table_name());
String alias = generateAlias(bqDatasetName, tableName);
if (tableName == null) {
throw new InvalidQueryException(
"All column names must be qualified with a dataset and table name. "
+ "Please ensure that your query uses the format `dataset.table.column` for columns. "
+ "For example, use `my_dataset.my_table.my_column` instead of just `my_column`."
+ "Unqualified column name: "
+ getNameFromContext(ctx.column_name()));
}
String alias = generateAlias(prefixDatasetName(datasetName), tableName);
String columnName = getNameFromContext(ctx.column_name());
return String.format("`%s`.%s", alias, columnName);
}
Expand Down
55 changes: 50 additions & 5 deletions src/test/java/bio/terra/grammar/GrammarTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
import static org.hamcrest.CoreMatchers.hasItems;
import static org.hamcrest.MatcherAssert.assertThat;
import static org.hamcrest.Matchers.containsInAnyOrder;
import static org.hamcrest.Matchers.containsString;
import static org.hamcrest.Matchers.equalTo;
import static org.hamcrest.Matchers.equalToCompressingWhiteSpace;
import static org.junit.jupiter.api.Assertions.assertThrows;
Expand Down Expand Up @@ -220,7 +221,7 @@ void testBqTranslate() {
Query.parse(
"SELECT dataset.table.datarepo_row_id FROM dataset.table WHERE dataset.table.x = 'string'");
String translated = query.translateSql(bqVisitor);
String aliasedTableName = bqVisitor.generateAlias(bqDatasetName, tableName);
String aliasedTableName = BigQueryVisitor.generateAlias(bqDatasetName, tableName);
assertThat(
"query translates to valid bigquery syntax",
translated,
Expand All @@ -237,6 +238,50 @@ void testBqTranslate() {
+ "`.x = 'string'"));
}

@ParameterizedTest
@MethodSource
void testRequiredQualifiedNameOnQueryTranslation(
String testQuery, boolean queryHasQualifiedNames) {
BigQueryVisitor bqVisitor = new BigQueryVisitor(datasetMap);
Query parsedQuery = Query.parse(testQuery);
if (queryHasQualifiedNames) {
String bqDatasetName = PdaoConstant.PDAO_PREFIX + "dataset";
String tableName = "table";
String translated = parsedQuery.translateSql(bqVisitor);
String aliasedTableName = BigQueryVisitor.generateAlias(bqDatasetName, tableName);
assertThat(
"query translates to valid bigquery syntax",
translated,
containsString(aliasedTableName));
} else {
assertThrows(
InvalidQueryException.class,
() -> parsedQuery.translateSql(bqVisitor),
"All column names must be qualified with a dataset and table name. ");
}
}

static Stream<Arguments> testRequiredQualifiedNameOnQueryTranslation() {
return Stream.of(
Arguments.of(
"SELECT dataset.table.datarepo_row_id FROM dataset.table WHERE dataset.table.x = 'string'",
true),
Arguments.of(
"SELECT datarepo_row_id FROM dataset.table WHERE dataset.table.x = 'string'", false),
Arguments.of("SELECT * FROM dataset.table WHERE dataset.table.x = 'string'", true),
Arguments.of("SELECT datarepo_row_id FROM dataset.table WHERE x = 'string'", false));
}

@Test
void testRequiredQualifiedDatasetName() {
assertThrows(
InvalidQueryException.class,
() ->
Query.parse(
"SELECT dataset.table.datarepo_row_id FROM table WHERE dataset.table.x = 'string'"),
"All table names must be qualified with a dataset name.");
}

@Test
void testBqTranslateTwoTables() {
BigQueryVisitor bqVisitor = new BigQueryVisitor(datasetMap);
Expand All @@ -252,8 +297,8 @@ void testBqTranslateTwoTables() {
Query.parse(
"SELECT foo.bar.datarepo_row_id FROM foo.bar, baz.quux WHERE foo.bar.x = baz.quux.y");
String translated = query.translateSql(bqVisitor);
String aliasedTable1Name = bqVisitor.generateAlias(bqDataset1Name, table1Name);
String aliasedTable2Name = bqVisitor.generateAlias(bqDataset2Name, table2Name);
String aliasedTable1Name = BigQueryVisitor.generateAlias(bqDataset1Name, table1Name);
String aliasedTable2Name = BigQueryVisitor.generateAlias(bqDataset2Name, table2Name);
assertThat(
"query translates to valid bigquery syntax",
translated,
Expand Down Expand Up @@ -293,8 +338,8 @@ void testBqTranslateTwoTablesOrdered() {
Query.parse(
"SELECT baz.quux.datarepo_row_id FROM foo.bar, baz.quux WHERE foo.bar.x = baz.quux.y");
String translated = query.translateSql(bqVisitor);
String aliasedTable1Name = bqVisitor.generateAlias(bqDataset1Name, table1Name);
String aliasedTable2Name = bqVisitor.generateAlias(bqDataset2Name, table2Name);
String aliasedTable1Name = BigQueryVisitor.generateAlias(bqDataset1Name, table1Name);
String aliasedTable2Name = BigQueryVisitor.generateAlias(bqDataset2Name, table2Name);
assertThat(
"query translates to valid bigquery syntax",
translated,
Expand Down