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: remove the SQL execution interfaces in Datanode #1135

Merged

Conversation

MichaelScofield
Copy link
Collaborator

@MichaelScofield MichaelScofield commented Mar 7, 2023

I hereby agree to the terms of the GreptimeDB CLA

What's changed and what's your intention?

Motivation: #1010

This PR:

  1. Remove the implementation of SqlQueryHandler for Datanode Instance and DistInstance.
  2. "select" is executed in query engine, while other SQLs are executed via a new StatementHandler. When more statements are executed in the form of logical plan, this StatementHandler will be removed eventually.
  3. Both Datanode instance and DistInstance implement StatementHandler.

So the SQL execution path is like this:

Frontend Instance still impl the SqlQueryHandler, which is called by servers. Inside the impl:

fn do_query(&self, sql: &str) -> Output {
  let stmt = parse(sql);
  match stmt {
    Statement::Query | Statement::Tql => {
        let plan = plan(stmt);
        self.query_engine.execute(plan)
    }
    _ => {
        self.statement_handler.handle(stmt)
    }
  }
}

The plan is to gradually move all statement execution to logical plan execution. Finally unified to query_engine's execution of logical plan.

The difference between standalone and distributed execution of logical plan should be revealed in different implementation of QueryEngine, which will be in the following PRs.

  1. Remove the MySQL server implementation from Datanode.
  2. Extract the codes that create logical plan in query engine to a LogicalPlanner, making codes more clear.
  3. Other codes clean up.

Checklist

  • I have written the necessary rustdoc comments.
  • I have added the necessary unit tests and integration tests.

Refer to a related PR or issue link (optional)

#1010

@codecov
Copy link

codecov bot commented Mar 7, 2023

Codecov Report

Merging #1135 (9ecd791) into develop (c7f114c) will increase coverage by 0.13%.
The diff coverage is 74.91%.

@@             Coverage Diff             @@
##           develop    #1135      +/-   ##
===========================================
+ Coverage    85.02%   85.15%   +0.13%     
===========================================
  Files          482      484       +2     
  Lines        71386    71152     -234     
===========================================
- Hits         60697    60593     -104     
+ Misses       10689    10559     -130     

src/datanode/src/instance/sql.rs Outdated Show resolved Hide resolved
src/datanode/src/server.rs Show resolved Hide resolved
src/datanode/src/tests.rs Show resolved Hide resolved
src/datanode/src/tests/promql_test.rs Outdated Show resolved Hide resolved
@MichaelScofield
Copy link
Collaborator Author

Resolved, PTAL @evenyag

Copy link
Contributor

@evenyag evenyag left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

src/datanode/src/instance/grpc.rs Outdated Show resolved Hide resolved
src/query/src/planner.rs Show resolved Hide resolved
@MichaelScofield MichaelScofield force-pushed the refactor/datanode-no-sql branch from cb0e020 to 0b44788 Compare March 10, 2023 03:40
@killme2008
Copy link
Contributor

@MichaelScofield There are some conflicts.

@MichaelScofield MichaelScofield force-pushed the refactor/datanode-no-sql branch from 0b44788 to 9ecd791 Compare March 13, 2023 10:22
@MichaelScofield
Copy link
Collaborator Author

resolved, PTAL @killme2008 @sunng87

@sunng87
Copy link
Member

sunng87 commented Mar 13, 2023

LGTM. Good job!

@MichaelScofield MichaelScofield merged commit 604c20a into GreptimeTeam:develop Mar 13, 2023
@MichaelScofield MichaelScofield deleted the refactor/datanode-no-sql branch March 13, 2023 10:45
paomian pushed a commit to paomian/greptimedb that referenced this pull request Oct 19, 2023
…am#1135)

* refactor: remove the SQL execution interfaces in Datanode
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs-required This change requires docs update.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants