refactor: remove substrait ser/de for region query in standalone #3812
Conversation
MichaelScofield
force-pushed
the
refactor/no-serde-for-standalone-plan
branch
from
ee45357
to
7e77982
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3812 +/- ##
==========================================
- Coverage 85.47% 85.18% -0.30%
==========================================
Files 991 991
Lines 173053 173061 +8
==========================================
- Hits 147920 147414 -506
- Misses 25133 25647 +514 |
MichaelScofield
force-pushed
the
refactor/no-serde-for-standalone-plan
branch
from
7e77982
to
8aabf78
Compare
MichaelScofield
force-pushed
the
refactor/no-serde-for-standalone-plan
branch
from
8aabf78
to
e8c9d37
Compare
| /// The query request to be handled by the RegionServer(Datanode). | ||
| pub struct QueryRequest { | ||
| /// The header of this request. Often to store some context of the query. None means all to defaults. | ||
| pub header: Option<RegionRequestHeader>, | ||
|
|
||
| /// The id of the region to be queried. | ||
| pub region_id: RegionId, | ||
|
|
||
| /// The form of the query: a logical plan. | ||
| pub plan: LogicalPlan, | ||
| } |
There was a problem hiding this comment.
Putting the QueryRequest in the common-meta is weird. What about moving it to common-query?
There was a problem hiding this comment.
Can't do it because RegionId is in store-api, and store-api depends on common-query.
There was a problem hiding this comment.
Do we need to keep region_id in the QueryRequest? We can pass region_id to handle_read() like handle_request().
greptimedb/src/datanode/src/region_server.rs
Lines 117 to 121 in a6a702d
There was a problem hiding this comment.
I think it's better to lessen the args of a function. The QueryRequest is directly used in trait Datanode defined in the same file, if QueryRequest is to be not placed in common-meta crate, so is that trait. I think we can do it in the future, to better organize the codes.
| @@ -229,24 +215,3 @@ impl TreeNodeVisitor for TableNameExtractor { | |||
| } | |||
| } | |||
| } | |||
|
|
|||
| struct TableNameRewriter; | |||
There was a problem hiding this comment.
Why we don't need to rewrite the table name?
There was a problem hiding this comment.
It seems it's not needed at the first place. Substrait is able to serde the fully qualified table name correctly. Rewriting table name like this could make physical plan failed at the optimization stage because the schema is not match before and after.
MichaelScofield
force-pushed
the
refactor/no-serde-for-standalone-plan
branch
2 times, most recently
from
48520a2
to
50ac605
Compare
|
@waynexia Please take a look. |
|
I'm going to look into this after v0.8 release. |
MichaelScofield
force-pushed
the
refactor/no-serde-for-standalone-plan
branch
from
50ac605
to
da59aee
Compare
|
@waynexia PTAL |
MichaelScofield
force-pushed
the
refactor/no-serde-for-standalone-plan
branch
from
da59aee
to
6cc8833
Compare
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?
close #3520
This PR makes the logical plan in region query request not always be serialized to and deserialized from substrait bytes, which is not necessary in standalone mode.
Checklist