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

*: refactor Executor.Next() to receive RecordBatch #8994

Merged
merged 3 commits into from
Jan 14, 2019
Merged

*: refactor Executor.Next() to receive RecordBatch #8994

merged 3 commits into from
Jan 14, 2019

Conversation

lysu
Copy link
Contributor

@lysu lysu commented Jan 9, 2019 •
edited
Loading

What problem does this PR solve?

Change Executor#Next's method signature from

Next(ctx context.Context, chk *chunk.Chunk) error

to

Next(ctx context.Context, req *chunk.RecordBatch) error

To make it easy to add other parameters to Next.

What is changed and how it works?

only refactor, don't change any logic even though there are obvious optimize way exists, so it can make reviewer easier.

Check List

Tests

  • Old test case

Code changes

  • Has exported function/method change

Side effects

  • N/A

Related changes

  • N/A

This change is Reviewable

@lysu
Copy link
Contributor Author

lysu commented Jan 9, 2019

/run-all-tests

@lysu lysu requested review from XuHuaiyu and zz-jason January 9, 2019 06:47
@lysu
Copy link
Contributor Author

lysu commented Jan 9, 2019

just refactor and easy to conflict with other PR, @zz-jason @XuHuaiyu PTAL thx

@lysu lysu added the sig/execution SIG execution label Jan 9, 2019
@codecov-io
Copy link

codecov-io commented Jan 9, 2019 •
edited
Loading

Codecov Report

Merging #8994 into master will increase coverage by <.01%.
The diff coverage is 81.58%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #8994      +/-   ##
==========================================
+ Coverage   67.42%   67.43%   +<.01%     
==========================================
  Files         370      371       +1     
  Lines       75732    75739       +7     
==========================================
+ Hits        51066    51074       +8     
  Misses      20169    20169              
+ Partials     4497     4496       -1
Impacted Files Coverage Δ
util/chunk/recordbatch.go 0% <0%> (ø)
executor/radix_hash_join.go 69.49% <0%> (ø) ⬆️
executor/load_stats.go 0% <0%> (ø) ⬆️
store/mockstore/mocktikv/analyze.go 0% <0%> (ø) ⬆️
executor/analyze.go 63.46% <100%> (ø) ⬆️
executor/set.go 76.35% <100%> (ø) ⬆️
statistics/sample.go 89.01% <100%> (+0.12%) ⬆️
executor/explain.go 78.94% <100%> (ø) ⬆️
statistics/ddl.go 65.09% <100%> (+0.33%) ⬆️
privilege/privileges/cache.go 77.47% <100%> (ø) ⬆️
... and 38 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e1381b4...3a4f240. Read the comment docs.

winoros
winoros reviewed Jan 9, 2019
View reviewed changes
util/execution/req.go Outdated Show resolved Hide resolved
@qw4990 qw4990 self-requested a review January 10, 2019 05:47
qw4990
qw4990 reviewed Jan 10, 2019
View reviewed changes
executor/ddl_test.go Outdated Show resolved Hide resolved
zz-jason
zz-jason reviewed Jan 10, 2019
View reviewed changes
util/execution/req.go Outdated Show resolved Hide resolved
util/execution/req.go Outdated Show resolved Hide resolved
util/execution/req.go Outdated Show resolved Hide resolved
@lysu
Copy link
Contributor Author

lysu commented Jan 10, 2019

/run-all-tests

@winoros winoros changed the title *: refactor Executor#Next using ExecuteRequest as input param *: refactor Executor#Next using RecordBatch as input param Jan 10, 2019
@lysu
Copy link
Contributor Author

lysu commented Jan 11, 2019

@zz-jason @qw4990 comment addressed PTAL thx

qw4990
qw4990 previously approved these changes Jan 11, 2019
View reviewed changes
zz-jason
zz-jason reviewed Jan 11, 2019
View reviewed changes
cmd/benchdb/main.go Outdated Show resolved Hide resolved
cmd/benchdb/main.go Outdated Show resolved Hide resolved
@qw4990 qw4990 dismissed their stale review January 11, 2019 05:29

click mistakenly

qw4990
qw4990 reviewed Jan 11, 2019
View reviewed changes
Copy link
Contributor

@qw4990 qw4990 left a comment

Choose a reason for hiding this comment

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

LGTM

@zz-jason zz-jason mentioned this pull request Jan 13, 2019
Open
@lysu
Copy link
Contributor Author

lysu commented Jan 14, 2019

/run-all-tests

@lysu
Copy link
Contributor Author

lysu commented Jan 14, 2019

@zz-jason some comment addressed, PTAL if free thx

@zz-jason zz-jason changed the title *: refactor Executor#Next using RecordBatch as input param *: refactor Executor.Next() to receive RecordBatch Jan 14, 2019
@zz-jason
Copy link
Member

LGTM

@zz-jason
Copy link
Member

/run-integration-common-test

@zz-jason zz-jason added the status/LGT2 Indicates that a PR has LGTM 2. label Jan 14, 2019
@zz-jason zz-jason merged commit 00c4ff4 into pingcap:master Jan 14, 2019
@qw4990 qw4990 mentioned this pull request Jan 24, 2019
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@qw4990 qw4990 qw4990 left review comments

@winoros winoros winoros left review comments

@zz-jason zz-jason zz-jason approved these changes

@XuHuaiyu XuHuaiyu Awaiting requested review from XuHuaiyu

Assignees

@zz-jason zz-jason

Labels
sig/execution SIG execution status/LGT2 Indicates that a PR has LGTM 2.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@lysu @codecov-io @zz-jason @qw4990 @winoros