diff --git a/plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/CachingJdbcClient.java b/plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/CachingJdbcClient.java index 25b85a78e95c..2fa946c49c05 100644 --- a/plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/CachingJdbcClient.java +++ b/plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/CachingJdbcClient.java @@ -412,28 +412,28 @@ public void setTableComment(ConnectorSession session, JdbcTableHandle handle, Op public void setColumnComment(ConnectorSession session, JdbcTableHandle handle, JdbcColumnHandle column, Optional comment) { delegate.setColumnComment(session, handle, column, comment); - invalidateColumnsCache(handle.asPlainTable().getSchemaTableName()); + invalidateTableCaches(handle.asPlainTable().getSchemaTableName()); } @Override public void addColumn(ConnectorSession session, JdbcTableHandle handle, ColumnMetadata column) { delegate.addColumn(session, handle, column); - invalidateColumnsCache(handle.asPlainTable().getSchemaTableName()); + invalidateTableCaches(handle.asPlainTable().getSchemaTableName()); } @Override public void dropColumn(ConnectorSession session, JdbcTableHandle handle, JdbcColumnHandle column) { delegate.dropColumn(session, handle, column); - invalidateColumnsCache(handle.asPlainTable().getSchemaTableName()); + invalidateTableCaches(handle.asPlainTable().getSchemaTableName()); } @Override public void renameColumn(ConnectorSession session, JdbcTableHandle handle, JdbcColumnHandle jdbcColumn, String newColumnName) { delegate.renameColumn(session, handle, jdbcColumn, newColumnName); - invalidateColumnsCache(handle.asPlainTable().getSchemaTableName()); + invalidateTableCaches(handle.asPlainTable().getSchemaTableName()); } @Override @@ -578,6 +578,12 @@ CacheStats getTableNamesCacheStats() return tableNamesCache.stats(); } + @VisibleForTesting + CacheStats getTableHandlesByNameCacheStats() + { + return tableHandlesByNameCache.stats(); + } + @VisibleForTesting CacheStats getTableHandlesByQueryCacheStats() { diff --git a/plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/DefaultJdbcMetadata.java b/plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/DefaultJdbcMetadata.java index a3e10fc6161c..fdea0914fd7b 100644 --- a/plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/DefaultJdbcMetadata.java +++ b/plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/DefaultJdbcMetadata.java @@ -264,7 +264,12 @@ public Optional> applyProjecti return Optional.empty(); } - verify(tableColumnSet.containsAll(newColumnSet), "applyProjection called with columns %s and some are not available in existing query: %s", newColumnSet, tableColumnSet); + Set tableSyntheticColumnSet = ImmutableSet.builder() + .addAll(tableColumnSet) + .add((JdbcColumnHandle) getDeleteRowIdColumnHandle(session, table)) + .build(); + + verify(tableSyntheticColumnSet.containsAll(newColumnSet), "applyProjection called with columns %s and some are not available in existing query: %s", newColumnSet, tableSyntheticColumnSet); } return Optional.of(new ProjectionApplicationResult<>( diff --git a/plugin/trino-base-jdbc/src/test/java/io/trino/plugin/jdbc/TestCachingJdbcClient.java b/plugin/trino-base-jdbc/src/test/java/io/trino/plugin/jdbc/TestCachingJdbcClient.java index 8be4639e2dab..39213583364b 100644 --- a/plugin/trino-base-jdbc/src/test/java/io/trino/plugin/jdbc/TestCachingJdbcClient.java +++ b/plugin/trino-base-jdbc/src/test/java/io/trino/plugin/jdbc/TestCachingJdbcClient.java @@ -31,6 +31,7 @@ import io.trino.spi.statistics.Estimate; import io.trino.spi.statistics.TableStatistics; import io.trino.testing.TestingConnectorSession; +import org.jetbrains.annotations.NotNull; import org.testng.annotations.AfterMethod; import org.testng.annotations.BeforeMethod; import org.testng.annotations.Test; @@ -55,6 +56,7 @@ import static com.google.common.collect.ImmutableMap.toImmutableMap; import static io.airlift.concurrent.Threads.daemonThreadsNamed; import static io.trino.plugin.jdbc.TestCachingJdbcClient.CachingJdbcCache.STATISTICS_CACHE; +import static io.trino.plugin.jdbc.TestCachingJdbcClient.CachingJdbcCache.TABLE_HANDLES_BY_NAME_CACHE; import static io.trino.plugin.jdbc.TestCachingJdbcClient.CachingJdbcCache.TABLE_HANDLES_BY_QUERY_CACHE; import static io.trino.spi.session.PropertyMetadata.stringProperty; import static io.trino.spi.testing.InterfaceTestUtils.assertAllMethodsOverridden; @@ -273,6 +275,38 @@ public void testTableHandleOfQueryCached() dropTable(phantomTable); } + @Test + public void testTableHandleInvalidatedOnColumnsModifications() + { + JdbcTableHandle table = createTable(new SchemaTableName(schema, "a_table")); + JdbcColumnHandle existingColumn = addColumn(table, "a_column"); + + // warm-up cache + assertTableHandlesByNameCacheIsInvalidated(table); + JdbcColumnHandle newColumn = addColumn(cachingJdbcClient, table, "new_column"); + assertTableHandlesByNameCacheIsInvalidated(table); + cachingJdbcClient.setColumnComment(SESSION, table, newColumn, Optional.empty()); + assertTableHandlesByNameCacheIsInvalidated(table); + cachingJdbcClient.renameColumn(SESSION, table, newColumn, "new_column_name"); + assertTableHandlesByNameCacheIsInvalidated(table); + cachingJdbcClient.dropColumn(SESSION, table, existingColumn); + assertTableHandlesByNameCacheIsInvalidated(table); + + dropTable(table); + } + + private void assertTableHandlesByNameCacheIsInvalidated(JdbcTableHandle table) + { + SchemaTableName tableName = table.asPlainTable().getSchemaTableName(); + + assertCacheStats(cachingJdbcClient, TABLE_HANDLES_BY_NAME_CACHE).misses(1).loads(1).afterRunning(() -> { + assertThat(cachingJdbcClient.getTableHandle(SESSION, tableName).orElseThrow()).isEqualTo(table); + }); + assertCacheStats(cachingJdbcClient, TABLE_HANDLES_BY_NAME_CACHE).hits(1).afterRunning(() -> { + assertThat(cachingJdbcClient.getTableHandle(SESSION, tableName).orElseThrow()).isEqualTo(table); + }); + } + @Test public void testEmptyTableHandleIsCachedWhenCacheMissingIsTrue() { @@ -304,6 +338,11 @@ private JdbcTableHandle createTable(SchemaTableName phantomTable) return jdbcClient.getTableHandle(SESSION, phantomTable).orElseThrow(); } + private void dropTable(JdbcTableHandle tableHandle) + { + jdbcClient.dropTable(SESSION, tableHandle); + } + private void dropTable(SchemaTableName phantomTable) { JdbcTableHandle tableHandle = jdbcClient.getTableHandle(SESSION, phantomTable).orElseThrow(); @@ -819,10 +858,16 @@ private JdbcColumnHandle addColumn(JdbcTableHandle tableHandle) } private JdbcColumnHandle addColumn(JdbcTableHandle tableHandle, String columnName) + { + return addColumn(jdbcClient, tableHandle, columnName); + } + + @NotNull + private JdbcColumnHandle addColumn(JdbcClient client, JdbcTableHandle tableHandle, String columnName) { ColumnMetadata columnMetadata = new ColumnMetadata(columnName, INTEGER); - jdbcClient.addColumn(SESSION, tableHandle, columnMetadata); - return jdbcClient.getColumns(SESSION, tableHandle) + client.addColumn(SESSION, tableHandle, columnMetadata); + return client.getColumns(SESSION, tableHandle) .stream() .filter(jdbcColumnHandle -> jdbcColumnHandle.getColumnMetadata().equals(columnMetadata)) .findAny() @@ -995,6 +1040,7 @@ enum CachingJdbcCache { TABLE_NAMES_CACHE(CachingJdbcClient::getTableNamesCacheStats), TABLE_HANDLES_BY_QUERY_CACHE(CachingJdbcClient::getTableHandlesByQueryCacheStats), + TABLE_HANDLES_BY_NAME_CACHE(CachingJdbcClient::getTableHandlesByNameCacheStats), COLUMNS_CACHE(CachingJdbcClient::getColumnsCacheStats), STATISTICS_CACHE(CachingJdbcClient::getStatisticsCacheStats), /**/; diff --git a/plugin/trino-base-jdbc/src/test/java/io/trino/plugin/jdbc/TestingH2JdbcClient.java b/plugin/trino-base-jdbc/src/test/java/io/trino/plugin/jdbc/TestingH2JdbcClient.java index 0a41962ea655..98d09cd1af34 100644 --- a/plugin/trino-base-jdbc/src/test/java/io/trino/plugin/jdbc/TestingH2JdbcClient.java +++ b/plugin/trino-base-jdbc/src/test/java/io/trino/plugin/jdbc/TestingH2JdbcClient.java @@ -41,6 +41,7 @@ import java.util.Map; import java.util.Optional; +import static com.google.common.base.Verify.verify; import static io.trino.plugin.jdbc.StandardColumnMappings.bigintColumnMapping; import static io.trino.plugin.jdbc.StandardColumnMappings.bigintWriteFunction; import static io.trino.plugin.jdbc.StandardColumnMappings.booleanColumnMapping; @@ -109,6 +110,13 @@ public Optional getTableComment(ResultSet resultSet) return Optional.empty(); } + @Override + public void setColumnComment(ConnectorSession session, JdbcTableHandle handle, JdbcColumnHandle column, Optional comment) + { + // do not throw when invoked, however do not allow to set non-empty comment until the connector supports setting column comments + verify(comment.isEmpty(), "This connector does not support setting column comments"); + } + @Override public boolean supportsAggregationPushdown(ConnectorSession session, JdbcTableHandle table, List aggregates, Map assignments, List> groupingSets) {