Navigation Menu

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

*: fix union scan for partitioned table #8871

Merged
merged 9 commits into from Feb 28, 2019

Conversation

tiancaiamao
Copy link
Contributor

@tiancaiamao tiancaiamao commented Dec 29, 2018

What problem does this PR solve?

MySQL [test]> show create table t1;
+-------+---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------+
| Table | Create Table                                                                                                                                                                                                                                                                                                                          |
+-------+---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------+
| t1    | CREATE TABLE `t1` (
  `c1` smallint(6) NOT NULL,
  `c2` char(5) DEFAULT NULL
) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_bin
PARTITION BY RANGE ( `c1` ) (
  PARTITION p0 VALUES LESS THAN (10),
  PARTITION p1 VALUES LESS THAN (20),
  PARTITION p2 VALUES LESS THAN (30),
  PARTITION p3 VALUES LESS THAN (MAXVALUE)
) |
+-------+---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------+
1 row in set (0.00 sec)

MySQL [test]> begin;
Query OK, 0 rows affected (0.00 sec)

MySQL [test]> insert into t1 values (1,1);
Query OK, 1 row affected (0.00 sec)

MySQL [test]> select * from t1;
Empty set (0.01 sec)

What is changed and how it works?

How does union scan works for partitioned table:

  1. In rule_partition_processor.go, the rewrite rule changes
UnionScan->DataSource

to

UnionAll{UnionScan->Partition1, UnionScan->Partition2 ...}

Note that UnionScan is now the parent of every Partition, rather than the previous

UnionScan->UnionAll{Partition1, Partition2 ...}
  1. UnionScan->Partition1 transformed to UnionScanExec+three reader, the reader read data from partition, and UnionExec get the incremental buffer changes on partition, not table

  2. To get the incremental buffer changes on partition:

  • The id should be physicalID, instead of tableID
  • Dirty table is organised using partition, i.e, DirtyDB = map(physicalID -> changes)
  1. In the three kinds of reader(TableReader,IndexReader,IndexLookup) struct, table is also changed to partition (PhysicalTable)

  2. The function signature StmtAddDirtyTableOP should be

StmtAddDirtyTableOP(op int, physicalID int64, handle int64, row []types.Datum)

instead of

StmtAddDirtyTableOP(op int, tid int64, handle int64, row []types.Datum)

This change is Reviewable

1. dirty table is table level, so AddRecord should use tableID instead of physicalTableID
2. UnionScan should not be removed for the Union generated by table partition
@tiancaiamao tiancaiamao added type/bug-fix This PR fixes a bug. sig/execution SIG execution labels Dec 29, 2018
@tiancaiamao tiancaiamao added this to In Progress in Table Partitioning via automation Dec 29, 2018
@tiancaiamao
Copy link
Contributor Author

PTAL @lamxTyler @zz-jason

@tiancaiamao
Copy link
Contributor Author

PTAL @zz-jason @lamxTyler

executor/builder.go Outdated Show resolved Hide resolved
planner/core/physical_plans.go Outdated Show resolved Hide resolved
executor/builder.go Outdated Show resolved Hide resolved
@zz-jason
Copy link
Member

@tiancaiamao any update?

@codecov-io
Copy link

Codecov Report

Merging #8871 into master will increase coverage by 0.01%.
The diff coverage is 83.67%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #8871      +/-   ##
==========================================
+ Coverage    67.4%   67.42%   +0.01%     
==========================================
  Files         373      373              
  Lines       78544    78593      +49     
==========================================
+ Hits        52946    52990      +44     
- Misses      20838    20844       +6     
+ Partials     4760     4759       -1
Impacted Files Coverage Δ
table/tables/tables.go 52.41% <0%> (ø) ⬆️
executor/builder.go 83.91% <100%> (+0.11%) ⬆️
executor/union_scan.go 78.61% <25%> (-0.13%) ⬇️
planner/core/rule_partition_processor.go 74.19% <75%> (+0.16%) ⬆️
util/systimemon/systime_mon.go 80% <0%> (-20%) ⬇️
ddl/ddl_worker.go 87.65% <0%> (-0.65%) ⬇️
sessionctx/variable/varsutil.go 25.45% <0%> (-0.48%) ⬇️
planner/core/exhaust_physical_plans.go 93.81% <0%> (-0.32%) ⬇️
util/admin/admin.go 67.07% <0%> (-0.25%) ⬇️
sessionctx/variable/session.go 30.25% <0%> (-0.2%) ⬇️
... and 8 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 839772b...3e3f503. Read the comment docs.

@tiancaiamao
Copy link
Contributor Author

I'll do a tiny refactor to remove the physicalID from three reader after this PR.

Copy link
Member

@zz-jason zz-jason left a comment

Choose a reason for hiding this comment

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

LGTM

@zz-jason zz-jason added the status/LGT1 Indicates that a PR has LGTM 1. label Feb 28, 2019
@zz-jason
Copy link
Member

/run-all-tests

@zz-jason zz-jason requested review from alivxxx and winoros and removed request for alivxxx February 28, 2019 04:33
@tiancaiamao
Copy link
Contributor Author

/run-all-tests

@tiancaiamao
Copy link
Contributor Author

/run-integration-compatibility-test

@tiancaiamao
Copy link
Contributor Author

PTAL @lamxTyler

Copy link
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. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Feb 28, 2019
@tiancaiamao tiancaiamao merged commit 8b84b94 into pingcap:master Feb 28, 2019
Table Partitioning automation moved this from In Progress to Done Feb 28, 2019
@tiancaiamao tiancaiamao deleted the partition-union-scan branch February 28, 2019 05:43
@crazycs520
Copy link
Contributor

@tiancaiamao Need cherry pick to v2.1.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
sig/execution SIG execution status/LGT2 Indicates that a PR has LGTM 2. type/bug-fix This PR fixes a bug.
Projects
No open projects
Table Partitioning
  
PR: Done
Development

Successfully merging this pull request may close these issues.

None yet

6 participants