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: support specify table partition in optimize hint #17638

Merged

Conversation

lzmhhh123
Copy link
Contributor

What problem does this PR solve?

Issue Number: close #17113

Problem Summary: support specify table parition in optimize hint

What is changed and how it works?

What's Changed: support specify table parition in optimize hint

Related changes

  • PR to update pingcap/docs/pingcap/docs-cn: after this PR merge, we should update the docs.
  • Need to cherry-pick to the release branch

Check List

Tests

  • Unit test
  • Integration test

Side effects

  • Performance regression
    • Consumes more CPU
    • Consumes more MEM

Release note

  • support specify table partition in optimize hint

@lzmhhh123
Copy link
Contributor Author

/run-unit-test

@codecov
Copy link

codecov bot commented Jun 3, 2020

Codecov Report

Merging #17638 into master will increase coverage by 0.6366%.
The diff coverage is 79.1081%.

@@               Coverage Diff                @@
##             master     #17638        +/-   ##
================================================
+ Coverage   79.4228%   80.0594%   +0.6366%     
================================================
  Files           524        526         +2     
  Lines        142143     145994      +3851     
================================================
+ Hits         112894     116882      +3988     
+ Misses        20101      19969       -132     
+ Partials       9148       9143         -5     

@lzmhhh123 lzmhhh123 added this to the v4.0.1 milestone Jun 5, 2020
if hintInfo.ifPreferTiFlash(alias) {
if ds.preferStoreType != 0 {
if hintTbl := hintInfo.ifPreferTiFlash(alias); hintTbl != nil {
if ds.preferStoreType != 0 && ds.tableInfo.Partition == nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

Add a comment for why we do not raise an error when ds.tableInfo.Partition != nil

@@ -502,7 +503,8 @@ type DataSource struct {
// it is converted from statisticTable, and used for IO/network cost estimating.
TblColHists *statistics.HistColl
//preferStoreType means the DataSource is enforced to which storage.
preferStoreType int
preferStoreType int
preferStoreTypePartitions map[int][]model.CIStr
Copy link
Contributor

Choose a reason for hiding this comment

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

  1. preferPartitions is enough?
  2. Add a comment for why this is a map

case TiDBMergeJoin, HintSMJ, TiDBIndexNestedLoopJoin, HintINLJ, HintINLHJ, HintINLMJ, TiDBHashJoin, HintHJ:
if len(tableInfo.partitions) > 0 {
ctx.GetSessionVars().StmtCtx.AppendWarning(
errors.New(fmt.Sprintf("%s hint do not support specify partitions", hintName)))
Copy link
Contributor

Choose a reason for hiding this comment

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

  1. Do we need to invoke restore2StorageHint here?
  2. Do we need to extend restore2StorageHint?

Copy link
Contributor

@XuHuaiyu XuHuaiyu Jun 5, 2020

Choose a reason for hiding this comment

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

The error message can be refined to Optimizer Hint %s is inapplicable on specified partitions

Copy link
Contributor

@XuHuaiyu XuHuaiyu left a comment

Choose a reason for hiding this comment

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

Do we need to raise a warning for sql like if the partition is unknown in the hint:
select /*+ use_index(t partition(p_non_exist)) */ from t partition(p1,p2)

@lzmhhh123
Copy link
Contributor Author

Do we need to raise a warning for sql like if the partition is unknown in the hint:
select /*+ use_index(t partition(p_non_exist)) */ from t partition(p1,p2)

OK, I try to support it.

@lzmhhh123
Copy link
Contributor Author

/rebuild

if len(table.partitions) > 0 {
buffer.WriteString(" PARTITION(")
for j, partition := range table.partitions {
buffer.WriteString(partition.L)
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be moved to line 262

for _, p := range partitions {
if !partitionSet.Exist(p.L) {
ds.ctx.GetSessionVars().StmtCtx.AppendWarning(
errors.New(fmt.Sprintf("partition `%s` in hint can't hit any source", p.L)))
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we return the partition name back to checkHintsApplicable, and build a more comprehensible error message like Unknow partition %s in optimizer hint %s

@lzmhhh123
Copy link
Contributor Author

/run-unit-test

@bb7133 bb7133 modified the milestones: v4.0.1, v4.0.2 Jun 6, 2020
@tiancaiamao
Copy link
Contributor

LGTM

@tiancaiamao tiancaiamao added the status/LGT1 Indicates that a PR has LGTM 1. label Jun 18, 2020
@zz-jason zz-jason modified the milestones: v4.0.2, v4.0.3 Jun 19, 2020
@lzmhhh123 lzmhhh123 requested a review from a team as a code owner June 19, 2020 09:46
@zz-jason zz-jason modified the milestones: v4.0.3, v5.0-alpha Jun 19, 2020
@tiancaiamao
Copy link
Contributor

@XuHuaiyu

Copy link
Contributor

@XuHuaiyu XuHuaiyu left a comment

Choose a reason for hiding this comment

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

LGTM

@XuHuaiyu XuHuaiyu modified the milestones: v5.0-alpha, v4.0.3 Jun 22, 2020
@XuHuaiyu XuHuaiyu added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Jun 22, 2020
@XuHuaiyu
Copy link
Contributor

/merge

@ti-srebot
Copy link
Contributor

Sorry @XuHuaiyu, you don't have permission to trigger auto merge event on this branch. You are not a committer for the related sigs:planner(slack).

@XuHuaiyu
Copy link
Contributor

/run-all-tests

@lzmhhh123 lzmhhh123 merged commit 3e5db05 into pingcap:master Jun 22, 2020
@lzmhhh123 lzmhhh123 deleted the dev/support_partition_in_table_hints branch June 22, 2020 03:21
ti-srebot pushed a commit to ti-srebot/tidb that referenced this pull request Jun 22, 2020
@ti-srebot
Copy link
Contributor

cherry pick to release-4.0 in PR #18157

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
sig/planner SIG: Planner status/LGT2 Indicates that a PR has LGTM 2.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

semantic of /*+ read_from_storage(tiflash[t1 partition(p0)]) */ is confusing
6 participants