Improve error message for DROP/RENAME TABLE on MV #8869

lukasz-stec · 2021-08-12T12:27:23Z

Add information that operation fails due to action on a materialized view.
Add tests that confirm DROP/RENAME TABLE/VIEW does not work on MATERIALIZED VIEW

losipiuk · 2021-08-13T07:02:08Z

core/trino-main/src/main/java/io/trino/execution/DropTableTask.java

@@ -54,6 +55,14 @@ public String getName()
        Session session = stateMachine.getSession();
        QualifiedObjectName tableName = createQualifiedObjectName(session, statement, statement.getTableName());

+        Optional<ConnectorMaterializedViewDefinition> view = metadata.getMaterializedView(session, tableName);
+        if (view.isPresent()) {


Does metadata.getTableHandle return non-empty for materialized view? If not please move the new code to if (tableHandle.isEmpty()) {

Shouldn't we check for if (!statement.isExists()) { here as below?

Shouldn't we check for if (!statement.isExists()) { here as below?

Good catch. I think we should. ie. we don't want the statement:
DROP TABLE existing_materialized view IF EXISTS to fail.

I added guards for both view and materialized view. Hive connector returns view from getTableHandle method so it needs to happen even if getTableHandle returns a not empty value.
I added case where IF EXISTS is used + tests

// those two guards are here because some connectors violate SPI contract and return views or materialized views from getTableHandle.

Which connectors? Can we fix them? Also, that would argue for detecting misbehaving connectors and failing with some kind of internal error to indicate so.

hive connector returns views from getTableHandle. This in turn makes GRANT SELECT ON... or COMMENT ON TABLE work on views.

losipiuk · 2021-08-13T07:04:58Z

testing/trino-testing/src/main/java/io/trino/testing/BaseConnectorTest.java

+
+        assertQueryFails(
+                "DROP VIEW " + viewName,
+                format("line 1:1: View '.*.%s' does not exist", viewName));


Can we have consistent message for view and table?

losipiuk

some comments/questions

lukasz-stec

comments addressed

lukasz-stec · 2021-08-13T07:49:29Z

core/trino-main/src/main/java/io/trino/execution/DropTableTask.java

@@ -54,6 +55,14 @@ public String getName()
        Session session = stateMachine.getSession();
        QualifiedObjectName tableName = createQualifiedObjectName(session, statement, statement.getTableName());

+        Optional<ConnectorMaterializedViewDefinition> view = metadata.getMaterializedView(session, tableName);
+        if (view.isPresent()) {


Shouldn't we check for if (!statement.isExists()) { here as below?

Good catch. I think we should. ie. we don't want the statement:
DROP TABLE existing_materialized view IF EXISTS to fail.

lukasz-stec · 2021-08-13T11:18:03Z

core/trino-main/src/main/java/io/trino/execution/DropTableTask.java

@@ -54,6 +55,14 @@ public String getName()
        Session session = stateMachine.getSession();
        QualifiedObjectName tableName = createQualifiedObjectName(session, statement, statement.getTableName());

+        Optional<ConnectorMaterializedViewDefinition> view = metadata.getMaterializedView(session, tableName);
+        if (view.isPresent()) {


I added guards for both view and materialized view. Hive connector returns view from getTableHandle method so it needs to happen even if getTableHandle returns a not empty value.
I added case where IF EXISTS is used + tests

core/trino-main/src/main/java/io/trino/execution/DropTableTask.java

core/trino-main/src/main/java/io/trino/execution/RenameTableTask.java

losipiuk · 2021-08-16T10:49:01Z

testing/trino-testing/src/main/java/io/trino/testing/BaseConnectorTest.java

+
+        assertUpdate(format("ALTER TABLE IF EXISTS %s RENAME TO %s_new", viewName, viewName));
+
+        assertQueryFails(


does it still behave same way if connector supports materialized views but not views (not sure if we have a case like that now)

I think we don't have a case like that. The only connector that supports materialized views is the iceberg connector and it also supports views.

losipiuk

LGTM. nits.

lukasz-stec

applied suggestions and squashed

lukasz-stec · 2021-08-16T11:31:29Z

testing/trino-testing/src/main/java/io/trino/testing/BaseConnectorTest.java

+
+        assertUpdate(format("ALTER TABLE IF EXISTS %s RENAME TO %s_new", viewName, viewName));
+
+        assertQueryFails(


I think we don't have a case like that. The only connector that supports materialized views is the iceberg connector and it also supports views.

sopel39

Could you also enhance DropMaterializedViewTask in a similar way?

sopel39 · 2021-08-17T11:50:50Z

core/trino-main/src/main/java/io/trino/execution/DropTableTask.java

+            }
+            return immediateVoidFuture();
+        }
+        Optional<ConnectorMaterializedViewDefinition> materializedView = metadata.getMaterializedView(session, tableName);


you should check MV first. Follow object resolution order from io.trino.sql.analyzer.StatementAnalyzer.Visitor#visitTable (MV, view, table)

done. Question though, given all 3 object types should be in the same namespace. Is this only for the sake of code consistency or there is another reason?

sopel39 · 2021-08-17T11:53:25Z

core/trino-main/src/main/java/io/trino/execution/DropTableTask.java

@@ -54,6 +56,28 @@ public String getName()
        Session session = stateMachine.getSession();
        QualifiedObjectName tableName = createQualifiedObjectName(session, statement, statement.getTableName());

+        // those two guards are here because some connectors violate SPI contract and return views or materialized views from getTableHandle.


I would remove this comment. It's not only about connectors, but also about hinting user at using correct command

sopel39 · 2021-08-17T11:56:29Z

core/trino-main/src/main/java/io/trino/execution/DropViewTask.java

@@ -57,6 +58,13 @@ public String getName()
        Optional<ConnectorViewDefinition> view = metadata.getView(session, name);
        if (view.isEmpty()) {
            if (!statement.isExists()) {
+                Optional<ConnectorMaterializedViewDefinition> materializedView = metadata.getMaterializedView(session, name);


Check the existence of MV before if (view.isEmpty()) (again, follow check order of MV, view, table).

Here (after view existence check), you should check if table is present instead.

core/trino-main/src/main/java/io/trino/execution/RenameTableTask.java

sopel39 · 2021-08-17T11:59:07Z

core/trino-main/src/main/java/io/trino/execution/RenameViewTask.java

@@ -58,6 +59,13 @@ public String getName()
        QualifiedObjectName viewName = createQualifiedObjectName(session, statement, statement.getSource());
        Optional<ConnectorViewDefinition> viewDefinition = metadata.getView(session, viewName);
        if (viewDefinition.isEmpty()) {
+            Optional<ConnectorMaterializedViewDefinition> materializedView = metadata.getMaterializedView(session, viewName);


ditto, move MV check above if (viewDefinition.isEmpty()). Here (after view existence check), you can check that table handle does not exist

sopel39 · 2021-08-17T13:01:00Z

testing/trino-testing/src/main/java/io/trino/testing/BaseConnectorTest.java

@@ -537,6 +537,28 @@ public void testMaterializedView()
                        "FROM\n" +
                        "  nation");

+        assertQueryFails(


these aren't really a connector test. Could you add unit test similar to TestCreateMaterializedViewTask for each task that you change? Alternatively, could you extract these test cases as separate tests here (however, I would still prefer TestCreateMaterializedViewTask style tests)?

sopel39 · 2021-08-17T13:03:37Z

also, checkout ShowQueriesRewrite.Visitor#visitShowCreate how we deal with similar issue (object resolution) as here.

lukasz-stec

Add enhancements in DropMaterializedViewTask + order of checks changed to always MV/VIEW/TABLE.
Tests moved from BaseConnectorTest to separate unit tests for *Task classes.

lukasz-stec · 2021-08-18T13:54:24Z

core/trino-main/src/main/java/io/trino/execution/DropTableTask.java

+            }
+            return immediateVoidFuture();
+        }
+        Optional<ConnectorMaterializedViewDefinition> materializedView = metadata.getMaterializedView(session, tableName);


done. Question though, given all 3 object types should be in the same namespace. Is this only for the sake of code consistency or there is another reason?

sopel39

lgtm % minor comments

sopel39 · 2021-08-19T09:43:35Z

core/trino-main/src/main/java/io/trino/execution/RenameTableTask.java

@@ -74,6 +99,7 @@ public String getName()
        if (!tableName.getCatalogName().equals(target.getCatalogName())) {
            throw semanticException(NOT_SUPPORTED, statement, "Table rename across catalogs is not supported");
        }
+


sopel39 · 2021-08-19T09:44:41Z

core/trino-main/src/main/java/io/trino/execution/RenameViewTask.java

+                throw semanticException(
+                        TABLE_NOT_FOUND,
+                        statement,
+                        "View '%s' does not exist. Attempting to use view DDL syntax on a table. Did you mean RENAME TABLE %s...?", viewName, viewName);


there is no RENAME TABLE command in Trino

replaced with Did you mean ALTER TABLE %s RENAME

sopel39 · 2021-08-19T09:49:33Z

core/trino-main/src/test/java/io/trino/execution/BaseDataDefinitionTaskTest.java

+                true);
+    }
+
+    private QueryStateMachine stateMachine(TransactionManager transactionManager, MetadataManager metadata, AccessControl accessControl)


can it be static?

I made it static (with one additional parameter)

sopel39 · 2021-08-19T09:52:29Z

core/trino-main/src/test/java/io/trino/execution/BaseDataDefinitionTaskTest.java

+        @Override
+        public void renameTable(Session session, TableHandle tableHandle, QualifiedObjectName newTableName)
+        {
+            tables.put(newTableName.asSchemaTableName(), tables.get(getTableName(tableHandle)));


validate that tables.get is not null

sopel39 · 2021-08-19T09:53:07Z

core/trino-main/src/test/java/io/trino/execution/BaseDataDefinitionTaskTest.java

+        @Override
+        public void renameView(Session session, QualifiedObjectName source, QualifiedObjectName target)
+        {
+            views.put(target.asSchemaTableName(), views.get(source.asSchemaTableName()));


make sure view.get is not null

sopel39 · 2021-08-19T09:55:44Z

core/trino-main/src/test/java/io/trino/execution/TestDropMaterializedViewTask.java

+        // no exception
+    }
+
+    private ListenableFuture<Void> executeDropMaterializedView(DropMaterializedView statement)


could you make it accept viewName, ifExists instead of statement? This way we don't need to create DropMaterializedView object in tests

done. I refactored execute... the same way for all new test classes

sopel39 · 2021-08-19T10:03:28Z

core/trino-main/src/test/java/io/trino/execution/TestDropTableTask.java

+        // no exception
+    }
+
+    private ListenableFuture<Void> executeDropTable(DropTable statement)


make executeDropTable create DropTable statement itself

sopel39 · 2021-08-19T10:03:57Z

core/trino-main/src/test/java/io/trino/execution/TestDropViewTask.java

+
+    private ListenableFuture<Void> executeDropView(DropView statement)
+    {
+        return new DropViewTask().execute(statement, transactionManager, metadata, new AllowAllAccessControl(), queryStateMachine, ImmutableList.of(), WarningCollector.NOOP);


make executeDropView create DropView statement itself

sopel39 · 2021-08-19T10:04:15Z

core/trino-main/src/test/java/io/trino/execution/TestRenameViewTask.java

+                .hasMessage("View '%s' does not exist. Attempting to use view DDL syntax on a materialized view.", viewName);
+    }
+
+    private ListenableFuture<Void> executeRenameView(RenameView statement)


make executeRenameView create RenameView itself

lukasz-stec

Comments addressed

lukasz-stec · 2021-08-19T10:35:33Z

core/trino-main/src/main/java/io/trino/execution/RenameViewTask.java

+                throw semanticException(
+                        TABLE_NOT_FOUND,
+                        statement,
+                        "View '%s' does not exist. Attempting to use view DDL syntax on a table. Did you mean RENAME TABLE %s...?", viewName, viewName);


replaced with Did you mean ALTER TABLE %s RENAME

lukasz-stec · 2021-08-19T10:52:19Z

core/trino-main/src/test/java/io/trino/execution/TestDropMaterializedViewTask.java

+        // no exception
+    }
+
+    private ListenableFuture<Void> executeDropMaterializedView(DropMaterializedView statement)


done. I refactored execute... the same way for all new test classes

lukasz-stec · 2021-08-19T10:54:13Z

core/trino-main/src/test/java/io/trino/execution/BaseDataDefinitionTaskTest.java

+                true);
+    }
+
+    private QueryStateMachine stateMachine(TransactionManager transactionManager, MetadataManager metadata, AccessControl accessControl)


I made it static (with one additional parameter)

martint · 2021-08-19T16:58:42Z

core/trino-main/src/main/java/io/trino/execution/DropMaterializedViewTask.java

+                    throw semanticException(
+                            TABLE_NOT_FOUND,
+                            statement,
+                            "Materialized view '%s' does not exist. Attempting to use materialized view DDL syntax on a view. Did you mean DROP VIEW %s?", name, name);


"DDL" is not a very intuitive term for users. I would just rephrase as:

Materialized view '%s' does not exist, but a view with that name exists. Did you mean DROP VIEW %s?

ok, I updated all the new error messages to this pattern.

Add information that operation fails due to action on a view, materialized view or table. Add tests that confirm DROP/RENAME TABLE does not work on VIEW and MATERIALIZED VIEW Add tests that confirm DROP/RENAME TABLE/VIEW does not work on VIEW and MATERIALIZED VIEW

sopel39 · 2021-08-20T09:30:48Z

@martint do you want to take a final look?

hashhar · 2021-08-30T08:24:48Z

core/trino-main/src/main/java/io/trino/execution/DropTableTask.java

+                        statement,
+                        "Table '%s' does not exist, but a view with that name exists. Did you mean DROP VIEW %s?", tableName, tableName);
+            }
+            return immediateVoidFuture();


Shouldn't we be throwing an error from here? returning a void future here means that DROP TABLE IF EXISTS actually_a_view succeeds without dropping anything.

In HiveQueryRunner:

trino:tpch> create view a_view as select * from nation; CREATE VIEW trino:tpch> drop table a_view; Query 20210830_082947_00044_cki4q failed: line 1:1: Table 'hive.tpch.a_view' does not exist, but a view with that name exists. Did you mean DROP VIEW hive.tpch.a_view? drop table a_view trino:tpch> drop table if exists a_view; DROP TABLE trino:tpch>

This changes the semantics - I'm not sure if this was expected or not.

IMO this is correct behaviour since the table does not exist and IF EXISTS was used.

cla-bot bot added the cla-signed label Aug 12, 2021

lukasz-stec requested a review from raunaqmorarka August 12, 2021 12:27

raunaqmorarka approved these changes Aug 12, 2021

View reviewed changes

raunaqmorarka requested review from martint and losipiuk August 13, 2021 04:51

losipiuk reviewed Aug 13, 2021

View reviewed changes

lukasz-stec force-pushed the ls/improve-msg-drop-table-on-mv branch from ffd89e0 to d5e292d Compare August 13, 2021 11:13

lukasz-stec commented Aug 13, 2021

View reviewed changes

lukasz-stec requested a review from losipiuk August 13, 2021 11:19

losipiuk reviewed Aug 16, 2021

View reviewed changes

core/trino-main/src/main/java/io/trino/execution/DropTableTask.java Outdated Show resolved Hide resolved

losipiuk reviewed Aug 16, 2021

View reviewed changes

core/trino-main/src/main/java/io/trino/execution/RenameTableTask.java Outdated Show resolved Hide resolved

losipiuk reviewed Aug 16, 2021

View reviewed changes

losipiuk approved these changes Aug 16, 2021

View reviewed changes

lukasz-stec force-pushed the ls/improve-msg-drop-table-on-mv branch from 61e1a86 to a49d545 Compare August 16, 2021 11:33

lukasz-stec commented Aug 16, 2021

View reviewed changes

lukasz-stec requested a review from sopel39 August 16, 2021 12:15

sopel39 reviewed Aug 17, 2021

View reviewed changes

lukasz-stec force-pushed the ls/improve-msg-drop-table-on-mv branch from a49d545 to ec09bb5 Compare August 18, 2021 15:53

lukasz-stec commented Aug 18, 2021

View reviewed changes

lukasz-stec requested a review from sopel39 August 18, 2021 20:39

sopel39 approved these changes Aug 19, 2021

View reviewed changes

lukasz-stec force-pushed the ls/improve-msg-drop-table-on-mv branch from ec09bb5 to 469a7ac Compare August 19, 2021 11:09

lukasz-stec commented Aug 19, 2021

View reviewed changes

lukasz-stec requested a review from sopel39 August 19, 2021 14:03

martint reviewed Aug 19, 2021

View reviewed changes

lukasz-stec force-pushed the ls/improve-msg-drop-table-on-mv branch from 469a7ac to 0e9a2b5 Compare August 19, 2021 19:18

lukasz-stec requested a review from martint August 19, 2021 21:00

sopel39 approved these changes Aug 20, 2021

View reviewed changes

sopel39 merged commit 7dff335 into trinodb:master Aug 23, 2021

sopel39 mentioned this pull request Aug 23, 2021

Release notes for 361 #8732

Closed

10 tasks

hashhar reviewed Aug 30, 2021

View reviewed changes

findepi deleted the ls/improve-msg-drop-table-on-mv branch August 30, 2021 09:10

findinpath mentioned this pull request Mar 18, 2022

Prevent DROP/RENAME TABLE IF EXISTS from silently ignoring views #11555

Merged


		assertUpdate(format("ALTER TABLE IF EXISTS %s RENAME TO %s_new", viewName, viewName));

		assertQueryFails(

Improve error message for DROP/RENAME TABLE on MV #8869

Improve error message for DROP/RENAME TABLE on MV #8869

Conversation

lukasz-stec commented Aug 12, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

losipiuk left a comment

Choose a reason for hiding this comment

lukasz-stec left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

losipiuk left a comment

Choose a reason for hiding this comment

lukasz-stec left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sopel39 left a comment

Choose a reason for hiding this comment

sopel39 Aug 17, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sopel39 Aug 17, 2021 • edited Loading

Choose a reason for hiding this comment

sopel39 Aug 17, 2021 • edited Loading

Choose a reason for hiding this comment

sopel39 Aug 17, 2021 • edited Loading

Choose a reason for hiding this comment

sopel39 commented Aug 17, 2021

lukasz-stec left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sopel39 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lukasz-stec left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sopel39 commented Aug 20, 2021

hashhar Aug 30, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sopel39 Aug 17, 2021 •

edited

Loading

sopel39 Aug 17, 2021 •

edited

Loading

sopel39 Aug 17, 2021 •

edited

Loading

sopel39 Aug 17, 2021 •

edited

Loading

hashhar Aug 30, 2021 •

edited

Loading