Skip to content

fix(plugin-iceberg): Drop data table if the MV fails validation #26994

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.

Sign up for GitHub

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

Merged
tdcmeehan merged 1 commit into from
Jan 21, 2026
+90 −27
Merged

fix(plugin-iceberg): Drop data table if the MV fails validation #26994

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
72 changes: 46 additions & 26 deletions presto-iceberg/src/main/java/com/facebook/presto/iceberg/IcebergAbstractMetadata.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -230,11 +230,11 @@
import static com.facebook.presto.spi.MaterializedViewStatus.MaterializedViewState.NOT_MATERIALIZED;
import static com.facebook.presto.spi.MaterializedViewStatus.MaterializedViewState.PARTIALLY_MATERIALIZED;
import static com.facebook.presto.spi.StandardErrorCode.ALREADY_EXISTS;
import static com.facebook.presto.spi.StandardErrorCode.INVALID_VIEW;
import static com.facebook.presto.spi.StandardErrorCode.NOT_FOUND;
import static com.facebook.presto.spi.StandardErrorCode.NOT_SUPPORTED;
import static com.facebook.presto.spi.connector.RowChangeParadigm.DELETE_ROW_AND_INSERT_ROW;
import static com.facebook.presto.spi.statistics.TableStatisticType.ROW_COUNT;
import static com.google.common.base.Preconditions.checkState;
import static com.google.common.base.Strings.isNullOrEmpty;
import static com.google.common.base.Verify.verify;
import static com.google.common.collect.ImmutableList.toImmutableList;
Expand Down Expand Up @@ -1658,32 +1658,44 @@ public void createMaterializedView(
viewMetadata.getComment());
createTable(session, storageTableMetadata, false);

Map<String, String> properties = new HashMap<>();
properties.put(PRESTO_MATERIALIZED_VIEW_FORMAT_VERSION, CURRENT_MATERIALIZED_VIEW_FORMAT_VERSION + "");
properties.put(PRESTO_MATERIALIZED_VIEW_ORIGINAL_SQL, viewDefinition.getOriginalSql());
properties.put(PRESTO_MATERIALIZED_VIEW_STORAGE_SCHEMA, storageTableName.getSchemaName());
properties.put(PRESTO_MATERIALIZED_VIEW_STORAGE_TABLE_NAME, storageTableName.getTableName());

String baseTablesStr = serializeSchemaTableNames(viewDefinition.getBaseTables());
properties.put(PRESTO_MATERIALIZED_VIEW_BASE_TABLES, baseTablesStr);
properties.put(PRESTO_MATERIALIZED_VIEW_COLUMN_MAPPINGS, serializeColumnMappings(viewDefinition.getColumnMappings()));
checkState(viewDefinition.getOwner().isPresent(), "Materialized view owner is required");
properties.put(PRESTO_MATERIALIZED_VIEW_OWNER, viewDefinition.getOwner().get());
checkState(viewDefinition.getSecurityMode().isPresent(), "Materialized view security mode is required");
properties.put(PRESTO_MATERIALIZED_VIEW_SECURITY_MODE, viewDefinition.getSecurityMode().get().name());

getStaleReadBehavior(materializedViewProperties)
.ifPresent(behavior -> properties.put(PRESTO_MATERIALIZED_VIEW_STALE_READ_BEHAVIOR, behavior.name()));
getStalenessWindow(materializedViewProperties)
.ifPresent(window -> properties.put(PRESTO_MATERIALIZED_VIEW_STALENESS_WINDOW, window.toString()));
MaterializedViewRefreshType refreshType = getRefreshType(materializedViewProperties);
properties.put(PRESTO_MATERIALIZED_VIEW_REFRESH_TYPE, refreshType.name());

for (SchemaTableName baseTable : viewDefinition.getBaseTables()) {
properties.put(getBaseTableViewPropertyName(baseTable), "0");
}
try {
Map<String, String> properties = new HashMap<>();
properties.put(PRESTO_MATERIALIZED_VIEW_FORMAT_VERSION, CURRENT_MATERIALIZED_VIEW_FORMAT_VERSION + "");
properties.put(PRESTO_MATERIALIZED_VIEW_ORIGINAL_SQL, viewDefinition.getOriginalSql());
properties.put(PRESTO_MATERIALIZED_VIEW_STORAGE_SCHEMA, storageTableName.getSchemaName());
properties.put(PRESTO_MATERIALIZED_VIEW_STORAGE_TABLE_NAME, storageTableName.getTableName());

String baseTablesStr = serializeSchemaTableNames(viewDefinition.getBaseTables());
properties.put(PRESTO_MATERIALIZED_VIEW_BASE_TABLES, baseTablesStr);
properties.put(PRESTO_MATERIALIZED_VIEW_COLUMN_MAPPINGS, serializeColumnMappings(viewDefinition.getColumnMappings()));
properties.put(PRESTO_MATERIALIZED_VIEW_OWNER, viewDefinition.getOwner()
.orElseThrow(() -> new PrestoException(INVALID_VIEW, "Materialized view owner is required")));
properties.put(PRESTO_MATERIALIZED_VIEW_SECURITY_MODE, viewDefinition.getSecurityMode()
.orElseThrow(() -> new PrestoException(INVALID_VIEW, "Materialized view security mode is required (set legacy_materialized_views=false)"))
.name());
Comment thread
hantangwangd marked this conversation as resolved.

getStaleReadBehavior(materializedViewProperties)
.ifPresent(behavior -> properties.put(PRESTO_MATERIALIZED_VIEW_STALE_READ_BEHAVIOR, behavior.name()));
getStalenessWindow(materializedViewProperties)
.ifPresent(window -> properties.put(PRESTO_MATERIALIZED_VIEW_STALENESS_WINDOW, window.toString()));
MaterializedViewRefreshType refreshType = getRefreshType(materializedViewProperties);
properties.put(PRESTO_MATERIALIZED_VIEW_REFRESH_TYPE, refreshType.name());

for (SchemaTableName baseTable : viewDefinition.getBaseTables()) {
properties.put(getBaseTableViewPropertyName(baseTable), "0");
}

createIcebergView(session, viewName, viewMetadata.getColumns(), viewDefinition.getOriginalSql(), properties);
createIcebergView(session, viewName, viewMetadata.getColumns(), viewDefinition.getOriginalSql(), properties);
}
catch (Exception e) {
try {
dropStorageTable(session, storageTableName);
}
catch (Exception cleanupException) {
e.addSuppressed(cleanupException);
}
throw e;
}
}
catch (PrestoException e) {
if (e.getErrorCode() == NOT_SUPPORTED.toErrorCode()) {
Expand All @@ -1693,6 +1705,14 @@ public void createMaterializedView(
}
}

private void dropStorageTable(ConnectorSession session, SchemaTableName storageTableName)
{
ConnectorTableHandle storageTableHandle = getTableHandle(session, storageTableName);
if (storageTableHandle != null) {
dropTable(session, storageTableHandle);
}
}

@Override
public List<SchemaTableName> listMaterializedViews(ConnectorSession session, String schemaName)
{
Expand Down
45 changes: 44 additions & 1 deletion ...ceberg/src/test/java/com/facebook/presto/iceberg/TestIcebergMaterializedViewMetadata.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
package com.facebook.presto.iceberg;

import com.facebook.airlift.http.server.testing.TestingHttpServer;
import com.facebook.presto.Session;
import com.facebook.presto.testing.QueryRunner;
import com.facebook.presto.tests.AbstractTestQueryFramework;
import com.google.common.collect.ImmutableMap;
Expand Down Expand Up @@ -98,7 +99,9 @@ protected QueryRunner createQueryRunner()
.setDataDirectory(Optional.of(warehouseLocation.toPath()))
.setSchemaName("test_schema")
.setCreateTpchTables(false)
.setExtraProperties(ImmutableMap.of("experimental.legacy-materialized-views", "false"))
.setExtraProperties(ImmutableMap.of(
"experimental.legacy-materialized-views", "false",
"experimental.allow-legacy-materialized-views-toggle", "true"))
.build().getQueryRunner();
}

Expand Down Expand Up @@ -756,4 +759,44 @@ public void testStalenessPropertiesStoredInView()
assertUpdate("DROP MATERIALIZED VIEW test_staleness_props_mv");
assertUpdate("DROP TABLE test_staleness_props_base");
}

@Test
public void testNoOrphanStorageTableOnValidationFailure()
throws Exception
{
try (RESTCatalog catalog = new RESTCatalog()) {
assertUpdate("CREATE TABLE test_orphan_base (id BIGINT, value BIGINT)");
assertUpdate("INSERT INTO test_orphan_base VALUES (1, 100)", 1);

Session legacySession = Session.builder(getSession())
.setSystemProperty("legacy_materialized_views", "true")
Comment on lines +763 to +772
Copy link
Copy Markdown
Contributor

@sourcery-ai sourcery-ai bot Jan 21, 2026

Choose a reason for hiding this comment

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

suggestion (testing): Also assert that the materialized view itself is not registered after the failed creation

Your test already checks that the storage table is removed at both the SQL layer and via the REST catalog. To strengthen coverage from a user perspective, add an assertion that no materialized view entry exists in metadata after the failure (e.g., via SHOW TABLES, SHOW MATERIALIZED VIEWS, or system metadata tables) so we catch cases where the storage table is dropped but a stale MV record remains.

Suggested implementation:

    @Test
    public void testNoOrphanStorageTableOnValidationFailure()
            throws Exception
    {
        try (RESTCatalog catalog = new RESTCatalog()) {
            assertUpdate("CREATE TABLE test_orphan_base (id BIGINT, value BIGINT)");
            assertUpdate("INSERT INTO test_orphan_base VALUES (1, 100)", 1);

            Session legacySession = Session.builder(getSession())
                    .setSystemProperty("legacy_materialized_views", "true")
                    .build();

            // Ensure no stale materialized view metadata entry is registered after the failed creation.
            // This verifies that both the storage table and the MV record are fully cleaned up.
            assertQueryReturnsEmptyResult("SHOW MATERIALIZED VIEWS LIKE 'test_orphan_%'");

This patch assumes:

  1. The materialized view created (and expected to fail) in this test uses a name that matches the pattern test_orphan_% (e.g., test_orphan_mv). If a different name is used, adjust the LIKE pattern in SHOW MATERIALIZED VIEWS accordingly to specifically target the MV under test (for example: SHOW MATERIALIZED VIEWS LIKE 'test_orphan_mv').
  2. The method assertQueryReturnsEmptyResult(String sql) is available in the test base class (it is common in Presto tests). If it is not available in this particular test base, replace the assertion with an equivalent that checks for an empty result set, such as:
    • assertQuery("SHOW MATERIALIZED VIEWS LIKE 'test_orphan_%'", "SELECT * FROM (VALUES 1) WHERE 1 = 0"), or
    • another project-specific helper for asserting an empty result.
  3. If the failure and cleanup logic occurs later in the method (after the snippet you provided), move the assertQueryReturnsEmptyResult("SHOW MATERIALIZED VIEWS LIKE 'test_orphan_%'"); line to just after the point where you currently assert that the Iceberg storage table has been removed (both in SQL and via the REST catalog). This ensures the assertion is executed after the failed materialized view creation and cleanup, which is the intended behavior for the test.

.build();

String mvName = "test_orphan_mv";
String storageTableName = "__mv_storage__" + mvName;
Comment on lines +775 to +776
Copy link
Copy Markdown
Contributor

@sourcery-ai sourcery-ai bot Jan 21, 2026

Choose a reason for hiding this comment

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

nitpick (testing): Clarify intent of hard-coded storage table naming to avoid future brittleness

This test intentionally depends on the current storage table naming convention ("__mv_storage__" + mvName) to verify that the specific storage table is removed. Please add a short comment to make this explicit, so future refactors of the naming logic either update this test or introduce a helper for computing the storage table name instead of silently breaking the assertion.


assertQueryFails(
legacySession,
"CREATE MATERIALIZED VIEW " + mvName + " AS SELECT id, value FROM test_orphan_base",
".*Materialized view security mode is required.*");

assertQueryFails(
"SELECT COUNT(*) FROM \"" + storageTableName + "\"",
".*(does not exist|not found).*");

Map<String, String> catalogProps = new HashMap<>();
catalogProps.put("uri", serverUri);
catalogProps.put("warehouse", warehouseLocation.getAbsolutePath());
catalog.initialize("test_catalog", catalogProps);

TableIdentifier storageTableId = TableIdentifier.of(Namespace.of("test_schema"), storageTableName);
boolean tableExists = catalog.tableExists(storageTableId);
assertFalse(tableExists,
"Storage table should not exist after failed MV creation. " +
"This would indicate validation happened after storage table creation.");
}
finally {
assertUpdate("DROP TABLE test_orphan_base");
}
}
}
Loading