Handle missing StorageDescriptor in Hive Glue materialized views by grantatspothero · Pull Request #17276 · trinodb/trino

grantatspothero · 2023-04-27T21:33:00Z

Description

Additional context and related issues

Related PR that fixed some of the issues with null storage descriptors with glue, but did not handle materialized views: https://github.com/trinodb/trino/pull/11092/files

Release notes

( ) This is not user-visible or docs only and no release notes are required.
( ) Release notes are required, please propose a release note for me.
(x) Release notes are required, with the following suggested text:

# Section
* Allow the Hive Glue Metastore to redirect Iceberg Materialized Views which are missing a StorageDescriptor

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

...o-hive/src/main/java/io/trino/plugin/hive/metastore/glue/converter/GlueToTrinoConverter.java

...n/trino-hive/src/test/java/io/trino/plugin/hive/metastore/glue/TestGlueToTrinoConverter.java

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

...berg/src/main/java/io/trino/plugin/iceberg/catalog/hms/AbstractMetastoreTableOperations.java

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

...src/main/java/io/trino/plugin/deltalake/metastore/HiveMetastoreBackedDeltaLakeMetastore.java

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

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

findepi · 2023-05-08T08:24:04Z

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

isHiveOrPrestoView(mv) // returns false isPrestoView(mv) // returns true

however, per it's name, isHiveOrPrestoView should be superset of isPrestoView, right?

Makes sense from a consistency standpoint.

Removed isHiveOrPrestoView and the behavior can be replicated with isViewOrMaterializedView(view) && !isTrinoMaterializedView(view).

I think this makes sense from an API standpoint. I do not think a method like isView should exist because it is unclear if view refers to materialized or not.

findepi · 2023-05-08T08:46:42Z

This PR this good in a way it approaches to untangle technical debt around isXxxView methods.
However, the increased ambitions mean the bug fix is delayed, so I created slimmed down version of this: #17383.

isViewOrMaterializedView and isTrinoMaterializedView

grantatspothero · 2023-05-08T21:35:43Z

Almost there! Fixed final comment and added one rename commit for isPrestoView -> isTrinoView. I think the API makes a lot more sense now (isTrinoView, isTrinoMaterializedView, isViewOrMaterializedView)

Please take another look.

renamed for consistency with the rest of the isView methods

findepi · 2023-05-09T08:41:01Z

...src/main/java/io/trino/plugin/deltalake/metastore/HiveMetastoreBackedDeltaLakeMetastore.java

 import static io.trino.plugin.deltalake.DeltaLakeMetadata.PATH_PROPERTY;
 import static io.trino.plugin.hive.TableType.MANAGED_TABLE;
-import static io.trino.plugin.hive.ViewReaderUtil.isHiveOrPrestoView;
+import static io.trino.plugin.hive.ViewReaderUtil.isViewOrMaterializedView;


Remove isHiveOrPrestoView and replace usages with

This ^ is the commit title. Looks truncated. See https://github.com/trinodb/trino/blob/master/.github/DEVELOPMENT.md#format-git-commit-messages

findepi · 2023-05-09T08:44:27Z

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

    public static boolean isTrinoMaterializedView(Map<String, String> tableParameters)
    {
-        return isPrestoView(tableParameters) && tableParameters.get(TABLE_COMMENT).equalsIgnoreCase(ICEBERG_MATERIALIZED_VIEW_COMMENT);
+        return isTrinoView(tableParameters) && tableParameters.get(TABLE_COMMENT).equalsIgnoreCase(ICEBERG_MATERIALIZED_VIEW_COMMENT);


is it obvious that isTrinoView can be true also for materialized view?

to me, a materialized view is a separate type, not a subclass of view. especially given that it has separate handling in ConnectorMetadata

that's perhaps why you introduced isViewOrMaterializedView method -- you didn't call it isView, as would be sufficient if materialized views were always treated as a subcategory of views

i am doing this in #18570

findepi · 2023-05-09T08:59:37Z

...o-hive/src/main/java/io/trino/plugin/hive/metastore/glue/converter/GlueToTrinoConverter.java

-        if (isIcebergTable(tableParameters) || (sd == null && isDeltaLakeTable(tableParameters))) {
+        if (isIcebergTable(tableParameters) ||
+                (sd == null && isDeltaLakeTable(tableParameters)) ||
+                (sd == null && isTrinoMaterializedView(firstNonNull(glueTable.getParameters(), Map.of())))) {


you already have tableParameters

(will address this in #17383)

github-actions · 2024-01-16T17:03:53Z

This pull request has gone a while without any activity. Tagging the Trino developer relations team: @bitsondatadev @colebow @mosabua

mosabua · 2024-01-16T18:42:29Z

👋 @grantatspothero @findepi - this PR has become inactive. If you're still interested in working on it, please let us know.

We're working on closing out old and inactive PRs, so if you're too busy or this has too many merge conflicts to be worth picking back up, we'll be making another pass to close it out in a few weeks.

github-actions · 2024-02-09T17:03:11Z

This pull request has gone a while without any activity. Tagging the Trino developer relations team: @bitsondatadev @colebow @mosabua

github-actions · 2024-03-01T17:03:27Z

Closing this pull request, as it has been stale for six weeks. Feel free to re-open at any time.

mosabua · 2024-03-01T18:50:29Z

Feels like this should be reopened and continued.. wdyt @grantatspothero ?

cla-bot bot added the cla-signed label Apr 27, 2023

grantatspothero requested review from Randgalt, alexjo2144 and findepi April 27, 2023 21:33

grantatspothero commented Apr 27, 2023

View reviewed changes

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

grantatspothero force-pushed the gn/fixGlueToTrinoConverterMaterializedViewHandling branch from a018848 to 309e8f7 Compare April 27, 2023 21:47

github-actions bot added hive Hive connector tests:hive labels Apr 28, 2023

findepi reviewed Apr 28, 2023

View reviewed changes

...o-hive/src/main/java/io/trino/plugin/hive/metastore/glue/converter/GlueToTrinoConverter.java Outdated Show resolved Hide resolved

findepi reviewed Apr 28, 2023

View reviewed changes

...n/trino-hive/src/test/java/io/trino/plugin/hive/metastore/glue/TestGlueToTrinoConverter.java Outdated Show resolved Hide resolved

grantatspothero force-pushed the gn/fixGlueToTrinoConverterMaterializedViewHandling branch 2 times, most recently from 0ebce8b to 26948f5 Compare April 28, 2023 18:15

github-actions bot added the iceberg Iceberg connector label Apr 28, 2023

grantatspothero force-pushed the gn/fixGlueToTrinoConverterMaterializedViewHandling branch from 26948f5 to 9e5876f Compare May 1, 2023 16:33

github-actions bot added the delta-lake Delta Lake connector label May 1, 2023

grantatspothero force-pushed the gn/fixGlueToTrinoConverterMaterializedViewHandling branch from 9e5876f to 10867ab Compare May 1, 2023 18:32

findepi reviewed May 2, 2023

View reviewed changes

grantatspothero force-pushed the gn/fixGlueToTrinoConverterMaterializedViewHandling branch 3 times, most recently from 127fb6c to 17f2300 Compare May 5, 2023 17:45

findepi reviewed May 8, 2023

View reviewed changes

findepi mentioned this pull request May 8, 2023

Handle missing MV StorageDescriptor in Glue #17383

Merged

grantatspothero added 2 commits May 8, 2023 16:19

Remove isHiveOrPrestoView and replace usages with

a4d6603

isViewOrMaterializedView and isTrinoMaterializedView

Handle missing StorageDescriptor in Hive Glue materialized views

78b9dc8

grantatspothero force-pushed the gn/fixGlueToTrinoConverterMaterializedViewHandling branch from 17f2300 to 78b9dc8 Compare May 8, 2023 21:29

grantatspothero force-pushed the gn/fixGlueToTrinoConverterMaterializedViewHandling branch 2 times, most recently from 16f5623 to a43a1ba Compare May 8, 2023 21:41

Rename isPrestoView to isTrinoView

e8a44d1

renamed for consistency with the rest of the isView methods

grantatspothero force-pushed the gn/fixGlueToTrinoConverterMaterializedViewHandling branch from a43a1ba to e8a44d1 Compare May 8, 2023 21:53

findepi reviewed May 9, 2023

View reviewed changes

github-actions bot added the stale label Jan 16, 2024

github-actions bot removed the stale label Jan 18, 2024

github-actions bot added the stale label Feb 9, 2024

github-actions bot closed this Mar 1, 2024

Conversation

grantatspothero commented Apr 27, 2023

Description

Additional context and related issues

Release notes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

findepi May 8, 2023

Choose a reason for hiding this comment

Uh oh!

grantatspothero May 8, 2023

Choose a reason for hiding this comment

Uh oh!

findepi commented May 8, 2023

Uh oh!

grantatspothero commented May 8, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

findepi May 9, 2023

Choose a reason for hiding this comment

Uh oh!

findepi May 9, 2023

Choose a reason for hiding this comment

Uh oh!

findepi Aug 7, 2023

Choose a reason for hiding this comment

Uh oh!

findepi May 9, 2023

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Jan 16, 2024

Uh oh!

mosabua commented Jan 16, 2024

Uh oh!

github-actions bot commented Feb 9, 2024

Uh oh!

github-actions bot commented Mar 1, 2024

Uh oh!

mosabua commented Mar 1, 2024

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

4 participants

grantatspothero commented May 8, 2023 •

edited

Loading