Skip to content

Commit

Permalink
Improve error message for catalog errors when listing metadata
Browse files Browse the repository at this point in the history 
Depending on the client that initiated a metadata query, error messages
returned from `ConnectorMetadata` can be confusing for users. For
example, a query to `system.jdbc` tables with no catalog predicate, on
exceptions in the catalog, will propagate those exceptions, with no way
for the user to know which catalog has thrown that exception.

When there are a lot of catalogs, this makes investigating the issue
very hard for Trino admins. We can make it easier by always including
the problematic catalog's name in the error message.
  • Loading branch information
@Laonel @kokosing
Laonel authored and kokosing committed Mar 14, 2023
1 parent a01e5d5 commit e9b931f
Show file tree
Hide file tree
Showing 5 changed files with 161 additions and 3 deletions.
94 changes: 94 additions & 0 deletions core/trino-main/src/main/java/io/trino/metadata/MetadataListing.java
View file
Expand Up @@ -18,6 +18,7 @@
import com.google.common.collect.ImmutableSortedSet;
import io.trino.Session;
import io.trino.security.AccessControl;
import io.trino.spi.ErrorCodeSupplier;
import io.trino.spi.TrinoException;
import io.trino.spi.connector.CatalogHandle;
import io.trino.spi.connector.ColumnMetadata;
Expand All @@ -35,6 +36,7 @@
import static com.google.common.collect.ImmutableList.toImmutableList;
import static com.google.common.collect.ImmutableMap.toImmutableMap;
import static com.google.common.collect.ImmutableSet.toImmutableSet;
import static io.trino.spi.StandardErrorCode.GENERIC_INTERNAL_ERROR;
import static io.trino.spi.StandardErrorCode.TABLE_REDIRECTION_ERROR;

public final class MetadataListing
Expand Down Expand Up @@ -82,6 +84,16 @@ public static SortedSet<String> listSchemas(Session session, Metadata metadata,
}

public static SortedSet<String> listSchemas(Session session, Metadata metadata, AccessControl accessControl, String catalogName, Optional<String> schemaName)
{
try {
return doListSchemas(session, metadata, accessControl, catalogName, schemaName);
}
catch (RuntimeException exception) {
throw handleListingException(exception, "schemas", catalogName);
}
}

private static SortedSet<String> doListSchemas(Session session, Metadata metadata, AccessControl accessControl, String catalogName, Optional<String> schemaName)
{
Set<String> schemaNames = ImmutableSet.copyOf(metadata.listSchemaNames(session, catalogName));
if (schemaName.isPresent()) {
Expand All @@ -95,6 +107,16 @@ public static SortedSet<String> listSchemas(Session session, Metadata metadata,
}

public static Set<SchemaTableName> listTables(Session session, Metadata metadata, AccessControl accessControl, QualifiedTablePrefix prefix)
{
try {
return doListTables(session, metadata, accessControl, prefix);
}
catch (RuntimeException exception) {
throw handleListingException(exception, "tables", prefix.getCatalogName());
}
}

private static Set<SchemaTableName> doListTables(Session session, Metadata metadata, AccessControl accessControl, QualifiedTablePrefix prefix)
{
Set<SchemaTableName> tableNames = metadata.listTables(session, prefix).stream()
.map(QualifiedObjectName::asSchemaTableName)
Expand All @@ -107,6 +129,16 @@ public static Set<SchemaTableName> listTables(Session session, Metadata metadata
}

public static Set<SchemaTableName> listViews(Session session, Metadata metadata, AccessControl accessControl, QualifiedTablePrefix prefix)
{
try {
return doListViews(session, metadata, accessControl, prefix);
}
catch (RuntimeException exception) {
throw handleListingException(exception, "views", prefix.getCatalogName());
}
}

private static Set<SchemaTableName> doListViews(Session session, Metadata metadata, AccessControl accessControl, QualifiedTablePrefix prefix)
{
Set<SchemaTableName> tableNames = metadata.listViews(session, prefix).stream()
.map(QualifiedObjectName::asSchemaTableName)
Expand All @@ -115,6 +147,16 @@ public static Set<SchemaTableName> listViews(Session session, Metadata metadata,
}

public static Map<SchemaTableName, ViewInfo> getViews(Session session, Metadata metadata, AccessControl accessControl, QualifiedTablePrefix prefix)
{
try {
return doGetViews(session, metadata, accessControl, prefix);
}
catch (RuntimeException exception) {
throw handleListingException(exception, "views", prefix.getCatalogName());
}
}

private static Map<SchemaTableName, ViewInfo> doGetViews(Session session, Metadata metadata, AccessControl accessControl, QualifiedTablePrefix prefix)
{
Map<SchemaTableName, ViewInfo> views = metadata.getViews(session, prefix).entrySet().stream()
.collect(toImmutableMap(entry -> entry.getKey().asSchemaTableName(), Entry::getValue));
Expand All @@ -127,6 +169,16 @@ public static Map<SchemaTableName, ViewInfo> getViews(Session session, Metadata
}

public static Set<SchemaTableName> listMaterializedViews(Session session, Metadata metadata, AccessControl accessControl, QualifiedTablePrefix prefix)
{
try {
return doListMaterializedViews(session, metadata, accessControl, prefix);
}
catch (RuntimeException exception) {
throw handleListingException(exception, "materialized views", prefix.getCatalogName());
}
}

private static Set<SchemaTableName> doListMaterializedViews(Session session, Metadata metadata, AccessControl accessControl, QualifiedTablePrefix prefix)
{
Set<SchemaTableName> tableNames = metadata.listMaterializedViews(session, prefix).stream()
.map(QualifiedObjectName::asSchemaTableName)
Expand All @@ -135,6 +187,16 @@ public static Set<SchemaTableName> listMaterializedViews(Session session, Metada
}

public static Map<SchemaTableName, ViewInfo> getMaterializedViews(Session session, Metadata metadata, AccessControl accessControl, QualifiedTablePrefix prefix)
{
try {
return doGetMaterializedViews(session, metadata, accessControl, prefix);
}
catch (RuntimeException exception) {
throw handleListingException(exception, "materialized views", prefix.getCatalogName());
}
}

private static Map<SchemaTableName, ViewInfo> doGetMaterializedViews(Session session, Metadata metadata, AccessControl accessControl, QualifiedTablePrefix prefix)
{
Map<SchemaTableName, ViewInfo> materializedViews = metadata.getMaterializedViews(session, prefix).entrySet().stream()
.collect(toImmutableMap(entry -> entry.getKey().asSchemaTableName(), Entry::getValue));
Expand All @@ -147,6 +209,16 @@ public static Map<SchemaTableName, ViewInfo> getMaterializedViews(Session sessio
}

public static Set<GrantInfo> listTablePrivileges(Session session, Metadata metadata, AccessControl accessControl, QualifiedTablePrefix prefix)
{
try {
return doListTablePrivileges(session, metadata, accessControl, prefix);
}
catch (RuntimeException exception) {
throw handleListingException(exception, "table privileges", prefix.getCatalogName());
}
}

private static Set<GrantInfo> doListTablePrivileges(Session session, Metadata metadata, AccessControl accessControl, QualifiedTablePrefix prefix)
{
List<GrantInfo> grants = metadata.listTablePrivileges(session, prefix);
Set<SchemaTableName> allowedTables = accessControl.filterTables(
Expand All @@ -160,6 +232,16 @@ public static Set<GrantInfo> listTablePrivileges(Session session, Metadata metad
}

public static Map<SchemaTableName, List<ColumnMetadata>> listTableColumns(Session session, Metadata metadata, AccessControl accessControl, QualifiedTablePrefix prefix)
{
try {
return doListTableColumns(session, metadata, accessControl, prefix);
}
catch (RuntimeException exception) {
throw handleListingException(exception, "table columns", prefix.getCatalogName());
}
}

private static Map<SchemaTableName, List<ColumnMetadata>> doListTableColumns(Session session, Metadata metadata, AccessControl accessControl, QualifiedTablePrefix prefix)
{
List<TableColumnsMetadata> catalogColumns = metadata.listTableColumns(session, prefix);

Expand Down Expand Up @@ -233,4 +315,16 @@ public static Map<SchemaTableName, List<ColumnMetadata>> listTableColumns(Sessio

return result.buildOrThrow();
}

private static TrinoException handleListingException(RuntimeException exception, String type, String catalogName)
{
ErrorCodeSupplier result = GENERIC_INTERNAL_ERROR;
if (exception instanceof TrinoException trinoException) {
result = trinoException::getErrorCode;
}
return new TrinoException(
result,
"Error listing %s for catalog %s: %s".formatted(type, catalogName, exception.getMessage()),
exception);
}
}
2 changes: 1 addition & 1 deletion ...in/trino-base-jdbc/src/test/java/io/trino/plugin/jdbc/BaseCaseInsensitiveMappingTest.java
View file
Expand Up @@ -145,7 +145,7 @@ public void testSchemaNameClash()
AutoCloseable ignore4 = withSchema("some_schema");
AutoCloseable ignore5 = withTable("some_schema", "some_table", "(c int)")) {
assertThat(computeActual("SHOW SCHEMAS").getOnlyColumn().filter("casesensitivename"::equals)).hasSize(1); // TODO change io.trino.plugin.jdbc.JdbcClient.getSchemaNames to return a List
assertQueryFails("SHOW TABLES FROM casesensitivename", "Failed to find remote schema name: Ambiguous name: casesensitivename");
assertQueryFails("SHOW TABLES FROM casesensitivename", "Error listing tables for catalog \\w+: Failed to find remote schema name: Ambiguous name: casesensitivename");
assertQueryFails("SELECT * FROM casesensitivename.some_table_name", "Failed to find remote schema name: Ambiguous name: casesensitivename");
assertQuery("SHOW TABLES FROM some_schema", "VALUES 'some_table'");
assertQueryReturnsEmptyResult("SELECT * FROM some_schema.some_table");
Expand Down
2 changes: 1 addition & 1 deletion ...n/trino-cassandra/src/test/java/io/trino/plugin/cassandra/TestCassandraConnectorTest.java
View file
Expand Up @@ -919,7 +919,7 @@ public void testKeyspaceNameAmbiguity()
// There is no way to figure out what the exactly keyspace we want to retrieve tables from
assertQueryFailsEventually(
"SHOW TABLES FROM cassandra.keyspace_3",
"More than one keyspace has been found for the case insensitive schema name: keyspace_3 -> \\(KeYsPaCe_3, kEySpAcE_3\\)",
"Error listing tables for catalog cassandra: More than one keyspace has been found for the case insensitive schema name: keyspace_3 -> \\(KeYsPaCe_3, kEySpAcE_3\\)",
new Duration(1, MINUTES));

session.execute("DROP KEYSPACE \"KeYsPaCe_3\"");
Expand Down
2 changes: 1 addition & 1 deletion ...ino-pinot/src/test/java/io/trino/plugin/pinot/BasePinotIntegrationConnectorSmokeTest.java
View file
Expand Up @@ -1214,7 +1214,7 @@ public void testAmbiguousTables()
assertQueryFails("SELECT * FROM " + DUPLICATE_TABLE_MIXED_CASE, "Ambiguous table names: (" + DUPLICATE_TABLE_LOWERCASE + ", " + DUPLICATE_TABLE_MIXED_CASE + "|" + DUPLICATE_TABLE_MIXED_CASE + ", " + DUPLICATE_TABLE_LOWERCASE + ")");
assertQueryFails("SELECT * FROM \"SELECT * FROM " + DUPLICATE_TABLE_LOWERCASE + "\"", "Ambiguous table names: (" + DUPLICATE_TABLE_LOWERCASE + ", " + DUPLICATE_TABLE_MIXED_CASE + "|" + DUPLICATE_TABLE_MIXED_CASE + ", " + DUPLICATE_TABLE_LOWERCASE + ")");
assertQueryFails("SELECT * FROM \"SELECT * FROM " + DUPLICATE_TABLE_MIXED_CASE + "\"", "Ambiguous table names: (" + DUPLICATE_TABLE_LOWERCASE + ", " + DUPLICATE_TABLE_MIXED_CASE + "|" + DUPLICATE_TABLE_MIXED_CASE + ", " + DUPLICATE_TABLE_LOWERCASE + ")");
assertQueryFails("SELECT * FROM information_schema.columns", "Ambiguous table names: (" + DUPLICATE_TABLE_LOWERCASE + ", " + DUPLICATE_TABLE_MIXED_CASE + "|" + DUPLICATE_TABLE_MIXED_CASE + ", " + DUPLICATE_TABLE_LOWERCASE + ")");
assertQueryFails("SELECT * FROM information_schema.columns", "Error listing table columns for catalog pinot: Ambiguous table names: (" + DUPLICATE_TABLE_LOWERCASE + ", " + DUPLICATE_TABLE_MIXED_CASE + "|" + DUPLICATE_TABLE_MIXED_CASE + ", " + DUPLICATE_TABLE_LOWERCASE + ")");
}

@Test
Expand Down
64 changes: 64 additions & 0 deletions testing/trino-tests/src/test/java/io/trino/tests/TestInformationSchemaConnector.java
View file
Expand Up @@ -13,15 +13,21 @@
*/
package io.trino.tests;

import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
import io.trino.Session;
import io.trino.connector.MockConnectorFactory;
import io.trino.plugin.tpch.TpchPlugin;
import io.trino.spi.Plugin;
import io.trino.spi.TrinoException;
import io.trino.spi.connector.ConnectorFactory;
import io.trino.testing.AbstractTestQueryFramework;
import io.trino.testing.CountingMockConnector;
import io.trino.testing.CountingMockConnector.MetadataCallsCount;
import io.trino.testing.DistributedQueryRunner;
import org.testng.annotations.Test;

import static io.trino.spi.StandardErrorCode.GENERIC_INTERNAL_ERROR;
import static io.trino.testing.TestingSession.testSessionBuilder;
import static org.testng.Assert.assertEquals;

Expand Down Expand Up @@ -218,6 +224,30 @@ public void testMetadataCalls()
.withListSchemasCount(1));
}

@Test
public void testMetadataListingExceptionHandling()
{
assertQueryFails(
"SELECT * FROM broken_catalog.information_schema.schemata",
"Error listing schemas for catalog broken_catalog: Catalog is broken");

assertQueryFails(
"SELECT * FROM broken_catalog.information_schema.tables",
"Error listing tables for catalog broken_catalog: Catalog is broken");

assertQueryFails(
"SELECT * FROM broken_catalog.information_schema.views",
"Error listing views for catalog broken_catalog: Catalog is broken");

assertQueryFails(
"SELECT * FROM broken_catalog.information_schema.table_privileges",
"Error listing table privileges for catalog broken_catalog: Catalog is broken");

assertQueryFails(
"SELECT * FROM broken_catalog.information_schema.columns",
"Error listing table columns for catalog broken_catalog: Catalog is broken");
}

@Override
protected DistributedQueryRunner createQueryRunner()
throws Exception
Expand All @@ -232,6 +262,9 @@ protected DistributedQueryRunner createQueryRunner()

queryRunner.installPlugin(countingMockConnector.getPlugin());
queryRunner.createCatalog("test_catalog", "mock", ImmutableMap.of());

queryRunner.installPlugin(new FailingMockConnectorPlugin());
queryRunner.createCatalog("broken_catalog", "failing_mock", ImmutableMap.of());
return queryRunner;
}
catch (Exception e) {
Expand All @@ -249,4 +282,35 @@ private void assertMetadataCalls(String actualSql, String expectedSql, MetadataC

assertEquals(actualMetadataCallsCount, expectedMetadataCallsCount);
}

private static final class FailingMockConnectorPlugin
implements Plugin
{
@Override
public Iterable<ConnectorFactory> getConnectorFactories()
{
return ImmutableList.of(
MockConnectorFactory.builder()
.withName("failing_mock")
.withListSchemaNames(session -> {
throw new TrinoException(GENERIC_INTERNAL_ERROR, "Catalog is broken");
})
.withListTables((session, schema) -> {
throw new TrinoException(GENERIC_INTERNAL_ERROR, "Catalog is broken");
})
.withGetViews((session, prefix) -> {
throw new TrinoException(GENERIC_INTERNAL_ERROR, "Catalog is broken");
})
.withGetMaterializedViews((session, prefix) -> {
throw new TrinoException(GENERIC_INTERNAL_ERROR, "Catalog is broken");
})
.withListTablePrivileges((session, prefix) -> {
throw new TrinoException(GENERIC_INTERNAL_ERROR, "Catalog is broken");
})
.withStreamTableColumns((session, prefix) -> {
throw new TrinoException(GENERIC_INTERNAL_ERROR, "Catalog is broken");
})
.build());
}
}
}

0 comments on commit e9b931f

Please sign in to comment.