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

executor: split interface Executor to a single package #6800

Closed
wants to merge 7 commits into from

Conversation

zz-jason
Copy link
Member

@zz-jason zz-jason commented Jun 10, 2018

What have you changed? (mandatory)

This is the first step to split the executor package. The executor package is too large, which contains a lot of source files with different functionality:

  1. physical operator implementations
  2. query adaptors
  3. record set implementations

This pull request renames Executor to Operator and move it to a new package named operator. For further improvements, we can:

  • make a new package named joins inside the operator package and move all the join implementations into it
  • make a new package named readers inside the operator package and move all the reader implementations into it
  • ...

What are the type of the changes (mandatory)?

  • Improvement (non-breaking change which is an improvement to an existing feature)

How has this PR been tested (mandatory)?

  • unit test
  • explain test

@zz-jason
Copy link
Member Author

@tiancaiamao PTAL

@zz-jason
Copy link
Member Author

/run-all-tests

@@ -0,0 +1,154 @@
// Copyright 2015 PingCAP, Inc.
Copy link
Member

Choose a reason for hiding this comment

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

The year should be 2018.

@winoros
Copy link
Member

winoros commented Jun 11, 2018

Will the structure become the following?

- executor:
    - base_operator:
    - join_operator:
    - agg_operator:
...

Or:

- executor:
    - operator:
        - join_operator:
        - agg_operator:
...

@winoros
Copy link
Member

winoros commented Jun 11, 2018

And the the structs whose name is end with Exec should be updated to Op?
Since the operator actually is execution operator, which is the same thing with executor.

@zz-jason
Copy link
Member Author

@winoros Both ways are acceptable, I think we can just remove the Exec suffix?

@@ -356,7 +357,7 @@ func (a *ExecStmt) logSlowQuery(txnTS uint64, succ bool) {
}

// IsPointGetWithPKOrUniqueKeyByAutoCommit returns true when meets following conditions:
// 1. ctx is auto commit tagged
// 1. Sctx is auto commit tagged
Copy link
Contributor

Choose a reason for hiding this comment

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

I think SeCtx or SCtx is better.

// A typical usage of this function is:
// chk := operator.NewChunk()
// err := operator.Next(ctx, chk)
NewChunk() *chunk.Chunk
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this really necessary to be in the Operator interface, as long as []*types.FieldType is provided ?

Copy link
Contributor

Choose a reason for hiding this comment

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

In the OO style, any Operator inherit a BaseOperator, so those methods are get from BaseOperator for free.
In Go, interface should be minimal so they can be composed more easy.
The more methods an interface contains, the less flexible it is.

Copy link
Member Author

Choose a reason for hiding this comment

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

we can do it in another PR, for these PRs, we just move code.


// RetTypes returns the types of all the output columns, which can be found
// in the result of function Schema().
RetTypes() []*types.FieldType
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this really necessary to be in the Operator interface, as long as Schema() is provided ?

// interface in the basic way, Operator implementations should implement their
// own methods if necessary.
type BaseOperator struct {
Sctx sessionctx.Context
Copy link
Contributor

Choose a reason for hiding this comment

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

How about move the BaseOperator back to executor package, so those field need not to be exposed?
And we can better separate interface and implementation.

Copy link
Member Author

Choose a reason for hiding this comment

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

Agree this comment. As for other comments, I'll leave them for further improvement. These kind of PRs should only focus on the refactoring of code positions.

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh, the whole plan is to move all the operator implementations into the new package "operator", during the development, we could have some operators inside the "operator" package, while other operators are outside that package. These fields are only exported temporarily. After all the operators are moved into that package, the exported field will be un-exported back.

@zz-jason
Copy link
Member Author

This PR should be merged when release 2.1 is ready, for now I just leave it here for discussing.

@zz-jason zz-jason added type/enhancement The issue or PR belongs to an enhancement. sig/planner SIG: Planner sig/execution SIG execution and removed status/DNM sig/planner SIG: Planner labels Jun 21, 2018
@zz-jason zz-jason closed this Sep 25, 2018
@zz-jason zz-jason deleted the dev/split-executor branch September 25, 2018 09:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
sig/execution SIG execution type/enhancement The issue or PR belongs to an enhancement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants