feat(plugin-iceberg): Add support for materialized views with Hive catalog#26958
Merged
tdcmeehan merged 3 commits intoprestodb:masterfrom Jan 30, 2026
Merged
feat(plugin-iceberg): Add support for materialized views with Hive catalog#26958tdcmeehan merged 3 commits intoprestodb:masterfrom
tdcmeehan merged 3 commits intoprestodb:masterfrom
Conversation
tdcmeehan
requested review from
a team,
ZacBlanco and
hantangwangd
as code owners
prestodb-ci
requested review from
a team,
bibith4 and
infvg
and removed request for
a team
Contributor
Reviewer's GuideRefactors Iceberg view handling to use a shared IcebergViewMetadata abstraction and adds full Hive-metastore catalog support for Iceberg-backed materialized views, with tests split to validate both REST and Hive catalogs. Sequence diagram for creating an Iceberg materialized view with Hive catalogsequenceDiagram
actor User
participant PrestoEngine
participant IcebergHiveMetadata
participant Metastore
User->>PrestoEngine: CREATE MATERIALIZED VIEW sql
PrestoEngine->>IcebergHiveMetadata: createMaterializedView(session, viewName, definition)
IcebergHiveMetadata->>IcebergHiveMetadata: createIcebergView(session, viewName, columns, viewSql, properties)
IcebergHiveMetadata->>IcebergHiveMetadata: build tableProperties with PRESTO_MATERIALIZED_VIEW_FLAG and metadata
IcebergHiveMetadata->>Metastore: createTable(metastoreContext, table, privileges, partitions)
Metastore-->>IcebergHiveMetadata: table created
IcebergHiveMetadata->>IcebergHiveMetadata: tableCache.invalidate(viewName)
IcebergHiveMetadata-->>PrestoEngine: materialized view created
PrestoEngine-->>User: success
Class diagram for Iceberg view metadata handling and materialized viewsclassDiagram
class IcebergAbstractMetadata {
<<abstract>>
+Table getIcebergTable(ConnectorSession session, SchemaTableName schemaTableName)
+ConnectorTableMetadata getTableOrViewMetadata(ConnectorSession session, SchemaTableName schemaTableName)
+List~SchemaTableName~ listMaterializedViews(ConnectorSession session, String schemaName)
+Optional~MaterializedViewDefinition~ getMaterializedView(ConnectorSession session, SchemaTableName viewName)
+MaterializedViewStatus getMaterializedViewStatus(ConnectorSession session, SchemaTableName materializedViewName)
#Table getRawIcebergTable(ConnectorSession session, SchemaTableName schemaTableName)
#Optional~IcebergViewMetadata~ getViewMetadata(ConnectorSession session, SchemaTableName viewName)
#void createIcebergView(ConnectorSession session, SchemaTableName viewName, List~ViewColumn~ columns, String viewSql, Map~String,String~ properties)
#void dropIcebergView(ConnectorSession session, SchemaTableName schemaTableName)
#void updateIcebergViewProperties(ConnectorSession session, SchemaTableName viewName, Map~String,String~ properties)
-boolean viewExists(ConnectorSession session, ConnectorTableMetadata viewMetadata)
}
class IcebergNativeMetadata {
+Table getRawIcebergTable(ConnectorSession session, SchemaTableName schemaTableName)
-View getIcebergView(ConnectorSession session, SchemaTableName schemaTableName)
+Optional~IcebergViewMetadata~ getViewMetadata(ConnectorSession session, SchemaTableName viewName)
+boolean tableExists(ConnectorSession session, SchemaTableName schemaTableName)
-Map~SchemaTableName,View~ icebergViews
-CatalogFactory catalogFactory
}
class IcebergHiveMetadata {
+Table getRawIcebergTable(ConnectorSession session, SchemaTableName schemaTableName)
+Optional~IcebergViewMetadata~ getViewMetadata(ConnectorSession session, SchemaTableName viewName)
+boolean tableExists(ConnectorSession session, SchemaTableName schemaTableName)
+List~SchemaTableName~ listMaterializedViews(ConnectorSession session, String schemaName)
+void createIcebergView(ConnectorSession session, SchemaTableName viewName, List~ViewColumn~ columns, String viewSql, Map~String,String~ properties)
+void dropIcebergView(ConnectorSession session, SchemaTableName schemaTableName)
+void updateIcebergViewProperties(ConnectorSession session, SchemaTableName viewName, Map~String,String~ properties)
-boolean isIcebergMaterializedView(Table table)
-Optional~Table~ getHiveTable(ConnectorSession session, SchemaTableName schemaTableName)
-Metastore metastore
-TableCache tableCache
}
class IcebergViewMetadata {
-Map~String,String~ properties
-ConnectorTableMetadata tableMetadata
+IcebergViewMetadata(Map~String,String~ properties, ConnectorTableMetadata tableMetadata)
+Map~String,String~ getProperties()
+ConnectorTableMetadata getTableMetadata()
+boolean isMaterializedView()
}
class ConnectorTableMetadata {
+SchemaTableName getTable()
+List~ColumnMetadata~ getColumns()
+Map~String,Object~ getProperties()
+Optional~String~ getComment()
}
class Table {
+Map~String,String~ getParameters()
+List~HiveColumn~ getDataColumns()
+String getOwner()
+static Table builder(Table table)
+Table setParameters(Map~String,String~ parameters)
+Table build()
}
class Metastore {
+Optional~List~String~~ getAllViews(MetastoreContext context, String schemaName)
+void createTable(MetastoreContext context, Table table, PrincipalPrivileges privileges, List~Partition~ partitions)
+void dropTable(MetastoreContext context, String schemaName, String tableName, boolean deleteData)
+void replaceTable(MetastoreContext context, String schemaName, String tableName, Table table, PrincipalPrivileges privileges)
}
class CatalogFactory {
+Catalog getCatalog(ConnectorSession session)
}
class Catalog {
}
class ViewCatalog {
}
IcebergAbstractMetadata <|-- IcebergNativeMetadata
IcebergAbstractMetadata <|-- IcebergHiveMetadata
IcebergAbstractMetadata --> IcebergViewMetadata : uses
IcebergNativeMetadata --> IcebergViewMetadata : constructs
IcebergHiveMetadata --> IcebergViewMetadata : constructs
IcebergViewMetadata --> ConnectorTableMetadata : wraps
IcebergNativeMetadata --> CatalogFactory
CatalogFactory --> Catalog
ViewCatalog ..|> Catalog
IcebergHiveMetadata --> Metastore
IcebergHiveMetadata --> Table
IcebergHiveMetadata --> TableCache
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The
isIcebergMaterializedViewhelper checks forPRESTO_MATERIALIZED_VIEW_FORMAT_VERSION, butcreateIcebergViewonly setsPRESTO_MATERIALIZED_VIEW_FLAGand other properties; aligning the detection logic with the properties written at creation time will avoid misclassifying views. dropIcebergViewunconditionally drops the Hive table without checkingisIcebergMaterializedView, whereasupdateIcebergViewPropertiesandgetIcebergViewvalidate the type; consider adding a similar guard to avoid dropping non-materialized-view tables via this path.- In
TestIcebergMaterializedViewsBase, thecatalogTypefield and constructor parameter are currently unused; either remove them or use them in the subclasses’createQueryRunnerto avoid dead code.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `isIcebergMaterializedView` helper checks for `PRESTO_MATERIALIZED_VIEW_FORMAT_VERSION`, but `createIcebergView` only sets `PRESTO_MATERIALIZED_VIEW_FLAG` and other properties; aligning the detection logic with the properties written at creation time will avoid misclassifying views.
- `dropIcebergView` unconditionally drops the Hive table without checking `isIcebergMaterializedView`, whereas `updateIcebergViewProperties` and `getIcebergView` validate the type; consider adding a similar guard to avoid dropping non-materialized-view tables via this path.
- In `TestIcebergMaterializedViewsBase`, the `catalogType` field and constructor parameter are currently unused; either remove them or use them in the subclasses’ `createQueryRunner` to avoid dead code.
## Individual Comments
### Comment 1
<location> `presto-iceberg/src/main/java/com/facebook/presto/iceberg/IcebergHiveMetadata.java:794-803` </location>
<code_context>
}
@Override
protected void dropIcebergView(ConnectorSession session, SchemaTableName schemaTableName)
{
- throw new PrestoException(NOT_SUPPORTED, "Iceberg Hive catalog does not support native Iceberg views for materialized views.");
+ MetastoreContext metastoreContext = getMetastoreContext(session);
+
+ try {
+ metastore.dropTable(
+ metastoreContext,
+ schemaTableName.getSchemaName(),
+ schemaTableName.getTableName(),
+ true);
+ }
+ catch (TableNotFoundException e) {
+ throw new PrestoException(NOT_FOUND, "Materialized view not found: " + schemaTableName);
+ }
+
</code_context>
<issue_to_address>
**issue (bug_risk):** dropIcebergView drops any table with the given name without verifying it is an Iceberg materialized view.
To align with `updateIcebergViewProperties` and `getIcebergView`, this method should first load the table, verify `isIcebergMaterializedView`, and only then call `dropTable`. If the table is missing or not an Iceberg materialized view, it should throw a NOT_FOUND (or similar) error instead of dropping it.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
794
to
+803
| protected void dropIcebergView(ConnectorSession session, SchemaTableName schemaTableName) | ||
| { | ||
| throw new PrestoException(NOT_SUPPORTED, "Iceberg Hive catalog does not support native Iceberg views for materialized views."); | ||
| MetastoreContext metastoreContext = getMetastoreContext(session); | ||
|
|
||
| try { | ||
| metastore.dropTable( | ||
| metastoreContext, | ||
| schemaTableName.getSchemaName(), | ||
| schemaTableName.getTableName(), | ||
| true); |
Contributor
There was a problem hiding this comment.
issue (bug_risk): dropIcebergView drops any table with the given name without verifying it is an Iceberg materialized view.
To align with updateIcebergViewProperties and getIcebergView, this method should first load the table, verify isIcebergMaterializedView, and only then call dropTable. If the table is missing or not an Iceberg materialized view, it should throw a NOT_FOUND (or similar) error instead of dropping it.
tdcmeehan
force-pushed
the
mv-ice-hive
branch
from
08c5800 to
8dc268c
Compare
tdcmeehan
force-pushed
the
mv-ice-hive
branch
4 times, most recently
from
aa6ea71 to
ca9238b
Compare
Contributor
There was a problem hiding this comment.
Hey - I've found 3 issues, and left some high level feedback:
- The change in
IcebergAbstractMetadata.getTableOrViewMetadatafromgetIcebergViewthrowingNoSuchViewException/NOT_SUPPORTEDtogetViewMetadatareturningOptional.empty()means catalogs that don’t implementViewCatalogwill now surfaceTableNotFoundExceptioninstead of a more explicit NOT_SUPPORTED error for views; consider whether this behavioral change is acceptable or if a NOT_SUPPORTED path should be preserved. - In
IcebergHiveMetadata.getViewMetadata, theConnectorTableMetadatais built withImmutableMap.of()as properties, ignoringtable.getParameters()(other than comment); if any of these parameters are expected downstream (e.g., for compatibility with native view metadata), consider passing through relevant parameters or aligning withcreateViewMetadataPropertiesused in the native path. - The Hive-specific
isIcebergMaterializedViewandlistMaterializedViewslogic is keyed solely offPRESTO_MATERIALIZED_VIEW_FORMAT_VERSIONand duplicates some of the generic materialized-view detection; consider centralizing this inIcebergViewMetadata.isMaterializedView()/getViewMetadatato reduce the chance of divergence between Hive and native catalogs over time.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The change in `IcebergAbstractMetadata.getTableOrViewMetadata` from `getIcebergView` throwing `NoSuchViewException`/`NOT_SUPPORTED` to `getViewMetadata` returning `Optional.empty()` means catalogs that don’t implement `ViewCatalog` will now surface `TableNotFoundException` instead of a more explicit NOT_SUPPORTED error for views; consider whether this behavioral change is acceptable or if a NOT_SUPPORTED path should be preserved.
- In `IcebergHiveMetadata.getViewMetadata`, the `ConnectorTableMetadata` is built with `ImmutableMap.of()` as properties, ignoring `table.getParameters()` (other than comment); if any of these parameters are expected downstream (e.g., for compatibility with native view metadata), consider passing through relevant parameters or aligning with `createViewMetadataProperties` used in the native path.
- The Hive-specific `isIcebergMaterializedView` and `listMaterializedViews` logic is keyed solely off `PRESTO_MATERIALIZED_VIEW_FORMAT_VERSION` and duplicates some of the generic materialized-view detection; consider centralizing this in `IcebergViewMetadata.isMaterializedView()`/`getViewMetadata` to reduce the chance of divergence between Hive and native catalogs over time.
## Individual Comments
### Comment 1
<location> `presto-iceberg/src/main/java/com/facebook/presto/iceberg/IcebergHiveMetadata.java:843-849` </location>
<code_context>
+ .setParameters(mergedProperties.buildKeepingLast())
+ .build();
+
+ PrincipalPrivileges privileges = buildInitialPrivilegeSet(table.getOwner());
+ metastore.replaceTable(
+ metastoreContext,
+ viewName.getSchemaName(),
+ viewName.getTableName(),
+ updatedTable,
+ privileges);
+
+ tableCache.invalidate(viewName);
</code_context>
<issue_to_address>
**🚨 issue (security):** Updating view properties may unintentionally reset privileges instead of preserving existing ones.
In `updateIcebergViewProperties`, calling `buildInitialPrivilegeSet(table.getOwner())` before `replaceTable` will reinitialize privileges and discard any existing fine-grained grants on the view. If this method is only meant to change properties, consider preserving the current privileges (or passing `Optional.empty()`/an equivalent no-op per the metastore API) so that access control is not unintentionally altered by a properties-only update.
</issue_to_address>
### Comment 2
<location> `presto-iceberg/src/test/java/com/facebook/presto/iceberg/hive/TestIcebergMaterializedViewsHive.java:32` </location>
<code_context>
+import static com.google.common.io.RecursiveDeleteOption.ALLOW_INSECURE;
+
+@Test(singleThreaded = true)
+public class TestIcebergMaterializedViewsHive
+ extends TestIcebergMaterializedViewsBase
+{
</code_context>
<issue_to_address>
**suggestion (testing):** Consider adding a Hive-specific test that asserts `SHOW MATERIALIZED VIEWS`/`listMaterializedViews` only returns Iceberg materialized views, not regular Iceberg tables or views
Since the Hive implementation of `listMaterializedViews` relies on `metastore.getAllViews` and `isIcebergMaterializedView`, it would be good to add a Hive-specific assertion in this test class. For example:
- Create an Iceberg materialized view plus a regular Iceberg table and/or non-materialized view in `test_schema`.
- Assert that `SHOW MATERIALIZED VIEWS FROM test_schema` (or the equivalent `information_schema` query) returns only the materialized view.
This will exercise the Hive metastore wiring and `isIcebergMaterializedView` behavior and guard against regressions in how HMS identifies materialized views.
</issue_to_address>
### Comment 3
<location> `presto-iceberg/src/test/java/com/facebook/presto/iceberg/rest/TestIcebergMaterializedViewsRest.java:35` </location>
<code_context>
+import static com.google.common.io.RecursiveDeleteOption.ALLOW_INSECURE;
+
+@Test(singleThreaded = true)
+public class TestIcebergMaterializedViewsRest
+ extends TestIcebergMaterializedViewsBase
+{
</code_context>
<issue_to_address>
**suggestion (testing):** Add a REST-catalog-specific assertion around materialized view metadata retrieval failures (e.g., invalid or missing MV properties)
Since the REST catalog still uses native Iceberg views (vs. Hive’s HMS tables), the new `getMaterializedView` logic that depends on `IcebergViewMetadata` isn’t fully exercised by the shared base tests. Please add a REST-specific test in this class that corrupts or removes materialized view properties (via direct catalog/metastore manipulation or `ALTER TABLE/VIEW`, if possible) and verifies that:
- `SHOW CREATE MATERIALIZED VIEW` or `SELECT * FROM system.metadata.materialized_views` fails with `ICEBERG_INVALID_MATERIALIZED_VIEW`, or
- `getMaterializedView` returns empty when required properties are missing.
This will directly test the REST-specific `IcebergViewMetadata` wiring and error handling.
Suggested implementation:
```java
import java.util.Optional;
import org.apache.iceberg.catalog.TableIdentifier;
import org.apache.iceberg.rest.RESTCatalog;
import org.apache.iceberg.view.View;
import org.apache.iceberg.view.ViewUpdate;
import static com.facebook.presto.iceberg.CatalogType.REST;
import static com.facebook.presto.iceberg.IcebergQueryRunner.builder;
import static com.facebook.presto.iceberg.rest.IcebergRestTestUtil.getRestServer;
import static com.facebook.presto.iceberg.rest.IcebergRestTestUtil.restConnectorProperties;
import static com.google.common.io.MoreFiles.deleteRecursively;
import static com.google.common.io.RecursiveDeleteOption.ALLOW_INSECURE;
```
To fully implement the REST-catalog-specific assertions for invalid/missing materialized view metadata, add the following to `TestIcebergMaterializedViewsRest`:
1. **Helper to corrupt MV properties in the REST/Iceberg catalog**
Inside `TestIcebergMaterializedViewsRest` (as a `private` method), add something along these lines, using the REST catalog that backs `restServer`:
```java
private void corruptMaterializedViewProperties(String schema, String name)
{
// Obtain the underlying Iceberg REST catalog from the test REST server.
// Adjust this depending on the actual type returned by getRestServer().
RESTCatalog catalog = (RESTCatalog) getRestServer(restServer).getCatalog();
TableIdentifier identifier = TableIdentifier.of(schema, name);
View view = catalog.loadView(identifier);
// Copy and corrupt properties by removing / altering MV-specific ones
// (these keys must match what IcebergViewMetadata expects)
java.util.Map<String, String> properties = new java.util.HashMap<>(view.properties());
properties.remove("presto.materialized-view.target");
properties.remove("presto.materialized-view.query");
// You can also deliberately corrupt required properties instead of removing them:
// properties.put("presto.materialized-view.target", "invalid-target");
ViewUpdate update = catalog.updateView(identifier);
update.setProperties(properties);
update.commit();
}
```
You will likely need to adjust `getRestServer(restServer).getCatalog()` to match the actual API. The important part is:
- obtain the `RESTCatalog`,
- load the Iceberg `View` corresponding to the Presto materialized view,
- remove or corrupt the properties that `IcebergViewMetadata` depends on.
2. **REST-specific test: SHOW CREATE MATERIALIZED VIEW fails with ICEBERG_INVALID_MATERIALIZED_VIEW**
Still in `TestIcebergMaterializedViewsRest`, add a new test method:
```java
@Test
public void testShowCreateMaterializedViewFailsOnInvalidMetadata()
{
String schema = "tpch"; // adjust if the base class uses a different default schema
String mvName = "test_invalid_mv_metadata_rest";
assertUpdate("CREATE MATERIALIZED VIEW " + mvName + " AS SELECT 1 x");
// Corrupt the underlying Iceberg view metadata via REST catalog
corruptMaterializedViewProperties(schema, mvName);
// SHOW CREATE MATERIALIZED VIEW should now fail with ICEBERG_INVALID_MATERIALIZED_VIEW
assertQueryFails(
"SHOW CREATE MATERIALIZED VIEW " + mvName,
".*ICEBERG_INVALID_MATERIALIZED_VIEW.*");
}
```
This directly exercises the `IcebergViewMetadata` lookup path used by `getMaterializedView` for REST and verifies that invalid metadata is surfaced as `ICEBERG_INVALID_MATERIALIZED_VIEW`.
3. **(Optional but recommended) REST-specific test via `system.metadata.materialized_views`**
To additionally cover the `getMaterializedView` wiring used by `system.metadata.materialized_views`, add:
```java
@Test
public void testSystemMetadataMaterializedViewsFailsOnInvalidMetadata()
{
String schema = "tpch";
String mvName = "test_invalid_mv_metadata_system_rest";
assertUpdate("CREATE MATERIALIZED VIEW " + mvName + " AS SELECT 1 x");
corruptMaterializedViewProperties(schema, mvName);
// Accessing MV metadata via system table should also fail with the same error
assertQueryFails(
"SELECT * FROM system.metadata.materialized_views WHERE catalog_name = 'iceberg' AND schema_name = '" + schema + "' AND name = '" + mvName + "'",
".*ICEBERG_INVALID_MATERIALIZED_VIEW.*");
}
```
4. **If `getMaterializedView` is exposed directly in the base test infrastructure**
If the base test class exposes a helper that calls `ConnectorMetadata.getMaterializedView`, add an additional test that calls that helper after corrupting the properties and verifies that it returns `Optional.empty()` when the properties are missing rather than malformed. For example:
```java
@Test
public void testGetMaterializedViewReturnsEmptyWhenPropertiesMissing()
{
String schema = "tpch";
String mvName = "test_missing_mv_properties_rest";
assertUpdate("CREATE MATERIALIZED VIEW " + mvName + " AS SELECT 1 x");
// Adjust the helper to *remove* all MV-identifying properties so that metadata lookup treats it as absent
corruptMaterializedViewProperties(schema, mvName);
Optional<?> materializedView = getMaterializedView("iceberg", schema, mvName);
assertEquals(materializedView, Optional.empty());
}
```
You’ll need to align the call to `getMaterializedView(...)` with whatever helper the base class provides (catalog name, schema name, and MV name parameters).
These additions will ensure that the REST-specific `IcebergViewMetadata` path is exercised and that invalid or missing materialized view metadata is correctly surfaced as `ICEBERG_INVALID_MATERIALIZED_VIEW` or as an empty result from `getMaterializedView`, as requested in your review comment.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+843
to
+849
| PrincipalPrivileges privileges = buildInitialPrivilegeSet(table.getOwner()); | ||
| metastore.replaceTable( | ||
| metastoreContext, | ||
| viewName.getSchemaName(), | ||
| viewName.getTableName(), | ||
| updatedTable, | ||
| privileges); |
Contributor
There was a problem hiding this comment.
🚨 issue (security): Updating view properties may unintentionally reset privileges instead of preserving existing ones.
In updateIcebergViewProperties, calling buildInitialPrivilegeSet(table.getOwner()) before replaceTable will reinitialize privileges and discard any existing fine-grained grants on the view. If this method is only meant to change properties, consider preserving the current privileges (or passing Optional.empty()/an equivalent no-op per the metastore API) so that access control is not unintentionally altered by a properties-only update.
| import static com.google.common.io.RecursiveDeleteOption.ALLOW_INSECURE; | ||
|
|
||
| @Test(singleThreaded = true) | ||
| public class TestIcebergMaterializedViewsHive |
Contributor
There was a problem hiding this comment.
suggestion (testing): Consider adding a Hive-specific test that asserts SHOW MATERIALIZED VIEWS/listMaterializedViews only returns Iceberg materialized views, not regular Iceberg tables or views
Since the Hive implementation of listMaterializedViews relies on metastore.getAllViews and isIcebergMaterializedView, it would be good to add a Hive-specific assertion in this test class. For example:
- Create an Iceberg materialized view plus a regular Iceberg table and/or non-materialized view in
test_schema. - Assert that
SHOW MATERIALIZED VIEWS FROM test_schema(or the equivalentinformation_schemaquery) returns only the materialized view.
This will exercise the Hive metastore wiring and isIcebergMaterializedView behavior and guard against regressions in how HMS identifies materialized views.
| import static com.google.common.io.RecursiveDeleteOption.ALLOW_INSECURE; | ||
|
|
||
| @Test(singleThreaded = true) | ||
| public class TestIcebergMaterializedViewsRest |
Contributor
There was a problem hiding this comment.
suggestion (testing): Add a REST-catalog-specific assertion around materialized view metadata retrieval failures (e.g., invalid or missing MV properties)
Since the REST catalog still uses native Iceberg views (vs. Hive’s HMS tables), the new getMaterializedView logic that depends on IcebergViewMetadata isn’t fully exercised by the shared base tests. Please add a REST-specific test in this class that corrupts or removes materialized view properties (via direct catalog/metastore manipulation or ALTER TABLE/VIEW, if possible) and verifies that:
SHOW CREATE MATERIALIZED VIEWorSELECT * FROM system.metadata.materialized_viewsfails withICEBERG_INVALID_MATERIALIZED_VIEW, orgetMaterializedViewreturns empty when required properties are missing.
This will directly test the REST-specific IcebergViewMetadata wiring and error handling.
Suggested implementation:
import java.util.Optional;
import org.apache.iceberg.catalog.TableIdentifier;
import org.apache.iceberg.rest.RESTCatalog;
import org.apache.iceberg.view.View;
import org.apache.iceberg.view.ViewUpdate;
import static com.facebook.presto.iceberg.CatalogType.REST;
import static com.facebook.presto.iceberg.IcebergQueryRunner.builder;
import static com.facebook.presto.iceberg.rest.IcebergRestTestUtil.getRestServer;
import static com.facebook.presto.iceberg.rest.IcebergRestTestUtil.restConnectorProperties;
import static com.google.common.io.MoreFiles.deleteRecursively;
import static com.google.common.io.RecursiveDeleteOption.ALLOW_INSECURE;To fully implement the REST-catalog-specific assertions for invalid/missing materialized view metadata, add the following to TestIcebergMaterializedViewsRest:
-
Helper to corrupt MV properties in the REST/Iceberg catalog
Inside
TestIcebergMaterializedViewsRest(as aprivatemethod), add something along these lines, using the REST catalog that backsrestServer:private void corruptMaterializedViewProperties(String schema, String name) { // Obtain the underlying Iceberg REST catalog from the test REST server. // Adjust this depending on the actual type returned by getRestServer(). RESTCatalog catalog = (RESTCatalog) getRestServer(restServer).getCatalog(); TableIdentifier identifier = TableIdentifier.of(schema, name); View view = catalog.loadView(identifier); // Copy and corrupt properties by removing / altering MV-specific ones // (these keys must match what IcebergViewMetadata expects) java.util.Map<String, String> properties = new java.util.HashMap<>(view.properties()); properties.remove("presto.materialized-view.target"); properties.remove("presto.materialized-view.query"); // You can also deliberately corrupt required properties instead of removing them: // properties.put("presto.materialized-view.target", "invalid-target"); ViewUpdate update = catalog.updateView(identifier); update.setProperties(properties); update.commit(); }
You will likely need to adjust
getRestServer(restServer).getCatalog()to match the actual API. The important part is:- obtain the
RESTCatalog, - load the Iceberg
Viewcorresponding to the Presto materialized view, - remove or corrupt the properties that
IcebergViewMetadatadepends on.
- obtain the
-
REST-specific test: SHOW CREATE MATERIALIZED VIEW fails with ICEBERG_INVALID_MATERIALIZED_VIEW
Still in
TestIcebergMaterializedViewsRest, add a new test method:@Test public void testShowCreateMaterializedViewFailsOnInvalidMetadata() { String schema = "tpch"; // adjust if the base class uses a different default schema String mvName = "test_invalid_mv_metadata_rest"; assertUpdate("CREATE MATERIALIZED VIEW " + mvName + " AS SELECT 1 x"); // Corrupt the underlying Iceberg view metadata via REST catalog corruptMaterializedViewProperties(schema, mvName); // SHOW CREATE MATERIALIZED VIEW should now fail with ICEBERG_INVALID_MATERIALIZED_VIEW assertQueryFails( "SHOW CREATE MATERIALIZED VIEW " + mvName, ".*ICEBERG_INVALID_MATERIALIZED_VIEW.*"); }
This directly exercises the
IcebergViewMetadatalookup path used bygetMaterializedViewfor REST and verifies that invalid metadata is surfaced asICEBERG_INVALID_MATERIALIZED_VIEW. -
(Optional but recommended) REST-specific test via
system.metadata.materialized_viewsTo additionally cover the
getMaterializedViewwiring used bysystem.metadata.materialized_views, add:@Test public void testSystemMetadataMaterializedViewsFailsOnInvalidMetadata() { String schema = "tpch"; String mvName = "test_invalid_mv_metadata_system_rest"; assertUpdate("CREATE MATERIALIZED VIEW " + mvName + " AS SELECT 1 x"); corruptMaterializedViewProperties(schema, mvName); // Accessing MV metadata via system table should also fail with the same error assertQueryFails( "SELECT * FROM system.metadata.materialized_views WHERE catalog_name = 'iceberg' AND schema_name = '" + schema + "' AND name = '" + mvName + "'", ".*ICEBERG_INVALID_MATERIALIZED_VIEW.*"); }
-
If
getMaterializedViewis exposed directly in the base test infrastructureIf the base test class exposes a helper that calls
ConnectorMetadata.getMaterializedView, add an additional test that calls that helper after corrupting the properties and verifies that it returnsOptional.empty()when the properties are missing rather than malformed. For example:@Test public void testGetMaterializedViewReturnsEmptyWhenPropertiesMissing() { String schema = "tpch"; String mvName = "test_missing_mv_properties_rest"; assertUpdate("CREATE MATERIALIZED VIEW " + mvName + " AS SELECT 1 x"); // Adjust the helper to *remove* all MV-identifying properties so that metadata lookup treats it as absent corruptMaterializedViewProperties(schema, mvName); Optional<?> materializedView = getMaterializedView("iceberg", schema, mvName); assertEquals(materializedView, Optional.empty()); }
You’ll need to align the call to
getMaterializedView(...)with whatever helper the base class provides (catalog name, schema name, and MV name parameters).
These additions will ensure that the REST-specific IcebergViewMetadata path is exercised and that invalid or missing materialized view metadata is correctly surfaced as ICEBERG_INVALID_MATERIALIZED_VIEW or as an empty result from getMaterializedView, as requested in your review comment.
tdcmeehan
force-pushed
the
mv-ice-hive
branch
from
ca9238b to
6da6dfb
Compare
Member
hantangwangd
left a comment
hantangwangd
left a comment
There was a problem hiding this comment.
Thanks @tdcmeehan for this feature, overall looks good! I've left some quick questions and points for discussion.
presto-iceberg/src/main/java/com/facebook/presto/iceberg/IcebergAbstractMetadata.java
Show resolved
Hide resolved
presto-iceberg/src/main/java/com/facebook/presto/iceberg/IcebergHiveMetadata.java
Outdated
Show resolved
Hide resolved
presto-iceberg/src/main/java/com/facebook/presto/iceberg/IcebergHiveMetadata.java
Show resolved
Hide resolved
presto-iceberg/src/main/java/com/facebook/presto/iceberg/IcebergHiveMetadata.java
Outdated
Show resolved
Hide resolved
presto-iceberg/src/main/java/com/facebook/presto/iceberg/IcebergHiveMetadata.java
Outdated
Show resolved
Hide resolved
tdcmeehan
force-pushed
the
mv-ice-hive
branch
from
dcf051c to
7b07ca4
Compare
Member
hantangwangd
left a comment
hantangwangd
left a comment
There was a problem hiding this comment.
Thanks @tdcmeehan for the fix and clarification. Just a few nits left, otherwise looks good to me!
presto-iceberg/src/main/java/com/facebook/presto/iceberg/IcebergHiveMetadata.java
Outdated
Show resolved
Hide resolved
presto-iceberg/src/main/java/com/facebook/presto/iceberg/IcebergHiveMetadata.java
Outdated
Show resolved
Hide resolved
presto-iceberg/src/main/java/com/facebook/presto/iceberg/IcebergHiveMetadata.java
Show resolved
Hide resolved
Contributor
Author
|
As always, thanks @hantangwangd for your thorough review. I've updated based on your feedback. |
hantangwangd
approved these changes
Jan 29, 2026
Member
hantangwangd
left a comment
hantangwangd
left a comment
There was a problem hiding this comment.
Lgtm! Thanks for the fix @tdcmeehan.
This was referenced Mar 31, 2026
15 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Add support for materialized views with Hive catalog
Motivation and Context
So users of the Hive metastore catalog can use Materialized Views as well
Impact
Now all catalogs will support materialized views
Test Plan
I've migrated tests to use both HMS and REST catalogs
Contributor checklist
Release Notes
Please follow release notes guidelines and fill in the release notes below.
Summary by Sourcery
Add a shared Iceberg view metadata abstraction and extend the Iceberg connector to support materialized views in both REST and Hive catalogs.
New Features:
Enhancements:
Tests: