executor: move tidb_reader code to its named files. #7065

zhexuany · 2018-07-16T16:19:30Z

What have you changed? (mandatory)

Just move code around.

What is 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)

No need to add the new test.

Does this PR affect documentation (docs/docs-cn) update? (mandatory)

Nope

Does this PR affect tidb-ansible update? (mandatory)

Nope

Does this PR need to be added to the release notes? (mandatory)

zhexuany · 2018-07-16T16:20:14Z

This is yet another PR splitting from #7016. I hope this one can be merge soon.

zhexuany · 2018-07-16T16:20:54Z

@tiancaiamao @winoros If you have any time, please take a look. Thanks a ton, ;)

winoros

i agree that move the things of table reader to a separate file. but it seems that we don't need to move buildExplain and buildTableReader

zimulala · 2018-07-17T02:04:52Z

executor/table_reader.go

@@ -0,0 +1,187 @@
+// Copyright 2017 PingCAP, Inc.


s/2017/2018

zimulala · 2018-07-17T02:05:40Z

executor/table_reader.go

+	"golang.org/x/net/context"
+)
+
+// TableReaderExecutor sends dag request and reads table data from kv layer.


s/dag/DAG ?

zimulala · 2018-07-17T02:08:25Z

executor/table_reader.go

+	tipb "github.com/pingcap/tipb/go-tipb"
+	"golang.org/x/net/context"
+)
+


Do we need to move _ Executor = &TableReaderExecutor{} here?

No need to do so, actually. baseExecutor implements all method of Executor's interface. TableReaderExecutor embed baseExecutor as anonymous
variable which makes sure TableReaderExecutor implementing Executor interface.

We can use _ Executor = &TableReaderExecutor{} to navigate codes and makes sure that TableReaderExecutor implements the Executor interface.

zimulala · 2018-07-17T02:11:34Z

Do we need to rename distsql.go? It only has the IndexReaderExecutor and IndexLookUpExecutor now.

tiancaiamao · 2018-07-17T02:36:59Z

@zz-jason also has a PR for refactor #6800
The downside of those refactor is that it's harder to cherry-pick bug fix to release-2.0 any longer.
Personally I'l happy with this kind of change. How about @shenli ?

zz-jason · 2018-07-17T02:39:09Z

executor/explain.go

@@ -27,6 +28,19 @@ type ExplainExec struct {
 	cursor int
 }

+// buildExplain builds a explain executor. `e.rows` collects final result and
+// displays it in sql-shell.
+func (b *executorBuilder) buildExplain(v *plan.Explain) Executor {


this function should stay in the builder.go

zz-jason · 2018-07-17T02:39:44Z

executor/table_reader.go

+}
+
+// Close implements the Executor Close interface.
+func (e *TableReaderExecutor) Close() error {


switch the function positions of Open() and Close()

zz-jason · 2018-07-17T02:40:15Z

executor/table_reader.go

+	return e, nil
+}
+
+func (b *executorBuilder) buildTableReader(v *plan.PhysicalTableReader) *TableReaderExecutor {


this function should stay in the builder.go

zz-jason · 2018-07-17T02:54:27Z

I think we can use this PR to split the distsql.go

alivxxx · 2018-07-17T02:58:18Z

executor/table_reader.go

+	}
+
+	e.resultHandler = &tableResultHandler{}
+	firstPartRanges, secondPartRanges := splitRanges(e.ranges, e.keepOrder)


The splitRanges and tableResultHandler should be moved too, it is used for the table reader.

But it's also used in index reader.

So put into a util_reader.go?

We can leave it remains in the distsql.go

coocood · 2018-07-17T05:23:13Z

executor/table_reader.go

+	return result, nil
+}
+
+func buildNoRangeTableReader(b *executorBuilder, v *plan.PhysicalTableReader) (*TableReaderExecutor, error) {


This function should be placed in builder.go.

tiancaiamao · 2018-07-17T12:35:27Z

Please address comments @zhexuany

zz-jason · 2018-07-19T00:21:32Z

executor/builder.go

@@ -618,6 +618,8 @@ func (b *executorBuilder) buildDDL(v *plan.DDL) Executor {
 	return e
 }

+// buildExplain builds a explain executor. `e.rows` collects final result and


I it's probably not proper explain the internal functionality for ExplainExec here. We could put the comment starts with "e.rows collects final result and ..." to the ExplainExec. Also, the result can not only be displays in the console, it can be any mysql client actually.

zz-jason · 2018-07-19T00:24:44Z

executor/builder.go

@@ -1456,6 +1458,20 @@ func containsLimit(execs []*tipb.Executor) bool {
 	return false
 }

+func (b *executorBuilder) buildTableReader(v *plan.PhysicalTableReader) *TableReaderExecutor {


We'd better put it to the old place, which is after the definition of buildNoRangeTableReader.

zz-jason · 2018-07-19T00:26:30Z

executor/table_reader.go

+	tipb "github.com/pingcap/tipb/go-tipb"
+	"golang.org/x/net/context"
+)
+


We can use _ Executor = &TableReaderExecutor{} to navigate codes and makes sure that TableReaderExecutor implements the Executor interface.

zz-jason · 2018-07-19T00:27:45Z

executor/table_reader.go

+	}
+
+	e.resultHandler = &tableResultHandler{}
+	firstPartRanges, secondPartRanges := splitRanges(e.ranges, e.keepOrder)


We can leave it remains in the distsql.go

zz-jason · 2018-07-19T13:46:40Z

LGTM

tiancaiamao · 2018-07-19T14:17:45Z

LGTM

zhexuany added 2 commits July 17, 2018 00:15

move code tidbreader code to tidb_reader

9b201a1

move explain code to explain.go

f0169aa

zhexuany requested review from tiancaiamao and winoros July 16, 2018 16:20

zhexuany added status/all tests passed type/enhancement labels Jul 16, 2018

zhexuany mentioned this pull request Jul 16, 2018

*: tidb tracing prototype #7016

Merged

3 tasks

winoros reviewed Jul 16, 2018

View reviewed changes

zimulala reviewed Jul 17, 2018

View reviewed changes

zz-jason reviewed Jul 17, 2018

View reviewed changes

address reviewer's comment

333fd30

zhexuany force-pushed the move_code_table_reader branch from 1b83b1f to 333fd30 Compare July 17, 2018 02:54

zhexuany changed the title ~~executor: move explain and tidb_reader code to their files.~~ executor: move explain and tidb_reader code to their named files. Jul 17, 2018

alivxxx reviewed Jul 17, 2018

View reviewed changes

undo move buildTableReader

c5d3114

zhexuany force-pushed the move_code_table_reader branch from 3f21ecc to c5d3114 Compare July 17, 2018 03:03

move table_reader code to table_reader.go

fd80f19

coocood reviewed Jul 17, 2018

View reviewed changes

winoros changed the title ~~executor: move explain and tidb_reader code to their named files.~~ executor: move tidb_reader code to its named files. Jul 17, 2018

address comment

b7ac778

zz-jason reviewed Jul 19, 2018

View reviewed changes

undo splitranges

0a5dd96

move code around

6941bb1

tiancaiamao added the status/LGT1 Indicates that a PR has LGTM 1. label Jul 19, 2018

Merge branch 'master' into move_code_table_reader

184bead

tiancaiamao approved these changes Jul 19, 2018

View reviewed changes

tiancaiamao merged commit 387a53a into pingcap:master Jul 19, 2018

zhexuany deleted the move_code_table_reader branch October 22, 2018 15:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

executor: move tidb_reader code to its named files. #7065

executor: move tidb_reader code to its named files. #7065

zhexuany commented Jul 16, 2018

zhexuany commented Jul 16, 2018

zhexuany commented Jul 16, 2018

winoros left a comment

zimulala Jul 17, 2018

zimulala Jul 17, 2018

zimulala Jul 17, 2018

zhexuany Jul 17, 2018

zz-jason Jul 19, 2018

zimulala commented Jul 17, 2018

tiancaiamao commented Jul 17, 2018

zz-jason Jul 17, 2018

zz-jason Jul 17, 2018

zz-jason Jul 17, 2018

zhexuany Jul 17, 2018

zz-jason commented Jul 17, 2018

alivxxx Jul 17, 2018 •

edited

zhexuany Jul 17, 2018

coocood Jul 17, 2018

zhexuany Jul 18, 2018

zz-jason Jul 19, 2018

coocood Jul 17, 2018

tiancaiamao commented Jul 17, 2018

zz-jason Jul 19, 2018

zz-jason Jul 19, 2018

zz-jason Jul 19, 2018

zz-jason Jul 19, 2018

zz-jason commented Jul 19, 2018

tiancaiamao commented Jul 19, 2018

executor: move tidb_reader code to its named files. #7065

executor: move tidb_reader code to its named files. #7065

Conversation

zhexuany commented Jul 16, 2018

What have you changed? (mandatory)

What is the type of the changes? (mandatory)

How has this PR been tested? (mandatory)

Does this PR affect documentation (docs/docs-cn) update? (mandatory)

Does this PR affect tidb-ansible update? (mandatory)

Does this PR need to be added to the release notes? (mandatory)

zhexuany commented Jul 16, 2018

zhexuany commented Jul 16, 2018

winoros left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zimulala commented Jul 17, 2018

tiancaiamao commented Jul 17, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zz-jason commented Jul 17, 2018

alivxxx Jul 17, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tiancaiamao commented Jul 17, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zz-jason commented Jul 19, 2018

tiancaiamao commented Jul 19, 2018

alivxxx Jul 17, 2018 •

edited