kbendick commented on a change in pull request #3275:
URL: https://github.com/apache/iceberg/pull/3275#discussion_r744556754
##########
File path: core/src/main/java/org/apache/iceberg/jdbc/JdbcCatalog.java
##########
@@ -116,6 +118,19 @@ private void initializeCatalogTables() throws
InterruptedException, SQLException
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, null,
JdbcUtil.NAMESPACE_PROPERTIES_TABLE_NAME, null);
Review comment:
Nit: Consider either wrapping this with a utility method or adding
inline comments where the nulls are, documenting what parameter they’re for,
like `dbMeta.getTables(null /* ??? */, null …. )`.
I’d personally use the named function approach. This many nullls (or Boolean
literals, etc) makes it hard for the reader to grok the function call’s
signature and therefore what it’s doing. In this case, because you have a good
variable name, it’s not as difficult to tell what the intended code is. But
it’s still arguably pretty difficult to look at this and visually assert that
this is likely correct or what all of the unused parameters are.
Given that this code path isn’t exactly hot by any means, I’d go with the
utility function to wrap this. It could even just return the boolean, as the
ResultSet is otherwise unused.
--
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]
