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

Fix listTables to include views #11063

Merged
merged 2 commits into from
Feb 24, 2022
Merged

Conversation

findepi
Copy link
Member

@findepi findepi commented Feb 16, 2022

Per ConnectorMetadata.listTables contract, this should include views
and materialized views. Make Iceberg implementation follow the contract.
This fixes IcebergMetadata, AccumuloMetadata and RaptorMetadata.
The HiveMetadata was correct and other connectors do not support views
currently.

This also fixes system.jdbc.tables and system.jdbc.columns --
Iceberg views were not included in these tables.

Fixes #11060

@cla-bot cla-bot bot added the cla-signed label Feb 16, 2022
@findepi findepi force-pushed the findepi/list-ice-view branch 2 times, most recently from 4543431 to f99dfed Compare February 16, 2022 15:53
@findepi findepi changed the title Fix IcebergMetadata.listTables to include views Fix listTables to include views Feb 16, 2022
This makes `system.jdbc.columns` assertions consistent with preceding
`information_schema.columns` assertions.
@raunaqmorarka
Copy link
Member

raunaqmorarka commented Feb 17, 2022

Can we rename listTables to listRelations to avoid confusion ? It's easy to miss that code comment about listTables contract and assume that it is meant to list tables.
Alternatively, could we consider having a listRelations SPI which uses listTables, listViews and listMVs by default and can be overridden by connector to provide more efficient implementation if needed.

@raunaqmorarka
Copy link
Member

Do we need an update to BlackHoleMetadata as well ?

@findepi
Copy link
Member Author

findepi commented Feb 18, 2022

Do we need an update to BlackHoleMetadata as well ?

fixed

Can we rename listTables to listRelations to avoid confusion ?

out of scope

Copy link
Member

@hashhar hashhar left a comment

Choose a reason for hiding this comment

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

MemoryMetadata doesn't have the deduplication part.
Does TestingMetadata also need this change?

Looks good otherwise.

@findepi
Copy link
Member Author

findepi commented Feb 18, 2022

MemoryMetadata doesn't have the deduplication part.

io.trino.plugin.memory.MemoryMetadata#listTables is synchronized

Does TestingMetadata also need this change?

whatever

@@ -441,12 +444,16 @@ public void dropView(ConnectorSession session, SchemaTableName schemaViewName)
{
// Filter on PRESTO_VIEW_COMMENT to distinguish from 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.

move comment to listViews

Per `ConnectorMetadata.listTables` contract, this should include views
and materialized views. Make Iceberg implementation follow the contract.
This fixes `IcebergMetadata`, `AccumuloMetadata`, `RaptorMetadata` and
`BlackHoleMetadata.  The `HiveMetadata` was correct and other connectors
do not support views currently.

This also fixes `system.jdbc.tables` and `system.jdbc.columns` --
Iceberg views were not included in these tables.
@findepi findepi merged commit 4a44c7c into trinodb:master Feb 24, 2022
@findepi findepi deleted the findepi/list-ice-view branch February 24, 2022 14:27
@findepi findepi mentioned this pull request Feb 24, 2022
@github-actions github-actions bot added this to the 372 milestone Feb 24, 2022
@findepi findepi added the jdbc Relates to Trino JDBC driver label Mar 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla-signed jdbc Relates to Trino JDBC driver
Development

Successfully merging this pull request may close these issues.

Iceberg view not registered in system.jdbc.tables
4 participants