-
Notifications
You must be signed in to change notification settings - Fork 2.2k
[BugFix] fixed ALTER STORAGE VOLUME [IF EXISTS]
#66691
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
[BugFix] fixed ALTER STORAGE VOLUME [IF EXISTS]
#66691
Conversation
🧪 CI InsightsHere's what we observed from your CI run for 38c975c. 🟢 All jobs passed!But CI Insights is watching 👀 |
ALTER STORAGE VOLUME [IF EXISTS]ALTER STORAGE VOLUME [IF EXISTS]
089a517 to
c2c97f2
Compare
|
@cursor review |
fe/fe-core/src/test/java/com/starrocks/server/SharedDataStorageVolumeMgrTest.java
Outdated
Show resolved
Hide resolved
ac052e4 to
38c975c
Compare
|
@cursor review |
e2f2d13 to
a5ac114
Compare
|
@cursor review |
fe/fe-core/src/test/java/com/starrocks/server/SharedDataStorageVolumeMgrTest.java
Outdated
Show resolved
Hide resolved
|
@cursor review |
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.
Pull request overview
This PR adds support for the IF EXISTS clause to the ALTER STORAGE VOLUME statement, aligning the implementation with the documented SQL syntax. The change propagates the ifExists flag through the AST and updates the storage volume manager to throw MetaNotFoundException instead of using precondition checks when a volume doesn't exist.
Key Changes:
- Added
IF EXISTSsyntax support to the SQL grammar forALTER STORAGE VOLUME - Threaded
ifExistsboolean flag throughAlterStorageVolumeStmtand related code - Changed
StorageVolumeMgr.updateStorageVolumemethods to throwMetaNotFoundExceptioninstead of failing precondition checks
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| fe/fe-grammar/src/main/antlr/com/starrocks/grammar/StarRocks.g4 | Added optional IF EXISTS clause to alterStorageVolumeStatement grammar rule |
| fe/fe-parser/src/main/java/com/starrocks/sql/ast/AlterStorageVolumeStmt.java | Added ifExists field, updated constructor signature, and added isSetIfExists() accessor method |
| fe/fe-core/src/main/java/com/starrocks/sql/parser/AstBuilder.java | Updated to parse IF EXISTS and pass it to AlterStorageVolumeStmt constructor |
| fe/fe-core/src/main/java/com/starrocks/server/StorageVolumeMgr.java | Changed all updateStorageVolume overloads to throw MetaNotFoundException; replaced precondition check with explicit exception throw |
| fe/fe-core/src/main/java/com/starrocks/qe/DDLStmtExecutor.java | Reformatted lambda for alter storage volume execution (added braces) |
| fe/fe-core/src/test/java/com/starrocks/qe/RedirectStatusTest.java | Updated test to use new AlterStorageVolumeStmt constructor with ifExists parameter |
| fe/fe-core/src/test/java/com/starrocks/server/SharedDataStorageVolumeMgrTest.java | Added MetaNotFoundException to method signature and fixed variable naming consistency |
|
@cursor review |
|
Any update? |
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.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
fe/fe-parser/src/main/java/com/starrocks/sql/ast/AlterStorageVolumeStmt.java:60
- Missing toSql() method implementation. Similar storage volume statements like DropStorageVolumeStmt implement toSql() to properly represent the IF EXISTS clause in the SQL string. AlterStorageVolumeStmt should also implement toSql() to include "IF EXISTS" when the ifExists flag is true, ensuring consistency across storage volume statements and proper SQL representation for logging and debugging purposes.
@Override
public <R, C> R accept(AstVisitor<R, C> visitor, C context) {
return visitor.visitAlterStorageVolumeStatement(this, context);
}
kevincai
left a comment
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.
new test case shall be added to cover this new added IF EXISTS check. either in unit test or in sql-test
fe/fe-core/src/test/java/com/starrocks/server/SharedDataStorageVolumeMgrTest.java
Show resolved
Hide resolved
a4c47a7 to
797c309
Compare
|
@mergify rebase |
Signed-off-by: Dan-J-D <[email protected]>
Signed-off-by: Dan-J-D <[email protected]>
Signed-off-by: Dan-J-D <[email protected]>
Signed-off-by: Dan-J-D <[email protected]>
Signed-off-by: Dan-J-D <[email protected]>
Signed-off-by: Dan-J-D <[email protected]>
Signed-off-by: Dan-J-D <[email protected]>
Co-authored-by: Kevin Cai <[email protected]> Signed-off-by: Dan-J-D <[email protected]>
Co-authored-by: Kevin Cai <[email protected]> Signed-off-by: Dan-J-D <[email protected]>
Signed-off-by: Dan-J-D <[email protected]>
✅ Branch has been successfully rebased |
929645e to
487193f
Compare
|
@cursor review |
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.
✅ Bugbot reviewed your changes and found no bugs!
|
[Java-Extensions Incremental Coverage Report]✅ pass : 0 / 0 (0%) |
[FE Incremental Coverage Report]❌ fail : 5 / 9 (55.56%) file detail
|
[BE Incremental Coverage Report]✅ pass : 0 / 0 (0%) |
|
@Mergifyio backport branch-4.0 |
✅ Backports have been createdDetails
|
## Why I'm doing: According to [this](https://docs.starrocks.io/docs/sql-reference/sql-statements/cluster-management/storage_volume/ALTER_STORAGE_VOLUME/), the syntax is ```sql ALTER STORAGE VOLUME [ IF EXISTS ] <storage_volume_name> { COMMENT = '<comment_string>' | SET ("key" = "value"[,...]) } ``` But doesn't support `IF EXISTS`. ## What I'm doing: Fixes syntax to add `IF EXISTS`. Signed-off-by: Dan-J-D <[email protected]> Co-authored-by: Kevin Cai <[email protected]> (cherry picked from commit bf73f06) # Conflicts: # fe/fe-core/src/main/java/com/starrocks/sql/ast/AlterStorageVolumeStmt.java # fe/fe-core/src/test/java/com/starrocks/qe/RedirectStatusTest.java



Why I'm doing:
According to this, the syntax is
But doesn't support
IF EXISTS.What I'm doing:
Fixes syntax to add
IF EXISTS.What type of PR is this:
Does this PR entail a change in behavior?
If yes, please specify the type of change:
Checklist:
Bugfix cherry-pick branch check:
Note
Implements
IF EXISTSforALTER STORAGE VOLUMEand aligns not-found behavior across layers.ALTER STORAGE VOLUME (IF EXISTS)? ...;AstBuilderpasses the flag;AlterStorageVolumeStmtstoresifExists.visitAlterStorageVolumeStatementignoresMetaNotFoundExceptionwhenIF EXISTSis set.StorageVolumeMgr.updateStorageVolume(...)now throwsMetaNotFoundExceptionif the volume is missing; signature updated and immutable-property checks preserved.MetaNotFoundException; added SQL tests forALTER STORAGE VOLUME IF EXISTSbehavior.Written by Cursor Bugbot for commit 487193f. This will update automatically on new commits. Configure here.