feat: impl drop view #4231
feat: impl drop view #4231
Conversation
WalkthroughThe changes introduce functionality for dropping views in a database system. Key modifications include the addition of the Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
killme2008
force-pushed
the
feature/drop-view
branch
from
31689c5
to
2d5844b
Compare
killme2008
requested review from
WenyXu,
waynexia and
sunng87
and removed request for
a team and
MichaelScofield
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 17
Outside diff range and nitpick comments (10)
src/common/meta/src/ddl/drop_view.rs (6)
38-43: Add visibility modifier forDropViewProcedurefields.The fields
contextanddataare marked aspub(crate). Consider making them private and using getters if needed.- pub(crate) context: DdlContext, - pub(crate) data: DropViewData, + context: DdlContext, + data: DropViewData,
45-57: Add documentation forDropViewProceduremethods.The methods in the
DropViewProcedurestruct lack documentation. Adding doc comments will improve code readability and maintainability./// Creates a new `DropViewProcedure`.
65-68: Remove unnecessary test-specific code from production code.The
statemethod is only used in tests. Consider using#[cfg(test)]to conditionally compile this method only for tests.#[cfg(test)] impl DropViewProcedure { pub(crate) fn state(&self) -> DropViewState { self.data.state } }
139-150: Add logging foron_delete_metadata.Adding logging will help in debugging and monitoring the deletion process.
info!("Deleting metadata for view {view_id}");
210-226: Add documentation forDropViewDatamethods.The methods in the
DropViewDatastruct lack documentation. Adding doc comments will improve code readability and maintainability./// Returns a reference to the table.
228-237: Add documentation forDropViewStatevariants.The variants in the
DropViewStateenum lack documentation. Adding doc comments will improve code readability and maintainability./// Prepares to drop the view.src/sql/src/statements/drop.rs (2)
140-147: Add documentation forDropViewstruct and fields.The
DropViewstruct and its fields lack documentation. Adding doc comments will improve code readability and maintainability./// Represents a `DROP VIEW` statement.
Line range hint
293-340: Add more test cases forDropView.Consider adding more test cases to cover additional edge cases, such as invalid view names and missing keywords.
#[test] pub fn test_drop_view_invalid_name() { let sql = "DROP VIEW "; let result = ParserContext::create_with_dialect(sql, &GreptimeDbDialect {}, ParseOptions::default()); assert!(result.is_err()); } #[test] pub fn test_drop_view_missing_keyword() { let sql = "DROP foo"; let result = ParserContext::create_with_dialect(sql, &GreptimeDbDialect {}, ParseOptions::default()); assert!(result.is_err()); }src/common/meta/src/ddl/tests/create_view.rs (1)
Line range hint
47-278: Add more test cases forCreateViewProcedure.Consider adding more test cases to cover additional edge cases, such as invalid view names and missing keywords.
#[tokio::test] async fn test_create_view_invalid_name() { let node_manager = Arc::new(MockDatanodeManager::new(())); let ddl_context = new_ddl_context(node_manager); let cluster_id = 1; let task = test_create_view_task(""); let mut procedure = CreateViewProcedure::new(cluster_id, task, ddl_context); let err = procedure.on_prepare().await.unwrap_err(); assert_matches!(err, Error::InvalidViewName { .. }); } #[tokio::test] async fn test_create_view_missing_keyword() { let node_manager = Arc::new(MockDatanodeManager::new(())); let ddl_context = new_ddl_context(node_manager); let cluster_id = 1; let task = test_create_view_task("foo"); let mut procedure = CreateViewProcedure::new(cluster_id, task, ddl_context); let err = procedure.on_prepare().await.unwrap_err(); assert_matches!(err, Error::MissingKeyword { .. }); }src/sql/src/parsers/drop_parser.rs (1)
293-340: Add more test cases forparse_drop_view.Consider adding more test cases to cover additional edge cases, such as invalid view names and missing keywords.
#[test] pub fn test_parse_drop_view_invalid_name() { let sql = "DROP VIEW "; let result = ParserContext::create_with_dialect(sql, &GreptimeDbDialect {}, ParseOptions::default()); assert!(result.is_err()); } #[test] pub fn test_parse_drop_view_missing_keyword() { let sql = "DROP foo"; let result = ParserContext::create_with_dialect(sql, &GreptimeDbDialect {}, ParseOptions::default()); assert!(result.is_err()); }
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (24)
- src/common/meta/src/ddl.rs (1 hunks)
- src/common/meta/src/ddl/drop_view.rs (1 hunks)
- src/common/meta/src/ddl/tests.rs (1 hunks)
- src/common/meta/src/ddl/tests/create_view.rs (1 hunks)
- src/common/meta/src/ddl/tests/drop_view.rs (1 hunks)
- src/common/meta/src/ddl_manager.rs (7 hunks)
- src/common/meta/src/error.rs (2 hunks)
- src/common/meta/src/metrics.rs (1 hunks)
- src/common/meta/src/rpc/ddl.rs (11 hunks)
- src/frontend/src/instance.rs (1 hunks)
- src/operator/src/error.rs (2 hunks)
- src/operator/src/statement.rs (1 hunks)
- src/operator/src/statement/ddl.rs (2 hunks)
- src/sql/src/parsers/drop_parser.rs (5 hunks)
- src/sql/src/statements/drop.rs (1 hunks)
- src/sql/src/statements/statement.rs (3 hunks)
- tests/cases/standalone/common/view/columns.result (2 hunks)
- tests/cases/standalone/common/view/columns.sql (2 hunks)
- tests/cases/standalone/common/view/create.result (5 hunks)
- tests/cases/standalone/common/view/create.sql (2 hunks)
- tests/cases/standalone/common/view/show_create.result (2 hunks)
- tests/cases/standalone/common/view/show_create.sql (2 hunks)
- tests/cases/standalone/common/view/view.result (2 hunks)
- tests/cases/standalone/common/view/view.sql (2 hunks)
Files skipped from review due to trivial changes (4)
- src/common/meta/src/ddl.rs
- src/common/meta/src/ddl/tests.rs
- tests/cases/standalone/common/view/columns.result
- tests/cases/standalone/common/view/columns.sql
Additional comments not posted (42)
tests/cases/standalone/common/view/show_create.sql (1)
41-41: Clarify the purpose of this statement.The
SHOW CREATE VIEW v1;statement is used to display the SQL statement that creates the viewv1. However, this view was dropped in line 37. Ensure that this is the intended behavior or add comments to clarify the purpose.tests/cases/standalone/common/view/view.sql (1)
35-35: Clarify the purpose of this statement.The
SELECT * FROM v1;statement is used to retrieve data from the viewv1. However, this view was dropped in line 33. Ensure that this is the intended behavior or add comments to clarify the purpose.tests/cases/standalone/common/view/create.sql (1)
49-49: Clarify the purpose of this statement.The
SELECT * FROM test_view LIMIT 10;statement is used to retrieve data from the viewtest_view. However, this view was dropped in line 45. Ensure that this is the intended behavior or add comments to clarify the purpose.tests/cases/standalone/common/view/view.result (1)
59-61: Clarify the purpose of this statement.The
SELECT * FROM v1;statement is used to retrieve data from the viewv1. However, this view was dropped in line 55. Ensure that this is the intended behavior or add comments to clarify the purpose.tests/cases/standalone/common/view/show_create.result (3)
121-122: Ensure correctness of theDROP VIEWstatement.The
DROP VIEW v1;statement should drop the viewv1. Verify that the viewv1is correctly dropped and that no errors occur.
125-126: Ensure correctness of theDROP TABLEstatement.The
DROP TABLE t1;statement should drop the tablet1. Verify that the tablet1is correctly dropped and that no errors occur.
129-132: Verify error handling forSHOW CREATE VIEWafter drop.The
SHOW CREATE VIEW v1;statement should result in an error since the viewv1was dropped. Ensure that the error message is correct and that it indicates the view was not found.src/common/meta/src/metrics.rs (1)
58-63: Add a new histogram metric forDROP VIEWprocedure.The new histogram metric
METRIC_META_PROCEDURE_DROP_VIEWshould follow the same pattern as the existing metrics. Ensure it is correctly defined and registered.src/sql/src/statements/statement.rs (2)
28-28: AddDropViewvariant toStatementenum.The
DropViewvariant should be correctly added to theStatementenum following the existing pattern.
64-67: AddDropViewvariant toDisplayimplementation.The
DropViewvariant should be correctly handled in theDisplayimplementation for theStatementenum.src/common/meta/src/ddl/tests/drop_view.rs (5)
30-38: Add helper functionnew_drop_view_task.The
new_drop_view_taskhelper function should correctly create aDropViewTaskwith the provided parameters.
40-66: Add testtest_on_prepare_view_not_exists_err.The
test_on_prepare_view_not_exists_errtest function should correctly handle the case where the view does not exist and return the appropriate error.
68-92: Add testtest_on_prepare_not_view_err.The
test_on_prepare_not_view_errtest function should correctly handle the case where the object is not a view and return the appropriate error.
94-127: Add testtest_on_prepare_success.The
test_on_prepare_successtest function should correctly handle the successful preparation of theDropViewProcedure.
129-180: Add testtest_drop_view_success.The
test_drop_view_successtest function should correctly handle the successful execution of theDropViewProcedureand verify that the view is dropped.src/common/meta/src/ddl/tests/create_view.rs (1)
Line range hint
35-41: LGTM!The
test_table_namesfunction is straightforward and does not require changes.tests/cases/standalone/common/view/create.result (10)
17-17: Ensure the error message for existing table is accurate.The error message indicates that the table already exists. Verify that this is the expected behavior for the
CREATE VIEWstatement.
22-22: Verify error message forCREATE VIEW IF NOT EXISTS.The error message should correctly indicate that the table already exists even when using
IF NOT EXISTS.
27-27: Check error message forCREATE OR REPLACE VIEW.The error message indicates that the table already exists. Confirm that this is the expected behavior for the
CREATE OR REPLACE VIEWstatement.
36-36: Validate error message for existing view.The error message should accurately reflect that the view already exists.
51-64: ReviewSHOW FULL TABLESresults.The
SHOW FULL TABLEScommand should list all tables and their types, including views. Ensure that the results are accurate.
70-105: ValidateINFORMATION_SCHEMA.TABLESoutput.The output should correctly display the details of tables, including views, in the
INFORMATION_SCHEMA.TABLEStable.
110-114: CheckINFORMATION_SCHEMA.TABLESfor views.Ensure that the output accurately lists views in the
INFORMATION_SCHEMA.TABLEStable.
148-148: Verify successfulDROP VIEWoperation.The
DROP VIEWcommand should execute successfully and the affected rows should be 0.
152-152: Confirm successfulDROP TABLEoperation.The
DROP TABLEcommand should execute successfully and the affected rows should be 0.
158-158: Validate error message for querying dropped view.The error message should correctly indicate that the view does not exist after being dropped.
src/operator/src/statement.rs (1)
195-209: ValidateDropViewstatement handling.The
DropViewstatement handling includes breaking down the view name into catalog, schema, and view name components and invoking thedrop_viewmethod. Ensure that:
- The
table_idents_to_full_namefunction is correctly used.- Proper error handling is in place.
- The
drop_viewmethod is correctly called with the appropriate parameters.src/common/meta/src/error.rs (1)
708-708: Verify status code forInvalidViewInfoerror.Ensure that the
InvalidViewInfoerror is correctly categorized underStatusCode::InvalidArguments.src/frontend/src/instance.rs (1)
494-496: ValidateDropViewstatement validation.Ensure that the
validate_paramfunction is correctly used to validate the view name in theDropViewstatement.src/operator/src/error.rs (2)
158-161: New error variantInvalidViewadded.The addition of the
InvalidViewerror variant is correct and consistent with other error variants.
786-786: Integration ofInvalidViewerror variant intostatus_codemethod.The integration of the
InvalidViewerror variant into thestatus_codemethod is appropriate and consistent with other similar error variants.src/common/meta/src/ddl_manager.rs (5)
36-36: ImportDropViewProcedureadded.The import of
DropViewProcedureis correct and necessary for the changes.
135-136: RegisterDropViewProcedureloader.The registration of the
DropViewProcedureloader in theregister_loadersmethod is correct and consistent with other similar procedures.
325-337: New methodsubmit_drop_view_taskadded.The
submit_drop_view_taskmethod is implemented correctly and follows the pattern of similar methods.
618-638: New functionhandle_drop_view_taskadded.The
handle_drop_view_taskfunction is implemented correctly and follows the pattern of similar functions.
791-792: Integration ofDropViewtask handling intosubmit_ddl_taskmethod.The integration of the
DropViewtask handling into thesubmit_ddl_taskmethod is correct and consistent with other similar task handling.src/common/meta/src/rpc/ddl.rs (2)
77-80: LGTM! New function to createDdlTaskfor dropping a view.The implementation follows the pattern used by other
new_*functions in this file.
443-450: LGTM! Function to returnTableReferenceof the view.The implementation is straightforward and correct.
src/operator/src/statement/ddl.rs (4)
648-697: Ensure proper error handling for view not found scenario.The method handles the scenario where the view is not found and
drop_if_existsis false. Ensure that this scenario is tested.
675-682: Validate view type before dropping.The method ensures that the entity being dropped is indeed a view. This is a good practice to prevent accidental deletion of other table types.
694-696: LGTM! Ensure the procedure executor handles the task correctly.The drop view task is submitted to the procedure executor. Ensure that the procedure executor correctly handles this task.
699-714: Ensure proper error handling in the drop view procedure.The method submits the drop view task to the procedure executor. Ensure that potential errors are correctly handled and logged.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4231 +/- ##
==========================================
- Coverage 85.19% 84.89% -0.30%
==========================================
Files 1060 1061 +1
Lines 189165 189447 +282
==========================================
- Hits 161151 160834 -317
- Misses 28014 28613 +599 |
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- src/common/meta/src/ddl/drop_view.rs (1 hunks)
- src/sql/src/statements/drop.rs (1 hunks)
Files skipped from review as they are similar to previous changes (2)
- src/common/meta/src/ddl/drop_view.rs
- src/sql/src/statements/drop.rs
Co-authored-by: Jeremyhi <[email protected]>
Co-authored-by: Jeremyhi <[email protected]>
Co-authored-by: Jeremyhi <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- src/common/meta/src/ddl/drop_view.rs (1 hunks)
- src/common/meta/src/rpc/ddl.rs (11 hunks)
Files skipped from review as they are similar to previous changes (2)
- src/common/meta/src/ddl/drop_view.rs
- src/common/meta/src/rpc/ddl.rs
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- src/common/meta/src/ddl/drop_flow.rs (1 hunks)
Files skipped from review due to trivial changes (1)
- src/common/meta/src/ddl/drop_flow.rs
I hereby agree to the terms of the GreptimeDB CLA.
Refer to a related PR or issue link (optional)
#3729
What's changed and what's your intention?
Supports
DROP VIEWstatement:Main changes:
DropViewProcedureto drop view.DROP VIEW [IF EXISTS] viewstatement.drop view.Checklist
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
DropFlowProcedurestruct to ensure consistency.Tests