Support materialized view creation, drop and query by gggrace14 · Pull Request #15589 · prestodb/presto

gggrace14 · 2021-01-07T04:31:57Z

Depended by https://github.com/facebookexternal/presto-facebook/pull/1438

highker

Let's have meetings for this PR. There are a lot details to figure out. Also, we need a lot of tests for this

presto-hive-metastore/src/main/java/com/facebook/presto/hive/metastore/MetastoreUtil.java

presto-hive/src/main/java/com/facebook/presto/hive/HiveMetadata.java

presto-main/src/main/java/com/facebook/presto/execution/CreateMaterializedViewTask.java

highker · 2021-01-08T06:39:21Z

...tastore/src/main/java/com/facebook/presto/hive/metastore/SemiTransactionalHiveMetastore.java

We should make it a generic interface:

alterTable(ConnectorSession session, String databaseName, String tableName, Table newTable)

Then, in this function, we should verify everything else is the same and update the table params only. If we found something else is different (e.g., owner, column, etc), we should fail it. Same, even for table params, we should only touch PRESTO_DEPENDENT_MATERIALIZED_VIEW_LIST.

Add a javadoc to indicate the functionality is very limited

highker

We need a lot more real tests. TestHiveDistributedQueries is a good place to add.
Only half way through the first commit

presto-hive-metastore/src/main/java/com/facebook/presto/hive/metastore/MetastoreUtil.java

presto-hive/src/main/java/com/facebook/presto/hive/HiveMetadata.java

presto-spi/src/main/java/com/facebook/presto/spi/ConnectorMaterializedViewDefinition.java

presto-main/src/test/java/com/facebook/presto/execution/TestCreateMaterializedViewTask.java

presto-spi/src/main/java/com/facebook/presto/spi/ConnectorMaterializedViewDefinition.java

gggrace14 · 2021-02-04T02:47:28Z

We need a lot more real tests. TestHiveDistributedQueries is a good place to add.

Only half way through the first commit

jainxrohit · 2021-02-17T05:22:07Z

@gggrace14 can you please rebase these changes?

highker

Separate out the first commit as a separate PR. We probably don't want to merge the second commit at the moment. We will need DROP, REPLACE, SHOW CREATE, and REFRESH logic first.

presto-hive/src/main/java/com/facebook/presto/hive/HiveMetadata.java

presto-main/src/main/java/com/facebook/presto/sql/analyzer/Analysis.java

presto-hive/src/main/java/com/facebook/presto/hive/HiveMetadata.java

presto-hive/src/main/java/com/facebook/presto/hive/HiveClientModule.java

presto-hive/src/main/java/com/facebook/presto/hive/HiveMetadata.java

presto-main/src/main/java/com/facebook/presto/execution/CreateMaterializedViewTask.java

presto-main/src/main/java/com/facebook/presto/sql/analyzer/Analysis.java

presto-main/src/main/java/com/facebook/presto/sql/analyzer/StatementAnalyzer.java

presto-tests/src/main/java/com/facebook/presto/tests/AbstractTestDistributedQueries.java

presto-main/src/main/java/com/facebook/presto/execution/CreateMaterializedViewTask.java

highker · 2021-03-03T21:16:08Z

presto-main/src/test/java/com/facebook/presto/execution/TestCreateMaterializedViewTask.java

All these could be local variables.

forgot to address the comments?

Oh, forgot to comment. They're used in multiple Test cases (and initialized in setUp()). So might need to be class fields to be accessed by the Test cases.

Oh, forgot to reply. They're used by multiple Test cases (and initialized in setUp()). So might need to be class members.

I don't think they are used. Please check IntelliJ warnings. They can all be local variables than class members.

Got what you mean. Revised. Good to see the IntelliJ highlight.

presto-main/src/test/java/com/facebook/presto/execution/TestCreateMaterializedViewTask.java

presto-hive/src/main/java/com/facebook/presto/hive/HiveMetadata.java

presto-main/src/main/java/com/facebook/presto/sql/analyzer/StatementAnalyzer.java

presto-hive/src/test/java/com/facebook/presto/hive/TestHiveIntegrationSmokeTest.java

highker

minor comments only

highker · 2021-03-07T00:20:10Z

presto-main/src/main/java/com/facebook/presto/sql/analyzer/StatementAnalyzer.java

We need to add a TODO here right? This is where the partition stitching happens. Without TODO, it's very easy to miss this part.

highker · 2021-03-07T00:22:53Z

presto-main/src/test/java/com/facebook/presto/execution/TestCreateMaterializedViewTask.java

forgot to address the comments?

highker · 2021-03-07T00:30:55Z

presto-hive/src/test/java/com/facebook/presto/hive/TestHiveIntegrationSmokeTest.java

Is this correct? What would be the column mapping looks like?
test_region_nation_join_mv.nationkey = test_customer_base.nationkey
but test_orders_base is with orderstatus as the partition key right?

Right, revised.

I'm curious why this will pass assert? It should fail right. It doesn't look like a legit use case.

Suppose for the use case below, the exchange_rate is a small dimension table partitioned by 'country'. If we include country as the mv partition key, a reload of a country will trigger the refresh of impacted revenue_aggregation_mv partitions. If we don't include country as a mv partition key, a reload of country won't affect the past revenue_aggregation_mv data. Need to have this flexibility to handle different customer needs.

CREATE MATERIALIZED VIEW IF NOT EXISTS revenue_aggregation_mv
WITH (partitioned_by = ARRAY['ds'])
AS SELECT
SUM(l.legal_budget * r.to_usd) AS total_legal_budget_usd, country, ds
FROM revenue_aggregation_imp l JOIN exchange_rate r ON l.country = r.country
GROUP BY country, ds

presto-hive/src/test/java/com/facebook/presto/hive/TestHiveIntegrationSmokeTest.java

highker · 2021-03-09T04:58:27Z

presto-hive/src/test/java/com/facebook/presto/hive/TestHiveIntegrationSmokeTest.java

The comment is not addressed. We need to assertQueryFails if we are to insert into a materialized view will fail and delete from a materialized view will fail.

highker · 2021-03-09T05:06:27Z

presto-hive/src/test/java/com/facebook/presto/hive/TestHiveIntegrationSmokeTest.java

Can we add a test where a join query will fail? Especially more tests to cover partition column mapping.

highker · 2021-03-09T05:07:32Z

presto-main/src/main/java/com/facebook/presto/execution/AddColumnTask.java

This doesn't look right. We shouldn't throw MISSING_TABLE error. We should tell user we cannot alter a materialized view.

highker · 2021-03-09T05:09:11Z

presto-main/src/main/java/com/facebook/presto/execution/RenameColumnTask.java

same, we should throw errors other than MISSING_TABLE

highker · 2021-03-09T05:09:16Z

presto-main/src/main/java/com/facebook/presto/execution/RenameTableTask.java

highker · 2021-03-09T05:10:05Z

presto-main/src/main/java/com/facebook/presto/sql/analyzer/StatementAnalyzer.java

It's not a warning. It's a failure.

highker · 2021-03-09T05:10:18Z

presto-main/src/main/java/com/facebook/presto/sql/analyzer/StatementAnalyzer.java

break a line after this

highker · 2021-03-09T05:10:24Z

presto-main/src/main/java/com/facebook/presto/sql/analyzer/StatementAnalyzer.java

gggrace14 · 2021-03-11T02:20:48Z

...metastore/src/test/java/com/facebook/presto/hive/metastore/thrift/InMemoryHiveMetastore.java

@@ -172,15 +173,15 @@ public synchronized List<String> getAllDatabases()
    public synchronized void createTable(Table table)
    {
        TableType tableType = TableType.valueOf(table.getTableType());


In the class here along with other implementation at this level, org.apache.hadoop.hive.metastore.TableType is being called, while Hive 2.0 doesn't have TableType.MATERIALIZED_VIEW. So I need move the table type casting logic to one level above, ThriftMetastoreUtil.fromMetastoreApiTable() and toMetastoreApiTable().

highker

Early comments; otherwise LGTM

highker · 2021-03-11T06:30:12Z

...-hive-metastore/src/main/java/com/facebook/presto/hive/metastore/file/FileHiveMetastore.java

put a () around !table.getTableType().equals(MANAGED_TABLE) && !table.getTableType().equals(MATERIALIZED_VIEW)

highker · 2021-03-11T06:34:48Z

...e-metastore/src/main/java/com/facebook/presto/hive/metastore/thrift/ThriftMetastoreUtil.java

Add a TODO here to remove the type change after Hive 3.0 upgrade

highker · 2021-03-11T06:36:02Z

...e-metastore/src/main/java/com/facebook/presto/hive/metastore/thrift/ThriftMetastoreUtil.java

Don't mention Facebook. This is open source; we should be neutral. Add a TODO here to remove the type change after Hive 3.0 upgrade

highker · 2021-03-11T06:39:46Z

presto-spi/src/main/java/com/facebook/presto/spi/StandardWarningCode.java

no need to change this file anymore

highker · 2021-03-11T06:40:16Z

...e-metastore/src/main/java/com/facebook/presto/hive/metastore/thrift/ThriftMetastoreUtil.java

Accidental change? We don't have to modify this.

Here's where we cast managed_table back to materialized_view for getTable().

highker · 2021-03-12T04:21:58Z

presto-hive/src/test/java/com/facebook/presto/hive/TestHiveIntegrationSmokeTest.java

add assertUpdate("DROP TABLE test_customer_base"); and the same for test_orders_base. We wanna clean up the created table after unit test is done.

highker · 2021-03-12T04:22:10Z

presto-hive/src/test/java/com/facebook/presto/hive/TestHiveIntegrationSmokeTest.java

same drop table test_customer_base_1 here on exist

highker · 2021-03-12T04:22:19Z

presto-hive/src/test/java/com/facebook/presto/hive/TestHiveIntegrationSmokeTest.java

same, drop test_customer_base_2

highker · 2021-03-12T04:22:29Z

presto-hive/src/test/java/com/facebook/presto/hive/TestHiveIntegrationSmokeTest.java

same drop test_customer_base_3

highker · 2021-03-12T04:22:37Z

presto-hive/src/test/java/com/facebook/presto/hive/TestHiveIntegrationSmokeTest.java

drop test_customer_base_4

Add createMaterializedView API to MetadataManager and add CreateMaterializedViewTask that calls the API to perform MV creation. HiveMetadata implementation of the API is to add essential MV parameters and create MV as a standard table. ConnectorMaterializedViewDefinition is the json structure that contains all essential metadata we will save to Metastore. It will serialize into the viewOriginalText field of table metadata. Add MV partition. At the moment, we only support partitioned MV defined on partitioned base tables. MV must have one partition directly matched (selected) from base tables. Add MATERIALIZED_VIEW table type. We don't support alter schema of materialized view at the moment. We don't support MVs defined across different catalogs, which is enforced in CreateMaterializedViewTask

Add dropMaterializedView API to MetadataManager, and add DropMaterializedViewTask that calls the API to perform MV drop. HiveMetadata implementation of the API is to directly drop the MV table.

kaikalur

Did the language spec get reviewed? I had concerns earlier on VIEW vs MATERIALIZED VIEW? Were those addressed?

highker · 2021-03-16T02:55:26Z

Did the language spec get reviewed? I had concerns earlier on VIEW vs MATERIALIZED VIEW? Were those addressed?

Chatted offline with @rongrong once early this year. We are going to model MATERIALIZED VIEW as VIEW with strong consistency. MATERIALIZED VIEW is VIEW with or without backfilled/materialized data. We will have a lot of code in Presto to make sure that. Also, we will have metastore support to guarantee transactions so that base tables and materialized views are always in sync.

gggrace14 requested review from highker and jainxrohit January 7, 2021 04:40

gggrace14 mentioned this pull request Jan 7, 2021

Support drop materialized view #15590

Closed

gggrace14 force-pushed the mpb branch from 0645a32 to 01151ca Compare January 7, 2021 05:22

highker reviewed Jan 8, 2021

View reviewed changes

gggrace14 force-pushed the mpb branch 4 times, most recently from ae2354d to cea67a3 Compare February 2, 2021 06:04

gggrace14 mentioned this pull request Feb 2, 2021

Add capability to get partitions separately from MV and base tables #15662

Closed

gggrace14 requested a review from highker February 2, 2021 18:41

gggrace14 force-pushed the mpb branch from cea67a3 to 73c7931 Compare February 2, 2021 19:58

gggrace14 changed the title ~~Support materialized view creation and query~~ Support materialized view creation, drop and query Feb 2, 2021

gggrace14 force-pushed the mpb branch 3 times, most recently from 4e9afee to 59d3299 Compare February 3, 2021 07:26

highker reviewed Feb 3, 2021

View reviewed changes

gggrace14 closed this Feb 4, 2021

gggrace14 reopened this Feb 4, 2021

gggrace14 force-pushed the mpb branch from 59d3299 to 79629ed Compare February 19, 2021 20:41

highker self-requested a review February 22, 2021 02:12

highker requested changes Feb 22, 2021

View reviewed changes

gggrace14 force-pushed the mpb branch from 79629ed to 91c2af0 Compare March 2, 2021 01:06

gggrace14 requested a review from highker March 2, 2021 01:27

highker reviewed Mar 2, 2021

View reviewed changes

highker self-requested a review March 2, 2021 03:06

highker reviewed Mar 4, 2021

View reviewed changes

gggrace14 force-pushed the mpb branch from 91c2af0 to f437eb4 Compare March 5, 2021 08:33

gggrace14 commented Mar 5, 2021

View reviewed changes

presto-hive/src/test/java/com/facebook/presto/hive/TestHiveIntegrationSmokeTest.java Outdated Show resolved Hide resolved

gggrace14 requested a review from highker March 5, 2021 19:05

gggrace14 mentioned this pull request Mar 5, 2021

Support SHOW CREATE MATERIALIZED VIEW #15779

Merged

highker reviewed Mar 7, 2021

View reviewed changes

gggrace14 force-pushed the mpb branch 2 times, most recently from 63a6b44 to 4b48c96 Compare March 8, 2021 23:20

gggrace14 requested a review from highker March 8, 2021 23:23

highker requested changes Mar 9, 2021

View reviewed changes

gggrace14 force-pushed the mpb branch from 4b48c96 to 8cb1739 Compare March 10, 2021 07:50

gggrace14 commented Mar 11, 2021

View reviewed changes

gggrace14 force-pushed the mpb branch from 8cb1739 to 00a709f Compare March 11, 2021 05:03

highker mentioned this pull request Mar 11, 2021

Upgrade to Hive 3.0.0 #15659

Merged

highker reviewed Mar 11, 2021

View reviewed changes

gggrace14 force-pushed the mpb branch from 00a709f to 9988de9 Compare March 11, 2021 09:21

gggrace14 requested a review from highker March 11, 2021 10:05

gggrace14 force-pushed the mpb branch 3 times, most recently from 6eded58 to 8d9b1c1 Compare March 12, 2021 02:13

highker approved these changes Mar 12, 2021

View reviewed changes

highker self-assigned this Mar 12, 2021

gggrace14 added 2 commits March 12, 2021 21:13

Support DROP MATERIALIZED VIEW syntax

b9dacfb

gggrace14 force-pushed the mpb branch from 8d9b1c1 to 0613cff Compare March 13, 2021 05:44

Support execution of DROP MATERIALIZED VIEW

29c41f6

Add dropMaterializedView API to MetadataManager, and add DropMaterializedViewTask that calls the API to perform MV drop. HiveMetadata implementation of the API is to directly drop the MV table.

gggrace14 force-pushed the mpb branch from 0613cff to 29c41f6 Compare March 13, 2021 07:38

highker merged commit fc80960 into prestodb:master Mar 15, 2021

kaikalur reviewed Mar 16, 2021

View reviewed changes

varungajjala mentioned this pull request Mar 23, 2021

Add release notes for 0.250 #15865

Merged

6 tasks

gggrace14 added the materialized-view label Jun 9, 2021

Conversation

gggrace14 commented Jan 7, 2021 • edited by highker Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

highker left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

highker left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

gggrace14 commented Feb 4, 2021

Uh oh!

jainxrohit commented Feb 17, 2021

Uh oh!

highker left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

highker left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gggrace14 commented Jan 7, 2021 •

edited by highker

Loading

gggrace14 Mar 7, 2021 •

edited

Loading

gggrace14 Mar 11, 2021 •

edited

Loading