Skip to content

Add support for ALTER MATERIALIZED VIEW .. SET AUTHORIZATION #25910

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
Merged
merged 1 commit into from
Jun 17, 2025
Merged

Add support for ALTER MATERIALIZED VIEW .. SET AUTHORIZATION #25910

merged 1 commit into from
Jun 17, 2025

Conversation

@codyzwief
Copy link
Contributor

@codyzwief codyzwief commented Jun 3, 2025 •
edited
Loading

Description

This commit adds support for SET AUTHORIZATION on a materialized view. Previously, this was only available on views.

In this PR, I reused checkCanSetViewAuthorization for access control for materialized views. Certainly open to creating a new one to differentiate between views & MVs.

Additional context and related issues

In this PR support for generic SET AUTHORIZATION was added, but it did not include support for materialized views. Here, we add MATERIALIZED VIEW as a specific owned entity token, and support it in existing set authorization tasks.

Release notes

( ) This is not user-visible or is 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
* Add support for `ALTER MATERIALIZED VIEW ... SET AUTHORIZATION`.

@cla-bot cla-bot bot added the cla-signed label Jun 3, 2025
@codyzwief codyzwief force-pushed the codyzwief/alter-materialized-view branch 3 times, most recently from 5cc1ab4 to 8dbc8e2 Compare June 3, 2025 16:43
@codyzwief codyzwief changed the title [WIP] Add support for ALTER MATERIALIZED VIEW .. SET AUTHORIZATION Add support for ALTER MATERIALIZED VIEW .. SET AUTHORIZATION Jun 3, 2025
@codyzwief codyzwief removed the hive Hive connector label Jun 3, 2025
@codyzwief codyzwief force-pushed the codyzwief/alter-materialized-view branch from 8dbc8e2 to 0329246 Compare June 3, 2025 20:31
@codyzwief codyzwief marked this pull request as ready for review June 3, 2025 21:03
@codyzwief codyzwief force-pushed the codyzwief/alter-materialized-view branch 2 times, most recently from f2630f1 to b364e10 Compare June 10, 2025 20:44
@codyzwief
Copy link
Contributor Author

codyzwief commented Jun 11, 2025

Hi @martint and/or @kokosing -- would either of you be able to take a look at this PR when you get a free moment? Please feel free to reassign if there is someone who would be better equipped to review this change. Thanks!

kokosing
kokosing reviewed Jun 12, 2025
View reviewed changes
Copy link
Member

@kokosing kokosing left a comment

Choose a reason for hiding this comment

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

@homar please take a look

kokosing
kokosing approved these changes Jun 12, 2025
View reviewed changes
Copy link
Member

@kokosing kokosing left a comment

Choose a reason for hiding this comment

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

% comments

core/trino-parser/src/main/java/io/trino/sql/parser/AstBuilder.java Outdated
Copy link
Member

@kokosing kokosing Jun 12, 2025

Choose a reason for hiding this comment

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

Why this is needed?

Copy link
Contributor Author

@codyzwief codyzwief Jun 12, 2025

Choose a reason for hiding this comment

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

It's not necessary, but it's for keeping the owned entity kind to keep the spaces that MATERIALIZED VIEW has in it. Otherwise, the entity kind string would need to be MATERIALIZEDVIEW. I thought the former was more user friendly.

Happy to revert this change back and use MATERIALIZEDVIEW instead -- I can see the pros and cons of both approach.

...oolkit/src/test/java/io/trino/plugin/base/security/BaseFileBasedSystemAccessControlTest.java Outdated
Copy link
Member

@kokosing kokosing Jun 12, 2025

Choose a reason for hiding this comment

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

I think it would be better to extract a method and have to @Test methods one per each view kind.

Copy link
Contributor Author

@codyzwief codyzwief Jun 12, 2025

Choose a reason for hiding this comment

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

Done!

Copy link
Member

@kokosing kokosing Jun 16, 2025

Choose a reason for hiding this comment

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

Way better to review!

@homar
Copy link
Member

homar commented Jun 12, 2025

Are you sure we don't need specific methods for materialized views both in SystemAccessControl and in ConnectorMetadata ? I may be wrong about that but I believe it is safer to have materialized views distinguished from views.

@codyzwief
Copy link
Contributor Author

codyzwief commented Jun 12, 2025

Are you sure we don't need specific methods for materialized views both in SystemAccessControl and in ConnectorMetadata ? I may be wrong about that but I believe it is safer to have materialized views distinguished from views.

I mentioned that in the PR description:

In this PR, I reused checkCanSetViewAuthorization for access control for materialized views. Certainly open to creating a new one to differentiate between views & MVs.

I think you're probably right that it's safer, even if in most cases it'll be the same code. I'll make that change.

@codyzwief codyzwief force-pushed the codyzwief/alter-materialized-view branch 4 times, most recently from 23df798 to ed9cf76 Compare June 13, 2025 18:09
@github-actions github-actions bot added the hive Hive connector label Jun 13, 2025
@codyzwief codyzwief requested a review from homar June 13, 2025 20:39
@codyzwief
Add support for ALTER MATERIALIZED VIEW .. SET AUTHORIZATION
99c1447 
This commit adds support for `SET AUTHORIZATION` on a materialized view.
Previously, this was only available on views.
@codyzwief codyzwief force-pushed the codyzwief/alter-materialized-view branch from ed9cf76 to 99c1447 Compare June 16, 2025 13:38
kokosing
kokosing approved these changes Jun 16, 2025
View reviewed changes
...oolkit/src/test/java/io/trino/plugin/base/security/BaseFileBasedSystemAccessControlTest.java Outdated
Copy link
Member

@kokosing kokosing Jun 16, 2025

Choose a reason for hiding this comment

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

Way better to review!

@kokosing
Copy link
Member

kokosing commented Jun 16, 2025

I retried the failed job, let me know when it becomes green so I will merge it.

@codyzwief
Copy link
Contributor Author

codyzwief commented Jun 16, 2025

@kokosing Thank you for rerunning the failure. It succeeded, is it OK to merge?

@kokosing kokosing merged commit 8d251eb into trinodb:master Jun 17, 2025
185 of 186 checks passed
@github-actions github-actions bot added this to the 477 milestone Jun 17, 2025
This was referenced Aug 6, 2025
Merged
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@kokosing kokosing kokosing approved these changes

@martint martint Awaiting requested review from martint

@djsagain djsagain Awaiting requested review from djsagain

+1 more reviewer

@homar homar homar approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

cla-signed hive Hive connector syntax-needs-review

Milestone

477

Development

Successfully merging this pull request may close these issues.

3 participants

@codyzwief @homar @kokosing