Skip to content

Remove redundant table metadata calls #17269

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 2 commits into from
Apr 28, 2023
Merged

Remove redundant table metadata calls #17269

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
dd2a2eb
Simplify query pass-through handle creation
findepi Apr 27, 2023
220afce
Remove redundant table metadata calls
findepi Apr 27, 2023
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
9 changes: 5 additions & 4 deletions core/trino-main/src/main/java/io/trino/execution/TableInfo.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@
import io.trino.metadata.Metadata;
import io.trino.metadata.QualifiedObjectName;
import io.trino.metadata.TableProperties;
import io.trino.metadata.TableSchema;
import io.trino.spi.connector.CatalogSchemaTableName;
import io.trino.spi.connector.ColumnHandle;
import io.trino.spi.predicate.TupleDomain;
import io.trino.sql.planner.PlanFragment;
Expand Down Expand Up @@ -86,13 +86,14 @@ public static Map<PlanNodeId, TableInfo> extract(Session session, Metadata metad

private static TableInfo extract(Session session, Metadata metadata, TableScanNode node)
{
TableSchema tableSchema = metadata.getTableSchema(session, node.getTable());
Copy link
Member

@kokosing kokosing Apr 28, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we still nee getTableSchema? It was supposed to be faster than getting table metadata and it is still slow anyway?

Copy link
Member Author

@findepi findepi Apr 28, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good question. io.trino.metadata.Metadata#getTableSchema is still used & not going away

yes, io.trino.metadata.Metadata#getTableSchema is faster than getTableMetadata, but getTableName is even faster (usually no-op, since table handles typically contain the name)

CatalogSchemaTableName tableName = metadata.getTableName(session, node.getTable());
TableProperties tableProperties = metadata.getTableProperties(session, node.getTable());
Optional<String> connectorName = metadata.listCatalogs(session).stream()
.filter(catalogInfo -> catalogInfo.getCatalogName().equals(tableSchema.getCatalogName()))
.filter(catalogInfo -> catalogInfo.getCatalogName().equals(tableName.getCatalogName()))
.map(CatalogInfo::getConnectorName)
.map(ConnectorName::toString)
.findFirst();
return new TableInfo(connectorName, tableSchema.getQualifiedName(), tableProperties.getPredicate());
QualifiedObjectName objectName = new QualifiedObjectName(tableName.getCatalogName(), tableName.getSchemaTableName().getSchemaName(), tableName.getSchemaTableName().getTableName());
return new TableInfo(connectorName, objectName, tableProperties.getPredicate());
}
}
2 changes: 2 additions & 0 deletions core/trino-main/src/main/java/io/trino/metadata/Metadata.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -122,6 +122,8 @@ Optional<TableExecuteHandle> getTableHandleForExecute(

Optional<Object> getInfo(Session session, TableHandle handle);

CatalogSchemaTableName getTableName(Session session, TableHandle tableHandle);

/**
* Return table schema definition for the specified table handle.
* Table schema definition is a set of information
Expand Down
11 changes: 11 additions & 0 deletions core/trino-main/src/main/java/io/trino/metadata/MetadataManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -432,6 +432,17 @@ public Optional<Object> getInfo(Session session, TableHandle handle)
return metadata.getInfo(handle.getConnectorHandle());
}

@Override
public CatalogSchemaTableName getTableName(Session session, TableHandle tableHandle)
{
CatalogHandle catalogHandle = tableHandle.getCatalogHandle();
CatalogMetadata catalogMetadata = getCatalogMetadata(session, catalogHandle);
ConnectorMetadata metadata = catalogMetadata.getMetadataFor(session, catalogHandle);
SchemaTableName tableName = metadata.getTableName(session.toConnectorSession(catalogHandle), tableHandle.getConnectorHandle());

return new CatalogSchemaTableName(catalogMetadata.getCatalogName(), tableName);
}

@Override
public TableSchema getTableSchema(Session session, TableHandle tableHandle)
{
Expand Down
8 changes: 4 additions & 4 deletions core/trino-main/src/main/java/io/trino/sql/planner/InputExtractor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@
import io.trino.execution.Input;
import io.trino.metadata.Metadata;
import io.trino.metadata.TableHandle;
import io.trino.metadata.TableSchema;
import io.trino.spi.connector.CatalogSchemaTableName;
import io.trino.spi.connector.ColumnHandle;
import io.trino.spi.connector.ColumnMetadata;
import io.trino.spi.connector.SchemaTableName;
Expand Down Expand Up @@ -64,10 +64,10 @@ private static Column createColumn(ColumnMetadata columnMetadata)

private Input createInput(Session session, TableHandle table, Set<Column> columns, PlanFragmentId fragmentId, PlanNodeId planNodeId)
{
TableSchema tableSchema = metadata.getTableSchema(session, table);
SchemaTableName schemaTable = tableSchema.getTable();
CatalogSchemaTableName tableName = metadata.getTableName(session, table);
SchemaTableName schemaTable = tableName.getSchemaTableName();
Optional<Object> inputMetadata = metadata.getInfo(session, table);
return new Input(tableSchema.getCatalogName(), schemaTable.getSchemaName(), schemaTable.getTableName(), inputMetadata, ImmutableList.copyOf(columns), fragmentId, planNodeId);
return new Input(tableName.getCatalogName(), schemaTable.getSchemaName(), schemaTable.getTableName(), inputMetadata, ImmutableList.copyOf(columns), fragmentId, planNodeId);
}

private class Visitor
Expand Down
2 changes: 1 addition & 1 deletion core/trino-main/src/main/java/io/trino/sql/planner/QueryPlanner.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -881,7 +881,7 @@ private MergeWriterNode createMergePipeline(Table table, RelationPlan relationPl
columnNamesBuilder.add(columnSchema.getName());
});
MergeParadigmAndTypes mergeParadigmAndTypes = new MergeParadigmAndTypes(Optional.of(paradigm), typesBuilder.build(), columnNamesBuilder.build(), rowIdType);
MergeTarget mergeTarget = new MergeTarget(handle, Optional.empty(), metadata.getTableMetadata(session, handle).getTable(), mergeParadigmAndTypes);
MergeTarget mergeTarget = new MergeTarget(handle, Optional.empty(), metadata.getTableName(session, handle).getSchemaTableName(), mergeParadigmAndTypes);

ImmutableList.Builder<Symbol> columnSymbolsBuilder = ImmutableList.builder();
for (ColumnHandle columnHandle : mergeAnalysis.getDataColumnHandles()) {
Expand Down
4 changes: 1 addition & 3 deletions ...ino-main/src/main/java/io/trino/sql/planner/iterative/rule/ApplyTableScanRedirection.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,6 @@
import io.trino.matching.Pattern;
import io.trino.metadata.QualifiedObjectName;
import io.trino.metadata.TableHandle;
import io.trino.metadata.TableMetadata;
import io.trino.spi.TrinoException;
import io.trino.spi.connector.CatalogSchemaTableName;
import io.trino.spi.connector.ColumnHandle;
Expand Down Expand Up @@ -95,8 +94,7 @@ public Result apply(TableScanNode scanNode, Captures captures, Context context)
throw new TrinoException(NOT_SUPPORTED, format("Further redirection of destination table '%s' to '%s' is not supported", destinationObjectName, name));
});

TableMetadata tableMetadata = plannerContext.getMetadata().getTableMetadata(context.getSession(), scanNode.getTable());
CatalogSchemaTableName sourceTable = new CatalogSchemaTableName(tableMetadata.getCatalogName(), tableMetadata.getTable());
CatalogSchemaTableName sourceTable = plannerContext.getMetadata().getTableName(context.getSession(), scanNode.getTable());
if (destinationTable.equals(sourceTable)) {
return Result.empty();
}
Expand Down
4 changes: 2 additions & 2 deletions core/trino-main/src/main/java/io/trino/sql/planner/optimizations/BeginTableWrite.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -253,7 +253,7 @@ private WriterTarget createWriterTarget(WriterTarget target)
if (target instanceof InsertReference insert) {
return new InsertTarget(
metadata.beginInsert(session, insert.getHandle(), insert.getColumns()),
metadata.getTableMetadata(session, insert.getHandle()).getTable(),
metadata.getTableName(session, insert.getHandle()).getSchemaTableName(),
target.supportsReportingWrittenBytes(metadata, session),
target.supportsMultipleWritersPerPartition(metadata, session),
target.getMaxWriterTasks(metadata, session));
Expand All @@ -270,7 +270,7 @@ private WriterTarget createWriterTarget(WriterTarget target)
return new TableWriterNode.RefreshMaterializedViewTarget(
refreshMV.getStorageTableHandle(),
metadata.beginRefreshMaterializedView(session, refreshMV.getStorageTableHandle(), refreshMV.getSourceTableHandles()),
metadata.getTableMetadata(session, refreshMV.getStorageTableHandle()).getTable(),
metadata.getTableName(session, refreshMV.getStorageTableHandle()).getSchemaTableName(),
refreshMV.getSourceTableHandles());
}
if (target instanceof TableExecuteTarget tableExecute) {
Expand Down
9 changes: 4 additions & 5 deletions core/trino-main/src/main/java/io/trino/sql/planner/planprinter/IoPlanPrinter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@
import io.trino.cost.PlanNodeStatsEstimate;
import io.trino.cost.StatsAndCosts;
import io.trino.metadata.TableHandle;
import io.trino.metadata.TableMetadata;
import io.trino.spi.connector.CatalogSchemaTableName;
import io.trino.spi.connector.ColumnHandle;
import io.trino.spi.connector.ColumnMetadata;
Expand Down Expand Up @@ -729,15 +728,15 @@ else if (writerTarget instanceof CreateReference || writerTarget instanceof Inse
private void addInputTableConstraints(TupleDomain<ColumnHandle> filterDomain, TableScanNode tableScan, IoPlanBuilder context)
{
TableHandle table = tableScan.getTable();
TableMetadata tableMetadata = plannerContext.getMetadata().getTableMetadata(session, table);
CatalogSchemaTableName tableName = plannerContext.getMetadata().getTableName(session, table);
TupleDomain<ColumnHandle> predicateDomain = plannerContext.getMetadata().getTableProperties(session, table).getPredicate();
EstimatedStatsAndCost estimatedStatsAndCost = getEstimatedStatsAndCost(tableScan);
context.addInputTableColumnInfo(
new IoPlan.TableColumnInfo(
new CatalogSchemaTableName(
tableMetadata.getCatalogName(),
tableMetadata.getTable().getSchemaName(),
tableMetadata.getTable().getTableName()),
tableName.getCatalogName(),
tableName.getSchemaTableName().getSchemaName(),
tableName.getSchemaTableName().getTableName()),
parseConstraint(table, predicateDomain.intersect(filterDomain)),
estimatedStatsAndCost));
}
Expand Down
10 changes: 6 additions & 4 deletions core/trino-main/src/main/java/io/trino/sql/planner/planprinter/TableInfoSupplier.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,8 +18,9 @@
import io.trino.execution.TableInfo;
import io.trino.metadata.CatalogInfo;
import io.trino.metadata.Metadata;
import io.trino.metadata.QualifiedObjectName;
import io.trino.metadata.TableProperties;
import io.trino.metadata.TableSchema;
import io.trino.spi.connector.CatalogSchemaTableName;
import io.trino.sql.planner.plan.TableScanNode;

import java.util.Optional;
Expand All @@ -42,13 +43,14 @@ public TableInfoSupplier(Metadata metadata, Session session)
@Override
public TableInfo apply(TableScanNode node)
{
TableSchema tableSchema = metadata.getTableSchema(session, node.getTable());
CatalogSchemaTableName tableName = metadata.getTableName(session, node.getTable());
TableProperties tableProperties = metadata.getTableProperties(session, node.getTable());
Optional<String> connectorName = metadata.listCatalogs(session).stream()
.filter(catalogInfo -> catalogInfo.getCatalogName().equals(tableSchema.getCatalogName()))
.filter(catalogInfo -> catalogInfo.getCatalogName().equals(tableName.getCatalogName()))
.map(CatalogInfo::getConnectorName)
.map(ConnectorName::toString)
.findFirst();
return new TableInfo(connectorName, tableSchema.getQualifiedName(), tableProperties.getPredicate());
QualifiedObjectName objectName = new QualifiedObjectName(tableName.getCatalogName(), tableName.getSchemaTableName().getSchemaName(), tableName.getSchemaTableName().getTableName());
return new TableInfo(connectorName, objectName, tableProperties.getPredicate());
}
}
9 changes: 9 additions & 0 deletions core/trino-main/src/main/java/io/trino/tracing/TracingConnectorMetadata.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -216,6 +216,15 @@ public Optional<ConnectorPartitioningHandle> getCommonPartitioningHandle(Connect
}
}

@Override
public SchemaTableName getTableName(ConnectorSession session, ConnectorTableHandle table)
{
Span span = startSpan("getTableName", table);
try (var ignored = scopedSpan(span)) {
return delegate.getTableName(session, table);
}
}

@SuppressWarnings("deprecation")
@Override
public SchemaTableName getSchemaTableName(ConnectorSession session, ConnectorTableHandle table)
Expand Down
9 changes: 9 additions & 0 deletions core/trino-main/src/main/java/io/trino/tracing/TracingMetadata.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -261,6 +261,15 @@ public Optional<Object> getInfo(Session session, TableHandle handle)
}
}

@Override
public CatalogSchemaTableName getTableName(Session session, TableHandle tableHandle)
{
Span span = startSpan("getTableName", tableHandle);
try (var ignored = scopedSpan(span)) {
return delegate.getTableName(session, tableHandle);
}
}

@Override
public TableSchema getTableSchema(Session session, TableHandle tableHandle)
{
Expand Down
6 changes: 6 additions & 0 deletions core/trino-main/src/test/java/io/trino/metadata/AbstractMockMetadata.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -186,6 +186,12 @@ public Optional<Object> getInfo(Session session, TableHandle handle)
throw new UnsupportedOperationException();
}

@Override
public CatalogSchemaTableName getTableName(Session session, TableHandle tableHandle)
{
throw new UnsupportedOperationException();
}

@Override
public TableSchema getTableSchema(Session session, TableHandle tableHandle)
{
Expand Down
6 changes: 6 additions & 0 deletions core/trino-main/src/test/java/io/trino/metadata/CountingAccessMetadata.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -189,6 +189,12 @@ public Optional<Object> getInfo(Session session, TableHandle handle)
return delegate.getInfo(session, handle);
}

@Override
public CatalogSchemaTableName getTableName(Session session, TableHandle tableHandle)
{
return delegate.getTableName(session, tableHandle);
}

@Override
public TableSchema getTableSchema(Session session, TableHandle tableHandle)
{
Expand Down
6 changes: 3 additions & 3 deletions core/trino-main/src/test/java/io/trino/sql/planner/assertions/ColumnReference.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@
import io.trino.Session;
import io.trino.metadata.Metadata;
import io.trino.metadata.TableHandle;
import io.trino.metadata.TableMetadata;
import io.trino.spi.connector.CatalogSchemaTableName;
import io.trino.spi.connector.ColumnHandle;
import io.trino.sql.planner.Symbol;
import io.trino.sql.planner.plan.IndexSourceNode;
Expand Down Expand Up @@ -60,8 +60,8 @@ else if (node instanceof IndexSourceNode indexSourceNode) {
return Optional.empty();
}

TableMetadata tableMetadata = metadata.getTableMetadata(session, tableHandle);
String actualTableName = tableMetadata.getTable().getTableName();
CatalogSchemaTableName fullTableName = metadata.getTableName(session, tableHandle);
String actualTableName = fullTableName.getSchemaTableName().getTableName();

// Wrong table -> doesn't match.
if (!tableName.equalsIgnoreCase(actualTableName)) {
Expand Down
6 changes: 3 additions & 3 deletions core/trino-main/src/test/java/io/trino/sql/planner/assertions/IndexSourceMatcher.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@
import io.trino.Session;
import io.trino.cost.StatsProvider;
import io.trino.metadata.Metadata;
import io.trino.metadata.TableMetadata;
import io.trino.spi.connector.CatalogSchemaTableName;
import io.trino.sql.planner.plan.IndexSourceNode;
import io.trino.sql.planner.plan.PlanNode;

Expand Down Expand Up @@ -48,8 +48,8 @@ public MatchResult detailMatches(PlanNode node, StatsProvider stats, Session ses
checkState(shapeMatches(node), "Plan testing framework error: shapeMatches returned false in detailMatches in %s", this.getClass().getName());

IndexSourceNode indexSourceNode = (IndexSourceNode) node;
TableMetadata tableMetadata = metadata.getTableMetadata(session, indexSourceNode.getTableHandle());
String actualTableName = tableMetadata.getTable().getTableName();
CatalogSchemaTableName tableName = metadata.getTableName(session, indexSourceNode.getTableHandle());
String actualTableName = tableName.getSchemaTableName().getTableName();

if (!expectedTableName.equalsIgnoreCase(actualTableName)) {
return NO_MATCH;
Expand Down
6 changes: 3 additions & 3 deletions core/trino-main/src/test/java/io/trino/sql/planner/assertions/TableScanMatcher.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@
import io.trino.cost.StatsProvider;
import io.trino.metadata.Metadata;
import io.trino.metadata.TableHandle;
import io.trino.metadata.TableMetadata;
import io.trino.spi.connector.CatalogSchemaTableName;
import io.trino.spi.connector.ColumnHandle;
import io.trino.spi.predicate.Domain;
import io.trino.spi.predicate.TupleDomain;
Expand Down Expand Up @@ -58,8 +58,8 @@ public MatchResult detailMatches(PlanNode node, StatsProvider stats, Session ses
checkState(shapeMatches(node), "Plan testing framework error: shapeMatches returned false in detailMatches in %s", this.getClass().getName());

TableScanNode tableScanNode = (TableScanNode) node;
TableMetadata tableMetadata = metadata.getTableMetadata(session, tableScanNode.getTable());
String actualTableName = tableMetadata.getTable().getTableName();
CatalogSchemaTableName tableName = metadata.getTableName(session, tableScanNode.getTable());
String actualTableName = tableName.getSchemaTableName().getTableName();

// TODO (https://github.com/trinodb/trino/issues/17) change to equals()
if (!expectedTableName.equalsIgnoreCase(actualTableName)) {
Expand Down
13 changes: 12 additions & 1 deletion 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 @@ -223,7 +223,18 @@ default Optional<ConnectorPartitioningHandle> getCommonPartitioningHandle(Connec
*
* @throws RuntimeException if table handle is no longer valid
*/
@Deprecated // ... and optimized implementations already removed
default SchemaTableName getTableName(ConnectorSession session, ConnectorTableHandle table)
Copy link
Member

@kokosing kokosing Apr 28, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we need to do this refactor (rename)? Maybe separate commit?

Copy link
Member Author

@findepi findepi Apr 28, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i did this because this method was deprecated and almost removed, i didn't want ot add new usages for a method that's already known not to be implemented
would it help if we do #17267 first?

{
return getSchemaTableName(session, table);
Copy link
Contributor

@findinpath findinpath Apr 28, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is the new method calling a deprecated method?

Copy link
Member Author

@findepi findepi Apr 28, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

because getSchemaTableName already existed and can be implemented by a connector

this PR is effectively rename (from ConnectorMetadata perspective) and this is how you handle a rename in a backwards-compatible way

}

/**
* Return schema table name for the specified table handle.
* This method is useful when requiring only {@link SchemaTableName} without other objects.
*
* @throws RuntimeException if table handle is no longer valid
*/
@Deprecated // replaced with getTableName
default SchemaTableName getSchemaTableName(ConnectorSession session, ConnectorTableHandle table)
{
return getTableSchema(session, table).getTable();
Expand Down
8 changes: 8 additions & 0 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 @@ -241,6 +241,14 @@ public Optional<SystemTable> getSystemTable(ConnectorSession session, SchemaTabl
}
}

@Override
public SchemaTableName getTableName(ConnectorSession session, ConnectorTableHandle table)
{
try (ThreadContextClassLoader ignored = new ThreadContextClassLoader(classLoader)) {
return delegate.getTableName(session, table);
}
}

@Override
public ConnectorTableSchema getTableSchema(ConnectorSession session, ConnectorTableHandle table)
{
Expand Down
Loading