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

planner, executor: support point get as a option of DataSource #14775

Merged
merged 37 commits into from
Mar 9, 2020

Conversation

lzmhhh123
Copy link
Contributor

What problem does this PR solve?

As the title says.
fix #14044

Check List

Tests

  • Unit test
  • Integration test

Code changes

  • Has exported function/method change
  • Has exported variable/fields change
  • Has interface methods change
  • Has persistent data change

@lzmhhh123 lzmhhh123 requested a review from a team as a code owner February 13, 2020 07:15
@ghost ghost requested review from eurekaka and winoros and removed request for a team February 13, 2020 07:15
@lzmhhh123 lzmhhh123 requested a review from a team as a code owner February 17, 2020 09:06
@ghost ghost requested review from SunRunAway and XuHuaiyu and removed request for a team February 17, 2020 09:06
rTsk.p = sel
}
} else {
if !candidate.path.Index.Unique ||
Copy link
Member

Choose a reason for hiding this comment

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

Can we check this before allRangesIsPoint, so we don't need to do the wasted computation?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK.

prop.TaskTp == property.CopSingleReadTaskType && !candidate.isSingleScan {
return invalidTask, nil
}
if candidate.path.StoreType == kv.TiFlash {
Copy link
Member

Choose a reason for hiding this comment

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

Can we do all the checks before allRangeIsPoint? then this function doesn't need to return error.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK.

@coocood
Copy link
Member

coocood commented Mar 6, 2020

LGTM

@coocood coocood added the status/LGT1 Indicates that a PR has LGTM 1. label Mar 6, 2020
eurekaka
eurekaka previously approved these changes Mar 6, 2020
Copy link
Contributor

@eurekaka eurekaka 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/can-merge Indicates a PR has been approved by a committer. label Mar 6, 2020
@sre-bot
Copy link
Contributor

sre-bot commented Mar 6, 2020

/run-all-tests

@sre-bot
Copy link
Contributor

sre-bot commented Mar 6, 2020

@lzmhhh123 merge failed.

@lzmhhh123
Copy link
Contributor Author

/run-all-test

@lzmhhh123
Copy link
Contributor Author

/run-all-tests

@lzmhhh123
Copy link
Contributor Author

/merge

@sre-bot
Copy link
Contributor

sre-bot commented Mar 9, 2020

Your auto merge job has been accepted, waiting for 15114

@sre-bot
Copy link
Contributor

sre-bot commented Mar 9, 2020

/run-all-tests

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
sig/execution SIG execution sig/planner SIG: Planner status/can-merge Indicates a PR has been approved by a committer. status/LGT1 Indicates that a PR has LGTM 1. type/new-feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Let PointGet be a choice of DataSource's PhysicalPlan.
5 participants