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

planner: set the partition id to table id in DAG request #14745

Merged
merged 2 commits into from
Feb 14, 2020
Merged

planner: set the partition id to table id in DAG request #14745

merged 2 commits into from
Feb 14, 2020

Conversation

lzmhhh123
Copy link
Contributor

What problem does this PR solve?

When the tablescan and indexscan convert to dag request, the field TableID is always set as tableinfo.ID, it doesn't matter for TiKV. Because TiKV scans key ranges. But it's not always correct for TiFlash(schemaful). When approaching to the partition table, it should be partitionID.

What is changed and how it works?

Set the TableID as physicalTableID when it's a partition table.

Check List

Tests

  • Unit test
  • Integration test

Code changes

  • Has exported function/method change

@lzmhhh123 lzmhhh123 added type/bugfix This PR fixes a bug. sig/planner SIG: Planner labels Feb 12, 2020
@lzmhhh123 lzmhhh123 requested a review from a team as a code owner February 12, 2020 07:06
@ghost ghost requested review from eurekaka and francis0407 and removed request for a team February 12, 2020 07:06
@lzmhhh123
Copy link
Contributor Author

/rebuild

@lzmhhh123
Copy link
Contributor Author

/run-all-tests

@Rustin170506
Copy link
Member

@sre-bot /run-all-tests

@lzmhhh123
Copy link
Contributor Author

/rebuild
/run-all-tests

crazycs520
crazycs520 reviewed Feb 14, 2020
View reviewed changes
Copy link
Contributor

@crazycs520 crazycs520 left a comment

Choose a reason for hiding this comment

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

LGTM

zanmato1984
zanmato1984 approved these changes Feb 14, 2020
View reviewed changes
Copy link
Contributor

@zanmato1984 zanmato1984 left a comment

Choose a reason for hiding this comment

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

LFGTM

@lzmhhh123
Copy link
Contributor Author

/merge

@lzmhhh123 lzmhhh123 added the status/can-merge Indicates a PR has been approved by a committer. label Feb 14, 2020
@sre-bot
Copy link
Contributor

sre-bot commented Feb 14, 2020

/run-all-tests

@sre-bot
Copy link
Contributor

sre-bot commented Feb 14, 2020

@lzmhhh123 merge failed.

@lzmhhh123
Copy link
Contributor Author

/run-unit-test

@lzmhhh123 lzmhhh123 merged commit 36d2dbb into pingcap:master Feb 14, 2020
@lzmhhh123 lzmhhh123 deleted the bug-fix/wrong_dag_table_id_for_patition branch February 14, 2020 07:27
lzmhhh123 added a commit to lzmhhh123/tidb that referenced this pull request Feb 14, 2020
@lzmhhh123
planner: set the partition id to table id in DAG request (pingcap#14745)
4bca8a2
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@crazycs520 crazycs520 crazycs520 left review comments

@zanmato1984 zanmato1984 zanmato1984 approved these changes

@eurekaka eurekaka Awaiting requested review from eurekaka

@francis0407 francis0407 Awaiting requested review from francis0407

Assignees
No one assigned
Labels
sig/planner SIG: Planner status/can-merge Indicates a PR has been approved by a committer. type/bugfix This PR fixes a bug.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@lzmhhh123 @Rustin170506 @sre-bot @zanmato1984 @crazycs520