Skip to content

Remove deprecated version of getTableStatistics #12489

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
findepi merged 3 commits into from
Jun 9, 2022
Merged

Remove deprecated version of getTableStatistics #12489

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
7c25142
Remove deprecated version of getTableStatistics
May 20, 2022
86d718d
Deprecate receiving constraints in JdbcClient.getTableStatistics
May 31, 2022
9f17e62
Remove unnecessary TableStatisticsCacheKey wrapping JdbcTableHandle
May 31, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 1 addition & 12 deletions core/trino-spi/src/main/java/io/trino/spi/connector/ConnectorMetadata.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -264,23 +264,12 @@ default Stream<TableColumnsMetadata> streamTableColumns(ConnectorSession session
.map(entry -> TableColumnsMetadata.forTable(entry.getKey(), entry.getValue()));
}

/**
* Get statistics for table for given filtering constraint.
*
* @deprecated Use {@link #getTableStatistics(ConnectorSession, ConnectorTableHandle)}
*/
@Deprecated
default TableStatistics getTableStatistics(ConnectorSession session, ConnectorTableHandle tableHandle, Constraint constraint)
{
return TableStatistics.empty();
}

/**
* Get statistics for table.
*/
default TableStatistics getTableStatistics(ConnectorSession session, ConnectorTableHandle tableHandle)
{
return getTableStatistics(session, tableHandle, Constraint.alwaysTrue());
return TableStatistics.empty();
}

/**
Expand Down
8 changes: 0 additions & 8 deletions ...lkit/src/main/java/io/trino/plugin/base/classloader/ClassLoaderSafeConnectorMetadata.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -313,14 +313,6 @@ public Stream<TableColumnsMetadata> streamTableColumns(ConnectorSession session,
}
}

@Override
public TableStatistics getTableStatistics(ConnectorSession session, ConnectorTableHandle tableHandle, Constraint constraint)
{
try (ThreadContextClassLoader ignored = new ThreadContextClassLoader(classLoader)) {
return delegate.getTableStatistics(session, tableHandle, constraint);
}
}

@Override
public TableStatistics getTableStatistics(ConnectorSession session, ConnectorTableHandle tableHandle)
{
Expand Down
65 changes: 15 additions & 50 deletions plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/CachingJdbcClient.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,7 @@

import static com.google.common.base.MoreObjects.firstNonNull;
import static com.google.common.base.MoreObjects.toStringHelper;
import static com.google.common.base.Preconditions.checkArgument;
import static com.google.common.base.Throwables.throwIfInstanceOf;
import static com.google.common.collect.ImmutableList.toImmutableList;
import static com.google.common.collect.ImmutableMap.toImmutableMap;
Expand All @@ -85,7 +86,7 @@ public class CachingJdbcClient
private final Cache<TableHandlesByNameCacheKey, Optional<JdbcTableHandle>> tableHandlesByNameCache;
private final Cache<TableHandlesByQueryCacheKey, JdbcTableHandle> tableHandlesByQueryCache;
private final Cache<ColumnsCacheKey, List<JdbcColumnHandle>> columnsCache;
private final Cache<TableStatisticsCacheKey, TableStatistics> statisticsCache;
private final Cache<JdbcTableHandle, TableStatistics> statisticsCache;

@Inject
public CachingJdbcClient(
Expand Down Expand Up @@ -354,16 +355,22 @@ public PreparedStatement getPreparedStatement(Connection connection, String sql)
@Override
public TableStatistics getTableStatistics(ConnectorSession session, JdbcTableHandle handle, TupleDomain<ColumnHandle> tupleDomain)
{
TableStatisticsCacheKey key = new TableStatisticsCacheKey(handle, tupleDomain);
checkArgument(tupleDomain.isAll(), "Unexpected non-ALL constraint: %s", tupleDomain);
return getTableStatistics(session, handle);
Comment thread
nineinchnick marked this conversation as resolved.
Outdated
}

TableStatistics cachedStatistics = statisticsCache.getIfPresent(key);
@Override
public TableStatistics getTableStatistics(ConnectorSession session, JdbcTableHandle handle)
{
// TODO depend on Identity when needed
TableStatistics cachedStatistics = statisticsCache.getIfPresent(handle);
if (cachedStatistics != null) {
if (cacheMissing || !cachedStatistics.equals(TableStatistics.empty())) {
return cachedStatistics;
}
statisticsCache.invalidate(key);
statisticsCache.invalidate(handle);
}
return get(statisticsCache, key, () -> delegate.getTableStatistics(session, handle, tupleDomain));
return get(statisticsCache, handle, () -> delegate.getTableStatistics(session, handle));
}

@Override
Expand Down Expand Up @@ -489,7 +496,7 @@ public Optional<TableScanRedirectApplicationResult> getTableScanRedirection(Conn

public void onDataChanged(SchemaTableName table)
{
invalidateCache(statisticsCache, key -> key.tableHandle.references(table));
invalidateCache(statisticsCache, key -> key.references(table));
}

/**
Expand All @@ -500,7 +507,7 @@ public void onDataChanged(SchemaTableName table)
@Deprecated
public void onDataChanged(JdbcTableHandle handle)
{
invalidateCache(statisticsCache, key -> key.tableHandle.equals(handle));
invalidateCache(statisticsCache, key -> key.equals(handle));
}

@Override
Expand Down Expand Up @@ -558,7 +565,7 @@ private void invalidateTableCaches(SchemaTableName schemaTableName)
invalidateCache(tableHandlesByNameCache, key -> key.tableName.equals(schemaTableName));
tableHandlesByQueryCache.invalidateAll();
invalidateCache(tableNamesCache, key -> key.schemaName.equals(Optional.of(schemaTableName.getSchemaName())));
invalidateCache(statisticsCache, key -> key.tableHandle.references(schemaTableName));
invalidateCache(statisticsCache, key -> key.references(schemaTableName));
}

private void invalidateColumnsCache(SchemaTableName table)
Expand Down Expand Up @@ -754,48 +761,6 @@ private static <K, V> V get(Cache<K, V> cache, K key, Callable<V> loader)
}
}

private static final class TableStatisticsCacheKey
{
// TODO depend on Identity when needed
private final JdbcTableHandle tableHandle;
private final TupleDomain<ColumnHandle> tupleDomain;

private TableStatisticsCacheKey(JdbcTableHandle tableHandle, TupleDomain<ColumnHandle> tupleDomain)
{
this.tableHandle = requireNonNull(tableHandle, "tableHandle is null");
this.tupleDomain = requireNonNull(tupleDomain, "tupleDomain is null");
}

@Override
public boolean equals(Object o)
{
if (this == o) {
return true;
}
if (o == null || getClass() != o.getClass()) {
return false;
}
TableStatisticsCacheKey that = (TableStatisticsCacheKey) o;
return tableHandle.equals(that.tableHandle)
&& tupleDomain.equals(that.tupleDomain);
}

@Override
public int hashCode()
{
return Objects.hash(tableHandle, tupleDomain);
}

@Override
public String toString()
{
return toStringHelper(this)
.add("tableHandle", tableHandle)
.add("tupleDomain", tupleDomain)
.toString();
}
}

@Managed
@Nested
public CacheStatsMBean getSchemaNamesStats()
Expand Down
3 changes: 1 addition & 2 deletions plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/DefaultJdbcMetadata.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -855,8 +855,7 @@ public void setTableProperties(ConnectorSession session, ConnectorTableHandle ta
public TableStatistics getTableStatistics(ConnectorSession session, ConnectorTableHandle tableHandle)
{
JdbcTableHandle handle = (JdbcTableHandle) tableHandle;
// TODO passing constraint to getTableStatistics is deprecated, remove it from the JdbcClient interface
return jdbcClient.getTableStatistics(session, handle, Constraint.alwaysTrue().getSummary());
return jdbcClient.getTableStatistics(session, handle);
}

@Override
Expand Down
6 changes: 6 additions & 0 deletions plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/ForwardingJdbcClient.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -246,6 +246,12 @@ public TableStatistics getTableStatistics(ConnectorSession session, JdbcTableHan
return delegate().getTableStatistics(session, handle, tupleDomain);
}

@Override
public TableStatistics getTableStatistics(ConnectorSession session, JdbcTableHandle handle)
{
return delegate().getTableStatistics(session, handle);
}

@Override
public boolean supportsTopN(ConnectorSession session, JdbcTableHandle handle, List<JdbcSortItem> sortOrder)
{
Expand Down
9 changes: 9 additions & 0 deletions plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/JdbcClient.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -176,8 +176,17 @@ Connection getConnection(ConnectorSession session, JdbcOutputTableHandle handle)
PreparedStatement getPreparedStatement(Connection connection, String sql)
throws SQLException;

/**
* @deprecated Use {@link #getTableStatistics(ConnectorSession, JdbcTableHandle)}
*/
@Deprecated
TableStatistics getTableStatistics(ConnectorSession session, JdbcTableHandle handle, TupleDomain<ColumnHandle> tupleDomain);

default TableStatistics getTableStatistics(ConnectorSession session, JdbcTableHandle handle)
{
return getTableStatistics(session, handle, TupleDomain.all());
}

void createSchema(ConnectorSession session, String schemaName);

void dropSchema(ConnectorSession session, String schemaName);
Expand Down
6 changes: 6 additions & 0 deletions plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/jmx/StatisticsAwareJdbcClient.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -320,6 +320,12 @@ public TableStatistics getTableStatistics(ConnectorSession session, JdbcTableHan
return stats.getGetTableStatistics().wrap(() -> delegate().getTableStatistics(session, handle, tupleDomain));
}

@Override
public TableStatistics getTableStatistics(ConnectorSession session, JdbcTableHandle handle)
{
return stats.getGetTableStatistics().wrap(() -> delegate().getTableStatistics(session, handle));
}

@Override
public boolean supportsTopN(ConnectorSession session, JdbcTableHandle handle, List<JdbcSortItem> sortOrder)
{
Expand Down
47 changes: 23 additions & 24 deletions plugin/trino-base-jdbc/src/test/java/io/trino/plugin/jdbc/TestCachingJdbcClient.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@
import io.trino.collect.cache.CacheStatsAssertions;
import io.trino.plugin.base.session.SessionPropertiesProvider;
import io.trino.plugin.jdbc.credential.ExtraCredentialConfig;
import io.trino.spi.connector.ColumnHandle;
import io.trino.spi.connector.ColumnMetadata;
import io.trino.spi.connector.ConnectorSession;
import io.trino.spi.connector.ConnectorTableMetadata;
Expand Down Expand Up @@ -380,22 +379,22 @@ public void testGetTableStatistics()

// load first
assertStatisticsCacheStats(cachingJdbcClient).loads(1).misses(1).afterRunning(() -> {
assertThat(cachingJdbcClient.getTableStatistics(session, first, TupleDomain.all())).isEqualTo(NON_EMPTY_STATS);
assertThat(cachingJdbcClient.getTableStatistics(session, first)).isEqualTo(NON_EMPTY_STATS);
});

// read first from cache
assertStatisticsCacheStats(cachingJdbcClient).hits(1).afterRunning(() -> {
assertThat(cachingJdbcClient.getTableStatistics(session, first, TupleDomain.all())).isEqualTo(NON_EMPTY_STATS);
assertThat(cachingJdbcClient.getTableStatistics(session, first)).isEqualTo(NON_EMPTY_STATS);
});

// load second
assertStatisticsCacheStats(cachingJdbcClient).loads(1).misses(1).afterRunning(() -> {
assertThat(cachingJdbcClient.getTableStatistics(session, second, TupleDomain.all())).isEqualTo(NON_EMPTY_STATS);
assertThat(cachingJdbcClient.getTableStatistics(session, second)).isEqualTo(NON_EMPTY_STATS);
});

// read first from cache
assertStatisticsCacheStats(cachingJdbcClient).hits(1).afterRunning(() -> {
assertThat(cachingJdbcClient.getTableStatistics(session, first, TupleDomain.all())).isEqualTo(NON_EMPTY_STATS);
assertThat(cachingJdbcClient.getTableStatistics(session, first)).isEqualTo(NON_EMPTY_STATS);
});

// invalidate first
Expand All @@ -404,12 +403,12 @@ public void testGetTableStatistics()

// load first again
assertStatisticsCacheStats(cachingJdbcClient).loads(1).misses(1).afterRunning(() -> {
assertThat(cachingJdbcClient.getTableStatistics(session, secondFirst, TupleDomain.all())).isEqualTo(NON_EMPTY_STATS);
assertThat(cachingJdbcClient.getTableStatistics(session, secondFirst)).isEqualTo(NON_EMPTY_STATS);
});

// read first from cache
assertStatisticsCacheStats(cachingJdbcClient).hits(1).afterRunning(() -> {
assertThat(cachingJdbcClient.getTableStatistics(session, secondFirst, TupleDomain.all())).isEqualTo(NON_EMPTY_STATS);
assertThat(cachingJdbcClient.getTableStatistics(session, secondFirst)).isEqualTo(NON_EMPTY_STATS);
});

// cleanup
Expand Down Expand Up @@ -437,28 +436,28 @@ public void testCacheGetTableStatisticsWithQueryRelationHandle()

// load
assertStatisticsCacheStats(cachingJdbcClient).loads(1).misses(1).afterRunning(() -> {
assertThat(cachingJdbcClient.getTableStatistics(session, queryOnFirst, TupleDomain.all())).isEqualTo(NON_EMPTY_STATS);
assertThat(cachingJdbcClient.getTableStatistics(session, queryOnFirst)).isEqualTo(NON_EMPTY_STATS);
});

// read from cache
assertStatisticsCacheStats(cachingJdbcClient).hits(1).afterRunning(() -> {
assertThat(cachingJdbcClient.getTableStatistics(session, queryOnFirst, TupleDomain.all())).isEqualTo(NON_EMPTY_STATS);
assertThat(cachingJdbcClient.getTableStatistics(session, queryOnFirst)).isEqualTo(NON_EMPTY_STATS);
});

// invalidate 'second'
cachingJdbcClient.dropTable(SESSION, second);

// read from cache again (no invalidation)
assertStatisticsCacheStats(cachingJdbcClient).hits(1).afterRunning(() -> {
assertThat(cachingJdbcClient.getTableStatistics(session, queryOnFirst, TupleDomain.all())).isEqualTo(NON_EMPTY_STATS);
assertThat(cachingJdbcClient.getTableStatistics(session, queryOnFirst)).isEqualTo(NON_EMPTY_STATS);
});

// invalidate 'first'
cachingJdbcClient.dropTable(SESSION, first);

// load again
assertStatisticsCacheStats(cachingJdbcClient).loads(1).misses(1).afterRunning(() -> {
assertThat(cachingJdbcClient.getTableStatistics(session, queryOnFirst, TupleDomain.all())).isEqualTo(NON_EMPTY_STATS);
assertThat(cachingJdbcClient.getTableStatistics(session, queryOnFirst)).isEqualTo(NON_EMPTY_STATS);
});
}

Expand All @@ -472,25 +471,25 @@ public void testTruncateTable()

// load
assertStatisticsCacheStats(cachingJdbcClient).loads(1).misses(1).afterRunning(() -> {
assertThat(cachingJdbcClient.getTableStatistics(session, table, TupleDomain.all())).isEqualTo(NON_EMPTY_STATS);
assertThat(cachingJdbcClient.getTableStatistics(session, table)).isEqualTo(NON_EMPTY_STATS);
});

// read from cache
assertStatisticsCacheStats(cachingJdbcClient).hits(1).afterRunning(() -> {
assertThat(cachingJdbcClient.getTableStatistics(session, table, TupleDomain.all())).isEqualTo(NON_EMPTY_STATS);
assertThat(cachingJdbcClient.getTableStatistics(session, table)).isEqualTo(NON_EMPTY_STATS);
});

// invalidate
cachingJdbcClient.truncateTable(SESSION, table);

// load again
assertStatisticsCacheStats(cachingJdbcClient).loads(1).misses(1).afterRunning(() -> {
assertThat(cachingJdbcClient.getTableStatistics(session, table, TupleDomain.all())).isEqualTo(NON_EMPTY_STATS);
assertThat(cachingJdbcClient.getTableStatistics(session, table)).isEqualTo(NON_EMPTY_STATS);
});

// read from cache
assertStatisticsCacheStats(cachingJdbcClient).hits(1).afterRunning(() -> {
assertThat(cachingJdbcClient.getTableStatistics(session, table, TupleDomain.all())).isEqualTo(NON_EMPTY_STATS);
assertThat(cachingJdbcClient.getTableStatistics(session, table)).isEqualTo(NON_EMPTY_STATS);
});

// cleanup
Expand All @@ -509,7 +508,7 @@ protected JdbcClient delegate()
}

@Override
public TableStatistics getTableStatistics(ConnectorSession session, JdbcTableHandle handle, TupleDomain<ColumnHandle> tupleDomain)
public TableStatistics getTableStatistics(ConnectorSession session, JdbcTableHandle handle)
{
return NON_EMPTY_STATS;
}
Expand All @@ -525,11 +524,11 @@ public void testCacheEmptyStatistics()
JdbcTableHandle table = createTable(new SchemaTableName(schema, "table"));

assertStatisticsCacheStats(cachingJdbcClient).loads(1).misses(1).afterRunning(() -> {
assertThat(cachingJdbcClient.getTableStatistics(session, table, TupleDomain.all())).isEqualTo(TableStatistics.empty());
assertThat(cachingJdbcClient.getTableStatistics(session, table)).isEqualTo(TableStatistics.empty());
});

assertStatisticsCacheStats(cachingJdbcClient).hits(1).afterRunning(() -> {
assertThat(cachingJdbcClient.getTableStatistics(session, table, TupleDomain.all())).isEqualTo(TableStatistics.empty());
assertThat(cachingJdbcClient.getTableStatistics(session, table)).isEqualTo(TableStatistics.empty());
});

// cleanup
Expand All @@ -544,11 +543,11 @@ public void testGetTableStatisticsDoNotCacheEmptyWhenCachingMissingIsDisabled()
JdbcTableHandle table = createTable(new SchemaTableName(schema, "table"));

assertStatisticsCacheStats(cachingJdbcClient).loads(1).misses(1).afterRunning(() -> {
assertThat(cachingJdbcClient.getTableStatistics(session, table, TupleDomain.all())).isEqualTo(TableStatistics.empty());
assertThat(cachingJdbcClient.getTableStatistics(session, table)).isEqualTo(TableStatistics.empty());
});

assertStatisticsCacheStats(cachingJdbcClient).loads(1).hits(1).misses(1).afterRunning(() -> {
assertThat(cachingJdbcClient.getTableStatistics(session, table, TupleDomain.all())).isEqualTo(TableStatistics.empty());
assertThat(cachingJdbcClient.getTableStatistics(session, table)).isEqualTo(TableStatistics.empty());
});

// cleanup
Expand Down Expand Up @@ -596,12 +595,12 @@ public void testFlushCache()

// load table
assertStatisticsCacheStats(cachingJdbcClient).loads(1).misses(1).afterRunning(() -> {
assertThat(cachingJdbcClient.getTableStatistics(session, first, TupleDomain.all())).isEqualTo(NON_EMPTY_STATS);
assertThat(cachingJdbcClient.getTableStatistics(session, first)).isEqualTo(NON_EMPTY_STATS);
});

// read from cache
assertStatisticsCacheStats(cachingJdbcClient).hits(1).afterRunning(() -> {
assertThat(cachingJdbcClient.getTableStatistics(session, first, TupleDomain.all())).isEqualTo(NON_EMPTY_STATS);
assertThat(cachingJdbcClient.getTableStatistics(session, first)).isEqualTo(NON_EMPTY_STATS);
});

// flush cache
Expand All @@ -610,12 +609,12 @@ public void testFlushCache()

// load table again
assertStatisticsCacheStats(cachingJdbcClient).loads(1).misses(1).afterRunning(() -> {
assertThat(cachingJdbcClient.getTableStatistics(session, secondFirst, TupleDomain.all())).isEqualTo(NON_EMPTY_STATS);
assertThat(cachingJdbcClient.getTableStatistics(session, secondFirst)).isEqualTo(NON_EMPTY_STATS);
});

// read table from cache
assertStatisticsCacheStats(cachingJdbcClient).hits(1).afterRunning(() -> {
assertThat(cachingJdbcClient.getTableStatistics(session, secondFirst, TupleDomain.all())).isEqualTo(NON_EMPTY_STATS);
assertThat(cachingJdbcClient.getTableStatistics(session, secondFirst)).isEqualTo(NON_EMPTY_STATS);
});

// cleanup
Expand Down
Loading