Prevent Views from replacing MVs #15622
Conversation
`isPrestoView` returns true for both regular views and Materialized Views, so it could have been possible for a view to replace a MV. 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.
|
CI hit: #13633 |
| @@ -86,7 +88,9 @@ 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())) { | |||
| Table existingTable = existing.get(); | |||
| boolean shouldReplace = isPrestoView(existingTable) && !isTrinoMaterializedView(existingTable.getTableType(), existingTable.getParameters()); | |||
There was a problem hiding this comment.
isPrestoViewreturns true for both regular views and Materialized
Views, so it could have been possible for a view to replace a MV.
@raunaqmorarka why is that?
doesn't seem practical
on the code level, the MVs are handled pretty separately, so why would we want to conflate them?
There was a problem hiding this comment.
I'm not sure if that was on purpose or an accident, but we've been setting the "presto view" flag for MVs since the initial implementation https://github.com/trinodb/trino/pull/4832/files#diff-0a3bb720afd0370525ff486d8d55d6140e766028d20c95c37e8d60f801790881R765
Maybe it was a shortcut to ensure that MVs get listed in the metadata listing for views.
I agree that a clear separation would be better going forward.
There was a problem hiding this comment.
@alexjo2144 would it be possible to fix isPrestoView so that it doesn't return true for MVs?
Can you run this as a separate PR, potentially obsoleting this one?
There was a problem hiding this comment.
i am following-up on this in #18570
|
This pull request has gone a while without any activity. Tagging the Trino developer relations team: @bitsondatadev @colebow @mosabua |
|
Good bot.
@raunaqmorarka can we think about doing that separately and fix the bug here first? |
lol, just gonna reply here so it removes the stale label...carry on! |
| @@ -86,7 +88,9 @@ 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())) { | |||
| Table existingTable = existing.get(); | |||
| boolean shouldReplace = isPrestoView(existingTable) && !isTrinoMaterializedView(existingTable.getTableType(), existingTable.getParameters()); | |||
There was a problem hiding this comment.
@alexjo2144 would it be possible to fix isPrestoView so that it doesn't return true for MVs?
Can you run this as a separate PR, potentially obsoleting this one?
Description
isPrestoViewreturns true for both regular views and Materialized Views, so it could have been possible for a view to replace a MV.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.
Additional context and related issues
Noticed while reviewing: #15580
Release notes
( ) This is not user-visible or docs only and no release notes are required.
( ) Release notes are required, please propose a release note for me.
( ) Release notes are required, with the following suggested text: