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: support IndexMergeReaderExecutor #12305

Merged
merged 8 commits into from
Dec 31, 2019
Merged

executor: support IndexMergeReaderExecutor #12305

merged 8 commits into from
Dec 31, 2019

Conversation

hailanwhu
Copy link
Contributor

What problem does this PR solve?

In #11245 PR, we generate the PhysicalPlan for IndexMerge. In this PR, we add the executor and execution framework for IndexMerge.

What is changed and how it works?

Add new executor, IndexMergeReaderExecutor
Add execution framework of IndexMergeReaderExecutor, most of the code is borrowed from IndexLookUp and TableReader. However, it is difficult to refactor.

Check List

Tests

  • No code
    Test will be added after IndexMerge hint added.

Code changes

Side effects

  • Possible performance regression
  • Increased code complexity
  • Breaking backward compatibility

Release note

-new feature.

@sre-bot sre-bot added the contribution This PR is from a community contributor. label Sep 23, 2019
@codecov
Copy link

codecov bot commented Sep 23, 2019

Codecov Report

❗ No coverage uploaded for pull request base (master@933715f). Click here to learn what that means.
The diff coverage is 69.9422%.

@@             Coverage Diff             @@
##             master     #12305   +/-   ##
===========================================
  Coverage          ?   80.1134%           
===========================================
  Files             ?        484           
  Lines             ?     121992           
  Branches          ?          0           
===========================================
  Hits              ?      97732           
  Misses            ?      16458           
  Partials          ?       7802

planner/core/resolve_indices.go Outdated Show resolved Hide resolved
planner/core/resolve_indices.go Outdated Show resolved Hide resolved
executor/distsql.go Outdated Show resolved Hide resolved
@hailanwhu
Copy link
Contributor Author

@zz-jason I have fixed the comments. However, the unit-test failed and the log is [2019-09-24T13:25:49.372Z] gzip: stdin: unexpected end of file . I cannot get any useful information from it to fix this error.

@sre-bot
Copy link
Contributor

sre-bot commented Sep 26, 2019

@zz-jason, @SunRunAway, @XuHuaiyu, PTAL.

2 similar comments
@sre-bot
Copy link
Contributor

sre-bot commented Sep 28, 2019

@zz-jason, @SunRunAway, @XuHuaiyu, PTAL.

@sre-bot
Copy link
Contributor

sre-bot commented Oct 1, 2019

@zz-jason, @SunRunAway, @XuHuaiyu, PTAL.

executor/builder.go Outdated Show resolved Hide resolved
executor/builder.go Outdated Show resolved Hide resolved
@sre-bot
Copy link
Contributor

sre-bot commented Oct 10, 2019

@zz-jason, @XuHuaiyu, @SunRunAway, PTAL.

1 similar comment
@sre-bot
Copy link
Contributor

sre-bot commented Oct 12, 2019

@zz-jason, @XuHuaiyu, @SunRunAway, PTAL.

@zyxbest
Copy link
Contributor

zyxbest commented Oct 14, 2019

/run-unit-test

@sre-bot
Copy link
Contributor

sre-bot commented Oct 15, 2019

@zz-jason, @XuHuaiyu, @SunRunAway, PTAL.

@hailanwhu
Copy link
Contributor Author

@XuHuaiyu I add the comments and force executing using IndexMerge path.

@hailanwhu
Copy link
Contributor Author

@XuHuaiyu I meet some problems. I am not sure why it fails using testSuit1.

@hailanwhu hailanwhu requested a review from a team as a code owner December 4, 2019 05:59
@ghost ghost requested review from eurekaka and winoros and removed request for a team December 4, 2019 05:59
@eurekaka eurekaka removed their request for review December 4, 2019 06:07
@hailanwhu
Copy link
Contributor Author

@eurekaka I do not have a clear idea why using the testSuit1 failed.

@eurekaka
Copy link
Contributor

eurekaka commented Dec 4, 2019

I manually run the test SQL sequence in MySQL client, the case fails also. Looks like there are bugs in code logic.

@hailanwhu
Copy link
Contributor Author

@eurekaka last time, the manual test failed because of the setting of startTS. I updated the code. However, the test still failed.

@hailanwhu
Copy link
Contributor Author

@eurekaka all pass. Maybe the feedback is not handled well. Because every partial scan could have a feedback, it is not sure how to deal with them.

planner/core/physical_plans.go Outdated Show resolved Hide resolved
executor/index_merge_reader.go Outdated Show resolved Hide resolved
executor/index_merge_reader.go Outdated Show resolved Hide resolved
executor/index_merge_reader.go Outdated Show resolved Hide resolved
executor/index_merge_reader.go Outdated Show resolved Hide resolved
for i := 0; i < len(e.partialPlans); i++ {
_, ok := e.partialPlans[i][0].(*plannercore.PhysicalIndexScan)
if ok {
kvRanges, err := distsql.IndexRangesToKVRanges(e.ctx.GetSessionVars().StmtCtx, getPhysicalTableID(e.table), e.indexes[i].ID, e.ranges[i], e.feedback)
Copy link
Contributor

Choose a reason for hiding this comment

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

s/ kvRanges/ ranges


// Open implements the Executor Open interface
func (e *IndexMergeReaderExecutor) Open(ctx context.Context) error {
kvRangess := make([][]kv.KeyRange, 0, len(e.partialPlans))
Copy link
Contributor

Choose a reason for hiding this comment

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

s/ kvRangess/ kvRanges

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do I need to rename the variables?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, it's a typo.

executor/builder.go Outdated Show resolved Hide resolved
executor/builder.go Outdated Show resolved Hide resolved
executor/builder.go Outdated Show resolved Hide resolved
executor/index_merge_reader.go Outdated Show resolved Hide resolved
executor/index_merge_reader.go Outdated Show resolved Hide resolved
executor/index_merge_reader.go Outdated Show resolved Hide resolved
executor/index_merge_reader.go Outdated Show resolved Hide resolved
@sre-bot
Copy link
Contributor

sre-bot commented Dec 25, 2019

@zz-jason, @XuHuaiyu, @qw4990, @SunRunAway, PTAL.

1 similar comment
@sre-bot
Copy link
Contributor

sre-bot commented Dec 28, 2019

@zz-jason, @XuHuaiyu, @qw4990, @SunRunAway, PTAL.

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.

Should we check the enableIndexMerge before generating an IndexMerge plan at find_best_task.go:423

@XuHuaiyu
Copy link
Contributor

/run-all-tests

@XuHuaiyu
Copy link
Contributor

/run-all-tests

@XuHuaiyu
Copy link
Contributor

LGTM

@qw4990 qw4990 added the status/LGT1 Indicates that a PR has LGTM 1. label Dec 31, 2019
@qw4990
Copy link
Contributor

qw4990 commented Dec 31, 2019

LGTM

@qw4990 qw4990 added status/can-merge Indicates a PR has been approved by a committer. status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. status/PTAL labels Dec 31, 2019
@sre-bot
Copy link
Contributor

sre-bot commented Dec 31, 2019

/run-all-tests

@sre-bot
Copy link
Contributor

sre-bot commented Dec 31, 2019

@hailanwhu merge failed.

@XuHuaiyu
Copy link
Contributor

/run-unit-test

@XuHuaiyu XuHuaiyu changed the title executor: add IndexMergeReaderExecutor and its execution framework executor: support IndexMergeReaderExecutor Dec 31, 2019
@XuHuaiyu XuHuaiyu merged commit ae106f2 into pingcap:master Dec 31, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
contribution This PR is from a community contributor. sig/execution SIG execution status/can-merge Indicates a PR has been approved by a committer. status/LGT2 Indicates that a PR has LGTM 2.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants