Skip to content
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.

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

Add listMaterializedViews and getMaterializedViews SPI #8113

Merged
merged 6 commits into from Jun 7, 2021

Conversation

raunaqmorarka
Copy link
Member

Used listMaterializedViews to list MV names for information_schema.tables, system.jdbc.tables and SHOW TABLES
Used getMaterializedViews to list MV columns in information_schema.columns, system.jdbc.columns and SHOW COLUMNS

Fixed DESCRIBE, SHOW COLUMNS, information_schema.columns, system.jdbc.columns for MVs in Iceberg.

Table type used for MVs in information_schema.tables is left unchanged as BASE TABLE.
MVs are not listed in information_schema.views.
getMaterializedViews can be used in future to introduce information_schema.trino_materialized_views or system.metadata.materialized_views for showing detailed information about materialized view definitions.

@findepi
Copy link
Member

findepi commented May 28, 2021

@raunaqmorarka i am adding more tests, inclduding a bit of information_schema coverage for MVs in #8117
Can you please check how your PR interacts with the tests added there?

@raunaqmorarka
Copy link
Member Author

@raunaqmorarka i am adding more tests, inclduding a bit of information_schema coverage for MVs in #8117
Can you please check how your PR interacts with the tests added there?

Looked at it, since i've tested similar cases in this PR, I think it should result in the TODO cases of that PR getting solved.

@raunaqmorarka
Copy link
Member Author

@findepi I've rebased this to master and added changes to BaseConnectorTest#testMaterializedView

@@ -353,7 +353,10 @@ public static boolean isTablesEnumeratingTable(InformationSchemaTable table)
.flatMap(prefix -> tables.get().stream()
.filter(this::isLowerCase)
.map(table -> new QualifiedObjectName(catalogName, prefix.getSchemaName().get(), table)))
.filter(objectName -> !isColumnsEnumeratingTable(informationSchemaTable) || metadata.getTableHandle(session, objectName).isPresent() || metadata.getView(session, objectName).isPresent())
.filter(objectName -> !isColumnsEnumeratingTable(informationSchemaTable) ||
Copy link
Member

Choose a reason for hiding this comment

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

would it be possible to add test to TestInformationSchemaConnector or TestInformationSchemaMetadata?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is covered by BCT column listing tests

Copy link
Member

@sopel39 sopel39 left a comment

Choose a reason for hiding this comment

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

% small comments.

Should #8153 land first?

@sopel39
Copy link
Member

sopel39 commented Jun 3, 2021

please rebase on top of #8153

@raunaqmorarka
Copy link
Member Author

please rebase on top of #8153

done

Copy link
Member

@sopel39 sopel39 left a comment

Choose a reason for hiding this comment

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

lgtm % minor comments

Fixed and added tests for DESCRIBE, SHOW COLUMNS,
information_schema.columns and system.jdbc.columns
@sopel39 sopel39 merged commit e4cae79 into trinodb:master Jun 7, 2021
@raunaqmorarka raunaqmorarka deleted the list-mvs branch June 7, 2021 15:11
@@ -280,6 +280,7 @@ private void addTablesRecords(QualifiedTablePrefix prefix)
{
Set<SchemaTableName> tables = listTables(session, metadata, accessControl, prefix);
Set<SchemaTableName> views = listViews(session, metadata, accessControl, prefix);
// TODO (https://github.com/trinodb/trino/issues/8207) define a type for materialized views
Copy link
Member

Choose a reason for hiding this comment

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

nit: this should be separate commit

@sopel39 sopel39 mentioned this pull request Jun 7, 2021
13 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

None yet

3 participants