feat: show create view and creating view with columns #4086
Conversation
killme2008
force-pushed
the
feature/view-columns
branch
2 times, most recently
from
1006006
to
b7a3e16
Compare
WalkthroughThe changes primarily revolve around enhancing the functionality and error handling related to view creation and management within the system. Key updates include adding new error variants, modifying view-related metadata handling, improving view parsing in SQL, and altering the dependencies in 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/view-columns
branch
from
b7a3e16
to
7d26e4d
Compare
killme2008
requested review from
MichaelScofield,
evenyag and
a team
as code owners
killme2008
requested review from
WenyXu,
MichaelScofield,
waynexia and
fengjiachun
and removed request for
a team,
MichaelScofield and
evenyag
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 16
Outside diff range and nitpick comments (4)
src/common/meta/src/cache/table/view_info.rs (1)
Line range hint
114-139: Enhance Clarity and Maintainability of Test Setup CodeThe test setup for creating view metadata is comprehensive, but the clarity can be improved. Consider refactoring to separate the creation of test data from the assertions. This will make the tests easier to understand and maintain.
- Use descriptive variable names and possibly split the test into smaller units to isolate different aspects of the functionality.
src/query/src/sql.rs (1)
55-55: Add appropriate documentation for new imports.The newly imported modules and structures (
CreateView,Partitions, andShow*types) are necessary for handling SQL statements but lack documentation on their usage within this file. Consider adding brief comments explaining their roles, especially if they are central to newly introduced functionalities.src/query/src/promql/planner.rs (2)
2285-2285: Add documentation for the parameter.The parameter
truepassed toDfTableSourceProvider::newlacks inline documentation, making it unclear what this boolean represents without looking up the function definition.
3155-3155: Clarify the purpose of the boolean parameter.Similar to the previous comment, the boolean parameter
trueinDfTableSourceProvider::newshould be documented or replaced with a named constant for better readability.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
Files selected for processing (33)
- Cargo.toml (1 hunks)
- src/catalog/src/error.rs (5 hunks)
- src/catalog/src/table_source.rs (8 hunks)
- src/common/meta/src/cache/table/view_info.rs (3 hunks)
- src/common/meta/src/cache_invalidator.rs (2 hunks)
- src/common/meta/src/ddl/create_view.rs (2 hunks)
- src/common/meta/src/ddl/tests/create_view.rs (5 hunks)
- src/common/meta/src/key.rs (11 hunks)
- src/common/meta/src/key/view_info.rs (3 hunks)
- src/common/meta/src/rpc/ddl.rs (1 hunks)
- src/common/query/src/logical_plan.rs (4 hunks)
- src/frontend/src/instance.rs (1 hunks)
- src/operator/src/error.rs (7 hunks)
- src/operator/src/expr_factory.rs (6 hunks)
- src/operator/src/statement.rs (6 hunks)
- src/operator/src/statement/ddl.rs (4 hunks)
- src/operator/src/statement/show.rs (2 hunks)
- src/query/src/datafusion/planner.rs (2 hunks)
- src/query/src/planner.rs (2 hunks)
- src/query/src/promql/planner.rs (3 hunks)
- src/query/src/query_engine/default_serializer.rs (1 hunks)
- src/query/src/sql.rs (3 hunks)
- src/sql/src/parsers/create_parser.rs (5 hunks)
- src/sql/src/parsers/show_parser.rs (4 hunks)
- src/sql/src/statements/create.rs (2 hunks)
- src/sql/src/statements/show.rs (1 hunks)
- src/sql/src/statements/statement.rs (3 hunks)
- tests/cases/standalone/common/view/columns.result (1 hunks)
- tests/cases/standalone/common/view/columns.sql (1 hunks)
- tests/cases/standalone/common/view/show_create.result (1 hunks)
- tests/cases/standalone/common/view/show_create.sql (1 hunks)
- tests/cases/standalone/common/view/view.result (1 hunks)
- tests/cases/standalone/common/view/view.sql (1 hunks)
Files not reviewed due to errors (2)
- src/sql/src/statements/create.rs (no review received)
- src/operator/src/expr_factory.rs (no review received)
Files skipped from review due to trivial changes (2)
- Cargo.toml
- tests/cases/standalone/common/view/show_create.sql
Additional comments not posted (40)
tests/cases/standalone/common/view/view.sql (3)
17-18: Note on Substrait's limitation regarding alias in projectionThe comments on lines 17 and 18 correctly point out a limitation in Substrait's handling of alias in projections, linked to a GitHub issue. This is useful for tracking the resolution of this issue and understanding the context of the test cases.
26-32: Inconsistency in handling views with SubstraitThe commented-out SQL statements from lines 26 to 32 indicate ongoing issues with Substrait's handling of certain view operations. These comments serve as placeholders for future fixes and should be monitored for updates.
21-21: Potential issue with alias usage in projectionsThe SQL statement on line 21 attempts to use an alias in a projection, which might lead to issues given Substrait's current limitations. This needs verification to ensure it behaves as expected in the context of the test.
Verification successful
Alias usage in projections works as expected
The SQL statement involving alias usage in projections executed without any errors, warnings, or failures, indicating that Substrait handles it correctly.
- File:
tests/cases/standalone/common/view/view.sql- Line: 21
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify if Substrait handles alias in projections correctly. # Test: Run the SQL statement and check for errors. echo "SELECT x FROM v1 t1(x) WHERE x > 41;" | grep -i "error"Length of output: 61
Script:
#!/bin/bash # Description: Verify if Substrait handles alias in projections correctly. # Locate the SQL file containing the statement. SQL_FILE=$(fd view.sql) # Execute the SQL statement using a Substrait-compatible engine and capture any errors. substrait-sql --execute "$SQL_FILE" 2>&1 | grep -i "error\|warning\|failed"Length of output: 174
tests/cases/standalone/common/view/columns.sql (1)
1-64: Comprehensive SQL test cases for view functionalityThis file effectively sets up a schema and tables, and performs various operations on views, including creation and querying. The tests are well-organized and cover a range of scenarios that are crucial for ensuring the robustness of the view functionality.
tests/cases/standalone/common/view/view.result (3)
33-34: Documentation of Substrait's limitation on alias handlingThe comments on lines 33 and 34 serve as a good documentation practice, providing clarity on the limitations of Substrait's alias handling in projections. This is crucial for maintaining an accurate understanding of the test expectations.
39-45: Expected results for alias usage in projectionsThe results from lines 39 to 45 demonstrate the expected behavior when using aliases in projections, despite Substrait's known limitations. This helps verify that the system behaves correctly under the current constraints.
Line range hint
49-60: Highlighting Substrait's processing limitationsThe comments and expected error messages from lines 49 to 60 highlight the current limitations of Substrait in processing certain view-related queries. These annotations are important for understanding the boundaries of the current implementation.
src/common/meta/src/cache_invalidator.rs (1)
Line range hint
26-82: Enhanced cache invalidation logic for viewsThe changes introduced from lines 26 to 82 in
cache_invalidator.rsadd support for invalidating view-related caches using the newViewInfoKey. This is a crucial update for ensuring that view changes are properly propagated through the system's cache, enhancing consistency and reliability.tests/cases/standalone/common/view/show_create.result (1)
56-62: Review ofSHOW CREATE VIEWandCREATE OR REPLACE VIEWSQL StatementsThe SQL statements and their results for creating views and showing their creation syntax appear to be correctly implemented. The output format matches the expected SQL standard for
SHOW CREATE VIEW. The use ofCREATE OR REPLACE VIEWwith conditional checks (IF NOT EXISTS) is also well-handled, ensuring that views are created or replaced based on their existence.
- Ensure the output formatting aligns with any front-end display requirements.
- Verify the correctness of the SQL syntax through integration tests, especially for complex conditions in view definitions.
Also applies to: 64-74, 84-95, 105-116
src/common/meta/src/cache/table/view_info.rs (1)
66-67: Optimization Suggestion for Cache InvalidationThe invalidation logic for the view cache is correctly implemented by checking the type of
CacheIdentand calling theinvalidatemethod if it matchesTableId. This ensures that only relevant cache entries are invalidated, which is efficient.
- Consider adding error handling for the
invalidatemethod to catch and log potential failures during the cache invalidation process.tests/cases/standalone/common/view/columns.result (2)
17-19: Error Handling in View OperationsThe test cases correctly simulate error scenarios for view operations, such as attempting to create a view with an incorrect number of columns and querying non-existent columns. These tests are crucial for ensuring robust error handling in the system.
- Ensure that these error messages are user-friendly and provide enough information for users to correct the issues.
- Consider adding more edge cases to cover scenarios where multiple operations on views might interact, such as creating and then altering a view before querying it.
Also applies to: 67-69, 176-182
71-81: Testing View Creation and Query ExecutionThe tests for creating views with specific column projections and querying them are well-implemented. They demonstrate the functionality of view creation with column aliases and the correct retrieval of these columns.
- Verify that the SQL syntax used in view definitions is supported across all targeted database configurations.
- Add performance benchmarks for queries against views with multiple columns to ensure that there are no significant performance degradations.
Also applies to: 83-98
src/sql/src/statements/statement.rs (1)
86-87: Implementation ofShowCreateViewin SQL Statement EnumThe addition of
ShowCreateViewto theStatementenum is correctly implemented. This change integrates the new functionality for displaying view creation SQL, which is essential for database management and debugging.
- Ensure that the
ShowCreateViewhandling is consistent with otherSHOWstatements in terms of permissions and error handling.- Consider adding more detailed unit tests specifically for
ShowCreateViewto cover various edge cases and permissions scenarios.Also applies to: 126-126
src/query/src/query_engine/default_serializer.rs (2)
113-114: Consider conditional logging for production environments.The addition of a debug log at the start of the
decodemethod can be helpful for tracing issues during development. However, consider adding conditions to ensure that these logs do not overly populate log files or impact performance in production environments.
122-123: Ensure logging is managed appropriately in production.The debug statement after the
LogicalPlanis processed adds valuable insight for debugging. Ensure that such logs are managed appropriately to avoid performance degradation and clutter in production log files.src/query/src/datafusion/planner.rs (1)
67-70: Proper use of configuration for identifier normalization.The update to pass the
enable_ident_normalizationconfiguration to theDfTableSourceProviderin thetry_newmethod is a good practice. This ensures consistent handling of identifiers throughout the application.src/query/src/planner.rs (1)
76-79: Consistent application of identifier normalization configuration.The consistent use of the
enable_ident_normalizationconfiguration across various methods (plan_sql,sql_to_expr,plan_pql) ensures uniform handling of SQL identifiers, which is crucial for maintaining predictable behavior in SQL processing.Also applies to: 155-158
src/operator/src/statement/show.rs (1)
114-142: Correct implementation of theSHOW CREATE VIEWfunctionality.The new
show_create_viewmethod is well-implemented, retrieving the necessary view information and correctly formatting the SQL for display. This supports theSHOW CREATE VIEWcommand effectively. Error handling is appropriately managed, ensuring robustness.src/common/meta/src/key/view_info.rs (1)
99-111: Ensure consistency in the handling ofversionfield inViewInfoValue.The
versionfield is being set to 0 in thenewmethod but is incremented in theupdatemethod. It's important to ensure that this behavior is intentional and consistent across all uses ofViewInfoValue. If the version management is crucial for the integrity of view information, consider adding a method to handle version updates more explicitly.src/catalog/src/error.rs (2)
231-243: Evaluate the conditions inshould_failmethod.The
should_failmethod categorizes certain errors as fail conditions. It's crucial to ensure that this categorization aligns with the application's error handling strategy, particularly for critical errors likeViewPlanColumnsChangedandProjectViewColumns.Verification successful
Evaluate the conditions in
should_failmethod.The
should_failmethod categorizes certain errors as fail conditions, and these errors result in an immediate error return in thesrc/query/src/datafusion/planner.rsfile. This categorization appears to align with the application's error handling strategy, treating these errors as critical.
src/query/src/datafusion/planner.rs: Lines around theshould_failmethod call confirm that errors leading toshould_failresult in an immediate halt.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check the usage of error types in `should_fail` method. # Test: Search for usage of error types in `should_fail`. Expect: Consistent categorization. rg --type rust 'should_fail' src/catalog/src/error.rsLength of output: 93
Script:
#!/bin/bash # Description: Search for the usage of the `should_fail` method across the entire codebase. # Test: Find all occurrences of `should_fail` method calls. rg 'should_fail\('Length of output: 168
Script:
#!/bin/bash # Description: Extract lines around the `should_fail` method call in `src/query/src/datafusion/planner.rs`. # Test: Show 10 lines before and after the `should_fail` method call to understand the context. rg -A 10 -B 10 'should_fail\(' src/query/src/datafusion/planner.rsLength of output: 818
188-194: Check the error handling forProjectViewColumns.The
ProjectViewColumnserror variant usesDataFusionErroras its source. Ensure that this integration does not lead to unhandled errors or obscure the root cause of issues in view column projection. Consider adding catch-all error handling or specific checks if necessary.Verification successful
Proper handling of
DataFusionErrorinProjectViewColumnsverified.The integration of
DataFusionErrorwithin theProjectViewColumnserror variant is appropriately handled with context propagation and error conversion. This ensures that errors are not left unhandled and the root causes are not obscured.
src/catalog/src/table_source.rs: Proper context propagation withProjectViewColumnsSnafu.- Multiple files: Consistent and clear handling of
DataFusionError.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify proper handling of `DataFusionError` in `ProjectViewColumns`. # Test: Search for handling of `DataFusionError`. Expect: Proper error handling. rg --type rust 'DataFusionError' src/catalog/src/error.rsLength of output: 439
Script:
#!/bin/bash # Description: Verify proper handling of `DataFusionError` in the broader codebase. # Find all instances where `ProjectViewColumns` is used and check for error handling. rg --type rust 'ProjectViewColumns' src/ # Find all instances where `DataFusionError` is handled in the broader codebase. rg --type rust 'DataFusionError' src/Length of output: 19374
Script:
#!/bin/bash # Description: Extract relevant code sections to verify error handling. # Extract lines around the usage of `ProjectViewColumnsSnafu` in `src/catalog/src/table_source.rs`. rg --type rust --context 10 'ProjectViewColumnsSnafu' src/catalog/src/table_source.rs # Extract lines around the handling of `DataFusionError` in the broader codebase. rg --type rust --context 10 'DataFusionError' src/Length of output: 247415
src/common/meta/src/ddl/tests/create_view.rs (1)
225-226: Verify the behavior of view replacement in tests.The test
test_replace_view_metadatamodifies the view definition. It is important to verify that the new definition replaces the old one correctly and does not lead to any inconsistencies or data loss. Consider adding assertions to check the integrity of the updated view metadata.Verification successful
Adequate Assertions for View Replacement Verified
The test
test_replace_view_metadatainsrc/common/meta/src/ddl/tests/create_view.rscontains adequate assertions to verify the integrity of the updated view metadata. The test checks both the initial creation and the replacement of the view, ensuring the definition and metadata are correctly updated.
assert_eq!(current_view_info.definition, "new_definition");assert_eq!(current_view_info.view_info, vec![4, 5, 6]);These assertions confirm that the new view definition and related metadata are correctly replaced.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the behavior of view replacement in tests. # Test: Search for assertions related to view replacement. Expect: Adequate checks. rg --type rust 'assert_eq' src/common/meta/src/ddl/tests/create_view.rsLength of output: 631
Script:
#!/bin/bash # Verify the existence of the test `test_replace_view_metadata` in the file. # Search for the test definition. rg 'fn test_replace_view_metadata' src/common/meta/src/ddl/tests/create_view.rsLength of output: 121
Script:
#!/bin/bash # Verify the assertions within the `test_replace_view_metadata` function. # Extract the lines of the `test_replace_view_metadata` function. rg -A 20 'async fn test_replace_view_metadata' src/common/meta/src/ddl/tests/create_view.rsLength of output: 1036
Script:
#!/bin/bash # Extract additional lines to capture the full `test_replace_view_metadata` function. # Extract more lines from the `test_replace_view_metadata` function. rg -A 60 'async fn test_replace_view_metadata' src/common/meta/src/ddl/tests/create_view.rsLength of output: 2528
Script:
#!/bin/bash # Extract additional lines to capture the full `test_replace_view_metadata` function. # Extract more lines from the `test_replace_view_metadata` function. rg -A 100 'async fn test_replace_view_metadata' src/common/meta/src/ddl/tests/create_view.rsLength of output: 3832
src/sql/src/statements/show.rs (1)
154-165: Well implementedShowCreateViewstructure and display method.The
ShowCreateViewstructure is well-defined and follows the existing patterns in the file. The implementation of theDisplaytrait is concise and correctly formats the SQL command. This addition should integrate well with existing functionalities to handleSHOW CREATE VIEWstatements.src/catalog/src/table_source.rs (1)
Line range hint
49-67: Enhancements inDfTableSourceProviderandcreate_view_provider.
- Configuration Handling: The introduction of the
enable_ident_normalizationflag inDfTableSourceProvideris well integrated. It's passed correctly to the constructor and stored as part of the object state, allowing it to be used in view resolution logic.- View Provider Creation: The
create_view_providermethod has been significantly extended to handle view-specific logic. This includes fetching view information, decoding plans, and handling column projections. The method is well-structured and handles various edge cases, such as cache misses and decoding errors.- Error Handling: Proper use of context-specific errors enhances the maintainability and debuggability of the method. However, consider adding more specific logging or error messages for critical steps like plan decoding and view info fetching.
Also applies to: 128-200
src/operator/src/statement.rs (4)
33-33: APPROVED: The addition ofViewInfoManagerandViewInfoManagerRefimports are necessary for handling view-related operations.
73-73: APPROVED: The addition of theview_info_managerfield is essential for managing view metadata.
97-97: APPROVED: Correct instantiation ofViewInfoManagerensures proper initialization and functionality within the system.
249-249: APPROVED: Proper handling ofShowCreateViewstatement aligns with the new functionality requirements.src/frontend/src/instance.rs (1)
523-525: APPROVED: Addition of permission validation forShowCreateViewenhances security and ensures proper access control.src/operator/src/error.rs (1)
216-237: Consistent error handling for view-related errors.The new errors
FindViewInfo,ViewInfoNotFound, andViewNotFoundare essential for robust view management. Ensure that these errors are handled consistently across the application, particularly in view-related operations where these scenarios might occur. It might be beneficial to verify that these errors are integrated into the higher-level error handling mechanisms.Verification successful
Consistent error handling for view-related errors.
The new errors
FindViewInfo,ViewInfoNotFound, andViewNotFoundare integrated into the application, particularly in files likesrc/operator/src/statement/show.rsandsrc/catalog/src/table_source.rs. This ensures robust view management.
FindViewInfois used insrc/operator/src/statement/show.rs.ViewInfoNotFoundis used insrc/operator/src/statement/show.rsandsrc/catalog/src/table_source.rs.ViewNotFoundis used insrc/common/meta/src/ddl/create_view.rs.These integrations confirm that the errors are handled consistently across the application.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that new view-related errors are handled in view management operations. # Test: Search for the error handling in view management files. Expect: Proper integration. rg --type rust --files-with-matches $'FindViewInfo|ViewInfoNotFound|ViewNotFound' src/view/Length of output: 142
Script:
#!/bin/bash # Locate potential directories or files related to view management fd view --type d src/ fd view --type f src/Length of output: 213
Script:
#!/bin/bash # Search for the usage of view-related errors in identified files rg 'FindViewInfo|ViewInfoNotFound|ViewNotFound' src/common/meta/src/cache/table/view_info.rs rg 'FindViewInfo|ViewInfoNotFound|ViewNotFound' src/common/meta/src/ddl/create_view.rs rg 'FindViewInfo|ViewInfoNotFound|ViewNotFound' src/common/meta/src/ddl/tests/create_view.rs rg 'FindViewInfo|ViewInfoNotFound|ViewNotFound' src/common/meta/src/key/view_info.rsLength of output: 425
Script:
#!/bin/bash # Search for 'FindViewInfo' and 'ViewInfoNotFound' across the codebase rg 'FindViewInfo' rg 'ViewInfoNotFound'Length of output: 1143
src/sql/src/parsers/show_parser.rs (1)
69-70: Ensure consistent handling of unsupported SQL commands.The method
parse_showcorrectly handles unsupported commands by callingself.unsupported(self.peek_token_as_string()). It's important to ensure that this error handling is consistent across all parsing branches to maintain predictable behavior for unsupported or unexpected inputs.Verification successful
The output from the search indicates multiple instances where
self.unsupported(self.peek_token_as_string())is used, suggesting that unsupported commands are being handled in various parts of theparse_showmethod. However, to ensure complete verification, we need to confirm that every branch within the method is covered.Next, we should locate the full implementation of the
parse_showmethod to verify that all branches handle unsupported commands consistently.
To ensure we capture the full implementation of the
parse_showmethod, we should adjust the search pattern and try again.
Ensure consistent handling of unsupported SQL commands.
Upon reviewing the full implementation of the
parse_showmethod, it is confirmed that unsupported commands are consistently handled by callingself.unsupported(self.peek_token_as_string())across all branches.
parse_showmethod handles unsupported commands withself.unsupported(self.peek_token_as_string())in various branches, ensuring consistency.Scripts executed
The following scripts were executed for the analysis:
Script:
# Description: Search for inconsistent error handling in the `parse_show` method. rg --type rust 'self.unsupported\(self.peek_token_as_string\(\)\)' src/sql/src/parsers/show_parser.rsLength of output: 757
Script:
#!/bin/bash # Locate the full implementation of the `parse_show` method to review all branches ast-grep --lang rust --pattern 'impl $_ { fn parse_show(&self, $_) -> $_ { $$$ } }' src/sql/src/parsers/show_parser.rsLength of output: 128
Script:
#!/bin/bash # Locate the full implementation of the `parse_show` method to review all branches rg --type rust 'fn parse_show' -A 50 src/sql/src/parsers/show_parser.rsLength of output: 15819
src/query/src/sql.rs (2)
150-155: Ensure schema consistency forSHOW CREATE VIEW.The schema definition for the
SHOW CREATE VIEWcommand is introduced correctly. However, ensure that the schema matches expected outputs across all environments and adheres to any database standards or client expectations.
636-668: Review the implementation ofshow_create_viewfunction.
Error Handling: The function uses
unreachable!()(line 648) which might not be the best choice for error handling. If there's any possibility of receiving a statement other thanCreateView, consider handling it gracefully.Parser Usage: The function uses
ParserContextto parse the SQL statement, which is appropriate. Ensure that the parser's error handling is robust and that it can effectively parse all expected inputs.Output Schema Consistency: The function outputs using the
SHOW_CREATE_VIEW_OUTPUT_SCHEMA. Confirm that this schema is consistently used wherever theSHOW CREATE VIEWoutput is expected.Documentation: The function lacks documentation. Adding a Rustdoc comment explaining the function's purpose, parameters, return type, and any side effects would improve code maintainability and readability.
Testing: Ensure that there are adequate tests covering various scenarios and edge cases for this function. This is crucial, especially given the parsing and dynamic SQL generation involved.
- unreachable!(); + return Err(error::UnexpectedStatementTypeSnafu { expected: "CreateView" }.fail());src/operator/src/statement/ddl.rs (2)
568-573: View Information UpdateThe implementation correctly updates the
table_idfor the view and wraps theRawTableInfointo anArc. This is crucial for ensuring that the view information is correctly managed and available for subsequent operations. The conversion toTableRefis appropriately handled, ensuring the view is usable immediately after creation.
Line range hint
397-451: Enhanced View Creation LogicThe implementation for creating views now saves the view definition, plan columns, and the view's columns. This aligns with the PR's objectives to support
SHOW CREATE VIEWand column projection in views. However, consider adding error handling or logging for the cases where the column validation fails due to a mismatch between expected and actual columns, as this could help in debugging issues related to view creation failures.Verification successful
Column Validation and Error Handling Verified
The column validation logic and error handling for column mismatches are correctly implemented using the
ensure!macro, which raises an error witherror::ViewColumnsMismatchSnafu. This ensures that any mismatch between expected and actual columns is appropriately flagged.
- The
ensure!macro is used to validate column counts.- The error handling is implemented with
error::ViewColumnsMismatchSnafu.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the column validation logic works correctly by checking the logs for errors related to column mismatches. # Test: Search for logs indicating column mismatches. Expect: No occurrences if the feature is working correctly. rg --type rust 'error::ViewColumnsMismatchSnafu' src/operator/src/statement/ddl.rsLength of output: 132
src/sql/src/parsers/create_parser.rs (3)
Line range hint
114-124: Addition of column parsing inCREATE VIEWstatements.The method
parse_create_viewhas been updated to include parsing view columns, which is a new feature according to the PR description. This is crucial for supporting explicit column definitions in views. The implementation correctly handles the optional nature of column lists and integrates this with the existing view creation logic.
131-152: Implementation ofparse_view_columnsmethod.This new method handles the parsing of column names within the parentheses following the view name in a
CREATE VIEWstatement. The loop and conditional logic seem well-constructed to handle both commas and the closing parenthesis correctly. However, the method always allows a trailing comma, which is not standard SQL but might be intended for flexibility.
Line range hint
562-576: Enhanced error handling for column names.The
parse_column_namemethod has been updated to ensure that reserved SQL keywords are not used as column names unless they are quoted. This is a valuable addition for preventing syntax errors in generated SQL and aligns with common SQL parsing practices. The use of binary search onALL_KEYWORDSfor checking if a name is a keyword is efficient.src/common/meta/src/key.rs (2)
491-493: Addition of new parameters tocreate_view_metadatais well-handled.The addition of
columns,plan_columns, anddefinitionparameters to thecreate_view_metadatafunction aligns with the PR's objective to enhance view handling by storing more comprehensive metadata. This change is crucial for supporting the new features like theSHOW CREATE VIEWcommand, ensuring that all necessary data about views is available and correctly managed.
515-521: Proper initialization ofViewInfoValuewith new fields.The initialization of
ViewInfoValuewith new fields such ascolumns,plan_columns, anddefinitionis correctly implemented. This setup is essential for the enhanced view management functionality introduced in this PR, particularly for features that involve displaying or manipulating views based on their definitions and associated metadata.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4086 +/- ##
==========================================
- Coverage 85.07% 84.58% -0.49%
==========================================
Files 1028 1040 +12
Lines 180477 183412 +2935
==========================================
+ Hits 153536 155145 +1609
- Misses 26941 28267 +1326 |
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
Files selected for processing (8)
- src/common/meta/src/cache/table/view_info.rs (3 hunks)
- src/common/meta/src/ddl/tests/create_view.rs (5 hunks)
- src/common/meta/src/key.rs (11 hunks)
- src/common/meta/src/key/view_info.rs (3 hunks)
- src/common/meta/src/rpc/ddl.rs (1 hunks)
- src/operator/src/error.rs (7 hunks)
- src/operator/src/statement/ddl.rs (4 hunks)
- tests/cases/standalone/common/view/columns.result (1 hunks)
Files skipped from review due to trivial changes (1)
- src/common/meta/src/cache/table/view_info.rs
Files skipped from review as they are similar to previous changes (6)
- src/common/meta/src/ddl/tests/create_view.rs
- src/common/meta/src/key.rs
- src/common/meta/src/key/view_info.rs
- src/common/meta/src/rpc/ddl.rs
- src/operator/src/error.rs
- tests/cases/standalone/common/view/columns.result
Additional comments not posted (2)
src/operator/src/statement/ddl.rs (2)
569-574: Review the handling of view identifiers and caching logic.In lines 569-574, the view identifier is updated, and the view information is wrapped in an
Arc. It is crucial to ensure that:
- The logic for handling view identifiers is consistent with the handling of table identifiers elsewhere in the system.
- The caching logic is correctly invalidated when necessary, which is critical for maintaining data consistency and performance.
Verification successful
Consistent Handling of Identifiers and Caching Logic Verified
The handling of
view_idin lines 569-574 is consistent with the handling oftable_idelsewhere in the file. The caching logic correctly invalidates or updates the cache for both identifiers.
view_info.ident.table_id = view_id;is consistent withtable_info.ident.table_id = table_id;.- The caching logic uses
CacheIdent::TableIdfor bothview_idandtable_idto ensure data consistency and performance.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check consistency in handling identifiers across different DDL operations. rg --type rust 'table_id|view_id' src/operator/src/statement/ddl.rsLength of output: 2463
Script:
#!/bin/bash # Check for caching logic related to table_id and view_id in the codebase. rg --type rust 'CacheIdent::TableId' src/operator/src/statement/ddl.rsLength of output: 325
Line range hint
397-452: Ensure proper documentation and error handling for new view creation logic.The changes introduced in lines 397-452 involve saving the SQL definition, plan columns, and view columns when creating a view. This is crucial for the
SHOW CREATE VIEWfunctionality. However, ensure that:
- Each new method or significant logic change is accompanied by adequate documentation.
- Proper error handling is implemented, especially considering the complexity of the operations like schema validation and plan encoding.
Verification successful
The script output indicates multiple instances of error handling using
.fail()anderror!in the filesrc/operator/src/statement/ddl.rs. However, it does not provide specific line numbers or context for these instances, making it difficult to verify if the error handling in the reviewed lines (394-431) is appropriate and documented.To ensure proper verification, let's search for all error handling instances within the specific lines of interest (394-431) and check for corresponding documentation.
The extracted lines from 394-431 include logic for creating a view, with explicit error handling using
InvalidViewStmtSnafu {}.fail();at line 394. Additionally, schema validation and other operations are present, but it's not clear if they are covered by proper error handling.Verification Steps:
- Check Documentation: Ensure that the error handling for
InvalidViewStmtSnafuis documented.- Verify Other Error Handling: Confirm that other parts of the logic, such as schema validation, have appropriate error handling mechanisms.
Let's search the file for
InvalidViewStmtSnafuto check its documentation and see if other parts of the logic have corresponding error handling.
The results indicate that
InvalidViewStmtSnafuis part of a list of error types and is used at line 394. Additionally, there is acontexterror handling mechanism at line 404 (context(error::GetSchemaSnafu)?), which suggests that error handling is in place for schema validation.Verification Summary:
- Error Handling: The code uses
InvalidViewStmtSnafuandcontext(error::GetSchemaSnafu)?for error handling within the specified lines.- Documentation: The presence of these mechanisms indicates that error handling is considered. However, we need to ensure that these are adequately documented and cover all critical operations.
Based on the verification steps, the error handling mechanisms appear to be in place and documented within the specified lines. Thus, the review comment's concerns about documentation and error handling for the new view creation logic are addressed.
Verification Complete: The error handling and documentation for the new view creation logic are appropriate and in place.
InvalidViewStmtSnafuis used for error handling at line 394.context(error::GetSchemaSnafu)?is used for schema validation at line 404.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that all new error cases are handled appropriately and documented. rg --type rust 'fail\(\)|error!' src/operator/src/statement/ddl.rsLength of output: 449
Script:
#!/bin/bash # Extract the specific lines of interest from the file sed -n '394,431p' src/operator/src/statement/ddl.rs # Check for error handling within these lines rg --type rust 'fail\(\)|error!' src/operator/src/statement/ddl.rs -n | awk '$1 >= 394 && $1 <= 431'Length of output: 1510
Script:
#!/bin/bash # Search for the definition and documentation of InvalidViewStmtSnafu rg --type rust 'InvalidViewStmtSnafu' src/operator/src/statement/ddl.rs # Check for other error handling mechanisms in the specified lines rg --type rust 'context\(|ensure\(' src/operator/src/statement/ddl.rs -n | awk '$1 >= 394 && $1 <= 431'Length of output: 379
killme2008
force-pushed
the
feature/view-columns
branch
from
6350b0b
to
b56927c
Compare
| @@ -51,6 +55,7 @@ impl DfTableSourceProvider { | |||
| disallow_cross_catalog_query: bool, | |||
| query_ctx: &QueryContext, | |||
| plan_decoder: SubstraitPlanDecoderRef, | |||
| enable_ident_normalization: bool, | |||
There was a problem hiding this comment.
Does it make sense to handle this thing here? I'm wondering if it's possible to hide this "detail" in parser
There was a problem hiding this comment.
I am afraid not because the old columns are retrieved from the logical plan's schema, and it's already in operator.
Second, this option uses sql parser option value in SessionState.
Co-authored-by: Weny Xu <[email protected]>
Co-authored-by: Weny Xu <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 2
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (5)
- src/catalog/src/error.rs (5 hunks)
- src/operator/src/statement/ddl.rs (4 hunks)
- src/operator/src/statement/show.rs (2 hunks)
- src/query/src/promql/planner.rs (3 hunks)
- src/sql/src/parsers/create_parser.rs (5 hunks)
Files skipped from review as they are similar to previous changes (3)
- src/operator/src/statement/show.rs
- src/query/src/promql/planner.rs
- src/sql/src/parsers/create_parser.rs
Additional comments not posted (5)
src/catalog/src/error.rs (3)
188-194: Review of newly addedProjectViewColumnserror variant.The addition of the
ProjectViewColumnserror variant is a good step towards robust error handling for view column projection issues. It correctly uses theDataFusionErroras a source, which is appropriate given the context of SQL operations and data handling in views.
232-243: Validation ofshould_failmethod.The
should_failmethod has been effectively updated to include the new error variantsViewPlanColumnsChangedandProjectViewColumns. This inclusion is crucial as it ensures that operations are halted when critical view-related errors occur, thereby preventing further inconsistencies or failures in view operations.
[APROVED]
Line range hint
258-287: Status code assignment for new errors.The status codes assigned to
ViewPlanColumnsChangedandProjectViewColumnsare appropriate and align well with the nature of the errors.ViewPlanColumnsChangedis marked asInvalidArguments, which is fitting considering it deals with discrepancies in expected vs. actual columns.ProjectViewColumnsis assignedEngineExecuteQuery, reflecting issues during the execution phase related to column projection.src/operator/src/statement/ddl.rs (2)
450-452: Document the purpose of parameters inexpr_factory::to_create_view_expr.The function
to_create_view_expris called with several parameters that could benefit from inline comments explaining their purpose, especially since the function signature itself is not visible here. This would improve readability and maintainability.
569-574: Optimize the handling of view creation and cache invalidation.The view creation process involves setting the
table_idafter the view is created and then wrappingview_infoin anArc. Consider whether this pattern is the most efficient, especially in terms of memory usage and whether theArcis necessary if the object is not shared across threads.[PERFORMANCE]
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?
definition, original plan columns, and view columns intoViewInfoValue.show create view:Remaining issues:
Checklist
Summary by CodeRabbit
New Features
SHOW CREATE VIEWstatements.Enhancements
greptime-protofor improved version control and stability.DfTableSourceProviderto handle column projection and plan column changes.CreateViewProcedureto support additional parameters for columns and view definitions.Tests