yangshangqing95 commented on code in PR #17526:
URL: https://github.com/apache/iceberg/pull/17526#discussion_r3723454889
##########
core/src/main/java/org/apache/iceberg/jdbc/JdbcCatalog.java:
##########
@@ -168,15 +168,17 @@ private void atomicCreateTable(String tableName, String
sqlCommand, String reaso
connections.run(
conn -> {
DatabaseMetaData dbMeta = conn.getMetaData();
+ String catalog = JdbcUtil.metadataCatalog(conn);
+ String escape = dbMeta.getSearchStringEscape();
// check the existence of a table name
Predicate<String> tableTest =
name -> {
try (ResultSet result =
dbMeta.getTables(
- null /* catalog name */,
+ catalog,
null /* schemaPattern */,
Review Comment:
For databases that support schemas, such as Oracle and PostgreSQL, passing
`null` as `schemaPattern` does not restrict the lookup to the current schema:
> schemaPattern - a schema name pattern; must match the schema name as it is
stored in the database; "" retrieves those without a schema; null means that
the schema name should not be used to narrow the search
Could we use `Connection#getSchema()` and pass the escaped value to both
`getTables` and `getColumns`? A regression test with matching table names in
two schemas would also be helpful.
##########
core/src/main/java/org/apache/iceberg/jdbc/JdbcCatalog.java:
##########
@@ -235,9 +237,13 @@ private void updateSchemaIfRequired() {
connections.run(
conn -> {
DatabaseMetaData dbMeta = conn.getMetaData();
+ String escape = dbMeta.getSearchStringEscape();
try (ResultSet typeColumn =
dbMeta.getColumns(
- null, null, JdbcUtil.CATALOG_TABLE_VIEW_NAME,
JdbcUtil.RECORD_TYPE)) {
+ JdbcUtil.metadataCatalog(conn),
+ null,
Review Comment:
same as above comment, `Connection#getSchema()`?
##########
core/src/main/java/org/apache/iceberg/jdbc/JdbcUtil.java:
##########
@@ -530,6 +535,58 @@ static boolean isConstraintViolation(SQLException ex) {
|| (ex.getMessage() != null && ex.getMessage().contains("constraint
failed"));
}
+ /**
+ * Returns the catalog that {@link java.sql.DatabaseMetaData} lookups should
be restricted to, or
+ * null to leave them unrestricted.
+ *
+ * <p>A null catalog does not restrict a lookup to the database the
connection points at, so
+ * catalog tables in an unrelated database can be mistaken for this
catalog's own. An empty
+ * catalog name is not used because it selects only objects that belong to
no catalog.
+ *
+ * @param conn a connection to resolve the catalog of
+ */
+ static String metadataCatalog(Connection conn) {
+ try {
+ String catalog = conn.getCatalog();
+ return catalog == null || catalog.isEmpty() ? null : catalog;
+ } catch (SQLException e) {
Review Comment:
Catching all `SQLExceptions` here may hide real connection or database
errors and fall back to searching all catalogs, which can reintroduce the same
false-positive behavior.
Could we only handle `SQLFeatureNotSupportedException` and let other
`SQLExceptions` propagate?
##########
core/src/main/java/org/apache/iceberg/jdbc/JdbcUtil.java:
##########
@@ -530,6 +535,58 @@ static boolean isConstraintViolation(SQLException ex) {
|| (ex.getMessage() != null && ex.getMessage().contains("constraint
failed"));
}
+ /**
+ * Returns the catalog that {@link java.sql.DatabaseMetaData} lookups should
be restricted to, or
+ * null to leave them unrestricted.
+ *
+ * <p>A null catalog does not restrict a lookup to the database the
connection points at, so
+ * catalog tables in an unrelated database can be mistaken for this
catalog's own. An empty
+ * catalog name is not used because it selects only objects that belong to
no catalog.
+ *
+ * @param conn a connection to resolve the catalog of
+ */
+ static String metadataCatalog(Connection conn) {
+ try {
+ String catalog = conn.getCatalog();
+ return catalog == null || catalog.isEmpty() ? null : catalog;
Review Comment:
`null` and `""` are different in JDBC
> catalog - a catalog name; "" retrieves those without a catalog; null means
drop catalog name from the selection criteria
##########
core/src/main/java/org/apache/iceberg/jdbc/JdbcUtil.java:
##########
@@ -530,6 +535,58 @@ static boolean isConstraintViolation(SQLException ex) {
|| (ex.getMessage() != null && ex.getMessage().contains("constraint
failed"));
}
+ /**
+ * Returns the catalog that {@link java.sql.DatabaseMetaData} lookups should
be restricted to, or
+ * null to leave them unrestricted.
+ *
+ * <p>A null catalog does not restrict a lookup to the database the
connection points at, so
+ * catalog tables in an unrelated database can be mistaken for this
catalog's own. An empty
+ * catalog name is not used because it selects only objects that belong to
no catalog.
+ *
+ * @param conn a connection to resolve the catalog of
+ */
+ static String metadataCatalog(Connection conn) {
+ try {
+ String catalog = conn.getCatalog();
+ return catalog == null || catalog.isEmpty() ? null : catalog;
+ } catch (SQLException e) {
+ // databases without catalog support may fail instead of reporting no
catalog
+ LOG.debug("Cannot determine the connection catalog, searching all
catalogs", e);
+ return null;
+ }
+ }
+
+ /**
+ * Escapes a literal name so that it matches only itself when used as a
{@link
+ * java.sql.DatabaseMetaData} pattern.
+ *
+ * <p>Pattern arguments treat {@code _} and {@code %} as wildcards, so an
unescaped name such as
+ * {@code iceberg_tables} also matches unrelated names like {@code
iceberg1tables}. The name is
+ * returned unchanged when the driver reports no escape string, because
escaping is not supported
+ * in that case.
+ *
+ * @param name a literal table or column name
+ * @param escape the driver's escape string, from {@link
+ * java.sql.DatabaseMetaData#getSearchStringEscape()}
+ */
+ static String escapeMetadataPattern(String name, String escape) {
+ if (escape == null || escape.isEmpty()) {
Review Comment:
If this helper is also used for schemaPattern, name should be nullable
because null is a valid value.
##########
core/src/test/java/org/apache/iceberg/jdbc/TestJdbcUtil.java:
##########
@@ -173,4 +176,54 @@ public void emptyNamespaceInIdentifier() {
assertThat(JdbcUtil.stringToTableIdentifier("", "tblName"))
.isEqualTo(TableIdentifier.of(Namespace.empty(), "tblName"));
}
+
+ @Test
+ void metadataCatalogUsesConnectionCatalog() throws SQLException {
+ Connection conn = Mockito.mock(Connection.class);
+ Mockito.when(conn.getCatalog()).thenReturn("iceberg_db");
+
+ assertThat(JdbcUtil.metadataCatalog(conn)).isEqualTo("iceberg_db");
+ }
+
+ @Test
+ void metadataCatalogIsUnrestrictedWhenCatalogIsAbsent() throws SQLException {
+ Connection conn = Mockito.mock(Connection.class);
+
+ Mockito.when(conn.getCatalog()).thenReturn(null);
+ assertThat(JdbcUtil.metadataCatalog(conn)).isNull();
+
+ Mockito.when(conn.getCatalog()).thenReturn("");
+ assertThat(JdbcUtil.metadataCatalog(conn)).isNull();
Review Comment:
`""` should not be converted to `null`
##########
core/src/test/java/org/apache/iceberg/jdbc/TestJdbcCatalog.java:
##########
@@ -307,6 +308,68 @@ public void testInitSchemaV0() {
.hasMessage(JdbcCatalog.VIEW_WARNING_LOG_MESSAGE);
}
+ @Test
+ void catalogTablesAreCreatedWhenAnotherTableMatchesTheirNamePattern() throws
Exception {
+ // as this test uses different connections, we can't use memory database
(as it's per
+ // connection), but a file database instead
+ java.nio.file.Path dbFile = Files.createTempFile("icebergSimilarTable",
"db");
+ String jdbcUrl = "jdbc:sqlite:" + dbFile.toAbsolutePath();
+
+ // the underscore in iceberg_tables is a wildcard when used as a JDBC
metadata pattern, so
+ // this unrelated table is reported as the catalog table unless the
pattern is escaped
+ executeUpdate(jdbcUrl, "CREATE TABLE iceberg1tables (col VARCHAR(255))");
+
+ Map<String, String> properties = Maps.newHashMap();
+ properties.put(CatalogProperties.WAREHOUSE_LOCATION,
this.tableDir.toAbsolutePath().toString());
+ properties.put(CatalogProperties.URI, jdbcUrl);
+
+ try (JdbcCatalog jdbcCatalog = new JdbcCatalog()) {
+ jdbcCatalog.setConf(conf);
+ jdbcCatalog.initialize("similar_table_catalog", properties);
+
+ assertThat(catalogTablesExist(jdbcUrl)).isTrue();
+
+ TableIdentifier tableIdent = TableIdentifier.of(Namespace.of("ns1"),
"tbl");
+ jdbcCatalog.buildTable(tableIdent, SCHEMA).create();
+ assertThat(jdbcCatalog.loadTable(tableIdent).schema().asStruct())
+ .isEqualTo(SCHEMA.asStruct());
+ }
+ }
+
+ @Test
+ void schemaVersionIgnoresColumnsOfTablesMatchingTheCatalogTableNamePattern()
throws Exception {
+ // as this test uses different connections, we can't use memory database
(as it's per
+ // connection), but a file database instead
+ java.nio.file.Path dbFile = Files.createTempFile("icebergSimilarColumn",
"db");
+ String jdbcUrl = "jdbc:sqlite:" + dbFile.toAbsolutePath();
+
+ // create the catalog tables up front so that only schema version
detection is exercised
+ executeUpdate(jdbcUrl, JdbcUtil.V0_CREATE_CATALOG_SQL);
+ executeUpdate(jdbcUrl, JdbcUtil.CREATE_NAMESPACE_PROPERTIES_TABLE_SQL);
+ // this table and its column match iceberg_tables and iceberg_type through
the underscore
+ // wildcard, so an unescaped lookup reports view support that the catalog
table lacks
+ executeUpdate(jdbcUrl, "CREATE TABLE iceberg1tables (iceberg1type
VARCHAR(5))");
+
+ Map<String, String> properties = Maps.newHashMap();
+ properties.put(CatalogProperties.WAREHOUSE_LOCATION,
this.tableDir.toAbsolutePath().toString());
+ properties.put(CatalogProperties.URI, jdbcUrl);
+
+ try (JdbcCatalog jdbcCatalog = new JdbcCatalog()) {
+ jdbcCatalog.setConf(conf);
+ jdbcCatalog.initialize("similar_column_catalog", properties);
+
+ // committing as V1 would write iceberg_type, which this V0 catalog
table does not have
+ TableIdentifier tableIdent = TableIdentifier.of(Namespace.of("ns1"),
"tbl");
+ jdbcCatalog.buildTable(tableIdent, SCHEMA).create();
+ assertThat(jdbcCatalog.loadTable(tableIdent).schema().asStruct())
+ .isEqualTo(SCHEMA.asStruct());
+
+ assertThatThrownBy(() -> jdbcCatalog.listViews(Namespace.of("ns1")))
+ .isInstanceOf(UnsupportedOperationException.class)
+ .hasMessage(JdbcCatalog.VIEW_WARNING_LOG_MESSAGE);
+ }
+ }
+
Review Comment:
One concern is that the added tests cover metadata pattern escaping, but not
the cross-catalog scenario described in the issue.
Would it be possible to add a regression test with two catalogs/databases
containing iceberg_tables, where only the other catalog has the V1 column, and
verify that schema version detection only uses the current catalog?
--
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]
