Skip to content

planner, expression: remove TblID2Handle from Schema#11152

Merged
sre-bot merged 17 commits into
pingcap:masterfrom
winoros:remove-tblid2handle-in-schema
Aug 16, 2019
Merged

planner, expression: remove TblID2Handle from Schema#11152
sre-bot merged 17 commits into
pingcap:masterfrom
winoros:remove-tblid2handle-in-schema

Conversation

@winoros
Copy link
Copy Markdown
Member

@winoros winoros commented Jul 9, 2019 •
edited
Loading

What problem does this PR solve?

TblID2Handle can just be maintained when building plan. Only be held by just part of plan.
Not all of them need to hold it.
So remove it from Schema struct.

What is changed and how it works?

Use a stack to maintain it when building plan. And then let speficific plan hold it.

Check List

Tests

  • Unit test

Code changes

  • Has exported function/method change
  • Has exported variable/fields change

@winoros
Copy link
Copy Markdown
Member Author

winoros commented Jul 9, 2019

There're some small things can be splitted as small PRs. I'll do it later.

@codecov
Copy link
Copy Markdown

codecov Bot commented Jul 9, 2019 •
edited
Loading

Codecov Report

Merging #11152 into master will not change coverage.
The diff coverage is n/a.

@@             Coverage Diff             @@
##             master     #11152   +/-   ##
===========================================
  Coverage   82.1563%   82.1563%           
===========================================
  Files           433        433           
  Lines         96303      96303           
===========================================
  Hits          79119      79119           
  Misses        11713      11713           
  Partials       5471       5471

@winoros
Copy link
Copy Markdown
Member Author

winoros commented Jul 9, 2019

/run-all-tests

winoros
winoros commented Jul 10, 2019
View reviewed changes
Comment thread planner/core/logical_plan_builder.go
@winoros
Copy link
Copy Markdown
Member Author

winoros commented Jul 10, 2019

/run-all-tests

@winoros winoros marked this pull request as ready for review July 10, 2019 05:59
lzmhhh123
lzmhhh123 reviewed Jul 16, 2019
View reviewed changes
Comment thread planner/core/logical_plan_builder.go
Comment thread planner/core/logical_plan_builder.go Outdated
Comment thread planner/core/logical_plan_builder.go Outdated
Comment thread planner/core/logical_plans.go
alivxxx
alivxxx reviewed Jul 22, 2019
View reviewed changes
Comment thread executor/builder.go Outdated
Comment thread planner/core/point_get_plan.go Outdated
@eurekaka
Copy link
Copy Markdown
Contributor

eurekaka commented Jul 29, 2019

@winoros please resolve conflicts and fix CI.

@eurekaka eurekaka removed their request for review July 29, 2019 08:54
@winoros winoros force-pushed the remove-tblid2handle-in-schema branch from 7e26af5 to c9d49fb Compare July 31, 2019 05:45
@winoros
Copy link
Copy Markdown
Member Author

winoros commented Jul 31, 2019

/run-all-tests

@winoros winoros requested review from alivxxx and eurekaka July 31, 2019 06:52
lzmhhh123
lzmhhh123 reviewed Jul 31, 2019
View reviewed changes
Comment thread planner/core/point_get_plan.go
}
}

func (p *PointGetPlan) findHandleCol() *expression.Column {
Copy link
Copy Markdown
Contributor

@lzmhhh123 lzmhhh123 Jul 31, 2019

Choose a reason for hiding this comment

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

It's very same as DataSource.getPKIsHandleCol. Can we merge them?

alivxxx
alivxxx reviewed Aug 1, 2019
View reviewed changes
Comment thread planner/core/point_get_plan.go Outdated
@zz-jason
Copy link
Copy Markdown
Member

zz-jason commented Aug 13, 2019

@winoros Please fix CI to make this PR reviewable.

@winoros winoros force-pushed the remove-tblid2handle-in-schema branch from 09480e3 to 8544cb9 Compare August 13, 2019 07:05
@winoros winoros requested review from alivxxx and lzmhhh123 August 14, 2019 05:27
lzmhhh123
lzmhhh123 reviewed Aug 14, 2019
View reviewed changes
Comment thread planner/core/logical_plan_builder.go
alivxxx
alivxxx reviewed Aug 14, 2019
View reviewed changes
Comment thread executor/executor.go Outdated
lzmhhh123
lzmhhh123 reviewed Aug 15, 2019
View reviewed changes
Copy link
Copy Markdown
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.

@winoros winoros requested a review from alivxxx August 15, 2019 08:07
alivxxx
alivxxx approved these changes Aug 16, 2019
View reviewed changes
Copy link
Copy Markdown
Contributor

@alivxxx alivxxx left a comment

Choose a reason for hiding this comment

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

LGTM

@alivxxx alivxxx added status/LGT2 Indicates that a PR has LGTM 2. status/can-merge Indicates a PR has been approved by a committer. labels Aug 16, 2019
@sre-bot
Copy link
Copy Markdown
Contributor

sre-bot commented Aug 16, 2019

/run-all-tests

@sre-bot sre-bot merged commit 8a16172 into pingcap:master Aug 16, 2019
@winoros winoros deleted the remove-tblid2handle-in-schema branch September 5, 2019 06:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@eurekaka eurekaka Awaiting requested review from eurekaka

2 more reviewers

@lzmhhh123 lzmhhh123 lzmhhh123 left review comments

@alivxxx alivxxx alivxxx approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

component/expression sig/planner SIG: Planner status/can-merge Indicates a PR has been approved by a committer. 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.

6 participants

@winoros @eurekaka @zz-jason @sre-bot @alivxxx @lzmhhh123