-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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.
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
Add IF EXISTS to DROP ROLE #21985
base: master
Are you sure you want to change the base?
Add IF EXISTS to DROP ROLE #21985
Conversation
... in preparation for TestDropRoleTask.
core/trino-parser/src/main/java/io/trino/sql/tree/DropRole.java
Outdated
Show resolved
Hide resolved
*/ | ||
@TestInstance(PER_CLASS) | ||
@Execution(CONCURRENT) | ||
public class BaseRoleTaskTest |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't use inheritance for this. It makes it harder to maintain the tests, as the subclasses are intimately coupled to the details of the parent. Over time, as some of the tests evolve, the parent becomes a kitchen sink of functionality.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What would be a good way to factor out the boilerplate and make new tests easy to add? I thought about putting the DropRole tests and SetRole tests into the same class, but I like the regularity of Test<class name>Task
naming.
I could, of course, copy-paste the boilerplate into TestDropRoleTask
, but that felt unsatisfying.
5196faf
to
d665624
Compare
This pull request has gone a while without any activity. Tagging the Trino developer relations team: @bitsondatadev @colebow @mosabua |
@@ -3,7 +3,7 @@ | |||
## Synopsis | |||
|
|||
```text | |||
DROP ROLE role_name | |||
DROP ROLE [ IF EXISTS ] role_name |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add docs for this in the description as well
Description
Add optional
IF EXISTS
toDROP ROLE
, which suppresses the role-not-found error that you'd otherwise get.Additional context and related issues
Although this option isn't in the ANSI SQL, it's a UX improvement that Trino has traditionally admitted.
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: