Skip to content

Add support for ALTER VIEW RENAME TO statement#23749

Merged
tdcmeehan merged 3 commits intoprestodb:masterfrom
dmariamgeorge:alter_view_rename
Dec 17, 2024
Merged

Add support for ALTER VIEW RENAME TO statement#23749
tdcmeehan merged 3 commits intoprestodb:masterfrom
dmariamgeorge:alter_view_rename

Conversation

@dmariamgeorge
Copy link
Contributor

@dmariamgeorge dmariamgeorge commented Sep 30, 2024

Description

Add support for SQL statement ALTER VIEW <view_name> RENAME TO <new_view_name>.

Motivation and Context

Addresses issue: #23748

Impact

Support SQL statement ALTER VIEW <view_name> RENAME TO <new_view_name> in Iceberg connector.

Test Plan

  • Modified testTableRules() in TestFileBasedAccessControl.java to include check for rename view.
  • Added test testRenameView() in TestSqlParser.java.
  • In memory connector, added tests testRenameView() and testRenameViewIfNotExists() in TestMemorySmoke.java.
  • Modified test testViews() in TestMemoryMetadata.java to include check for rename view.
  • In Iceberg connector, added tests testRenameView() and testRenameViewIfNotExists() in TestIcebergMetadataListing.java.

Contributor checklist

  • Please make sure your submission complies with our development, formatting, commit message, and attribution guidelines.
  • PR description addresses the issue accurately and concisely. If the change is non-trivial, a GitHub Issue is referenced.
  • Documented new properties (with its default value), SQL syntax, functions, or other functionality.
  • If release notes are required, they follow the release notes guidelines.
  • Adequate tests were added if applicable.
  • CI passed.

Release Notes

Please follow release notes guidelines and fill in the release notes below.

== RELEASE NOTES ==
General Changes
* Add support for ``ALTER VIEW RENAME TO`` operation, including the necessary infrastructure for connector implementations. :pr:`23749`

Iceberg Connector Changes
* Add support for ``ALTER VIEW RENAME TO``. :pr:`23749`

Memory Connector Changes
* Add support for ``ALTER VIEW RENAME TO``. :pr:`23749`

@github-actions
Copy link

github-actions bot commented Sep 30, 2024

Codenotify: Notifying subscribers in CODENOTIFY files for diff e416ff1...5a61649.

Notify File(s)
@aditi-pandit presto-parser/src/main/antlr4/com/facebook/presto/sql/parser/SqlBase.g4
@elharo presto-parser/src/main/antlr4/com/facebook/presto/sql/parser/SqlBase.g4
@kaikalur presto-parser/src/main/antlr4/com/facebook/presto/sql/parser/SqlBase.g4
@rschlussel presto-parser/src/main/antlr4/com/facebook/presto/sql/parser/SqlBase.g4

@steveburnett
Copy link
Contributor

Consider adding documentation for this to https://prestodb.io/docs/current/connector/iceberg.html#sql-support.

steveburnett
steveburnett previously approved these changes Sep 30, 2024
Copy link
Contributor

@steveburnett steveburnett left a comment

Choose a reason for hiding this comment

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

LGTM! (docs)

Pull branch, local doc build, looks good. Thanks!

@agrawalreetika
Copy link
Member

agrawalreetika commented Sep 30, 2024

Thanks for the PR.

I have a few suggestions -

  • I think we can add one commit about adding SQL statement support and different commits for support for ALTER VIEW RENAME support for each connector support along with their connector document updates about the support
  • Please add tests as well in respective connector support commits
  • We can add ALTER VIEW documentation along with 1st commit of SQL Statement support here https://prestodb.io/docs/current/sql.html
  • Also it would be good to add SQL statement support for ALTER VIEW [ IF EXISTS ] as well and its respective tests

@ZacBlanco
Copy link
Contributor

@dmariamgeorge Can we split this into two commits like @agrawalreetika suggested? Once we have both commits I will also review

@steveburnett
Copy link
Contributor

Minor nits of formatting suggested for the release note entry.

== RELEASE NOTES ==
General Changes
* Add support for ``ALTER VIEW RENAME TO``. :pr:`23749`

Iceberg Connector Changes
* Add support for ``ALTER VIEW RENAME TO``. :pr:`23749`

@dmariamgeorge
Copy link
Contributor Author

@dmariamgeorge Can we split this into two commits like @agrawalreetika suggested? Once we have both commits I will also review

Sure @ZacBlanco. Working on the same.

@dmariamgeorge dmariamgeorge force-pushed the alter_view_rename branch 4 times, most recently from 4f1282b to 4cbfaee Compare November 11, 2024 13:46
@dmariamgeorge
Copy link
Contributor Author

@dmariamgeorge Can we split this into two commits like @agrawalreetika suggested? Once we have both commits I will also review

Sure @ZacBlanco. Working on the same.

@ZacBlanco - I have split the commits as suggested by @agrawalreetika. Can you please take a look?

agrawalreetika
agrawalreetika previously approved these changes Dec 2, 2024
Copy link
Contributor

@steveburnett steveburnett left a comment

Choose a reason for hiding this comment

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

Thanks for the updated doc! Two minor rephrasing suggestions.

@dmariamgeorge
Copy link
Contributor Author

Hi @ZacBlanco @agrawalreetika @steveburnett @hantangwangd

Could you please take another look at this PR? I’ve incorporated all the review comments. Thanks!

Copy link
Contributor

@steveburnett steveburnett left a comment

Choose a reason for hiding this comment

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

LGTM! (docs)

Pull updated build, new local doc build, reviewed pages in local doc build. Everything looks good. Thanks!

@ZacBlanco
Copy link
Contributor

ZacBlanco commented Dec 5, 2024

@dmariamgeorge instead of the screenshot in the PR description under "Test Plan", can you please replace it with a short summary of the unit and integration tests that have been added

@ZacBlanco
Copy link
Contributor

Also, I suggest in the release note section to be more specific in the "General Changes" section that we're adding the ability for connectors to implement ALTER VIEW RENAME TO. IMO It isn't clear from the two release notes that it contains the infrastructure and only an implementation for the Iceberg connector.

@dmariamgeorge
Copy link
Contributor Author

@ZacBlanco - I have revised the PR description. Could you please review it?

@dmariamgeorge
Copy link
Contributor Author

Hi @tdcmeehan

I have received approvals from @ZacBlanco, @agrawalreetika, @hantangwangd, and @steveburnett on this PR. Could you please help merge this PR? Thanks!

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants