Skip to content

Fix listTables to include views#11063

Merged
findepi merged 2 commits intotrinodb:masterfrom
findepi:findepi/list-ice-view
Feb 24, 2022
Merged

Fix listTables to include views#11063
findepi merged 2 commits intotrinodb:masterfrom
findepi:findepi/list-ice-view

Conversation

@findepi
Copy link
Copy Markdown
Member

@findepi findepi commented Feb 16, 2022 •
edited
Loading

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
@findepi
Assert consistently in testMaterializedView
6c24976 
This makes `system.jdbc.columns` assertions consistent with preceding
`information_schema.columns` assertions.
@findepi findepi force-pushed the findepi/list-ice-view branch from f99dfed to a083752 Compare February 16, 2022 15:56
@raunaqmorarka
Copy link
Copy Markdown
Member

raunaqmorarka commented Feb 17, 2022 •
edited
Loading

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
Copy Markdown
Member

raunaqmorarka commented Feb 17, 2022

Do we need an update to BlackHoleMetadata as well ?

@findepi findepi force-pushed the findepi/list-ice-view branch from a083752 to e2c643d Compare February 18, 2022 13:59
@findepi
Copy link
Copy Markdown
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

hashhar
hashhar reviewed Feb 18, 2022
View reviewed changes
Copy link
Copy Markdown
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
Copy Markdown
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

losipiuk
losipiuk reviewed Feb 23, 2022
View reviewed changes
plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/TrinoHiveCatalog.java Outdated
Copy link
Copy Markdown
Member

@losipiuk losipiuk Feb 23, 2022

Choose a reason for hiding this comment

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

move comment to listViews

@findepi
Fix listTables to include views
2c8bdf7 
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 force-pushed the findepi/list-ice-view branch from e2c643d to 2c8bdf7 Compare February 23, 2022 13:52
@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
Closed
@github-actions github-actions bot added this to the 372 milestone Feb 24, 2022
@mosabua mosabua mentioned this pull request Feb 24, 2022
Merged
@findepi findepi added the jdbc Relates to Trino JDBC driver label Mar 1, 2022
@findepi findepi mentioned this pull request Mar 22, 2022
Merged
This was referenced Nov 21, 2023
Merged
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@losipiuk losipiuk losipiuk approved these changes

@hashhar hashhar hashhar approved these changes

@raunaqmorarka raunaqmorarka Awaiting requested review from raunaqmorarka

Assignees

No one assigned

Labels

cla-signed jdbc Relates to Trino JDBC driver

Milestone

372

Development

Successfully merging this pull request may close these issues.

Iceberg view not registered in system.jdbc.tables

4 participants

@findepi @raunaqmorarka @losipiuk @hashhar