Fix Create or replace Materialized view error

The previous files for materialized views were not cleaned properly:
- during view replace, the existing view was deleted after creating the new one, not before (this case caused the error from the #25610 issue),
- during view drop, there was an attempt to delete the files twice (this case caused an error when dropping the materialized view).
- some leftover metadata and data files remained after recreate

Now, when recreating the materialized view, we are always deleting the existing storage using the dropMaterializedViewStorage method at the beginning of the process.

The dropMaterializedViewStorage had an extra metastore.dropTable call, which caused the DROP error, as the metastore.dropTable call needed files that were deleted by the call before.

In dropMaterializedViewStorage we are making sure the table is cleared from the metastore.

Enabled some unit tests that were ignored before due to the existing issues.

Author: codluca

plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/catalog/hms/TrinoHiveCatalog.java

@@ -575,6 +575,9 @@ public void createMaterializedView(
                 }
                 throw new TrinoException(ALREADY_EXISTS, "Materialized view already exists: " + viewName);
             }
+            else {
+                dropMaterializedViewStorage(session, existing.get());
+            }
         }
 
         if (hideMaterializedViewStorageTable) {
@@ -599,6 +602,8 @@ public void createMaterializedView(
             PrincipalPrivileges principalPrivileges = isUsingSystemSecurity ? NO_PRIVILEGES : buildInitialPrivilegeSet(session.getUser());
 
             try {
+                // Reload the existing table, in case it was deleted above
+                existing = metastore.getTable(viewName.getSchemaName(), viewName.getTableName());
                 if (existing.isPresent()) {
                     metastore.replaceTable(viewName.getSchemaName(), viewName.getTableName(), table, principalPrivileges, ImmutableMap.of());
                 }
@@ -618,20 +623,17 @@ public void createMaterializedView(
                 }
                 throw e;
             }
-
-            existing.ifPresent(existingView -> dropMaterializedViewStorage(session, existingView));
         }
         else {
-            createMaterializedViewWithStorageTable(session, viewName, definition, materializedViewProperties, existing);
+            createMaterializedViewWithStorageTable(session, viewName, definition, materializedViewProperties);
         }
     }
 
     private void createMaterializedViewWithStorageTable(
             ConnectorSession session,
             SchemaTableName viewName,
             ConnectorMaterializedViewDefinition definition,
-            Map<String, Object> materializedViewProperties,
-            Optional<io.trino.metastore.Table> existing)
+            Map<String, Object> materializedViewProperties)
     {
         SchemaTableName storageTable = createMaterializedViewStorageTable(session, viewName, definition, materializedViewProperties);
 
@@ -654,19 +656,6 @@ private void createMaterializedViewWithStorageTable(
                 .setViewExpandedText(Optional.of("/* " + ICEBERG_MATERIALIZED_VIEW_COMMENT + " */"));
         io.trino.metastore.Table table = tableBuilder.build();
         PrincipalPrivileges principalPrivileges = isUsingSystemSecurity ? NO_PRIVILEGES : buildInitialPrivilegeSet(session.getUser());
-        if (existing.isPresent()) {
-            // drop the current storage table
-            String oldStorageTable = existing.get().getParameters().get(STORAGE_TABLE);
-            if (oldStorageTable != null) {
-                String storageSchema = Optional.ofNullable(existing.get().getParameters().get(STORAGE_SCHEMA))
-                        .orElse(viewName.getSchemaName());
-                metastore.dropTable(storageSchema, oldStorageTable, true);
-            }
-            // Replace the existing view definition
-            metastore.replaceTable(viewName.getSchemaName(), viewName.getTableName(), table, principalPrivileges, ImmutableMap.of());
-            return;
-        }
-        // create the view definition
         metastore.createTable(table, principalPrivileges);
     }
 
@@ -720,9 +709,7 @@ public void dropMaterializedView(ConnectorSession session, SchemaTableName viewN
         if (!isTrinoMaterializedView(view.getTableType(), view.getParameters())) {
             throw new TrinoException(UNSUPPORTED_TABLE_TYPE, "Not a Materialized View: " + viewName);
         }
-
         dropMaterializedViewStorage(session, view);
-        metastore.dropTable(viewName.getSchemaName(), viewName.getTableName(), true);
     }
 
     private void dropMaterializedViewStorage(ConnectorSession session, io.trino.metastore.Table view)
@@ -749,6 +736,9 @@ private void dropMaterializedViewStorage(ConnectorSession session, io.trino.meta
                 log.warn(e, "Failed to delete storage table metadata '%s' for materialized view '%s'", storageMetadataLocation, viewName);
             }
         }
+        metastore.invalidateTable(viewName.getSchemaName(), viewName.getTableName());
+        metastore.getTable(viewName.getSchemaName(), viewName.getTableName())
+                .ifPresent(table -> metastore.dropTable(viewName.getSchemaName(), viewName.getTableName(), true));
     }
 
     @Override

plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/TestIcebergMaterializedView.java

@@ -134,4 +134,65 @@ AS SELECT sum(value) AS s FROM iceberg2.tpch.common_base_table
         assertUpdate(defaultIceberg, "DROP TABLE common_base_table");
         assertUpdate("DROP MATERIALIZED VIEW mv_on_iceberg2");
     }
+
+    @Test
+    public void testReplaceAndDropMaterializedView() {
+        testReplaceAndDropMaterializedView(false, false);
+        testReplaceAndDropMaterializedView(false, true);
+        testReplaceAndDropMaterializedView(true, false);
+        testReplaceAndDropMaterializedView(true, true);
+    }
+
+    /**
+     * Test CREATE OR REPLACE MATERIALIZED VIEW and DROP MATERIALIZED VIEW statements.
+     * The test creates an Iceberg table and a materialized view on top of it,
+     * then replaces the materialized view definition twice, and finally drops the materialized view.
+     */
+    private void testReplaceAndDropMaterializedView(boolean uniqueTableLocation, boolean hideStorageTable) {
+        QueryRunner queryRunner = getQueryRunner();
+        String catalog = String.format("iceberg_%s_%s", uniqueTableLocation ? "unique" : "not_unique",
+                hideStorageTable ? "storage" : "not_storage");
+        queryRunner.createCatalog(catalog, "iceberg", Map.of(
+                "iceberg.catalog.type", "TESTING_FILE_METASTORE",
+                "iceberg.unique-table-location", uniqueTableLocation ? "true" : "false",
+                "hive.metastore.catalog.dir", queryRunner.getCoordinator().getBaseDataDir().resolve(catalog + "-catalog").toString(),
+                "iceberg.hive-catalog-name", "hive",
+                "iceberg.materialized-views.hide-storage-table", hideStorageTable ? "true" : "false",
+                "fs.hadoop.enabled", "true"));
+        Session session = Session.builder(queryRunner.getDefaultSession())
+                .setCatalog(catalog)
+                .setSchema("default")
+                .build();
+        queryRunner.execute(session, "CREATE SCHEMA default");
+
+        assertUpdate(session, "CREATE TABLE replace_base_table AS SELECT 10 value", 1);
+
+        assertUpdate(session, "CREATE OR REPLACE MATERIALIZED VIEW replace_view" +
+                " AS SELECT sum(value) AS s FROM replace_base_table");
+        assertUpdate(session, "REFRESH MATERIALIZED VIEW replace_view", 1);
+        assertQuery(session, "SELECT count(*) FROM replace_view", "VALUES 1");
+        assertQuery(session, "SELECT * FROM replace_view", "VALUES 10");
+
+        assertUpdate(session, "CREATE OR REPLACE MATERIALIZED VIEW replace_view" +
+                " AS SELECT 2 * sum(value) AS t FROM replace_base_table");
+        assertUpdate(session, "REFRESH MATERIALIZED VIEW replace_view", 1);
+        assertQuery(session, "SELECT count(*) FROM replace_view", "VALUES 1");
+        assertQuery(session, "SELECT * FROM replace_view", "VALUES 20");
+
+        assertUpdate(session, "CREATE OR REPLACE MATERIALIZED VIEW replace_view" +
+                " AS SELECT 3 * sum(value) AS v FROM replace_base_table");
+        assertUpdate(session, "REFRESH MATERIALIZED VIEW replace_view", 1);
+        assertQuery(session, "SELECT count(*) FROM replace_view", "VALUES 1");
+        assertQuery(session, "SELECT * FROM replace_view", "VALUES 30");
+
+        assertUpdate(session, "DROP MATERIALIZED VIEW replace_view");
+
+        assertUpdate(session, "CREATE OR REPLACE MATERIALIZED VIEW replace_view" +
+                " AS SELECT 4 * sum(value) AS v FROM replace_base_table");
+        assertUpdate(session, "REFRESH MATERIALIZED VIEW replace_view", 1);
+        assertQuery(session, "SELECT count(*) FROM replace_view", "VALUES 1");
+        assertQuery(session, "SELECT * FROM replace_view", "VALUES 40");
+
+        assertUpdate(session, "DROP MATERIALIZED VIEW replace_view");
+    }
 }

plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/catalog/file/TestTrinoHiveCatalogWithFileMetastore.java

@@ -34,7 +34,6 @@
 import io.trino.spi.type.TestingTypeManager;
 import org.junit.jupiter.api.AfterAll;
 import org.junit.jupiter.api.BeforeAll;
-import org.junit.jupiter.api.Disabled;
 import org.junit.jupiter.api.Test;
 import org.junit.jupiter.api.TestInstance;
 import org.junit.jupiter.api.parallel.Execution;
@@ -56,7 +55,6 @@
 import static io.trino.plugin.iceberg.IcebergTestUtils.FILE_IO_FACTORY;
 import static io.trino.spi.type.IntegerType.INTEGER;
 import static io.trino.testing.TestingNames.randomNameSuffix;
-import static org.assertj.core.api.Assertions.assertThatThrownBy;
 import static org.junit.jupiter.api.TestInstance.Lifecycle.PER_CLASS;
 import static org.junit.jupiter.api.parallel.ExecutionMode.CONCURRENT;
 
@@ -109,7 +107,6 @@ protected TrinoCatalog createTrinoCatalog(boolean useUniqueTableLocations)
     }
 
     @Test
-    @Disabled
     public void testDropMaterializedView()
     {
         testDropMaterializedView(false);
@@ -156,12 +153,4 @@ private void testDropMaterializedView(boolean useUniqueTableLocations)
             }
         }
     }
-
-    @Test
-    @Override
-    public void testListTables()
-    {
-        // the test actually works but when cleanup up the materialized view the error is thrown
-        assertThatThrownBy(super::testListTables).hasMessageMatching("Table 'ns2.*.mv' not found");
-    }
 }