Skip to content

Refactor is*View ViewReaderUtil methods #18570

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 9 commits into from
Aug 11, 2023
Merged

Refactor is*View ViewReaderUtil methods #18570

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
0b79e1d
Inline condition and fix comment
findepi Aug 7, 2023
f637594
Remove dead code in TrinoViewUtil.getView
findepi Aug 7, 2023
edcd875
Improve comment accuracy
findepi Aug 7, 2023
335c414
Fix check in HiveMetadata.getView and update its name
findepi Aug 7, 2023
a85df69
Fix isTrinoView variable initialization
findepi Aug 8, 2023
a094339
Merge Optional.filter with Optional.map calls
findepi Aug 7, 2023
3360465
Refactor `is*View` ViewReaderUtil methods
findepi Aug 7, 2023
c340dc8
Use "looks like some view" utility in more places
findepi Aug 7, 2023
4ea0a43
Document HiveMetastore.getAllViews to be including materialized views.
findepi Aug 7, 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
6 changes: 3 additions & 3 deletions .../main/java/io/trino/plugin/deltalake/metastore/HiveMetastoreBackedDeltaLakeMetastore.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@
import static io.trino.plugin.deltalake.DeltaLakeErrorCode.DELTA_LAKE_INVALID_SCHEMA;
import static io.trino.plugin.deltalake.DeltaLakeMetadata.PATH_PROPERTY;
import static io.trino.plugin.hive.TableType.MANAGED_TABLE;
import static io.trino.plugin.hive.ViewReaderUtil.isHiveOrPrestoView;
import static io.trino.plugin.hive.ViewReaderUtil.isSomeKindOfAView;
import static java.lang.String.format;
import static java.util.Objects.requireNonNull;

Expand Down Expand Up @@ -86,8 +86,8 @@ public Optional<DeltaMetastoreTable> getTable(String databaseName, String tableN

public static void verifyDeltaLakeTable(Table table)
{
if (isHiveOrPrestoView(table)) {
// this is a Hive view, hence not a table
if (isSomeKindOfAView(table)) {
// Looks like a view, so not a table
throw new NotADeltaLakeTableException(table.getDatabaseName(), table.getTableName());
}
if (!TABLE_PROVIDER_VALUE.equalsIgnoreCase(table.getParameters().get(TABLE_PROVIDER_PROPERTY))) {
Expand Down
41 changes: 26 additions & 15 deletions plugin/trino-hive/src/main/java/io/trino/plugin/hive/HiveMetadata.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -255,9 +255,10 @@
import static io.trino.plugin.hive.ViewReaderUtil.PRESTO_VIEW_FLAG;
import static io.trino.plugin.hive.ViewReaderUtil.createViewReader;
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.isHiveView;
import static io.trino.plugin.hive.ViewReaderUtil.isSomeKindOfAView;
import static io.trino.plugin.hive.ViewReaderUtil.isTrinoMaterializedView;
import static io.trino.plugin.hive.ViewReaderUtil.isTrinoView;
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 @@ -615,8 +616,8 @@ private ConnectorTableMetadata doGetTableMetadata(ConnectorSession session, Sche
throw new TrinoException(UNSUPPORTED_TABLE_TYPE, format("Not a Hive table '%s'", tableName));
}

boolean isTrinoView = isPrestoView(table);
boolean isHiveView = !isTrinoView && isHiveOrPrestoView(table);
boolean isTrinoView = isTrinoView(table);
boolean isHiveView = isHiveView(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
Expand Down Expand Up @@ -1455,7 +1456,7 @@ public void setTableComment(ConnectorSession session, ConnectorTableHandle table
@Override
public void setViewComment(ConnectorSession session, SchemaTableName viewName, Optional<String> comment)
{
Table view = getView(viewName);
Table view = getTrinoView(viewName);
Copy link
Copy Markdown
Member

@raunaqmorarka raunaqmorarka Aug 10, 2023

Choose a reason for hiding this comment

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

Do we have a test verifying that COMMENT ON VIEW doesn't work on an MV ?

Copy link
Copy Markdown
Member Author

@findepi findepi Aug 10, 2023

Choose a reason for hiding this comment

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

idk. note that i am not changing this. toConnectorViewDefinition(...).orElseThrow on next line throws for anything that's not a view


ConnectorViewDefinition definition = toConnectorViewDefinition(session, viewName, Optional.of(view))
.orElseThrow(() -> new ViewNotFoundException(viewName));
Expand All @@ -1474,7 +1475,7 @@ public void setViewComment(ConnectorSession session, SchemaTableName viewName, O
@Override
public void setViewColumnComment(ConnectorSession session, SchemaTableName viewName, String columnName, Optional<String> comment)
{
Table view = getView(viewName);
Table view = getTrinoView(viewName);

ConnectorViewDefinition definition = toConnectorViewDefinition(session, viewName, Optional.of(view))
.orElseThrow(() -> new ViewNotFoundException(viewName));
Expand All @@ -1498,11 +1499,12 @@ public void setMaterializedViewColumnComment(ConnectorSession session, SchemaTab
hiveMaterializedViewMetadata.setMaterializedViewColumnComment(session, viewName, columnName, comment);
}

private Table getView(SchemaTableName viewName)
private Table getTrinoView(SchemaTableName viewName)
{
Table view = metastore.getTable(viewName.getSchemaName(), viewName.getTableName())
.filter(table -> isTrinoView(table) || isHiveView(table))
.orElseThrow(() -> new ViewNotFoundException(viewName));
if (translateHiveViews && !isPrestoView(view)) {
if (!isTrinoView(view)) {
throw new HiveViewNotSupportedException(viewName);
}
return view;
Expand Down Expand Up @@ -2641,7 +2643,7 @@ public void createView(ConnectorSession session, SchemaTableName viewName, Conne

Optional<Table> existing = metastore.getTable(viewName.getSchemaName(), viewName.getTableName());
if (existing.isPresent()) {
if (!replace || !isPrestoView(existing.get())) {
if (!replace || !isTrinoView(existing.get())) {
throw new ViewAlreadyExistsException(viewName);
}

Expand Down Expand Up @@ -2769,10 +2771,19 @@ public Optional<ConnectorViewDefinition> getView(ConnectorSession session, Schem
private Optional<ConnectorViewDefinition> toConnectorViewDefinition(ConnectorSession session, SchemaTableName viewName, Optional<Table> table)
{
return table
.filter(ViewReaderUtil::canDecodeView)
.map(view -> {
if (!translateHiveViews && !isPrestoView(view)) {
throw new HiveViewNotSupportedException(viewName);
.flatMap(view -> {
if (isTrinoView(view)) {
// can handle
}
else if (isHiveView(view)) {
if (!translateHiveViews) {
throw new HiveViewNotSupportedException(viewName);
}
// can handle
}
else {
// actually not a view
return Optional.empty();
}

ConnectorViewDefinition definition = createViewReader(metastore, session, view, typeManager, this::redirectTable, metadataProvider, hiveViewsRunAsInvoker, hiveViewsTimestampPrecision)
Expand All @@ -2788,7 +2799,7 @@ private Optional<ConnectorViewDefinition> toConnectorViewDefinition(ConnectorSes
view.getOwner(),
false);
}
return definition;
return Optional.of(definition);
});
}

Expand Down Expand Up @@ -3804,7 +3815,7 @@ public Optional<CatalogSchemaTableName> redirectTable(ConnectorSession session,
// we need to chop off any "$partitions" and similar suffixes from table name while querying the metastore for the Table object
TableNameSplitResult tableNameSplit = splitTableName(tableName.getTableName());
Optional<Table> table = metastore.getTable(tableName.getSchemaName(), tableNameSplit.getBaseTableName());
if (table.isEmpty() || VIRTUAL_VIEW.name().equals(table.get().getTableType())) {
if (table.isEmpty() || isSomeKindOfAView(table.get())) {
return Optional.empty();
}

Expand Down
9 changes: 4 additions & 5 deletions plugin/trino-hive/src/main/java/io/trino/plugin/hive/TrinoViewHiveMetastore.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@
import static io.trino.plugin.hive.TableType.VIRTUAL_VIEW;
import static io.trino.plugin.hive.TrinoViewUtil.createViewProperties;
import static io.trino.plugin.hive.ViewReaderUtil.encodeViewData;
import static io.trino.plugin.hive.ViewReaderUtil.isPrestoView;
import static io.trino.plugin.hive.ViewReaderUtil.isTrinoView;
import static io.trino.plugin.hive.metastore.MetastoreUtil.buildInitialPrivilegeSet;
import static io.trino.plugin.hive.metastore.PrincipalPrivileges.NO_PRIVILEGES;
import static io.trino.plugin.hive.metastore.StorageFormat.VIEW_STORAGE_FORMAT;
Expand Down Expand Up @@ -87,7 +87,7 @@ public void createView(ConnectorSession session, SchemaTableName schemaViewName,

Optional<io.trino.plugin.hive.metastore.Table> existing = metastore.getTable(schemaViewName.getSchemaName(), schemaViewName.getTableName());
if (existing.isPresent()) {
if (!replace || !isPrestoView(existing.get())) {
Copy link
Copy Markdown
Member Author

@findepi findepi Aug 8, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

In theory, the old code could replace an MV with a View (see #15622)

Copy link
Copy Markdown
Member

@raunaqmorarka raunaqmorarka Aug 10, 2023

Choose a reason for hiding this comment

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

Could we add a test to verify it isn't allowed now ?

Copy link
Copy Markdown
Member Author

@findepi findepi Aug 10, 2023

Choose a reason for hiding this comment

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

it would be very hard to write such a test. from linked PR desc

This would have only been possible during a rare race condition since the engine also checks that an existing entity is not a MV before calling on the connector to create or replace the view.

if (!replace || !isTrinoView(existing.get())) {
throw new ViewAlreadyExistsException(schemaViewName);
}

Expand Down Expand Up @@ -168,7 +168,6 @@ public Optional<ConnectorViewDefinition> getView(SchemaTableName viewName)
}
return metastore.getTable(viewName.getSchemaName(), viewName.getTableName())
.flatMap(view -> TrinoViewUtil.getView(
viewName,
view.getViewOriginalText(),
view.getTableType(),
view.getParameters(),
Expand All @@ -180,7 +179,7 @@ public void updateViewComment(ConnectorSession session, SchemaTableName viewName
io.trino.plugin.hive.metastore.Table view = metastore.getTable(viewName.getSchemaName(), viewName.getTableName())
.orElseThrow(() -> new ViewNotFoundException(viewName));

ConnectorViewDefinition definition = TrinoViewUtil.getView(viewName, view.getViewOriginalText(), view.getTableType(), view.getParameters(), view.getOwner())
ConnectorViewDefinition definition = TrinoViewUtil.getView(view.getViewOriginalText(), view.getTableType(), view.getParameters(), view.getOwner())
.orElseThrow(() -> new ViewNotFoundException(viewName));
ConnectorViewDefinition newDefinition = new ConnectorViewDefinition(
definition.getOriginalSql(),
Expand All @@ -199,7 +198,7 @@ public void updateViewColumnComment(ConnectorSession session, SchemaTableName vi
io.trino.plugin.hive.metastore.Table view = metastore.getTable(viewName.getSchemaName(), viewName.getTableName())
.orElseThrow(() -> new ViewNotFoundException(viewName));

ConnectorViewDefinition definition = TrinoViewUtil.getView(viewName, view.getViewOriginalText(), view.getTableType(), view.getParameters(), view.getOwner())
ConnectorViewDefinition definition = TrinoViewUtil.getView(view.getViewOriginalText(), view.getTableType(), view.getParameters(), view.getOwner())
.orElseThrow(() -> new ViewNotFoundException(viewName));
ConnectorViewDefinition newDefinition = new ConnectorViewDefinition(
definition.getOriginalSql(),
Expand Down
19 changes: 3 additions & 16 deletions plugin/trino-hive/src/main/java/io/trino/plugin/hive/TrinoViewUtil.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@
import com.google.common.collect.ImmutableMap;
import io.trino.spi.connector.ConnectorSession;
import io.trino.spi.connector.ConnectorViewDefinition;
import io.trino.spi.connector.SchemaTableName;

import java.util.Map;
import java.util.Optional;
Expand All @@ -28,30 +27,23 @@
import static io.trino.plugin.hive.HiveMetadata.TABLE_COMMENT;
import static io.trino.plugin.hive.HiveMetadata.TRINO_CREATED_BY;
import static io.trino.plugin.hive.ViewReaderUtil.PRESTO_VIEW_FLAG;
import static io.trino.plugin.hive.ViewReaderUtil.isHiveOrPrestoView;
import static io.trino.plugin.hive.ViewReaderUtil.isPrestoView;
import static io.trino.plugin.hive.ViewReaderUtil.isTrinoView;

public final class TrinoViewUtil
{
private TrinoViewUtil() {}

public static Optional<ConnectorViewDefinition> getView(
SchemaTableName viewName,
Optional<String> viewOriginalText,
String tableType,
Map<String, String> tableParameters,
Optional<String> tableOwner)
{
if (!isView(tableType, tableParameters)) {
// Filter out Tables and Materialized Views
if (!isTrinoView(tableType, tableParameters)) {
// Filter out Tables, Hive views and Trino Materialized Views
return Optional.empty();
}

if (!isPrestoView(tableParameters)) {
// Hive views are not compatible
throw new HiveViewNotSupportedException(viewName);
}

checkArgument(viewOriginalText.isPresent(), "viewOriginalText must be present");
ConnectorViewDefinition definition = ViewReaderUtil.PrestoViewReader.decodeViewData(viewOriginalText.get());
// use owner from table metadata if it exists
Expand All @@ -68,11 +60,6 @@ public static Optional<ConnectorViewDefinition> getView(
return Optional.of(definition);
}

private static boolean isView(String tableType, Map<String, String> tableParameters)
{
return isHiveOrPrestoView(tableType) && PRESTO_VIEW_COMMENT.equals(tableParameters.get(TABLE_COMMENT));
}

public static Map<String, String> createViewProperties(ConnectorSession session, String trinoVersion, String connectorName)
{
return ImmutableMap.<String, String>builder()
Expand Down
56 changes: 36 additions & 20 deletions plugin/trino-hive/src/main/java/io/trino/plugin/hive/ViewReaderUtil.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@
import static com.linkedin.coral.trino.rel2trino.functions.TrinoKeywordsConverter.quoteWordIfNotQuoted;
import static io.trino.plugin.hive.HiveErrorCode.HIVE_INVALID_VIEW_DATA;
import static io.trino.plugin.hive.HiveErrorCode.HIVE_VIEW_TRANSLATION_ERROR;
import static io.trino.plugin.hive.HiveMetadata.PRESTO_VIEW_COMMENT;
import static io.trino.plugin.hive.HiveMetadata.TABLE_COMMENT;
import static io.trino.plugin.hive.HiveSessionProperties.isHiveViewsLegacyTranslation;
import static io.trino.plugin.hive.HiveStorageFormat.TEXTFILE;
Expand Down Expand Up @@ -82,7 +83,7 @@ public static ViewReader createViewReader(
boolean runHiveViewRunAsInvoker,
HiveTimestampPrecision hiveViewsTimestampPrecision)
{
if (isPrestoView(table)) {
if (isTrinoView(table)) {
return new PrestoViewReader();
}
if (isHiveViewsLegacyTranslation(session)) {
Expand Down Expand Up @@ -130,26 +131,44 @@ private static CoralTableRedirectionResolver coralTableRedirectionResolver(
private static final JsonCodec<ConnectorViewDefinition> VIEW_CODEC =
new JsonCodecFactory(new ObjectMapperProvider()).jsonCodec(ConnectorViewDefinition.class);

public static boolean isPrestoView(Table table)
/**
* Returns true if table represents a Hive view, Trino/Presto view, materialized view or anything
* else that gets registered using table type "VIRTUAL_VIEW".
* Note: this method returns false for a table that represents Hive's own materialized view
* ("MATERIALIZED_VIEW" table type). Hive own's materialized views are currently treated as ordinary
* tables by Trino.
*/
public static boolean isSomeKindOfAView(Table table)
{
return isPrestoView(table.getParameters());
return table.getTableType().equals(VIRTUAL_VIEW.name());
}

public static boolean isPrestoView(Map<String, String> tableParameters)
public static boolean isHiveView(Table table)
{
// TODO isPrestoView should not return true for materialized views
return "true".equals(tableParameters.get(PRESTO_VIEW_FLAG));
return table.getTableType().equals(VIRTUAL_VIEW.name()) &&
!table.getParameters().containsKey(PRESTO_VIEW_FLAG);
}

public static boolean isHiveOrPrestoView(Table table)
/**
* Returns true when the table represents a "Trino view" (AKA "presto view").
* Returns false for Hive views or Trino materialized views.
*/
public static boolean isTrinoView(Table table)
{
return isHiveOrPrestoView(table.getTableType());
return isTrinoView(table.getTableType(), table.getParameters());
}

public static boolean isHiveOrPrestoView(String tableType)
/**
* Returns true when the table represents a "Trino view" (AKA "presto view").
* Returns false for Hive views or Trino materialized views.
*/
public static boolean isTrinoView(String tableType, Map<String, String> tableParameters)
{
// TODO isHiveOrPrestoView should not return true for materialized views
return tableType.equals(VIRTUAL_VIEW.name());
// A Trino view can be recognized by table type "VIRTUAL_VIEW" and table parameters presto_view="true" and comment="Presto View" since their first implementation see
// https://github.com/trinodb/trino/blame/38bd0dff736024f3ae01dbbe7d1db5bd1d50c43e/presto-hive/src/main/java/com/facebook/presto/hive/HiveMetadata.java#L902.
return tableType.equals(VIRTUAL_VIEW.name()) &&
"true".equals(tableParameters.get(PRESTO_VIEW_FLAG)) &&
tableParameters.get(TABLE_COMMENT).equalsIgnoreCase(PRESTO_VIEW_COMMENT);
}

public static boolean isTrinoMaterializedView(Table table)
Expand All @@ -159,15 +178,12 @@ public static boolean isTrinoMaterializedView(Table table)

public static boolean isTrinoMaterializedView(String tableType, Map<String, String> tableParameters)
{
// TODO isHiveOrPrestoView should not return true for materialized views
return isHiveOrPrestoView(tableType) && isPrestoView(tableParameters) && tableParameters.get(TABLE_COMMENT).equalsIgnoreCase(ICEBERG_MATERIALIZED_VIEW_COMMENT);
}

public static boolean canDecodeView(Table table)
{
// we can decode Hive or Presto view
return table.getTableType().equals(VIRTUAL_VIEW.name()) &&
!isTrinoMaterializedView(table.getTableType(), table.getParameters());
// A Trino materialized view can be recognized by table type "VIRTUAL_VIEW" and table parameters presto_view="true" and comment="Presto Materialized View"
// since their first implementation see
// https://github.com/trinodb/trino/blame/ff4a1e31fb9cb49f1b960abfc16ad469e7126a64/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/IcebergMetadata.java#L898
return tableType.equals(VIRTUAL_VIEW.name()) &&
"true".equals(tableParameters.get(PRESTO_VIEW_FLAG)) &&
tableParameters.get(TABLE_COMMENT).equalsIgnoreCase(ICEBERG_MATERIALIZED_VIEW_COMMENT);
}

public static String encodeViewData(ConnectorViewDefinition definition)
Expand Down
3 changes: 3 additions & 0 deletions plugin/trino-hive/src/main/java/io/trino/plugin/hive/metastore/HiveMetastore.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,9 @@ default void updatePartitionStatistics(Table table, String partitionName, Functi

List<String> getTablesWithParameter(String databaseName, String parameterKey, String parameterValue);

/**
* Lists views and materialized views from given database.
*/
List<String> getAllViews(String databaseName);

/**
Expand Down
5 changes: 3 additions & 2 deletions ...ino-hive/src/main/java/io/trino/plugin/hive/metastore/SemiTransactionalHiveMetastore.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,8 @@
import static io.trino.plugin.hive.HiveMetadata.PRESTO_QUERY_ID_NAME;
import static io.trino.plugin.hive.LocationHandle.WriteMode.DIRECT_TO_TARGET_NEW_DIRECTORY;
import static io.trino.plugin.hive.TableType.MANAGED_TABLE;
import static io.trino.plugin.hive.ViewReaderUtil.isPrestoView;
import static io.trino.plugin.hive.ViewReaderUtil.isTrinoMaterializedView;
import static io.trino.plugin.hive.ViewReaderUtil.isTrinoView;
import static io.trino.plugin.hive.acid.AcidTransaction.NO_ACID_TRANSACTION;
import static io.trino.plugin.hive.metastore.HivePrivilegeInfo.HivePrivilege.OWNERSHIP;
import static io.trino.plugin.hive.metastore.MetastoreUtil.buildInitialPrivilegeSet;
Expand Down Expand Up @@ -3296,7 +3297,7 @@ public void run(HiveMetastoreClosure metastore, AcidTransaction transaction)
}
tableCreated = true;

if (created && !isPrestoView(newTable)) {
if (created && !isTrinoView(newTable) && !isTrinoMaterializedView(newTable)) {
metastore.updateTableStatistics(newTable.getDatabaseName(), newTable.getTableName(), transaction, ignored -> statistics);
}
}
Expand Down
Loading