-
Notifications
You must be signed in to change notification settings - Fork 317
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
refactor: query handler #951
Conversation
Signed-off-by: Ruihang Xia <[email protected]>
Signed-off-by: Ruihang Xia <[email protected]>
and change its parameter from Statement to QueryStatement Add parse_multiple method to QueryLanguageParser Signed-off-by: Ruihang Xia <[email protected]>
new trait method query and default implementation Signed-off-by: Ruihang Xia <[email protected]>
add query and query_multiple, with default implementations Signed-off-by: Ruihang Xia <[email protected]>
Signed-off-by: Ruihang Xia <[email protected]>
Signed-off-by: Ruihang Xia <[email protected]>
Codecov Report
@@ Coverage Diff @@
## develop #951 +/- ##
===========================================
- Coverage 85.74% 85.65% -0.10%
===========================================
Files 444 445 +1
Lines 59828 59953 +125
===========================================
+ Hits 51301 51350 +49
- Misses 8527 8603 +76
Flags with carried forward coverage won't be shown. Click here to find out more.
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
Signed-off-by: Ruihang Xia <[email protected]>
Why remove the associate type |
Glad you asked @MichaelScofield. A concrete trait like But I do think return |
@waynexia |
I agree, so I'm thinking of moving
I think the problem is not that there are many What makes you think this PR adds too many |
"query" crate is good to me
As to |
Unless it's less abstract than
Absolutely, and all those |
Don't see that many
So snafu is not replaced, at least not now, and not the near future. If we have to bear with the burden of not able to use |
I've removed ~20 If the greptimedb/src/frontend/src/instance.rs Lines 69 to 82 in 803b7f0
And actually, I also plan to remove the parameter from |
Looks like we are in a dilemma. @killme2008 @sunng87 @fengjiachun @v0y4g3r @evenyag what do you think? |
It's fine for me to define a concrete error type for handlers instead of using an associated error type. Using
In fact, this is mostly related to using an enum as an error, not the snafu crate itself, or enum HandlerError {
External {
source: BoxedError,
// Or source: Box<dyn Error + Send>,
}
} But there are some ways to simplify our codes (suppose we use How to avoid
|
on Error type of SqlQueryHandlerFrom my experience of using
From this perspective, I think it's reasonable to have associated error types because these different implementations are doing totally different tasks. |
That's the root of why I decide to refactor this trait. When I try to add a new method in #924, I found it verbose to implement this trait. You should care about how to collect many results into one return value, lots of methods are unnecessary for mock structs, what But, as you said, the implementation should only care about how to call the query engine. So I start to refactor it to only need to provide one
This doesn't matter, different implementation can still have their own error context. Nothing is changed except you need to add one more |
On design of QueryHandler. As we discussed, I start to realize this QueryHandler is a dispatcher. It sits between protocol servers and query engine. I think an ideal design is refactoring What protocol servers need:
What new 1.given a statement, check the statement type, and dispatch to different handlers (datafusion query engine, or other table/db/data handlers)
What
|
@waynexia please go ahead and leave uncertain parts for future refactoring |
After offline discussion with @MichaelScofield and @fengjiachun, we've reached an agreement that (1) push forward this PR as-is, and (2) find a way to alleviate the flooded |
I hereby agree to the terms of the GreptimeDB CLA
What's changed and what's your intention?
This PR refactors the
SqlQueryHandler
trait, and serval changes around it. In detail, itSqlQueryHandler
toQueryHandler
. Because promql is also handled by it.QueryLanguage
enum, to represent both SQL and PromQLdo_xxx
inSqlQueryHandler
toxxx
, to align the naming rule with other places (do_xxx
is often left for implementation's inner method).statement_query
now acceptsQueryStatement
rather thanStatement
, which is only for SQL.SqlQueryInterceptor
now acceptsQueryLanguage
/QueryStatement
, for the same reason. cc @sunng87parse
&parse_multiple
,query
/query_multiple
. To reduce the redundant multiple statements handle logic, and only limit this behavior in the implementation ofQueryHandler
. cc @sunng87Error
inQueryHandler
. Now it only returnsservers::error
, in which sub-crate theQueryHandler
is defined. This place might not be appropriate, we may need to think about movingQueryHandler
to another place. cc @MichaelScofieldquery
andquery_multiple
. Now the only two required methods forQueryHandler
arestatement_query
andis_schema_valid
. BTW, I doubt ifQueryHandler
is a good place foris_schema_valid
?The commits are organized by content. It might be easier to review by commits.
Checklist
Refer to a related PR or issue link (optional)
Follows #924