refactor: replace DfLogicalPlan with datafusion_expr::LogicalPlan #4384
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThe changes primarily involve updating the import paths for 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 Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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/query/src/plan.rs (5 hunks)
Additional comments not posted (7)
src/query/src/plan.rs (7)
48-52: LGTM!The
schemafunction has been correctly refactored to takeplan: &DfLogicalPlanas a parameter. The error handling is appropriate.
63-63: LGTM!The
display_indentfunction has been correctly refactored to takeplan: &DfLogicalPlanas a parameter.
Line range hint
69-75:
LGTM!The
get_param_typesfunction has been correctly refactored to takeplan: &DfLogicalPlanas a parameter. The error handling and conversion logic are appropriate.
81-90: LGTM!The
replace_params_with_valuesfunction has been correctly refactored to takeplan: &DfLogicalPlanandvalues: &[ScalarValue]as parameters. The error handling and logic to replace parameters are appropriate.
94-98: LGTM!The
unwrap_df_planfunction has been correctly refactored to takeplan: &LogicalPlanas a parameter. The error handling using a panic statement is appropriate.
103-107: LGTM!The
df_planfunction has been correctly refactored to takeplan: &LogicalPlanas a parameter. The error handling using a panic statement is appropriate.
Line range hint
206-212:
LGTM!The
extract_and_rewrite_full_table_namesfunction's visibility has been correctly changed frompubto private. The logic to extract and rewrite table names is appropriate.
|
Thanks for submitting the PR. To remove the wrapper type, we also need to remove the definition of our #[derive(Clone, Debug)]
pub enum LogicalPlan {
DfPlan(DfLogicalPlan),
}You can refer to #3995 which removes the wrapper type for |
sunheyi6
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range, codebase verification and nitpick comments (1)
tests-integration/src/instance.rs (1)
236-236: Update required: Instances ofLogicalPlan::DfPlanfound.The following files still contain instances of the old usage pattern
LogicalPlan::DfPlanand need to be updated to the new usage ofLogicalPlan:
src/query/src/datafusion.rssrc/query/src/plan.rssrc/query/src/planner.rssrc/query/src/tests/query_engine_test.rssrc/servers/src/prom_store.rssrc/pipeline/src/manager/table.rsPlease ensure these instances are updated for consistency.
Analysis chain
Variable assignment change approved.
The variable assignment for
LogicalPlanhas been updated to useLogicalPlandirectly, aligning with the refactoring objectives.However, ensure that all variable assignments to
LogicalPlanare correctly updated across the codebase.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all variable assignments to `LogicalPlan` are correctly updated. # Test: Search for the variable assignments. Expect: Only occurrences of the new usage. rg --type rust -A 5 $'let LogicalPlan'Length of output: 4349
Script:
#!/bin/bash # Description: Verify that all instances of the old usage pattern `LogicalPlan::DfPlan` have been updated. # Test: Search for the old usage pattern. Expect: No occurrences. rg --type rust 'LogicalPlan::DfPlan'Length of output: 924
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (23)
- src/cmd/src/cli/repl.rs (2 hunks)
- src/datanode/src/tests.rs (1 hunks)
- src/flow/src/transform.rs (4 hunks)
- src/frontend/src/instance.rs (1 hunks)
- src/operator/src/statement.rs (1 hunks)
- src/operator/src/statement/copy_table_to.rs (2 hunks)
- src/operator/src/statement/tql.rs (1 hunks)
- src/pipeline/src/manager/table.rs (2 hunks)
- src/query/src/range_select/plan_rewrite.rs (2 hunks)
- src/script/src/table.rs (3 hunks)
- src/servers/src/http.rs (1 hunks)
- src/servers/src/interceptor.rs (1 hunks)
- src/servers/src/lib.rs (1 hunks)
- src/servers/src/mysql/handler.rs (1 hunks)
- src/servers/src/postgres/types.rs (1 hunks)
- src/servers/src/prom_store.rs (1 hunks)
- src/servers/src/query_handler/sql.rs (1 hunks)
- src/servers/tests/http/influxdb_test.rs (1 hunks)
- src/servers/tests/http/opentsdb_test.rs (1 hunks)
- src/servers/tests/http/prom_store_test.rs (1 hunks)
- src/servers/tests/mod.rs (1 hunks)
- tests-integration/src/grpc.rs (2 hunks)
- tests-integration/src/instance.rs (2 hunks)
Additional comments not posted (30)
src/servers/src/lib.rs (1)
21-21: LGTM! Verify the correctness of the new import.The import statement for
LogicalPlanhas been updated to usedatafusion_expr::LogicalPlan. Ensure that this change is consistent with the rest of the codebase.Verification successful
The new import statement for
LogicalPlanis consistent across the codebase.
- All instances of
LogicalPlanare imported fromdatafusion_expr.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the correctness of the new import for LogicalPlan. # Test: Search for the usage of LogicalPlan. Expect: Only occurances of datafusion_expr::LogicalPlan. rg --type rust -A 5 $'LogicalPlan'Length of output: 188344
src/servers/src/query_handler/sql.rs (1)
21-21: LGTM! Verify the correctness of the new import.The import statement for
LogicalPlanhas been updated to usedatafusion_expr::LogicalPlan. Ensure that this change is consistent with the rest of the codebase.Verification successful
The new import statement is correct and consistent with the rest of the codebase.
The import statement
use datafusion_expr::LogicalPlan;is used consistently across the codebase, and no conflicting imports or usage ofLogicalPlanfrom different modules were found.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the correctness of the new import for LogicalPlan. # Test: Search for the usage of LogicalPlan. Expect: Only occurances of datafusion_expr::LogicalPlan. rg --type rust -A 5 $'LogicalPlan'Length of output: 188344
src/operator/src/statement/tql.rs (1)
23-23: LGTM! Verify the correctness of the new import.The import statement for
LogicalPlanhas been updated to usedatafusion_expr::LogicalPlan. Ensure that this change is consistent with the rest of the codebase.Verification successful
The new import statement for
LogicalPlanis correct and consistent throughout the codebase.
- All instances of
LogicalPlanare usingdatafusion_expr::LogicalPlan.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the correctness of the new import for LogicalPlan. # Test: Search for the usage of LogicalPlan. Expect: Only occurances of datafusion_expr::LogicalPlan. rg --type rust -A 5 $'LogicalPlan'Length of output: 188344
src/operator/src/statement/copy_table_to.rs (2)
32-32: LGTM! Verify the correctness of the new import.The import statement for
LogicalPlanhas been updated to usedatafusion_expr::LogicalPlan. Ensure that this change is consistent with the rest of the codebase.Verification successful
The new import statement for
LogicalPlanis consistent across the codebase.The import statement
use datafusion_expr::LogicalPlan;is correctly used in all relevant instances.
- Verified in multiple files with no conflicting imports.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the correctness of the new import for LogicalPlan. # Test: Search for the usage of LogicalPlan. Expect: Only occurances of datafusion_expr::LogicalPlan. rg --type rust -A 5 $'LogicalPlan'Length of output: 188344
132-132: Verify the correctness of theexecutemethod call.The
executemethod call has been updated to useLogicalPlandirectly. Ensure that this change is correct and consistent with the rest of the codebase.Verification successful
Verify the correctness of the
executemethod call.The
executemethod with the signatureexecute(&self, plan: LogicalPlan, query_ctx: QueryContextRef) -> Result<Output>is correctly defined in thesrc/query/src/query_engine.rsandsrc/query/src/datafusion.rsfiles. The method call in the review comment aligns with these definitions, ensuring that the parametersLogicalPlanandquery_ctxare correctly used.
- Files with relevant
executemethod definitions:
src/query/src/query_engine.rssrc/query/src/datafusion.rsScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the correctness of the `execute` method call. # Test: Search for the usage of the execute method. Expect: Only occurances of the new method call. rg --type rust -A 5 $'execute'Length of output: 299676
Script:
#!/bin/bash # Locate the definition of the `execute` method to understand its parameters. ast-grep --lang rust --pattern 'fn execute($_, $_) -> $_ { $$$ }'Length of output: 13127
Script:
#!/bin/bash # Refine the search to locate the `execute` method with `LogicalPlan` and `QueryContextRef` parameters. ast-grep --lang rust --pattern 'fn execute(&self, plan: LogicalPlan, query_ctx: QueryContextRef) -> Result<Output> { $$$ }'Length of output: 125
Script:
#!/bin/bash # Broaden the search to locate the `execute` method with LogicalPlan and query_ctx in the context of query engines. rg --type rust -A 5 'fn execute' | grep -C 5 'LogicalPlan'Length of output: 2725
src/datanode/src/tests.rs (1)
28-28: Update import statement to usedatafusion_expr::LogicalPlan.The import statement has been updated to use
LogicalPlanfromdatafusion_exprinstead ofquery::plan. Ensure that this change is consistent with the rest of the codebase and that all references toLogicalPlanare correctly updated.src/servers/tests/http/prom_store_test.rs (1)
28-28: Update import statement to usedatafusion_expr::LogicalPlan.The import statement has been updated to use
LogicalPlanfromdatafusion_exprinstead ofquery::plan. Ensure that this change is consistent with the rest of the codebase and that all references toLogicalPlanare correctly updated.src/servers/tests/http/opentsdb_test.rs (1)
23-23: Update import statement to usedatafusion_expr::LogicalPlan.The import statement has been updated to use
LogicalPlanfromdatafusion_exprinstead ofquery::plan. Ensure that this change is consistent with the rest of the codebase and that all references toLogicalPlanare correctly updated.src/servers/tests/mod.rs (1)
25-25: Update import statement to usedatafusion_expr::LogicalPlan.The import statement has been updated to use
LogicalPlanfromdatafusion_exprinstead ofquery::plan. Ensure that this change is consistent with the rest of the codebase and that all references toLogicalPlanare correctly updated.src/cmd/src/cli/repl.rs (2)
39-39: LGTM!The import statement change aligns with the PR objective.
183-183: LGTM! But verify the function usage in the codebase.The code change is syntactically correct and aligns with the new import.
However, ensure that all function calls to
LogicalPlanmatch the new usage.src/servers/src/interceptor.rs (1)
25-25: LGTM!The import statement change aligns with the PR objective.
src/flow/src/transform.rs (3)
26-26: LGTM!The import statement change aligns with the PR objective.
137-137: LGTM! But verify the function usage in the codebase.The code change is syntactically correct and aligns with the new import.
However, ensure that all function calls to
LogicalPlanmatch the new usage.
204-204: LGTM! But verify the function usage in the codebase.The code change is syntactically correct and aligns with the new import.
However, ensure that all function calls to
LogicalPlanmatch the new usage.Verification successful
Verification Successful: LogicalPlan Usage
The
LogicalPlanusage is consistent with the new import fromdatafusion_expracross the codebase.
tests-integration/src/instance.rstests-integration/src/grpc.rssrc/query/src/dist_plan.rssrc/query/src/optimizer.rssrc/query/src/datafusion.rssrc/query/src/sql.rssrc/query/src/tests/query_engine_test.rssrc/query/src/range_select/planner.rssrc/query/src/range_select/plan_rewrite.rssrc/query/src/query_engine.rssrc/query/src/query_engine/state.rssrc/query/src/query_engine/context.rssrc/query/src/planner.rssrc/query/src/query_engine/default_serializer.rssrc/script/src/table.rssrc/query/src/promql/planner.rssrc/query/src/plan.rssrc/query/src/optimizer/type_conversion.rssrc/query/src/optimizer/order_hint.rssrc/query/src/logical_optimizer.rssrc/query/src/optimizer/count_wildcard.rssrc/query/src/range_select/plan.rssrc/query/src/dist_plan/merge_scan.rssrc/query/src/dist_plan/commutativity.rssrc/query/src/dist_plan/analyzer.rssrc/query/src/dist_plan/planner.rssrc/query/src/optimizer/string_normalization.rssrc/query/src/physical_planner.rssrc/servers/src/lib.rssrc/servers/tests/mod.rssrc/servers/src/prom_store.rssrc/servers/src/query_handler/sql.rssrc/servers/src/postgres/types.rssrc/servers/src/interceptor.rssrc/servers/tests/http/prom_store_test.rssrc/servers/tests/http/opentsdb_test.rssrc/servers/src/mysql/handler.rssrc/servers/src/http.rssrc/servers/tests/http/influxdb_test.rssrc/operator/src/error.rssrc/operator/src/statement.rssrc/operator/src/statement/copy_table_to.rssrc/operator/src/statement/ddl.rssrc/operator/src/statement/tql.rssrc/promql/src/extension_plan/instant_manipulate.rssrc/promql/src/extension_plan/series_divide.rssrc/promql/src/extension_plan/planner.rssrc/promql/src/extension_plan/empty_metric.rssrc/promql/src/extension_plan/scalar_calculate.rssrc/promql/src/extension_plan/normalize.rssrc/promql/src/extension_plan/range_manipulate.rssrc/operator/src/statement/copy_table_to.rssrc/operator/src/statement/ddl.rssrc/operator/src/statement/tql.rssrc/promql/src/extension_plan/union_distinct_on.rssrc/promql/src/extension_plan/histogram_fold.rssrc/pipeline/src/manager/table.rssrc/pipeline/src/manager/error.rssrc/flow/src/transform.rssrc/frontend/src/instance.rssrc/frontend/src/error.rssrc/frontend/src/instance/prom_store.rssrc/datanode/src/tests.rssrc/datanode/src/region_server.rssrc/datanode/src/error.rssrc/frontend/src/instance/grpc.rssrc/common/substrait/src/df_substrait.rssrc/catalog/src/table_source.rssrc/cmd/src/error.rssrc/common/query/src/test_util.rssrc/common/query/src/logical_plan.rssrc/cmd/src/cli/repl.rssrc/common/query/src/request.rssrc/common/meta/src/key/view_info.rssrc/client/src/database.rssrc/api/src/helper.rsScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all function calls to `LogicalPlan` match the new usage. # Test: Search for the function usage. Expect: Only occurrences of the new usage. rg --type rust -A 5 $'LogicalPlan'Length of output: 188344
src/servers/tests/http/influxdb_test.rs (1)
25-25: LGTM!The import statement change aligns with the PR objective.
src/script/src/table.rs (2)
36-36: Import change approved.The import statement for
LogicalPlanhas been updated to usedatafusion_expr::LogicalPlan, aligning with the refactoring objectives.
227-227: Method call change approved.The method calls to
.execute()have been updated to useLogicalPlandirectly, aligning with the refactoring objectives.However, ensure that all function calls to
executewithLogicalPlanare correctly updated across the codebase.Also applies to: 282-282
Verification successful
Method call change approved.
The method calls to
.execute()have been updated to useLogicalPlandirectly, aligning with the refactoring objectives. The updates have been applied consistently across the codebase.
src/script/src/table.rssrc/pipeline/src/manager/table.rssrc/operator/src/statement/copy_table_to.rsScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all function calls to `execute` with `LogicalPlan` are correctly updated. # Test: Search for the function usage. Expect: Only occurrences of the new usage. rg --type rust -A 5 $'.execute(LogicalPlan'Length of output: 124
Script:
#!/bin/bash # Description: Verify all function calls to `execute` with `LogicalPlan` are correctly updated. # Test: Search for the function usage. Expect: Only occurrences of the new usage. rg --type rust -A 5 '\.execute\(LogicalPlan'Length of output: 2023
tests-integration/src/instance.rs (1)
34-34: Import change approved.The import statement for
LogicalPlanhas been updated to usedatafusion_expr::LogicalPlan, aligning with the refactoring objectives.src/pipeline/src/manager/table.rs (2)
37-37: Import change approved.The import statement for
LogicalPlanhas been updated to usedatafusion_expr::LogicalPlan, aligning with the refactoring objectives.
440-440: Method call change approved.The method call to
.execute()has been updated to useLogicalPlandirectly, aligning with the refactoring objectives.However, ensure that all function calls to
executewithLogicalPlanare correctly updated across the codebase.Verification successful
Method call change approved.
The method call to
.execute()has been updated to useLogicalPlandirectly, aligning with the refactoring objectives. All instances of this method call have been correctly updated across the codebase.
- Verified files:
src/script/src/table.rssrc/pipeline/src/manager/table.rssrc/operator/src/statement/copy_table_to.rsScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all function calls to `execute` with `LogicalPlan` are correctly updated. # Test: Search for the function usage. Expect: Only occurrences of the new usage. rg --type rust -A 5 $'.execute(LogicalPlan'Length of output: 124
Script:
#!/bin/bash # Description: Verify all function calls to `execute` with `LogicalPlan` are correctly updated. # Test: Search for the function usage. Expect: Only occurrences of the new usage. rg --type rust -A 5 '.execute(LogicalPlan'Length of output: 124
Script:
#!/bin/bash # Description: Verify all function calls to `execute` with `LogicalPlan` are correctly updated. # Test: Search for the function usage. Expect: Only occurrences of the new usage. rg --type rust -A 5 '\.execute\(LogicalPlan'Length of output: 2023
src/operator/src/statement.rs (1)
42-42: Import change approved.The import statement for
LogicalPlanhas been updated to usedatafusion_expr::LogicalPlan, aligning with the refactoring objectives.src/frontend/src/instance.rs (1)
53-53: LGTM! The import statement change aligns with the PR objective.The import statement for
LogicalPlanhas been updated to use Datafusion'sLogicalPlandirectly, which aligns with the refactoring goal.src/servers/src/mysql/handler.rs (1)
35-35: LGTM! The import statement change aligns with the PR objective.The import statement for
LogicalPlanhas been updated to use Datafusion'sLogicalPlandirectly, which aligns with the refactoring goal.src/servers/src/prom_store.rs (1)
34-34: LGTM! The import statement change aligns with the PR objective.The import statement for
LogicalPlanhas been updated to use Datafusion'sLogicalPlandirectly, which aligns with the refactoring goal.tests-integration/src/grpc.rs (2)
36-36: Verify the correctness of the new import.Ensure that the
LogicalPlanfromdatafusion_expris the correct import and that it is consistent with the rest of the codebase.Verification successful
Verified the correctness of the new import.
The
LogicalPlanfromdatafusion_expris consistently used across the codebase, indicating that the new import is correct and aligned with the rest of the code.
tests-integration/src/instance.rstests-integration/src/grpc.rssrc/cmd/src/cli/repl.rssrc/servers/tests/mod.rssrc/script/src/table.rssrc/servers/tests/http/influxdb_test.rssrc/servers/tests/http/prom_store_test.rssrc/servers/tests/http/opentsdb_test.rssrc/servers/src/query_handler/sql.rssrc/servers/src/postgres/types.rssrc/servers/src/lib.rssrc/servers/src/interceptor.rssrc/servers/src/http.rssrc/servers/src/prom_store.rssrc/servers/src/mysql/handler.rssrc/query/src/plan.rssrc/query/src/optimizer.rssrc/query/src/query_engine/state.rssrc/query/src/range_select/plan_rewrite.rssrc/pipeline/src/manager/table.rssrc/operator/src/statement.rssrc/operator/src/statement/tql.rssrc/operator/src/statement/copy_table_to.rssrc/frontend/src/instance.rssrc/flow/src/transform.rssrc/datanode/src/tests.rssrc/common/substrait/src/df_substrait.rssrc/common/query/src/request.rsScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the correctness of the new import for `LogicalPlan`. # Test: Search for the import statement. Expect: Only occurances of the new import. rg --type rust -A 5 $'use datafusion_expr::LogicalPlan'Length of output: 13047
542-542: Verify the correctness of the new way of accessingLogicalPlan.Ensure that directly assigning
LogicalPlaninstead of pattern matchingLogicalPlan::DfPlan(plan)is correct and does not introduce any issues.src/servers/src/postgres/types.rs (1)
32-32: Import change approved.The import statement change aligns with the PR objective of refactoring the
LogicalPlancomponent to use Datafusion'sLogicalPlandirectly.src/servers/src/http.rs (1)
960-960: Import change approved.The import statement change aligns with the PR objective of refactoring the
LogicalPlancomponent to use Datafusion'sLogicalPlandirectly.src/query/src/range_select/plan_rewrite.rs (1)
621-621: Verify the correctness of the function with the new LogicalPlan.The change from
GreptimeLogicalPlan::DfPlan(plan)tolet plan: LogicalPlansimplifies the pattern matching. Ensure that the function works correctly with the newLogicalPlan.Verification successful
The function
query_plan_compareworks correctly with the newLogicalPlan.The function
query_plan_compareis used in multiple test cases within the filesrc/query/src/range_select/plan_rewrite.rs, covering a range of scenarios. This extensive usage demonstrates that the function operates correctly with the newLogicalPlan.
- The function
query_plan_compareis used in various test cases, ensuring its correctness.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the correctness of the function `query_plan_compare` with the new `LogicalPlan`. # Test: Search for the function usage. Expect: Correct usage of `LogicalPlan`. rg --type rust -A 5 $'query_plan_compare'Length of output: 13039
|
@sunheyi6 A conflict has to be resolved. |
OK, I'll handle it right away. |
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- src/cmd/src/cli/repl.rs (2 hunks)
- src/flow/src/transform.rs (4 hunks)
- src/operator/src/statement/copy_table_to.rs (2 hunks)
Files skipped from review as they are similar to previous changes (3)
- src/cmd/src/cli/repl.rs
- src/flow/src/transform.rs
- src/operator/src/statement/copy_table_to.rs
|
Please fix the compiler errors |
ok，i will fix |
|
https://github.com/GreptimeTeam/greptimedb/blob/main/CONTRIBUTING.md#before-pr |
Signed-off-by: Ruihang Xia <[email protected]>
Signed-off-by: Ruihang Xia <[email protected]>
I hereby agree to the terms of the GreptimeDB CLA.
Refer to a related PR or issue link (optional)
as title, part of #3884
What's changed and what's your intention?
As the title said.
Checklist
Summary by CodeRabbit
datafusion_expr::LogicalPlanfor improved code maintainability.