Skip to content

planner: a parition table's column UniqueID may mismatch or collide with planner's#13490

Merged
sre-bot merged 5 commits intopingcap:masterfrom
SunRunAway:parititiontable
Nov 19, 2019
Merged

planner: a parition table's column UniqueID may mismatch or collide with planner's#13490
sre-bot merged 5 commits intopingcap:masterfrom
SunRunAway:parititiontable

Conversation

@SunRunAway
Copy link
Contributor

@SunRunAway SunRunAway commented Nov 15, 2019

What problem does this PR solve?

Fix #13491
A partition table's column UniqueID is generated by itself using its own SessionContext and does not associate with planner's.
If mismatched, partition pruning does not take any effect.
If collided, partition pruning may prune extra partitions and lead wrong results.

What is changed and how it works?

Use planner's schema to build partition expression during optimization.

Check List

Tests

  • Unit test

Code changes

  • Has exported function/method change

Side effects

  • None

Related changes

  • Need to cherry-pick to the release branch

Release note

  • Fix partition pruning when partition table working with union.

@SunRunAway SunRunAway added type/bugfix This PR fixes a bug. sig/planner SIG: Planner labels Nov 15, 2019
@codecov
Copy link

codecov bot commented Nov 15, 2019

Codecov Report

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

@@             Coverage Diff             @@
##             master     #13490   +/-   ##
===========================================
  Coverage   80.2663%   80.2663%           
===========================================
  Files           472        472           
  Lines        115098     115098           
===========================================
  Hits          92385      92385           
  Misses        15465      15465           
  Partials       7248       7248

@SunRunAway
Copy link
Contributor Author

/run-all-tests

@SunRunAway SunRunAway marked this pull request as ready for review November 15, 2019 07:43
@SunRunAway
Copy link
Contributor Author

/run-all-tests

@winoros
Copy link
Member

winoros commented Nov 15, 2019

When do pruning, it's used ID instead of UniqueID?

@SunRunAway
Copy link
Contributor Author

SunRunAway commented Nov 15, 2019

When do pruning, it's used ID instead of UniqueID?

Pruning calls propagateConstantEQ to propagate constant equal, while propagateConstantEQ uses UniqueID to do column substitution.

@tiancaiamao tiancaiamao changed the title planner: a parition table's column UniqueID may mismatch or collide w… planner: a parition table's column UniqueID may mismatch or collide with planner's Nov 15, 2019
case model.PartitionTypeHash:
return generateHashPartitionExpr(pi, columns, names)
}
panic("cannot reach here")
Copy link
Contributor

Choose a reason for hiding this comment

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

create table t partition by list ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

TiDB currently supports hash partition and range partiion.

mysql> CREATE TABLE employees (
    ->     id INT NOT NULL,
    ->     fname VARCHAR(30),
    ->     lname VARCHAR(30),
    ->     hired DATE NOT NULL DEFAULT '1970-01-01',
    ->     separated DATE NOT NULL DEFAULT '9999-12-31',
    ->     job_code INT,
    ->     store_id INT
    -> )
    -> PARTITION BY LIST(store_id) (
    ->     PARTITION pNorth VALUES IN (3,5,6,9,17),
    ->     PARTITION pEast VALUES IN (1,2,10,11,19,20),
    ->     PARTITION pWest VALUES IN (4,12,13,14,18),
    ->     PARTITION pCentral VALUES IN (7,8,15,16)
    -> );
Query OK, 0 rows affected, 1 warning (0.16 sec)

mysql> show warnings;
+---------+------+---------------------------------------------------+
| Level   | Code | Message                                           |
+---------+------+---------------------------------------------------+
| Warning | 8200 | Unsupported partition type, treat as normal table |
+---------+------+---------------------------------------------------+
1 row in set (0.00 sec)

Copy link
Contributor

@tiancaiamao tiancaiamao Nov 19, 2019

Choose a reason for hiding this comment

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

And the code of the normal table doesn't run to here, so this panic is reasonable ...

func (t *partitionedTable) PartitionExpr() *PartitionExpr {
return t.partitionExpr
func (t *partitionedTable) PartitionExpr(columns []*expression.Column, names types.NameSlice) (*PartitionExpr, error) {
return newPartitionExprBySchema(t.meta, columns, names)
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there any way to avoid this allocation every time?

Copy link
Contributor Author

@SunRunAway SunRunAway Nov 19, 2019

Choose a reason for hiding this comment

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

Maybe we can traverse the Expression, find all columns and rewrite them. We can do it in the future.

@SunRunAway SunRunAway requested a review from lzmhhh123 November 19, 2019 07:33
@tiancaiamao
Copy link
Contributor

LGTM.
This change brings potential performance regression, but fix bug has a higher priority.

@tiancaiamao tiancaiamao added the status/LGT1 Indicates that a PR has LGTM 1. label Nov 19, 2019
lzmhhh123
lzmhhh123 previously approved these changes Nov 19, 2019
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 status/can-merge Indicates a PR has been approved by a committer. status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Nov 19, 2019
@sre-bot
Copy link
Contributor

sre-bot commented Nov 19, 2019

/run-all-tests

@sre-bot sre-bot merged commit d902895 into pingcap:master Nov 19, 2019
@SunRunAway SunRunAway deleted the parititiontable branch November 19, 2019 08:09
@SunRunAway
Copy link
Contributor Author

/run-cherry-picker

@sre-bot
Copy link
Contributor

sre-bot commented Nov 19, 2019

cherry pick to release-2.1 failed

@sre-bot
Copy link
Contributor

sre-bot commented Nov 19, 2019

cherry pick to release-3.0 failed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

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. type/bugfix This PR fixes a bug.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Partition pruning does not work with union.

5 participants