plan: move range calculation to package ranger #3208
Conversation
plan/ranger/range.go
Outdated
| @@ -11,7 +11,7 @@ | |||
| // See the License for the specific language governing permissions and | |||
| // limitations under the License. | |||
|
|
|||
| package plan | |||
| package ranger | |||
There was a problem hiding this comment.
It's better to move ranger to util package. Because statistic package still need to use it.
plan/util/util.go
Outdated
| // See the License for the specific language governing permissions and | ||
| // limitations under the License. | ||
|
|
||
| package util |
There was a problem hiding this comment.
Don't new a util package. If some errors are needed by ranger, just duplicate this error in ranger package.
plan/ranger/range_test.go
Outdated
| @@ -0,0 +1,230 @@ | |||
| package ranger_test | |||
|
LGTM @coocood PTAL |
| @@ -346,8 +347,8 @@ func (p *DataSource) tryToGetMemTask(prop *requiredProp) (task taskProfile, err | |||
| TableAsName: p.TableAsName, | |||
| }.init(p.allocator, p.ctx) | |||
| memTable.SetSchema(p.schema) | |||
| rb := &rangeBuilder{sc: p.ctx.GetSessionVars().StmtCtx} | |||
| memTable.Ranges = rb.buildTableRanges(fullRange) | |||
| rb := &ranger.Builder{Sc: p.ctx.GetSessionVars().StmtCtx} | |||
There was a problem hiding this comment.
Could you define a helper method to get StmtCtx, I saw redundant "p.ctx.GetSessionVars().StmtCtx"
There was a problem hiding this comment.
I will try to do this in a new pr.
model/model.go
Outdated
| @@ -119,6 +120,18 @@ func (t *TableInfo) Clone() *TableInfo { | |||
| return &nt | |||
| } | |||
|
|
|||
| //GetPkName will return the pk name if pk exitsts. | |||
| @@ -83,6 +83,25 @@ func Optimize(ctx context.Context, node ast.Node, is infoschema.InfoSchema) (Pla | |||
| return p, nil | |||
| } | |||
|
|
|||
| // BuildLogicalPlan is exported and only used for test. | |||
There was a problem hiding this comment.
If it is only used for test, can we put it in the test?
There was a problem hiding this comment.
In this way will make planBuilder and something exported, more complicated.
model/model.go
Outdated
| @@ -119,6 +120,18 @@ func (t *TableInfo) Clone() *TableInfo { | |||
| return &nt | |||
| } | |||
|
|
|||
| //GetPkName will return the pk name if pk exitsts. | |||
util/ranger/refiner.go
Outdated
| "github.com/pingcap/tidb/expression" | ||
| "github.com/pingcap/tidb/model" | ||
| "github.com/pingcap/tidb/mysql" | ||
| "github.com/pingcap/tidb/sessionctx/variable" | ||
| "github.com/pingcap/tidb/util/types" | ||
| ) | ||
|
|
||
| var fullRange = []rangePoint{ | ||
| // FullRange is (-∞,, +∞). |
util/ranger/range.go
Outdated
| } | ||
|
|
||
| // BuildFromConds is used for test. | ||
| func (r *Builder) BuildFromConds(conds []expression.Expression) ([]Point, error) { |
There was a problem hiding this comment.
Can we just move it to test file?
There was a problem hiding this comment.
This will make more function exported.
There was a problem hiding this comment.
@winoros Actually, you can add a foo_internal_test.go in the same package. This way, you do not need to export extra method.
| } | ||
| } | ||
| } | ||
| return CIStr{} |
There was a problem hiding this comment.
If pk is not handle, we always return empty string?
There was a problem hiding this comment.
Yes, and the outside will know whether there is a pk by check this.
|
@winoros |
|
@winoros Please fix the CI. |
|
@coocood now i just test the BuildTableRange and BuildIndexRange instead of testing the method of RangeBuilder. |
| if sf.FuncName.L == ast.EQ && checker.checkScalarFunction(sf) { | ||
| return controlTableScan | ||
| } | ||
| access, _ := ranger.DetachTableScanConditions(corColConds, pkCol.ColName) |
There was a problem hiding this comment.
why change this, is this an equivalence transformation？
@winoros
There was a problem hiding this comment.
Yes, it's the same as the logic inside the DetachTableScanConditions
|
LGTM |
PTAL @shenli @coocood @hanfei1991 @XuHuaiyu