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.

Sign up for GitHub

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

Jump to bottom

remove Next() function of all the Executors #5985

Closed
41 tasks done
XuHuaiyu opened this issue Mar 9, 2018 · 36 comments
Closed
41 tasks done

remove Next() function of all the Executors #5985

XuHuaiyu opened this issue Mar 9, 2018 · 36 comments
Assignees
@XuHuaiyu
Labels
good first issue Denotes an issue ready for a new contributor, according to the "help wanted" guidelines. help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines.

Comments

@XuHuaiyu
Copy link
Contributor

XuHuaiyu commented Mar 9, 2018 •
edited
Loading

We no longer need Next() for executors since we already have NextChunk(). Function Next() and all the functions and attributes called only by Next() should also be deleted.

After removing Next(), we can detect the unused functions by tool unused:

go get honnef.co/go/tools/cmd/unused
unused github.com/pingcap/tidb/executor

NOTE:

  1. all the unused functions and attributes should be deleted.
  2. some unit tests may fail and please fix them.
@XuHuaiyu XuHuaiyu self-assigned this Mar 9, 2018
@ngaut
Copy link
Member

ngaut commented Mar 9, 2018

Feels really great to see the thread. 👍

@zz-jason zz-jason added good first issue Denotes an issue ready for a new contributor, according to the "help wanted" guidelines. help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. labels Mar 9, 2018
@qxhy123
Copy link
Contributor

qxhy123 commented Mar 9, 2018

I'm working on it

@XuHuaiyu
Copy link
Contributor Author

XuHuaiyu commented Mar 9, 2018 •
edited
Loading

We can remove the code of XXXExec.Next(),
and return nil, nil in it.
To avoid wide range code changes in one PR.

Next function can be removed directly, this PR can be referred.

@winoros winoros mentioned this issue Mar 9, 2018
Closed
@zz-jason zz-jason changed the title remove Next for Executors remove Next() function of all the Executors Mar 9, 2018
@dreamquster
Copy link
Contributor

I'm working on it. I will remove next() function in distsql.go.

executor/distsql.go: TableReaderExecutor
executor/distsql.go: IndexReaderExecutor
executor/distsql.go: IndexLookUpExecutor

@zz-jason zz-jason mentioned this issue Mar 9, 2018
Merged
@zz-jason
Copy link
Member

zz-jason commented Mar 9, 2018

@qxhy123 @dreamquster Thanks for your help !

This was referenced Mar 9, 2018
Merged
Closed
@qxhy123
Copy link
Contributor

qxhy123 commented Mar 9, 2018 •
edited
Loading

I'm working on it. I will remove next() function in executor.go.
executor/executor.go: CancelDDLJobsExec
executor/executor.go: ShowDDLExec
executor/executor.go: ShowDDLJobsExec
executor/executor.go: CheckTableExec
executor/executor.go: SelectLockExec
executor/executor.go: LimitExec
executor/executor.go: ProjectionExec
executor/executor.go: TableDualExec
executor/executor.go: SelectionExec
executor/executor.go: TableScanExec
executor/executor.go: ExistsExec
executor/executor.go: MaxOneRowExec
executor/executor.go: UnionExec

This was referenced Mar 9, 2018
Merged
Merged
Merged
@yangwenmai
Copy link
Contributor

I'm working on it. I will remove next() function in executor/write.go.

executor/write.go: DeleteExec
executor/write.go: LoadData
executor/write.go: InsertExec
executor/write.go: ReplaceExec
executor/write.go: UpdateExec

@yangwenmai
Copy link
Contributor

I'm working on it. I will remove next() function in executor/union_scan.go.

executor/union_scan.go: UnionScanExec

@zz-jason zz-jason mentioned this issue Mar 9, 2018
Merged
@shizy818
Copy link
Contributor

shizy818 commented Mar 9, 2018

I'm working on it. I will remove next() function in
executor/analyze.go: AnalyzeExec
executor/ddl.go: DDLExec

@zz-jason
Copy link
Member

zz-jason commented Mar 9, 2018

@colinback Thanks for your participation, AnalyzeExec and DDLExec have been assigned to @XuHuaiyu , please chose other executors.

@zz-jason zz-jason mentioned this issue Mar 9, 2018
Merged
@Anteoy
Copy link
Contributor

Anteoy commented Mar 10, 2018

I'm working on it. I will remove next() function in executor/show.go.

executor/show.go: ShowExec

@shizy818
Copy link
Contributor

@zz-jason Oh sorry, I do not notice that. Then I choose to do executor/grant.go: GrantExec first.
BTW, have dead line?

@XuHuaiyu
Copy link
Contributor Author

@Anteoy Thanks for your help.

@XuHuaiyu
Copy link
Contributor Author

@colinback It would be great if you can finish it before next Wednesday.

@Anteoy Anteoy mentioned this issue Mar 10, 2018
Merged
@zz-jason zz-jason mentioned this issue Mar 10, 2018
Merged
@dreamquster
Copy link
Contributor

@XuHuaiyu ， All executors in distsql.go are referenced by RecordSet.Next. Should I change all RecordSet.Next into RecordSet.NextChunk in below picture?
_ _20180310205746

@yangwenmai
Copy link
Contributor

I'm working on it. I will remove next() function in executor/simple.go.
executor/simple.go: SimpleExec

@yangwenmai
Copy link
Contributor

I'm working on it. I will remove next() function in executor/explain.go.
executor/explain.go: ExplainExec

@yangwenmai
Copy link
Contributor

I'm working on it. I will remove next() function in executor/prepared.go.
executor/prepared.go: PrepareExec
executor/prepared.go: ExecuteExec
executor/prepared.go: DeallocateExec

@yangwenmai
Copy link
Contributor

I'm working on it. I will remove next() function in executor/revoke.go.
executor/revoke.go: RevokeExec

@yangwenmai
Copy link
Contributor

I'm working on it. I will remove next() function in executor/set.go.
executor/set.go: SetExecutor

@XuHuaiyu
Copy link
Contributor Author

@dreamquster Thank you for your help, I'm working on this.

@XuHuaiyu
Copy link
Contributor Author

@colinback Feel free if you wanna try executor/analyze.go: AnalyzeExec and
executor/ddl.go: DDLExec, I haven't stared them yet.

@shizy818
Copy link
Contributor

@XuHuaiyu I'm OK with it. But I have two problems now.

  1. I do not pass CLA, but I already sign 'Contributor License Agreement '
  2. After migrating go version to 1.10, go test still fail for master branch.
OOPS: 74 passed, 1 skipped, 1 FAILED
--- FAIL: TestT (104.67s)
FAIL
exit status 1
FAIL   	github.com/pingcap/tidb	104.698s

@XuHuaiyu
Copy link
Contributor Author

XuHuaiyu commented Mar 11, 2018 •
edited
Loading

@colinback For question 2:
Try go version, what's the output?
Upload the output result of go test, we'll take a look.

@XuHuaiyu
Copy link
Contributor Author

XuHuaiyu commented Mar 11, 2018 •
edited
Loading

@colinback For question 1:
You may check here.

@shizy818
Copy link
Contributor

For question 2, here is my go test log:
go_test_fail.log
For question 1, I forget that I set company email for enterprise github on my macbook. Already fix.

@XuHuaiyu
Copy link
Contributor Author

@colinback Thank you, we'll take a look.

@XuHuaiyu
Copy link
Contributor Author

@colinback The test fail is caused by the introduction of gofail. You can run go test -check.exclude TestFail OR make test at the root directory of TiDB.

@shizy818
Copy link
Contributor

@XuHuaiyu thanks. It works

@hsqlu
Copy link
Contributor

hsqlu commented Mar 12, 2018

I am working on it. I will remove next() function in executor/aggregate.go.
executor/aggregate.go: StreamAggExec

This was referenced Mar 12, 2018
Merged
Merged
Merged
Merged
@hsqlu
Copy link
Contributor

hsqlu commented Mar 12, 2018

I am working on it. I will remove next() function in executor/sort.go.
executor/sort.go: SortExec
executor/sort.go: TopNExec

@yangwenmai
Copy link
Contributor

I am working on it. I will remove next() function in executor/join.go.
executor/join.go: NestedLoopApplyExec

This was referenced Mar 13, 2018
Merged
Merged
@XuHuaiyu
Copy link
Contributor Author

Hi, a friendly ping. @dreamquster
Did you get any problems when removing the Next function for these three Executors?
executor/distsql.go: TableReaderExecutor
executor/distsql.go: IndexReaderExecutor
executor/distsql.go: IndexLookUpExecutor

@dreamquster
Copy link
Contributor

@XuHuaiyu , I found that all executors in distsql.go are referenced in too many places on March 10.

@XuHuaiyu
Copy link
Contributor Author

Hi, @dreamquster
This has been resolved in #6040

@XuHuaiyu
Copy link
Contributor Author

done

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@XuHuaiyu XuHuaiyu

Labels
good first issue Denotes an issue ready for a new contributor, according to the "help wanted" guidelines. help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines.
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
9 participants
@qxhy123 @ngaut @hsqlu @yangwenmai @shizy818 @dreamquster @zz-jason @XuHuaiyu @Anteoy