Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Support ALTER MATERIALIZED VIEW ... SET PROPERTIES ... #9613

Merged
merged 3 commits into from
Jan 25, 2022
Merged

Support ALTER MATERIALIZED VIEW ... SET PROPERTIES ... #9613

merged 3 commits into from
Jan 25, 2022

Conversation

sunningCGo
Copy link

@sunningCGo sunningCGo commented Oct 13, 2021 •
edited
Loading

This PR implements support for the ALTER MATERIALIZED VIEW ... SET PROPERTIES ... statement

@cla-bot cla-bot bot added the cla-signed label Oct 13, 2021
@sunningCGo sunningCGo requested a review from sopel39 October 13, 2021 04:30
@sunningCGo sunningCGo changed the title Implement ALTER MATERIALIZED VIEW ... SET PROPERTIES ... Support ALTER MATERIALIZED VIEW ... SET PROPERTIES ... Oct 13, 2021
@sopel39 sopel39 requested a review from lukasz-stec October 13, 2021 08:26
lukasz-stec
lukasz-stec reviewed Oct 13, 2021
View reviewed changes
Copy link
Member

@lukasz-stec lukasz-stec left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would be good to have high-level test like BaseConnectorTest.testRenameMaterializedView

core/trino-main/src/main/java/io/trino/execution/SetPropertiesTask.java Show resolved Hide resolved
core/trino-parser/src/main/antlr4/io/trino/sql/parser/SqlBase.g4 Outdated Show resolved Hide resolved
core/trino-main/src/main/java/io/trino/execution/SetPropertiesTask.java Outdated Show resolved Hide resolved
@sopel39
Copy link
Member

sopel39 commented Oct 13, 2021

Please base your PR on top of #9401

@electrum electrum requested a review from martint October 14, 2021 01:52
@sunningCGo
Copy link
Author

#9401 has been merged into master and I have rebased this PR onto master.

sopel39
sopel39 reviewed Oct 14, 2021
View reviewed changes
Copy link
Member

@sopel39 sopel39 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm % comments

core/trino-parser/src/main/java/io/trino/sql/parser/AstBuilder.java Show resolved Hide resolved
core/trino-parser/src/test/java/io/trino/sql/parser/TestSqlParser.java Outdated Show resolved Hide resolved
core/trino-main/src/main/java/io/trino/security/AccessControl.java Show resolved Hide resolved
core/trino-main/src/main/java/io/trino/execution/SetPropertiesTask.java Outdated Show resolved Hide resolved
@sunningCGo sunningCGo force-pushed the alter-mv-set-props branch 2 times, most recently from 30d5126 to b97ad90 Compare October 15, 2021 01:09
@sunningCGo sunningCGo requested a review from sopel39 October 15, 2021 01:16
sopel39
sopel39 reviewed Oct 15, 2021
View reviewed changes
Copy link
Member

@sopel39 sopel39 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm % comments

core/trino-main/src/main/java/io/trino/execution/SetPropertiesTask.java Show resolved Hide resolved
core/trino-main/src/main/java/io/trino/sql/analyzer/StatementAnalyzer.java Show resolved Hide resolved
core/trino-main/src/main/java/io/trino/execution/CreateMaterializedViewTask.java Outdated Show resolved Hide resolved
@sunningCGo sunningCGo force-pushed the alter-mv-set-props branch 2 times, most recently from 36331ed to 61b8681 Compare October 15, 2021 16:09
@sunningCGo sunningCGo requested a review from sopel39 October 15, 2021 16:20
electrum
electrum previously requested changes Oct 15, 2021
View reviewed changes
Copy link
Member

@electrum electrum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are some problems with the existing ALTER TABLE ... SET PROPERTIES that need to be resolved, so we need to hold off on this until that is done.

@sopel39
Copy link
Member

sopel39 commented Oct 15, 2021

There are some problems with the existing ALTER TABLE ... SET PROPERTIES that need to be resolved, so we need to hold off on this until that is done.

@electrum what problems do you mean?

@sunningCGo sunningCGo force-pushed the alter-mv-set-props branch 2 times, most recently from 77ff9f3 to f6b9ad6 Compare October 27, 2021 00:56
@sunningCGo sunningCGo force-pushed the alter-mv-set-props branch 3 times, most recently from 1d1a6ba to cd3beca Compare November 2, 2021 14:10
@sunningCGo sunningCGo force-pushed the alter-mv-set-props branch 2 times, most recently from 05d60cf to 6afac02 Compare November 22, 2021 05:19
@sunningCGo sunningCGo deleted the alter-mv-set-props branch December 20, 2021 02:10
@sunningCGo sunningCGo restored the alter-mv-set-props branch December 20, 2021 02:12
@sunningCGo sunningCGo reopened this Dec 20, 2021
@electrum electrum requested review from electrum and removed request for electrum January 15, 2022 01:07
sopel39
sopel39 approved these changes Jan 18, 2022
View reviewed changes
Copy link
Member

@sopel39 sopel39 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm % comments. @martint do you want to look?

core/trino-main/src/main/java/io/trino/execution/SetPropertiesTask.java Show resolved Hide resolved
core/trino-main/src/main/java/io/trino/metadata/AbstractCatalogPropertyManager.java Show resolved Hide resolved
core/trino-main/src/main/java/io/trino/metadata/AbstractPropertyManager.java Show resolved Hide resolved
core/trino-main/src/main/java/io/trino/metadata/AbstractPropertyManager.java Show resolved Hide resolved
core/trino-parser/src/main/java/io/trino/sql/tree/Property.java Show resolved Hide resolved
core/trino-parser/src/main/java/io/trino/sql/tree/Property.java Show resolved Hide resolved
@sunningCGo sunningCGo force-pushed the alter-mv-set-props branch 3 times, most recently from d9f41af to 4810808 Compare January 19, 2022 04:38
@sunningCGo sunningCGo mentioned this pull request Jan 19, 2022
Open
@sunningCGo sunningCGo force-pushed the alter-mv-set-props branch 5 times, most recently from f0db549 to 34bed1f Compare January 24, 2022 18:31
Sunning Go added 3 commits January 24, 2022 16:14
Support setting mv properties in core engine
3fbc25a 
This commit implements ALTER MATERIALIZED VIEW ... SET PROPERTIES... in
the core engine. The implementation supports the use of a newly
introduced SQL keyword "DEFAULT" on the right hand side of a property
assignment. The support of the DEFAULT keyword is implemented by
modifying certain classes related to properties in general (i.e. they
are not specific to materialized view). Consequently, several other SQL
statements also gain the support of the DEFAULT keyword "for free". They
are:

ALTER TABLE... ADD COLUMN...
ANALYZE
CREATE MATERIALIZED VIEW
CREATE SCHEMA
CREATE TABLE (both column properties and table properties)
CREATE TABLE AS

Note, however, that ALTER TABLE ... SET PROPERTIES ... is not in the
list above. Support for DEFAULT in ALTER TABLE...SET PROPERTIES...
requires extensive changes and will therefore be implemented in a
subsequent commit.
Fix logic that decides which checkCanCreateTable to use
172eaa7 
There are two versions of AccessControl.checkCanCreateTable: the old
deprecated version and the newer version which takes an additional
Map<String, Object> parameter "properties"(so that the properties can
be taken into account when making an access control decision). Which
version is to be used should depend solely on the FeatureConfig
parameter "disableSetPropertiesSecurityCheckForCreateDdl" - it should
not depend on whether the "properties" parameter is empty.
Add extension point for setting mv properties in Hive connector
842d8b2
@sopel39 sopel39 merged commit ea684e6 into trinodb:master Jan 25, 2022
@sopel39 sopel39 mentioned this pull request Jan 25, 2022
Closed
@github-actions github-actions bot added this to the 369 milestone Jan 25, 2022
@sunningCGo sunningCGo deleted the alter-mv-set-props branch January 25, 2022 00:41
This was referenced Jan 25, 2022
Merged
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lukasz-stec lukasz-stec lukasz-stec left review comments

@sopel39 sopel39 sopel39 approved these changes

@martint martint martint left review comments

@ebyhr ebyhr Awaiting requested review from ebyhr

@electrum electrum Awaiting requested review from electrum

Assignees
No one assigned
Labels
cla-signed syntax-needs-review
Milestone
369
Development

Successfully merging this pull request may close these issues.
5 participants
@sunningCGo @sopel39 @electrum @martint @lukasz-stec