refactor: update SqlPlan for more cleaner variants#7966
refactor: update SqlPlan for more cleaner variants#7966sunng87 wants to merge 4 commits intoGreptimeTeam:mainfrom
Conversation
sunng87
requested review from
a team,
discord9,
evenyag and
waynexia
as code owners
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the query execution and planning logic to rely more on LogicalPlan and the original query string rather than the parsed Statement AST. Key changes include redefining SqlPlan as an enum to handle different query types (Empty, Shortcut, Plan, Statement), updating the SqlQueryHandler trait to accept a query string instead of an optional statement, and implementing is_readonly_plan to determine read-only status directly from the logical plan. Additionally, the DescribeResult struct was simplified by removing the redundant schema field, and support for enterprise-specific 'show' statements was added to the Postgres handler. I have no feedback to provide as there were no review comments.
| &self, | ||
| stmt: Option<Statement>, | ||
| plan: LogicalPlan, | ||
| query: String, |
There was a problem hiding this comment.
Seems this query can be derived from plan inside this function and only when needed? So that you don't have to do let query = logical_plan.display_indent().to_string(); outside every time, which is costly.
There was a problem hiding this comment.
In the generated plan we lost the original query string, the string from logical_plan.display_indent().to_string() is actually the explain view of plan, instead of the original one.
The only case we need to use logical_plan.display_indent().to_string() is for the grpc query interface when no query string is provided. Actually I think that interface is not in use.
| /// Hardcoded SQL shortcuts | ||
| Shortcut(String), | ||
| /// Datafusion parsed execution plan with the original query string | ||
| Plan(LogicalPlan, String), |
There was a problem hiding this comment.
The original query string can be calculated from LogicalPlan, is it needed to store it here as well?
2 participants
I hereby agree to the terms of the GreptimeDB CLA.
Refer to a related PR or issue link (optional)
What's changed and what's your intention?
Our
SqlPlanhas been unclear with its members:Option<LogicalPlan>Option<Statement>Option<Schema>in this patch, I refactored it into several fixed variants:
In this case, we won't store 3 forms (string, parsed statement, plan) of every query.
PR Checklist
Please convert it to a draft if some of the following conditions are not met.