kbendick commented on a change in pull request #3275:
URL: https://github.com/apache/iceberg/pull/3275#discussion_r805054691
##########
File path: core/src/main/java/org/apache/iceberg/jdbc/JdbcCatalog.java
##########
@@ -108,15 +109,29 @@ private void initializeCatalogTables() throws
InterruptedException, SQLException
LOG.trace("Creating database tables (if missing) to store iceberg
catalog");
connections.run(conn -> {
DatabaseMetaData dbMeta = conn.getMetaData();
- ResultSet tableExists = dbMeta.getTables(null, null,
JdbcUtil.CATALOG_TABLE_NAME, null);
-
+ ResultSet tableExists = dbMeta.getTables(null /* catalog name */, null
/* schemaPattern */,
+ JdbcUtil.CATALOG_TABLE_NAME /* tableNamePattern */, null /* types
*/);
if (tableExists.next()) {
return true;
}
LOG.debug("Creating table {} to store iceberg catalog",
JdbcUtil.CATALOG_TABLE_NAME);
return conn.prepareStatement(JdbcUtil.CREATE_CATALOG_TABLE).execute();
});
+
+ connections.run(conn -> {
+ DatabaseMetaData dbMeta = conn.getMetaData();
+ ResultSet tableExists = dbMeta.getTables(null /* catalog name */, null
/* schemaPattern */,
+ JdbcUtil.NAMESPACE_PROPERTIES_TABLE_NAME /* tableNamePattern */,
null /* types */);
Review comment:
Probably using the comments to define the values that are `null` etc one
time is fine.
Maybe making this a private function that takes in `conn` and then has the
comments would enhance readability.
Like
```java
/**
* Check if a table exists in the database via a pattern to match against.
* Searches all schemas & catalogs.
* @return true if a table matching the pattern exists in the database
*/
private static boolean checkIfTableExists(Connection conn, String
tableNamePatternToCheck) {
DatabaseMetaData dbMeta = conn.getMetaData();
ResultSet tablesMatchingPattern = dbMeta.getTables(
null /* catalog name */, null /* schemaPattern */,
tableNamePatternToCheck, null /( types */);
return tablesMatchingPattern.hasNext();
}
```
I'm also not sure if there's a reason to use `.next()` instead of
`.hasNext()` on the `ResultSet`. I'm ok with either way, but `hasNext` seems
like the more intuitive API if there's no semantic difference. I don't know
enough about JDBC to be sure if there is a difference or not though so it might
be safer to just leave it as is.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
