Skip to content

Commit

Permalink
Fix HiveMetadata.streamTableColumns not to include views
Browse files Browse the repository at this point in the history
In the absence of explicit documentation, the use place
(`io.trino.metadata.MetadataManager#listTableColumns`) defines
`streamTableColumns` contract as covering tables only.  This commit also
supplements explicit documentation in `ConnectorMetadata`.

This relates to a logic introduced in `HiveMetadata.doGetTableMetadata`
in aafe479 commit. It was practical for
hive views and when view translation is not enabled, but was not correct
for trino views.
  • Loading branch information
findepi committed Jul 20, 2023
1 parent 50a41fd commit dc46f57
Show file tree
Hide file tree
Showing 3 changed files with 24 additions and 5 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -286,7 +286,7 @@ default ColumnMetadata getColumnMetadata(ConnectorSession session, ConnectorTabl
}

/**
* Gets the metadata for all columns that match the specified table prefix.
* Gets the metadata for all columns that match the specified table prefix. Columns of views and materialized views are not included.
*
* @deprecated use {@link #streamTableColumns} which handles redirected tables
*/
Expand All @@ -298,7 +298,7 @@ default Map<SchemaTableName, List<ColumnMetadata>> listTableColumns(ConnectorSes

/**
* Gets the metadata for all columns that match the specified table prefix. Redirected table names are included, but
* the column metadata for them is not.
* the column metadata for them is not. Views and materialized views are not included.
*/
default Iterator<TableColumnsMetadata> streamTableColumns(ConnectorSession session, SchemaTablePrefix prefix)
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -257,6 +257,7 @@
import static io.trino.plugin.hive.ViewReaderUtil.encodeViewData;
import static io.trino.plugin.hive.ViewReaderUtil.isHiveOrPrestoView;
import static io.trino.plugin.hive.ViewReaderUtil.isPrestoView;
import static io.trino.plugin.hive.ViewReaderUtil.isTrinoMaterializedView;
import static io.trino.plugin.hive.acid.AcidTransaction.NO_ACID_TRANSACTION;
import static io.trino.plugin.hive.acid.AcidTransaction.forCreateTable;
import static io.trino.plugin.hive.metastore.MetastoreUtil.buildInitialPrivilegeSet;
Expand Down Expand Up @@ -614,7 +615,24 @@ private ConnectorTableMetadata doGetTableMetadata(ConnectorSession session, Sche
throw new TrinoException(UNSUPPORTED_TABLE_TYPE, format("Not a Hive table '%s'", tableName));
}

if (!translateHiveViews && isHiveOrPrestoView(table)) {
boolean isTrinoView = isPrestoView(table);
boolean isHiveView = !isTrinoView && isHiveOrPrestoView(table);
boolean isTrinoMaterializedView = isTrinoMaterializedView(table);
if (isHiveView && translateHiveViews) {
// Produce metadata for a (translated) Hive view as if it was a table. This is incorrect from ConnectorMetadata.streamTableColumns
// perspective, but is done on purpose to keep information_schema.columns working.
// Because of fallback in ThriftHiveMetastoreClient.getAllViews, this method may return Trino/Presto views only,
// so HiveMetadata.getViews may fail to return Hive views.
}
else if (isHiveView) {
// When Hive view translation is not enabled, a Hive view is currently treated inconsistently
// - getView treats this as an unusable view (fails instead of returning Optional.empty)
// - getTableHandle treats this as a table (returns non-null)
// In any case, returning metadata is not useful.
throw new TableNotFoundException(tableName);
}
else if (isTrinoView || isTrinoMaterializedView) {
// streamTableColumns should not include views and materialized views
throw new TableNotFoundException(tableName);
}

Expand Down Expand Up @@ -837,6 +855,7 @@ private Stream<TableColumnsMetadata> streamTableColumns(ConnectorSession session
return Stream.empty();
}
catch (TableNotFoundException e) {
// it is not a table (e.g. it's a view) (TODO remove exception-driven logic for this case) OR
// table disappeared during listing operation
return Stream.empty();
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,7 @@ private void testFailingHiveViewsWithInformationSchema()
.hasMessageContaining("Failed to translate Hive view 'test_list_failing_views.failing_view'");

// Queries on information_schema.columns also trigger ConnectorMetadata#getViews. Columns from failing_view are
// listed too since HiveMetadata#listTableColumns does not ignore views.
// listed too since HiveMetadata#listTableColumns does not ignore Hive views.
assertThat(onTrino().executeQuery("SELECT table_name, column_name FROM information_schema.columns WHERE table_schema = 'test_list_failing_views'"))
.containsOnly(
row("correct_view", "n_nationkey"),
Expand Down Expand Up @@ -141,7 +141,7 @@ private void testFailingHiveViewsWithSystemJdbc()
.hasMessageContaining("Failed to translate Hive view 'test_list_failing_views.failing_view'");

// Queries on system.jdbc.columns also trigger ConnectorMetadata#getViews. Columns from failing_view are
// listed too since HiveMetadata#listTableColumns does not ignore views.
// listed too since HiveMetadata#listTableColumns does not ignore Hive views.
assertThat(onTrino().executeQuery("SELECT table_name, column_name FROM system.jdbc.columns WHERE table_cat = 'hive' AND table_schem = 'test_list_failing_views'"))
.containsOnly(
row("correct_view", "n_nationkey"),
Expand Down

0 comments on commit dc46f57

Please sign in to comment.