Skip to content
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 all protocol related code to frontend #1010

Closed
6 tasks done
v0y4g3r opened this issue Feb 15, 2023 · 1 comment
Closed
6 tasks done

Refactor all protocol related code to frontend #1010

v0y4g3r opened this issue Feb 15, 2023 · 1 comment
Assignees
Labels
C-enhancement Category Enhancements help wanted Extra attention is needed tracking-issue A tracking issue for a feature.
Milestone

Comments

@v0y4g3r
Copy link
Contributor

v0y4g3r commented Feb 15, 2023

What type of enhancement is this?

Tech debt reduction

What does the enhancement do?

We've been expecting that all protocol-related logic remains in frontend, including protocol parsing and logical planning. For some reason now datanode also keeps a parser and can also accepts SQL queries. Also frontend delegates SQL parsing to datanode in standalone mode:

sql_handler: StandaloneSqlQueryHandler::arc(dn_instance.clone()),

This compromise brings a lot of tech debt and maintenance overhead, for example, introducing a new protocol involves both frontend and datanode. Now it's time to get things right.

Steps

  • remove the SQL handler and MySQL server implementation from datanode refactor: remove the SQL execution interfaces in Datanode #1135
  • move the tests mod in Datanode to Frontend. They are largely sql and promql tests.
    • make "instance_test.rs" also ran with distributed instance
  • move promql exec to frontend
    • also move "promql_test.rs" to frontend
  • check sqlness test cases, some may move to "common"

Implementation challenges

  • Move all protocol parsing stuff from datanode to frontend
    • Datanode instance should not implement SqlQueryHandler trait, it should only keep a GrpcQueryHandler implementation.
      impl SqlQueryHandler for Instance {
    • The sql_handler field of datanode::instance::Instance should be moved to frontend instance. SqlHandler also has some field like table_engine and catalog_manager, which in standalone mode, datanode should share these components to frontend so that frontend can build a SqlHandler.
      pub(crate) sql_handler: SqlHandler,
    • Move thePromqlHandler implementation from datanode to frontend. In standalone mode it's also delegated to datanode.
      promql_handler.do_query(query).await
  • Improve our GrpcQueryHandler to accommodate more plans and data types.
    After the refactoring, datanode accepts only grpc requests with logical plans encoded as substrait (for inserts, gRPC protocol also has an Insert variant). Thus we need to support all the possible logical plans and data types in substrait.
@evenyag
Copy link
Contributor

evenyag commented Mar 8, 2023

@MichaelScofield I create a task list to track the tasks you mentioned in #1135

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-enhancement Category Enhancements help wanted Extra attention is needed tracking-issue A tracking issue for a feature.
Projects
No open projects
Status: Done
Development

No branches or pull requests

3 participants