Add Materialized View support to the Iceberg Glue Catalog by alexjo2144 · Pull Request #11780 · trinodb/trino

alexjo2144 · 2022-04-04T20:19:50Z

Description

Adds support for Materialized Views when using the Iceberg connector with Glue.

Is this change a fix, improvement, new feature, refactoring, or other?

New feature

Is this a change to the core query engine, a connector, client library, or the SPI interfaces? (be specific)

Connector, Iceberg.

How would you describe this change to a non-technical end user or system administrator?

Related issues, pull requests, and links

Documentation

Materialized Views are already described in the Iceberg documentation.

(x) No documentation is needed.
( ) Sufficient documentation is included in this PR.
( ) Documentation PR is available with #prnumber.
( ) Documentation issue #issuenumber is filed, and can be handled later.

Release notes

( ) No release notes entries required.
(x) Release notes entries required with the following suggested text:

# Iceberg
* Materialized View support to the Iceberg Glue Catalog

findepi · 2022-04-05T10:09:21Z

@alexjo2144 please rebase your PR (i guess it conflicted with #11766)

plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/catalog/glue/TrinoGlueCatalog.java

raunaqmorarka · 2022-04-05T11:55:57Z

plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/catalog/glue/TrinoGlueCatalog.java

schemaViewName -> materializedViewName

I think I'd rather rename this to have consistency with what it's called in ConnectorMetadata, viewName. https://github.com/trinodb/trino/blob/master/core/trino-spi/src/main/java/io/trino/spi/connector/ConnectorMetadata.java#L1247

If we want to use materializedViewName instead we can do from there down.

plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/catalog/glue/TrinoGlueCatalog.java

raunaqmorarka · 2022-04-05T12:10:26Z

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

Smoke tests are testing only cases around CREATE, SELECT, SHOW CREATE and DROP.
We should be testing CREATE OR REPLACE, listing of MVs and RENAME as well with Glue. Is that already covered by other tests ?

I didn't add any. Do you know if there's coverage in BaseConnectorTest? Yuya has a PR running that against Glue #11746

We have coverage for listing and RENAME of MVs in BaseConnectorTest, but CREATE OR REPLACE doesn't seem to be covered there. Maybe we can move or copy TestIcebergMaterializedViews#testReplace to BaseConnectorTest if that is going to be run with Glue in future.

For now I converted TestIcebergMaterializedViews to BaseIcebergMaterializedViewTest and added both a Hive and Glue backed implementation.

plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/catalog/glue/TrinoGlueCatalog.java

plugin/trino-hive/src/main/java/io/trino/plugin/hive/ViewReaderUtil.java

findepi · 2022-04-06T08:14:50Z

@alexjo2144 please squash and rebase

alexjo2144 · 2022-04-06T16:16:39Z

Squashed and rebased

alexjo2144 · 2022-04-06T16:30:43Z

@findepi can you create a Trino mirror branch to run the Glue tests on?

findepi · 2022-04-06T19:53:50Z

@findepi can you create a Trino mirror branch to run the Glue tests on?

#11833

homar · 2022-04-07T08:44:36Z

plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/catalog/glue/TrinoGlueCatalog.java

+                LOG.warn(e, "Failed to drop storage table '%s' for materialized view '%s'", storageTableName, viewName);
+            }
+        }
+        deleteTable(schemaName, viewName);


Silly question, does it make sense to delete table that was not dropped correctly?

alexjo2144 · 2022-04-07T19:15:46Z

Updated the GET_TABLE counts in TestIcebergGlueCatalogAccessOperations

plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/catalog/glue/TrinoGlueCatalog.java

...c/test/java/io/trino/plugin/iceberg/catalog/glue/TestIcebergGlueCatalogAccessOperations.java

findepi · 2022-04-08T08:36:33Z

...st/java/io/trino/plugin/iceberg/catalog/glue/TestIcebergGlueCatalogMaterializedViewTest.java

Will something eventually clean these up when test is aborted in the middle (cleanup isn't invoked or fails)?

I was going to put up a PR for cleanup across all these tests. I was thinking of adding a iceberg_integration_test_schema flag or something like that in the Glue schema properties to be able to filter them easily

...st/java/io/trino/plugin/iceberg/catalog/glue/TestIcebergGlueCatalogMaterializedViewTest.java

`viewName` matches the parameter name used in ConnectorMetadata.

findepi · 2022-04-13T09:28:26Z

( squashed and rebased )

findepi · 2022-04-13T09:29:59Z

( applied #11780 )

findepi · 2022-04-13T09:30:39Z

Updated #11833

cla-bot bot added the cla-signed label Apr 4, 2022

alexjo2144 requested a review from findepi April 4, 2022 20:21

findepi requested review from homar, raunaqmorarka and sopel39 April 5, 2022 10:08

raunaqmorarka reviewed Apr 5, 2022

View reviewed changes

sopel39 removed their request for review April 5, 2022 13:05

alexjo2144 force-pushed the glue-iceberg-mv branch from 274bba4 to 9c34392 Compare April 5, 2022 17:54

raunaqmorarka reviewed Apr 5, 2022

View reviewed changes

plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/catalog/glue/TrinoGlueCatalog.java Outdated Show resolved Hide resolved

raunaqmorarka reviewed Apr 5, 2022

View reviewed changes

plugin/trino-hive/src/main/java/io/trino/plugin/hive/ViewReaderUtil.java Outdated Show resolved Hide resolved

alexjo2144 force-pushed the glue-iceberg-mv branch from 9c34392 to ea48fc3 Compare April 5, 2022 20:26

github-actions bot added the tests:hive label Apr 5, 2022

findepi requested a review from raunaqmorarka April 6, 2022 08:05

raunaqmorarka approved these changes Apr 6, 2022

View reviewed changes

alexjo2144 force-pushed the glue-iceberg-mv branch from ea48fc3 to 982b7d1 Compare April 6, 2022 16:16

alexjo2144 force-pushed the glue-iceberg-mv branch from 982b7d1 to 7adb70e Compare April 6, 2022 18:54

findepi mentioned this pull request Apr 6, 2022

Test: Add Materialized View support to the Iceberg Glue Catalog #11833

Closed

homar reviewed Apr 7, 2022

View reviewed changes

homar approved these changes Apr 7, 2022

View reviewed changes

findepi requested a review from ebyhr April 7, 2022 12:08

alexjo2144 force-pushed the glue-iceberg-mv branch from 7adb70e to 5b4ea33 Compare April 7, 2022 19:15

findepi reviewed Apr 8, 2022

View reviewed changes

alexjo2144 requested a review from findepi April 11, 2022 14:52

findepi reviewed Apr 13, 2022

View reviewed changes

...st/java/io/trino/plugin/iceberg/catalog/glue/TestIcebergGlueCatalogMaterializedViewTest.java Outdated Show resolved Hide resolved

alexjo2144 added 2 commits April 13, 2022 11:28

Rename TrinoCatalog materialized view parameters

3613979

`viewName` matches the parameter name used in ConnectorMetadata.

Extract materialized view code out of TrinoHiveCatalog

4fbae73

findepi force-pushed the glue-iceberg-mv branch from 40175f4 to a3f00ad Compare April 13, 2022 09:28

Add Materialized View support to the Iceberg Glue Catalog

45b697a

findepi force-pushed the glue-iceberg-mv branch from a3f00ad to 45b697a Compare April 13, 2022 09:29

findepi approved these changes Apr 13, 2022

View reviewed changes

findepi merged commit f752c82 into trinodb:master Apr 13, 2022

findepi mentioned this pull request Apr 13, 2022

Release notes for 377 #11863

Closed

github-actions bot added this to the 377 milestone Apr 13, 2022

mosabua mentioned this pull request Apr 13, 2022

Add Trino 377 release notes #11859

Merged

Conversation

alexjo2144 commented Apr 4, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related issues, pull requests, and links

Documentation

Release notes

Uh oh!

findepi commented Apr 5, 2022

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

findepi commented Apr 6, 2022

Uh oh!

alexjo2144 commented Apr 6, 2022

Uh oh!

alexjo2144 commented Apr 6, 2022

Uh oh!

findepi commented Apr 6, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alexjo2144 commented Apr 7, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

findepi commented Apr 13, 2022

Uh oh!

findepi commented Apr 13, 2022

Uh oh!

findepi commented Apr 13, 2022

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

4 participants

alexjo2144 commented Apr 4, 2022 •

edited

Loading