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: fix wrong plan type for dataReaderBuilder error #17028

Merged
merged 9 commits into from
May 8, 2020

Conversation

windtalker
Copy link
Contributor

@windtalker windtalker commented May 8, 2020

What problem does this PR solve?

Issue Number:

Problem Summary:
After PR #16389 TiDB will push down all the expr supported by TiKV or TiFlash in predicate push down stage, so if there is an expr which only supported by TiFlash, a physical selection will be added after index read, but current implementation of buildExecutorForIndexJoin does not support physical selection, so it will throw wrong plan type for dataReaderBuilder error when meeting physical selection plan.

What is changed and how it works?

What's Changed:
support physical selection in buildExecutorForIndexJoin

Related changes

  • Need to cherry-pick to the release branch

Check List

Tests

  • Integration test

Release note

  • fix Wrong plan type for dataReaderBuilder error

@windtalker windtalker requested a review from a team as a code owner May 8, 2020 01:50
@ghost ghost requested review from qw4990 and removed request for a team May 8, 2020 01:50
@codecov
Copy link

codecov bot commented May 8, 2020

Codecov Report

Merging #17028 into master will increase coverage by 0.2934%.
The diff coverage is n/a.

@@               Coverage Diff                @@
##             master     #17028        +/-   ##
================================================
+ Coverage   79.9685%   80.2619%   +0.2933%     
================================================
  Files           510        510                
  Lines        138797     140232      +1435     
================================================
+ Hits         110994     112553      +1559     
+ Misses        18828      18712       -116     
+ Partials       8975       8967         -8     

Copy link
Contributor

@lzmhhh123 lzmhhh123 left a comment

Choose a reason for hiding this comment

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

LGTM

@lzmhhh123 lzmhhh123 added the status/LGT1 Indicates that a PR has LGTM 1. label May 8, 2020
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

@qw4990 qw4990 added status/LGT2 Indicates that a PR has LGTM 2. status/can-merge Indicates a PR has been approved by a committer. and removed status/LGT1 Indicates that a PR has LGTM 1. labels May 8, 2020
@qw4990
Copy link
Contributor

qw4990 commented May 8, 2020

@windtalker Should we pick this PR to v3.0.0?

@sre-bot
Copy link
Contributor

sre-bot commented May 8, 2020

/run-all-tests

@windtalker
Copy link
Contributor Author

@windtalker Should we pick this PR to v3.0.0?

No, only 3.1 is affected.

@lzmhhh123
Copy link
Contributor

/merge

@sre-bot
Copy link
Contributor

sre-bot commented May 8, 2020

/run-all-tests

@sre-bot
Copy link
Contributor

sre-bot commented May 8, 2020

@windtalker merge failed.

@lzmhhh123 lzmhhh123 changed the title fix Wrong plan type for dataReaderBuilder error executor: fix wrong plan type for dataReaderBuilder error May 8, 2020
@lzmhhh123 lzmhhh123 merged commit 3224393 into pingcap:master May 8, 2020
sre-bot pushed a commit to sre-bot/tidb that referenced this pull request May 8, 2020
@sre-bot
Copy link
Contributor

sre-bot commented May 8, 2020

cherry pick to release-3.1 in PR #17034

@sre-bot
Copy link
Contributor

sre-bot commented May 8, 2020

cherry pick to release-4.0 in PR #17036

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component/expression 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
4 participants