*: index refactor for table partition #7062

tiancaiamao · 2018-07-16T08:50:14Z

What have you changed? (mandatory)

The prefix key in index is changed to partition prefix, so AddRecord for index would work fine
executor build will replace table ID with partition ID
Move some code from PartitionedTable.AddRecord to newPartitionedTable
A tiny adjust for 'rule_partition_processor.go'

What is the type of the changes? (mandatory)

Improvement (non-breaking change which is an improvement to an existing feature)

How has this PR been tested? (mandatory)

unit test
The test case is still very naive, more testes will be add when I finish admin check table in the following PRs

PTAL @zimulala @winkyao @shenli

coocood · 2018-07-16T13:06:23Z

/run-all-tests

coocood · 2018-07-16T13:06:27Z

LGTM

tiancaiamao · 2018-07-16T15:21:45Z

/run-integration-ddl-test tidb-test=pr/587

crazycs520

LGTM

zhexuany

LGTM

zhexuany · 2018-07-17T02:34:21Z

table/tables/index.go

 	idxInfo *model.IndexInfo
 	prefix  kv.Key
 }

 // NewIndex builds a new Index object.
-func NewIndex(tableInfo *model.TableInfo, indexInfo *model.IndexInfo) table.Index {
+func NewIndex(partitionID int64, indexInfo *model.IndexInfo) table.Index {


I have a problem with the naming of partitionID. Is it always being paritonID? Is it possible being table ID?

Agree. Please comment it.

It may be partition ID or table ID, depends on whether the table is a partitioned table or not.

If that is the case, then partitionID is not a good choice. It would be nice if you could change partitionID to ID and add some comments like "ID could be partitionID or table ID".

zhexuany

reset LGTM

zz-jason · 2018-07-17T02:37:07Z

plan/physical_plans.go

@@ -334,6 +338,11 @@ type PhysicalUnionScan struct {
 	Conditions []expression.Expression
 }

+// IsPartition returns true and partition ID if it works on a partition.
+func (p *PhysicalIndexScan) IsPartition() (bool, int64) {


How about s/IsPartition/GetPartitionID/

I think IsPartition maybe better.

succ, _ := IsPartition() if succ { doSomething() }

It's also flexible when we need the partition id:

if succ, pid := IsPartition(); succ { doSomthingWith(pid) }

GetPartitionID looks unnatural if we define:

GetPartitionID(pid int64, bool) // wow, it maybe not a partition

If we use partition id == 0 to represent it's not a partition:

GetPartitionID(pid int64)

This code introduce an implicit assumption with the caller.

zz-jason · 2018-07-17T02:41:47Z

plan/rule_partition_processor.go

@@ -105,6 +105,12 @@ func (s *partitionProcessor) prune(ds *DataSource) (LogicalPlan, error) {
 		newDataSource.baseLogicalPlan = newBaseLogicalPlan(ds.context(), TypeTableScan, &newDataSource)
 		newDataSource.isPartition = true
 		newDataSource.partitionID = pi.Definitions[i].ID
+		// TODO: Find a better way to handle here.


I think we can remove this TODO

Any suggestion?

I think if we haven't find a way to optimize the old logic, we don't need a TODO.

zz-jason · 2018-07-17T02:43:38Z

table/tables/index.go

 	idxInfo *model.IndexInfo
 	prefix  kv.Key
 }

 // NewIndex builds a new Index object.
-func NewIndex(tableInfo *model.TableInfo, indexInfo *model.IndexInfo) table.Index {
+func NewIndex(partitionID int64, indexInfo *model.IndexInfo) table.Index {


Does the first parameter always the partition id? Can it be the table id?

shenli · 2018-07-17T02:40:49Z

table/tables/index.go

 	idxInfo *model.IndexInfo
 	prefix  kv.Key
 }

 // NewIndex builds a new Index object.
-func NewIndex(tableInfo *model.TableInfo, indexInfo *model.IndexInfo) table.Index {
+func NewIndex(partitionID int64, indexInfo *model.IndexInfo) table.Index {


Agree. Please comment it.

shenli · 2018-07-17T02:42:24Z

table/tables/partition.go

-	ret.recordPrefix = tablecodec.GenTableRecordPrefix(pid)
-	ret.indexPrefix = tablecodec.GenTableIndexPrefix(pid)
-	return &ret
+	return t.partitions[pid]


Why change here?

If GetPartition function create a Partition and Index everytime it's called, it's bad for performance.
So I store those information when PartitionedTable is created, and just get it here.

zimulala · 2018-07-17T02:55:43Z

table/tables/tables.go

-
-		idx := NewIndex(tblInfo, idxInfo)
-		t.indices = append(t.indices, idx)
+	if err := initTableCommon(&t.tableCommon, tblInfo, tblInfo.ID, columns, alloc); err != nil {


In the case of partitions, it seems that this function repeatedly builds the index. The first time is here, the other time is when building the index(https://github.com/pingcap/tidb/pull/7062/files#diff-c709fb799ac0eac6d7ea40b2278c90d7R69).

tiancaiamao · 2018-07-17T04:18:41Z

PTAL @zimulala @zz-jason

zhexuany

LGTM

zz-jason · 2018-07-17T05:00:34Z

plan/rule_partition_processor.go

@@ -105,6 +105,12 @@ func (s *partitionProcessor) prune(ds *DataSource) (LogicalPlan, error) {
 		newDataSource.baseLogicalPlan = newBaseLogicalPlan(ds.context(), TypeTableScan, &newDataSource)
 		newDataSource.isPartition = true
 		newDataSource.partitionID = pi.Definitions[i].ID
+		// Find a better way to handle here.
+		// There are so many place that refer to the old DataSource including the schema


The comment is confusion, I think you should directly explain why the newDataSource.id is set to the ds.id. It's not because we hold the reference of the Datasource, it's because we use the id as the FromID of the Columns in the Expression of parent.

"use the id as the FromID of the Columns in the Expression of parent"
This is a kind of reference!

Yes, I mean, we should statement the comment in a straightforward way.

still need some discuss

zz-jason · 2018-07-17T05:49:26Z

LGTM

tiancaiamao added 2 commits July 16, 2018 16:34

*: index refactor for table partition

2fe11e2

Merge branch 'master' into index-refactor

45af171

coocood added the status/LGT1 Indicates that a PR has LGTM 1. label Jul 16, 2018

tiancaiamao added the status/all tests passed label Jul 16, 2018

crazycs520 reviewed Jul 16, 2018

View reviewed changes

shenli added the component/DDL-need-LGT3 label Jul 16, 2018

zhexuany reviewed Jul 17, 2018

View reviewed changes

zhexuany previously approved these changes Jul 17, 2018

View reviewed changes

zz-jason reviewed Jul 17, 2018

View reviewed changes

shenli reviewed Jul 17, 2018

View reviewed changes

zimulala reviewed Jul 17, 2018

View reviewed changes

tiancaiamao added 2 commits July 17, 2018 11:28

address comment

8b952bf

address comment

225dae7

coocood added status/LGT3 The PR has already had 3 LGTM. status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. status/LGT3 The PR has already had 3 LGTM. labels Jul 17, 2018

zhexuany previously approved these changes Jul 17, 2018

View reviewed changes

zz-jason reviewed Jul 17, 2018

View reviewed changes

tiancaiamao added 2 commits July 17, 2018 13:42

address comment

a076e4f

Merge branch 'master' into index-refactor

5484c0c

tiancaiamao force-pushed the index-refactor branch from a34e8dd to 5484c0c Compare July 17, 2018 05:44

zz-jason approved these changes Jul 17, 2018

View reviewed changes

coocood merged commit 44e34bd into pingcap:master Jul 17, 2018

coocood added status/LGT3 The PR has already had 3 LGTM. and removed status/LGT2 Indicates that a PR has LGTM 2. labels Jul 17, 2018

tiancaiamao deleted the index-refactor branch July 17, 2018 06:09

tiancaiamao added this to In Progress in Table Partitioning via automation Aug 26, 2018

tiancaiamao moved this from In Progress to Done in Table Partitioning Aug 26, 2018

you06 added the sig/sql-infra SIG: SQL Infra label Mar 4, 2020

zz-jason moved this from Issue: Done to PR: Donw in Table Partitioning Apr 21, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

*: index refactor for table partition #7062

*: index refactor for table partition #7062

tiancaiamao commented Jul 16, 2018

coocood commented Jul 16, 2018

coocood commented Jul 16, 2018

tiancaiamao commented Jul 16, 2018

crazycs520 left a comment

zhexuany left a comment

zhexuany Jul 17, 2018

shenli Jul 17, 2018

tiancaiamao Jul 17, 2018

zhexuany Jul 17, 2018

zhexuany left a comment

zz-jason Jul 17, 2018

tiancaiamao Jul 17, 2018

zz-jason Jul 17, 2018

tiancaiamao Jul 17, 2018

zz-jason Jul 17, 2018

zz-jason Jul 17, 2018

shenli Jul 17, 2018

shenli Jul 17, 2018

tiancaiamao Jul 17, 2018 •

edited

zimulala Jul 17, 2018 •

edited

tiancaiamao commented Jul 17, 2018

zhexuany left a comment

zz-jason Jul 17, 2018

tiancaiamao Jul 17, 2018

zz-jason Jul 17, 2018

zz-jason commented Jul 17, 2018

*: index refactor for table partition #7062

*: index refactor for table partition #7062

Conversation

tiancaiamao commented Jul 16, 2018

What have you changed? (mandatory)

What is the type of the changes? (mandatory)

How has this PR been tested? (mandatory)

coocood commented Jul 16, 2018

coocood commented Jul 16, 2018

tiancaiamao commented Jul 16, 2018

crazycs520 left a comment

Choose a reason for hiding this comment

zhexuany left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhexuany left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tiancaiamao Jul 17, 2018 • edited

Choose a reason for hiding this comment

zimulala Jul 17, 2018 • edited

Choose a reason for hiding this comment

tiancaiamao commented Jul 17, 2018

zhexuany left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zz-jason commented Jul 17, 2018

tiancaiamao Jul 17, 2018 •

edited

zimulala Jul 17, 2018 •

edited