Repository navigation
Conversation
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces support for discovering Property Graphs and related metadata in the BigQuery JDBC driver via a new EnableGraphMetadata connection property. It updates BigQueryDatabaseMetaData to query INFORMATION_SCHEMA.PROPERTY_GRAPHS and expose these as 'GRAPH TABLE' types in getTables and getTableTypes. The code review feedback suggests two key improvements: dynamically padding the table metadata values with nulls to match the schema size rather than hardcoding five nulls, and gracefully catching and skipping projects on 'Permission denied' or 'Access denied' errors when querying metadata.
|
/gcbrun |
…cloud-java into jdbc/graph-get-tables
| private static final String[] DEFAULT_TABLE_TYPES = { | ||
| "EXTERNAL", "MATERIALIZED VIEW", "SNAPSHOT", "TABLE", "VIEW" | ||
| }; | ||
| private static final String[] TABLE_TYPES_WITH_GRAPH = { |
There was a problem hiding this comment.
nit: can we update it to be DEFAULT_TABLE_TYPES + {"GRAPH TABLE"} in some way?
| JSON_VALUE(t, '$.dataSourceTable.projectId'), '.', | ||
| JSON_VALUE(t, '$.dataSourceTable.datasetId'), '.', | ||
| JSON_VALUE(t, '$.dataSourceTable.tableId')) AS SOURCE_TABLE | ||
| FROM %1$s.%2$s.INFORMATION_SCHEMA.PROPERTY_GRAPHS, |
There was a problem hiding this comment.
Add escape ``` characters for the table name
| JSON_VALUE(t, '$.dataSourceTable.projectId'), '.', | ||
| JSON_VALUE(t, '$.dataSourceTable.datasetId'), '.', | ||
| JSON_VALUE(t, '$.dataSourceTable.tableId')) AS SOURCE_TABLE | ||
| FROM %1$s.%2$s.INFORMATION_SCHEMA.PROPERTY_GRAPHS, |
| boolean isExactGraphTableName = | ||
| tableNamePattern != null | ||
| && !tableNamePattern.contains("%") | ||
| && tableNamePattern.contains("."); |
There was a problem hiding this comment.
Please add a comment explaining this assumption that if table name contains ., we'll expect it to be graph
| formattedSql = | ||
| replaceSqlParameters( | ||
| getGraphTablesDatasetSqlContent(), | ||
| quoteIdentifier(project), |
There was a problem hiding this comment.
Per comment in SQL, rather than escaping identifiers here, better to do it at SQL file level
|
|
||
| for (String project : targetCatalogs) { | ||
| String formattedSql; | ||
| if (isExactDataset) { |
There was a problem hiding this comment.
nit: I'd rather do it this way, easier to see what's changing
formattedSql =
replaceSqlParameters(
getGraphTablesRegionSqlContent(),
quoteIdentifier(project),
isExactDataset ? quoteIdentifier(effectiveSchemaPattern) : quoteIdentifier(resolveGraphRegion()));
| if (isExactDataset) { | ||
| statement.setString(1, effectiveTablePattern); | ||
| } else { | ||
| statement.setString(1, effectiveSchemaPattern == null ? "%" : effectiveSchemaPattern); |
There was a problem hiding this comment.
I think we can have single SQL query, having an extra condition schema LIKE "%" is unlikely to have any perf implication (other than that they seem identical);
| String sourceTable = rs.getString("SOURCE_TABLE"); | ||
| String remarks = sourceTable != null ? "Source: " + sourceTable : null; | ||
|
|
||
| List<FieldValue> values = new ArrayList<>(resultSchemaResults.size()); |
There was a problem hiding this comment.
Can you add inline comments what maps to what?
| Assertions.assertTrue(graphTables.contains(graphName + ".Knows")); | ||
| Assertions.assertEquals(2, graphTables.size()); | ||
| } finally { | ||
| try (Connection cleanupConn = DriverManager.getConnection(ITBase.connectionUrl); |
There was a problem hiding this comment.
nit: no need to cleanup. We drop entire dataset after tests which will drop tables too
| // Add rows in unsorted order (based on TYPE, CAT, SCHEM, NAME) | ||
| results.add(createTableRow("cat_a", "sch_z", "table_1", "TABLE", schemaFields)); | ||
| results.add(createTableRow("cat_b", "sch_a", "view_1", "VIEW", schemaFields)); | ||
| results.add( |
There was a problem hiding this comment.
Can we move this to the last row, so that we just append to unittest
b/570033146
b/570032972