Refactor is*View ViewReaderUtil methods by findepi · Pull Request #18570 · trinodb/trino

findepi · 2023-08-07T16:41:58Z

A "trino view" and "trino materialized view" need separate handling in
connectors (regardless what philosophical question whether materialized
view is also a view), so let this be expressed on utility function
level.

This changes

isTrinoMaterializedView refactor-only change, no behavioral changes,
isPrestoView - deprecated, replaced with isTrinoView which returns
false for materialized views; isPrestoView is equivalent of
isTrinoView || isTrinoMaterializedView,
isHiveOrTrinoView - deprecated, replaced with isSomeKindOfAView which
still returns true for materialized views (but returns false for
Hive's own materialzied view which is treated as an ordinary table by
Trino),
isHiveView - new method.

findepi · 2023-08-08T09:07:25Z

(just rebased)

findepi · 2023-08-08T09:58:12Z

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

In theory, the old code could replace an MV with a View (see #15622)

Could we add a test to verify it isn't allowed now ?

it would be very hard to write such a test. from linked PR desc

This would have only been possible during a rare race condition since the engine also checks that an existing entity is not a MV before calling on the connector to create or replace the view.

findepi · 2023-08-08T09:59:09Z

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

In theory, the old code could replace an MV with a View.

Could we add a test to verify it isn't allowed now ?

same answer here: #18570 (comment)

findepi · 2023-08-09T11:31:57Z

rebased to resolve logical conflicts

findepi · 2023-08-09T14:50:26Z

CI happy. please review.

raunaqmorarka · 2023-08-10T04:04:18Z

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

Do we have a test verifying that COMMENT ON VIEW doesn't work on an MV ?

idk. note that i am not changing this. toConnectorViewDefinition(...).orElseThrow on next line throws for anything that's not a view

raunaqmorarka · 2023-08-10T04:11:40Z

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

Could we add a test to verify it isn't allowed now ?

raunaqmorarka · 2023-08-10T04:11:49Z

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

Could we add a test to verify it isn't allowed now ?

findepi · 2023-08-10T11:59:10Z

there are conflicts, will rebase

Just to help review later commits.

The check above excludes everything that does not look like a Presto/Trino view, so the second check looks redundant. This also makes one method parameter redundant, removed.

The method is supposed to get a Trino view that's going to be updated. Its check for `translateHiveViews` was redundant, the calling code is never supposed to operate on a Hive view, regardless of `translateHiveViews` flag. (And if it matter, the `translateHiveViews` condition would probably be negated.) It was probably not user-facing problem as the calling code is likely unreachable when Hive view translation is not enabled. The commit also adds a check that the table object being operated on does not represent a Trino Materialized View. The previously checked `isPrestoView` returns true for both, but the table is supposed to represent ordinary Trino View.

Previously the `isTrinoView` would be true for Trino VIEW or Trino MATERIALIZED VIEW (since `isPrestoView()` returns true for both). This wasn't intentional, the code reserved `isTrinoMaterializedView` variable for MVs. This didn't had any end-user visible effect because the variables are currently later OR-ed.

Just for easier review of subsequent commits.

A "trino view" and "trino materialized view" need separate handling in connectors (regardless what philosophical question whether materialized view is also a view), so let this be expressed on utility function level. This changes - `isTrinoMaterializedView` refactor-only change, no behavioral changes, - `isPrestoView` - deprecated, replaced with `isTrinoView` which returns false for materialized views; `isPrestoView` is equivalent of `isTrinoView || isTrinoMaterializedView`, - `isHiveOrTrinoView` - deprecated, replaced with `isSomeKindOfAView` which still returns true for materialized views (but returns false for Hive's own materialzied view which is treated as an ordinary table by Trino), - `isHiveView` - new method.

findepi · 2023-08-10T12:03:02Z

will merge if still green

alexjo2144 · 2023-08-10T19:02:17Z

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


+        String tableType = getTableType(table);
        Map<String, String> parameters = getTableParameters(table);
-        if (isPrestoView(parameters) && isHiveOrPrestoView(getTableType(table))) {


isSomeKindOfView?

( #18570 (comment) )

alexjo2144 · 2023-08-10T19:04:18Z

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

        Table table = getTable();

-        if (isPrestoView(table) && isHiveOrPrestoView(table)) {
+        if (isTrinoView(table) || isTrinoMaterializedView(table)) {


isSomeKindOfView?

no, we we allow trino V and MV here (and fail on eg Hive views which are not supported by Iceberg -- they are neither tables or views, so something needs to throw on them)

findepi added the no-release-notes This pull request does not require release notes entry label Aug 7, 2023

findepi requested review from alexjo2144, ebyhr, electrum, findinpath, grantatspothero and mdesmet August 7, 2023 16:41

cla-bot bot added the cla-signed label Aug 7, 2023

findepi force-pushed the findepi/view-inspection-methods branch from 5ef7b79 to ef9e1e4 Compare August 7, 2023 16:44

This was referenced Aug 7, 2023

Add support for view column comments #13800

Merged

Support views in Delta lake Connector #11763

Merged

Prevent Views from replacing MVs #15622

Closed

Handle missing StorageDescriptor in Hive Glue materialized views #17276

Closed

github-actions bot added tests:hive iceberg Iceberg connector delta-lake Delta Lake connector hive Hive connector labels Aug 7, 2023

findepi requested review from kokosing and raunaqmorarka August 7, 2023 19:40

findepi force-pushed the findepi/view-inspection-methods branch from ef9e1e4 to 42bcd40 Compare August 8, 2023 09:07

findepi force-pushed the findepi/view-inspection-methods branch from 42bcd40 to 15b8ee1 Compare August 8, 2023 09:55

findepi commented Aug 8, 2023

View reviewed changes

findepi force-pushed the findepi/view-inspection-methods branch 2 times, most recently from c001569 to 790056a Compare August 9, 2023 11:31

raunaqmorarka approved these changes Aug 10, 2023

View reviewed changes

Inline condition and fix comment

0b79e1d

Just to help review later commits.

findepi added 8 commits August 10, 2023 13:59

Remove dead code in TrinoViewUtil.getView

f637594

The check above excludes everything that does not look like a Presto/Trino view, so the second check looks redundant. This also makes one method parameter redundant, removed.

Improve comment accuracy

edcd875

Merge Optional.filter with Optional.map calls

a094339

Just for easier review of subsequent commits.

Use "looks like some view" utility in more places

c340dc8

Document HiveMetastore.getAllViews to be including materialized views.

4ea0a43

findepi force-pushed the findepi/view-inspection-methods branch from 790056a to 4ea0a43 Compare August 10, 2023 12:02

alexjo2144 reviewed Aug 10, 2023

View reviewed changes

findepi merged commit 7dea413 into master Aug 11, 2023

findepi deleted the findepi/view-inspection-methods branch August 11, 2023 07:54

github-actions bot added this to the 424 milestone Aug 11, 2023

colebow mentioned this pull request Aug 16, 2023

Add Trino 424 release notes #18704

Merged

Conversation

findepi commented Aug 7, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

findepi commented Aug 8, 2023

Uh oh!

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

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

findepi commented Aug 9, 2023

Uh oh!

findepi commented Aug 9, 2023

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!

findepi commented Aug 10, 2023

Uh oh!

findepi commented Aug 10, 2023

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!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

3 participants

findepi commented Aug 7, 2023 •

edited

Loading

findepi Aug 8, 2023 •

edited

Loading