From 4ba651fdd1f2fec3337d6b363c40fe9f7f8c6158 Mon Sep 17 00:00:00 2001 From: Piotr Findeisen Date: Mon, 23 Jan 2023 13:42:43 +0100 Subject: [PATCH 1/2] Refactor IcebergMetadata.isTableCurrent This is just a refactor - Inline `getTableToken` - Remove redundant `TableToken` (it is not used for populating DEPENDS_ON_TABLES field, so doesn't provide any encapsulation) - reduce visibility - remove redundant cast & conversions - move below the usage, as it is a helper - give the method a better name. --- .../trino/plugin/iceberg/IcebergMetadata.java | 53 +++++-------------- 1 file changed, 14 insertions(+), 39 deletions(-) diff --git a/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/IcebergMetadata.java b/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/IcebergMetadata.java index 781224626d4e..d14f3ad0aff1 100644 --- a/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/IcebergMetadata.java +++ b/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/IcebergMetadata.java @@ -2504,25 +2504,6 @@ public void renameMaterializedView(ConnectorSession session, SchemaTableName sou catalog.renameMaterializedView(session, source, target); } - public Optional getTableToken(ConnectorSession session, ConnectorTableHandle tableHandle) - { - IcebergTableHandle table = (IcebergTableHandle) tableHandle; - Table icebergTable = catalog.loadTable(session, table.getSchemaTableName()); - return Optional.ofNullable(icebergTable.currentSnapshot()) - .map(snapshot -> new TableToken(snapshot.snapshotId())); - } - - public boolean isTableCurrent(ConnectorSession session, ConnectorTableHandle tableHandle, Optional tableToken) - { - Optional currentToken = getTableToken(session, tableHandle); - - if (tableToken.isEmpty() || currentToken.isEmpty()) { - return false; - } - - return tableToken.get().getSnapshotId() == currentToken.get().getSnapshotId(); - } - @Override public MaterializedViewFreshness getMaterializedViewFreshness(ConnectorSession session, SchemaTableName materializedViewName) { @@ -2571,20 +2552,30 @@ else if (strings.size() != 2) { if (tableHandle == null) { throw new MaterializedViewNotFoundException(materializedViewName); } - Optional tableToken; + Optional snapshotAtRefresh; if (value.isEmpty()) { - tableToken = Optional.empty(); + snapshotAtRefresh = Optional.empty(); } else { - tableToken = Optional.of(new TableToken(Long.parseLong(value))); + snapshotAtRefresh = Optional.of(Long.parseLong(value)); } - if (!isTableCurrent(session, tableHandle, tableToken)) { + if (!isSnapshotCurrent(session, tableHandle, snapshotAtRefresh)) { return new MaterializedViewFreshness(STALE); } } return new MaterializedViewFreshness(hasUnknownTables ? UNKNOWN : FRESH); } + private boolean isSnapshotCurrent(ConnectorSession session, IcebergTableHandle table, Optional snapshotId) + { + Table icebergTable = catalog.loadTable(session, table.getSchemaTableName()); + Snapshot currentSnapshot = icebergTable.currentSnapshot(); + if (snapshotId.isEmpty() || currentSnapshot == null) { + return false; + } + return snapshotId.get() == currentSnapshot.snapshotId(); + } + @Override public boolean supportsReportingWrittenBytes(ConnectorSession session, ConnectorTableHandle connectorTableHandle) { @@ -2614,20 +2605,4 @@ private void beginTransaction(Table icebergTable) verify(transaction == null, "transaction already set"); transaction = icebergTable.newTransaction(); } - - private static class TableToken - { - // Current Snapshot ID of the table - private final long snapshotId; - - public TableToken(long snapshotId) - { - this.snapshotId = snapshotId; - } - - public long getSnapshotId() - { - return snapshotId; - } - } } From 387a9f02394e9c1254e9c6f82d8e5688ecb8b1e2 Mon Sep 17 00:00:00 2001 From: Piotr Findeisen Date: Mon, 30 Jan 2023 22:13:46 +0100 Subject: [PATCH 2/2] Fix materialized view failure mode when base table gone Reporting `MaterializedViewNotFoundException` for a freshness check resulted in `SELECT .. FROM mv` query failing in "materialized view mv does not exist", which was not correct, since it exists, just cannot be queried. --- .../trino/plugin/iceberg/IcebergMetadata.java | 4 ++-- .../jdbc/TestIcebergJdbcConnectorTest.java | 7 ++++++ .../io/trino/testing/BaseConnectorTest.java | 22 +++++++++++++++++++ 3 files changed, 31 insertions(+), 2 deletions(-) diff --git a/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/IcebergMetadata.java b/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/IcebergMetadata.java index d14f3ad0aff1..3dadd09d8f29 100644 --- a/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/IcebergMetadata.java +++ b/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/IcebergMetadata.java @@ -73,7 +73,6 @@ import io.trino.spi.connector.ConstraintApplicationResult; import io.trino.spi.connector.DiscretePredicates; import io.trino.spi.connector.MaterializedViewFreshness; -import io.trino.spi.connector.MaterializedViewNotFoundException; import io.trino.spi.connector.ProjectionApplicationResult; import io.trino.spi.connector.RetryMode; import io.trino.spi.connector.RowChangeParadigm; @@ -2550,7 +2549,8 @@ else if (strings.size() != 2) { IcebergTableHandle tableHandle = getTableHandle(session, schemaTableName, Optional.empty(), Optional.empty()); if (tableHandle == null) { - throw new MaterializedViewNotFoundException(materializedViewName); + // Base table is gone + return new MaterializedViewFreshness(STALE); } Optional snapshotAtRefresh; if (value.isEmpty()) { diff --git a/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/catalog/jdbc/TestIcebergJdbcConnectorTest.java b/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/catalog/jdbc/TestIcebergJdbcConnectorTest.java index 045a4df8523e..fa5abd31ea25 100644 --- a/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/catalog/jdbc/TestIcebergJdbcConnectorTest.java +++ b/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/catalog/jdbc/TestIcebergJdbcConnectorTest.java @@ -175,6 +175,13 @@ public void testMaterializedView() .hasMessage("createMaterializedView is not supported for Iceberg JDBC catalogs"); } + @Override + public void testMaterializedViewBaseTableGone(boolean initialized) + { + assertThatThrownBy(() -> super.testMaterializedViewBaseTableGone(initialized)) + .hasMessage("createMaterializedView is not supported for Iceberg JDBC catalogs"); + } + @Test(dataProvider = "testColumnNameDataProvider") @Override public void testMaterializedViewColumnName(String columnName) diff --git a/testing/trino-testing/src/main/java/io/trino/testing/BaseConnectorTest.java b/testing/trino-testing/src/main/java/io/trino/testing/BaseConnectorTest.java index 43f56efeb10b..5b49a232b317 100644 --- a/testing/trino-testing/src/main/java/io/trino/testing/BaseConnectorTest.java +++ b/testing/trino-testing/src/main/java/io/trino/testing/BaseConnectorTest.java @@ -1220,6 +1220,28 @@ public void testFederatedMaterializedView() }); } + @Test(dataProviderClass = DataProviders.class, dataProvider = "trueFalse") + public void testMaterializedViewBaseTableGone(boolean initialized) + { + skipTestUnless(hasBehavior(SUPPORTS_CREATE_MATERIALIZED_VIEW)); + + String catalog = getSession().getCatalog().orElseThrow(); + String schema = getSession().getSchema().orElseThrow(); + String viewName = "mv_base_table_missing_" + randomNameSuffix(); + String baseTable = "mv_base_table_missing_the_table_" + randomNameSuffix(); + + assertUpdate("CREATE TABLE " + baseTable + " AS SELECT 1 a", 1); + assertUpdate("CREATE MATERIALIZED VIEW " + viewName + " AS SELECT * FROM " + baseTable); + if (initialized) { + assertUpdate("REFRESH MATERIALIZED VIEW " + viewName, 1); + } + assertUpdate("DROP TABLE " + baseTable); + assertQueryFails( + "TABLE " + viewName, + "line 1:1: Failed analyzing stored view '%1$s\\.%2$s\\.%3$s': line 3:3: Table '%1$s\\.%2$s\\.%4$s' does not exist".formatted(catalog, schema, viewName, baseTable)); + assertUpdate("DROP MATERIALIZED VIEW " + viewName); + } + @Test public void testCompatibleTypeChangeForView() {