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 renaming materialized views #9492

Merged
merged 3 commits into from
Oct 8, 2021
Merged

Support renaming materialized views #9492

merged 3 commits into from
Oct 8, 2021

Conversation

lukasz-stec
Copy link
Member

Add support for ALTER MATERIALIZED VIEW (IF EXISTS)? ... RENAME TO ... syntax.
Implement the support in the iceberg connector.

@cla-bot cla-bot bot added the cla-signed label Oct 4, 2021
sopel39
sopel39 reviewed Oct 5, 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.

skimmed so for. Automation errors seem relevant

core/trino-main/src/main/java/io/trino/execution/RenameMaterializedViewTask.java Outdated Show resolved Hide resolved
core/trino-main/src/main/java/io/trino/execution/RenameMaterializedViewTask.java Show resolved Hide resolved
core/trino-main/src/main/java/io/trino/execution/RenameTableTask.java Show resolved Hide resolved
core/trino-main/src/main/java/io/trino/execution/RenameViewTask.java Show resolved Hide resolved
lukasz-stec
lukasz-stec commented Oct 6, 2021
View reviewed changes
Copy link
Member Author

@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.

some comments added. changes incoming

core/trino-main/src/main/java/io/trino/execution/RenameMaterializedViewTask.java Show resolved Hide resolved
core/trino-main/src/main/java/io/trino/execution/RenameTableTask.java Show resolved Hide resolved
core/trino-main/src/main/java/io/trino/execution/RenameMaterializedViewTask.java Outdated Show resolved Hide resolved
@lukasz-stec
Copy link
Member Author

changes from comments applied + iceberg test fixed

@lukasz-stec lukasz-stec requested a review from sopel39 October 6, 2021 09:23
sopel39
sopel39 reviewed Oct 6, 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 % small comments

core/trino-main/src/main/java/io/trino/execution/RenameMaterializedViewTask.java Show resolved Hide resolved
core/trino-main/src/main/java/io/trino/security/InjectedConnectorAccessControl.java Outdated Show resolved Hide resolved
plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/TestIcebergMaterializedViews.java Outdated Show resolved Hide resolved
testing/trino-testing/src/main/java/io/trino/testing/BaseConnectorTest.java Show resolved Hide resolved
testing/trino-testing/src/main/java/io/trino/testing/BaseConnectorTest.java Show resolved Hide resolved
plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/TestIcebergMaterializedViews.java
@@ -215,8 +216,8 @@ public void testRenameDenyPermission()
assertAccessDenied(
"ALTER MATERIALIZED VIEW materialized_view_rename_deny RENAME TO materialized_view_rename_deny_new",
"Cannot rename materialized view .*.materialized_view_rename_deny.*",
privilege("materialized_view_rename_deny", REFRESH_MATERIALIZED_VIEW));
assertUpdate("DROP MATERIALIZED VIEW materialized_view_drop_deny");
privilege("materialized_view_rename_deny", RENAME_MATERIALIZED_VIEW));
Copy link
Member

Choose a reason for hiding this comment

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

that should be previous commit

sopel39
sopel39 reviewed Oct 6, 2021
View reviewed changes
core/trino-main/src/main/java/io/trino/execution/RenameMaterializedViewTask.java
if (metadata.getMaterializedView(session, target).isPresent()) {
throw semanticException(TABLE_ALREADY_EXISTS, statement, "Target materialized view '%s' already exists", target);
}
if (metadata.getView(session, target).isPresent()) {
Copy link
Member

Choose a reason for hiding this comment

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

I think this can still fail when there is a Hive view that needs to be translated. I think we can leave with it

lukasz-stec
lukasz-stec commented Oct 6, 2021
View reviewed changes
Copy link
Member Author

@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.

comments addressed

testing/trino-testing/src/main/java/io/trino/testing/BaseConnectorTest.java Show resolved Hide resolved
@lukasz-stec lukasz-stec requested a review from sopel39 October 6, 2021 14:45
@trinodb trinodb deleted a comment from lukasz-stec Oct 7, 2021
@lukasz-stec
Copy link
Member Author

There was a dependence BaseConnectorTest between testRenameMaterializedView and testMaterializedView that could cause flakyness.
I removed it by using a separate scheme for testRenameMaterializedView + limiting the system.metadata.materialized_views filtered by catalog assert to containsAll

sopel39
sopel39 approved these changes Oct 7, 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

testing/trino-testing/src/main/java/io/trino/testing/BaseConnectorTest.java Outdated
getTestingMaterializedViewsResultRows(view, otherView));
assertThat(query(listMaterializedViewsSql("catalog_name = '" + view.getCatalogName() + "'")))
.skippingTypesCheck()
.containsAll(getTestingMaterializedViewsResultRows(view, otherView));

assertQuery(
Copy link
Member

Choose a reason for hiding this comment

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

This should be changed too (and couple of places below too). Also, please extract test changes to separate commit

Copy link
Member Author

Choose a reason for hiding this comment

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

done

testing/trino-testing/src/main/java/io/trino/testing/BaseConnectorTest.java Show resolved Hide resolved
sopel39
sopel39 approved these changes Oct 8, 2021
View reviewed changes
testing/trino-testing/src/main/java/io/trino/testing/BaseConnectorTest.java

assertQuery(
Copy link
Member

Choose a reason for hiding this comment

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

nit: this was resulting single row anyway

testing/trino-testing/src/main/java/io/trino/testing/BaseConnectorTest.java

assertQuery(
assertThat(query(
Copy link
Member

Choose a reason for hiding this comment

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

nit: this was resulting single row anyway

sopel39
sopel39 reviewed Oct 8, 2021
View reviewed changes
testing/trino-testing/src/main/java/io/trino/testing/BaseConnectorTest.java
@@ -637,30 +637,34 @@ public void testMaterializedView()
assertUpdate("DROP MATERIALIZED VIEW " + viewWithComment);

// test filtering materialized views in system metadata table
assertQuery(
Copy link
Member

Choose a reason for hiding this comment

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

nit: there is one assertQuery above that is not changed

@sopel39 sopel39 merged commit 329c402 into trinodb:master Oct 8, 2021
@sopel39
Copy link
Member

sopel39 commented Oct 8, 2021

this requires doc update cc @mosabua

@sopel39 sopel39 mentioned this pull request Oct 8, 2021
Closed
12 tasks
@github-actions github-actions bot added this to the 364 milestone Oct 8, 2021
@lukasz-stec
Copy link
Member Author

docs PR #9562

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sopel39 sopel39 sopel39 approved these changes

Assignees
No one assigned
Labels
cla-signed
Milestone
364
Development

Successfully merging this pull request may close these issues.
2 participants
@lukasz-stec @sopel39