planner: support push part of order property down to the partition table by winoros · Pull Request #36108 · pingcap/tidb

winoros · 2022-07-11T15:18:05Z

What problem does this PR solve?

Issue Number: ref #26166 (a quick solution)

Problem Summary:

The partition table shields the partition information in the execution phase. So we cannot convert the order by index limit x to LIMIT with the order property pushed to the index kv.

What is changed and how it works?

This time we don't change the codes of the execution. So we cannot push the full order property down to the index kv.
We just change the plan from TopN(TiDB)->Reader(TiDB)->TopN(TiKV)->ScanKV(TiKV, no order) to TopN(TiDB)->Reader(TiDB)->Limit(TiKV)->ScanKV(TiKV, in order).

Check List

Tests

Unit test
Integration test
Manual test (add detailed scripts or steps below)
No code

Side effects

Performance regression: Consumes more CPU
Performance regression: Consumes more Memory
Breaking backward compatibility

Documentation

Release note

Please refer to Release Notes Language Style Guide to write a quality release note.

push part of the order property down to the TiKV.

ti-chi-bot · 2022-07-11T15:18:09Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

AilinKid
fixdb

To complete the pull request process, please ask the reviewers in the list to review by filling /cc @reviewer in the comment.
After your PR has acquired the required number of LGTMs, you can assign this pull request to the committer in the list by filling /assign @committer in the comment to help you merge this pull request.

The full list of commands accepted by this bot can be found here.

Details

Reviewer can indicate their review by submitting an approval review.
Reviewer can cancel approval by submitting a request changes review.

time-and-fate · 2022-07-13T07:52:08Z

planner/core/task.go

+		}
+
+		// If we reach here, the index matches the order property of the top-n, we could push a limit down.
+		copTsk.keepOrder = true


It seems like we don't need to set copTask.keepOrder to true.

for partitions maybe

func buildIndexLookUpTask(ctx sessionctx.Context, t *copTask) *rootTask { newTask := &rootTask{cst: t.cst} p := PhysicalIndexLookUpReader{ tablePlan: t.tablePlan, indexPlan: t.indexPlan, ExtraHandleCol: t.extraHandleCol, CommonHandleCols: t.commonHandleCols, expectedCnt: t.expectCnt, keepOrder: t.keepOrder, }.Init(ctx, t.tablePlan.SelectBlockOffset())

This func uses it.

Oh actually we don't need the keep order, remove it.

time-and-fate · 2022-07-13T07:52:25Z

planner/core/task.go

+				return nil, false
+			}
+		}
+		copTsk.keepOrder = true


time-and-fate · 2022-07-13T07:57:10Z

planner/core/task.go

+		}
+		tblScan = finalTblScanPlan.(*PhysicalTableScan)
+	}
+	if !copTsk.indexPlanFinished {


We didn't consider that idxScan might be nil in the logic below.

When indexPlanFinished is not true, there should be a index scan to be finished later.

planner/core/task.go

time-and-fate · 2022-07-13T09:44:29Z

planner/core/task.go

+// pushTopNDownToDynamicPartition is a temp solution for partition table. It actually does the same thing as DataSource's isMatchProp.
+func (p *PhysicalTopN) pushTopNDownToDynamicPartition(copTsk *copTask) (task, bool) {


I think we need more detailed comments since it's a tricky way to modify the plan.
I think we should clarify what the resulting plan would be like and why we have to use this method as a temporary solution.

time-and-fate · 2022-07-13T10:08:14Z

planner/core/task.go

+		if tblScan.HandleCols.IsInt() {
+			pk := tblScan.HandleCols.GetCol(0)
+			if len(colsProp.SortItems) != 1 || !colsProp.SortItems[0].Col.Equal(p.SCtx(), pk) {


This is different from (*DataSource).isMatchProp. According to isMatchProp, tiflash doesn't support desc scan.

This change only affects tikv part.

time-and-fate · 2022-07-13T11:05:46Z

planner/core/task.go

+		if copTsk.tablePlan != nil && !idxScan.Table.IsCommonHandle && copTsk.extraHandleCol == nil {
+			// The clustered index would always add handle, so we don't need to consider that case.
+			copTsk.extraHandleCol = &expression.Column{
+				RetType:  types.NewFieldType(mysql.TypeLonglong),
+				ID:       model.ExtraHandleID,
+				UniqueID: idxScan.ctx.GetSessionVars().AllocPlanColumnID(),
+			}
+			tblScan.Schema().Append(copTsk.extraHandleCol)
+			tblScan.Columns = append(tblScan.Columns, model.NewExtraHandleColInfo())
+			copTsk.needExtraProj = true
+			copTsk.originSchema = idxScan.dataSourceSchema
+		}


We missed the case that the int primary key is not _tidb_rowid but a column specified by the user.
I think we can modify the (*PhysicalTableScan).appendExtraHandleCol a bit and reuse that.

sre-bot · 2022-07-13T12:42:31Z

Code Coverage Details: https://codecov.io/github/pingcap/tidb/commit/27324cbb5e663b42207d456b6b59dca30ab37351

AilinKid · 2022-07-18T09:31:28Z

planner/core/task.go

+func (p *PhysicalTopN) pushTopNDownToDynamicPartition(copTsk *copTask) (task, bool) {
+	var err error
+	copTsk = copTsk.copy().(*copTask)
+	if err != nil {


the err isn't assigned anything before

planner/core/task.go

AilinKid · 2022-07-18T09:52:01Z

planner/core/task.go

+		}
+
+		// If we reach here, the index matches the order property of the top-n, we could push a limit down.
+		copTsk.keepOrder = true


for partitions maybe

AilinKid · 2022-07-18T09:56:55Z

planner/core/task.go

+				if len(constColsByCond) == 0 || idxPos > len(constColsByCond) || !constColsByCond[idxPos] {
+					found = false
+					break
+				}


missed set found = true at the end? (indicating that we are sure that this col is constant, then just go to outer's continued column)

In:
order by (a,b,c), once index(a,c) exists and b=1, we can still access this path right?
reader -> limit -> selection -> indexScan

order by (a,c), once index(a,b,c) exists and b=1 is what actually it already does.

…-topn

AilinKid · 2022-11-28T05:16:48Z

kv/kv.go

+				return true
+			}
+			if len(j) == 0 {
+				return false


why the len(j) = 0 has different logic with the above？

AilinKid · 2022-11-28T06:48:44Z

planner/core/partition_pruner_test.go

 	valid := false
 	for i, tt := range input {
+		if tt == "select * from t1 where (id = 1 and a = 1) or a is null" {
+			fmt.Println("1")


remove the debug info

AilinKid · 2022-11-28T07:21:53Z

planner/core/task.go

+			return nil, false
+		}
+		finalTblScanPlan := copTsk.tablePlan
+		if len(finalTblScanPlan.Children()) > 0 {


s/ if / for?

AilinKid · 2022-11-28T07:34:39Z

planner/core/task.go

+		}
+		tblScan.Desc = isDesc
+		childProfile := copTsk.plan().statsInfo()
+		newCount := p.Offset + p.Count


should we consider there's a selection on the table side? the count here may won't pass the selection

In such case, the plan should be Limit->Selection->TableScan. The selection should not matter

get！make sense

AilinKid

Rest LGTM

AilinKid · 2022-11-29T03:50:19Z

/merge

ti-chi-bot · 2022-11-29T03:50:22Z

This pull request has been accepted and is ready to merge.

Details

Commit hash: 953273f

fixdb

+1

sre-bot · 2022-11-29T06:28:18Z

TiDB MergeCI notify

🔴 Bad News! New failing [1] after this pr merged.
These new failed integration tests seem to be caused by the current PR, please try to fix these new failed integration tests, thanks!

CI Name	Result	Duration	Compare with Parent commit
idc-jenkins-ci-tidb/integration-common-test	🟥 failed 1, success 16, total 17	30 min	New failing
idc-jenkins-ci-tidb/sqllogic-test-2	✅ all 28 tests passed	6 min 28 sec	Fixed
idc-jenkins-ci/integration-cdc-test	🟢 all 40 tests passed	20 min	Existing passed
idc-jenkins-ci-tidb/common-test	🟢 all 11 tests passed	11 min	Existing passed
idc-jenkins-ci-tidb/integration-ddl-test	🟢 all 6 tests passed	6 min 19 sec	Existing passed
idc-jenkins-ci-tidb/mybatis-test	🟢 all 1 tests passed	6 min 9 sec	Existing passed
idc-jenkins-ci-tidb/sqllogic-test-1	🟢 all 26 tests passed	5 min 50 sec	Existing passed
idc-jenkins-ci-tidb/tics-test	🟢 all 1 tests passed	5 min 30 sec	Existing passed
idc-jenkins-ci-tidb/integration-compatibility-test	🟢 all 1 tests passed	3 min 28 sec	Existing passed
idc-jenkins-ci-tidb/plugin-test	🟢 build success, plugin test success	4min	Existing passed

…nfigurable (pingcap#37420) ref pingcap#36108

winoros added 3 commits July 7, 2022 17:37

planner: support push part of order property down to the partition table

7c9e327

planner: support push part of order property to partition table

3993e00

fix fmt

3106166

ti-chi-bot added do-not-merge/needs-linked-issue release-note Denotes a PR that will be considered when it comes time to generate release notes. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jul 11, 2022

winoros added the sig/planner SIG: Planner label Jul 11, 2022

ti-chi-bot removed the do-not-merge/needs-linked-issue label Jul 11, 2022

winoros requested review from AilinKid and time-and-fate July 11, 2022 15:21

time-and-fate reviewed Jul 13, 2022

View reviewed changes

planner/core/task.go Outdated Show resolved Hide resolved

time-and-fate reviewed Jul 13, 2022

View reviewed changes

add tests

2c08dde

hawkingrei and others added 2 commits July 16, 2022 11:27

Merge branch 'master' into part-table-topn

39bca97

Merge branch 'master' into part-table-topn

896a4e0

AilinKid reviewed Jul 18, 2022

View reviewed changes

winoros added 3 commits July 19, 2022 01:08

address comments

9f57b9c

Merge remote-tracking branch 'yiding/part-table-topn' into part-table…

6ccdc30

…-topn

add tests

423f599

ti-chi-bot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jul 19, 2022

winoros added 2 commits July 20, 2022 17:23

make sure that the request is sent first by partition then by region

f940c71

fix make check

2bc49ff

winoros requested a review from AilinKid July 20, 2022 09:32

ti-chi-bot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Nov 23, 2022

winoros added 2 commits November 26, 2022 22:09

pass static check

09fb930

fix tiny error

e1b4211

winoros force-pushed the part-table-topn branch from 3dfbb1f to 7778f01 Compare November 26, 2022 16:09

Merge branch 'master' into part-table-topn

e285406

winoros force-pushed the part-table-topn branch from 7778f01 to e285406 Compare November 26, 2022 16:59

AilinKid reviewed Nov 28, 2022

View reviewed changes

kv/kv.go

return true

}

if len(j) == 0 {

return false

Copy link

Contributor

AilinKid Nov 28, 2022

Choose a reason for hiding this comment

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

why the len(j) = 0 has different logic with the above？

AilinKid reviewed Nov 28, 2022

View reviewed changes

address comments

953273f

AilinKid approved these changes Nov 29, 2022

View reviewed changes

ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Nov 29, 2022

ti-chi-bot added 2 commits November 29, 2022 11:50

Merge branch 'master' into part-table-topn

6b0699f

Merge branch 'master' into part-table-topn

2794d3e

fixdb approved these changes Nov 29, 2022

View reviewed changes

ti-chi-bot added 2 commits November 29, 2022 13:10

Merge branch 'master' into part-table-topn

1d38fcc

Merge branch 'master' into part-table-topn

743265c

ti-chi-bot merged commit f9a6e47 into pingcap:master Nov 29, 2022

winoros deleted the part-table-topn branch November 29, 2022 08:34

winoros added a commit to winoros/tidb that referenced this pull request Nov 30, 2022

*: push part of order prop to partitiontable and make auto-analyze co…

f6e811d

…nfigurable (pingcap#37420) ref pingcap#36108

winoros mentioned this pull request Nov 30, 2022

*: push part of order prop to partitiontable and make auto-analyze configurable #39501

Merged

12 tasks

you06 mentioned this pull request Dec 22, 2022

store/copr: move row hint into key range #40105

Merged

12 tasks

tiancaiamao mentioned this pull request Jan 20, 2023

coprocessor paging Limit1 exec return enough data but not stop #40741

Closed

time-and-fate mentioned this pull request Feb 2, 2023

Adjust the estimated row count when push Limit and keep order for partitioned table #40986

Closed

tangenta mentioned this pull request Jun 6, 2023

lightning/backend/local: fix buildIndexDupTasks #44442

Merged

12 tasks

ti-chi-bot mentioned this pull request Oct 21, 2023

lightning/backend/local: fix buildIndexDupTasks (#44442) #47894

Merged

12 tasks

		// pushTopNDownToDynamicPartition is a temp solution for partition table. It actually does the same thing as DataSource's isMatchProp.
		func (p PhysicalTopN) pushTopNDownToDynamicPartition(copTsk copTask) (task, bool) {

Conversation

winoros commented Jul 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What problem does this PR solve?

What is changed and how it works?

Check List

Release note

Uh oh!

ti-chi-bot commented Jul 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sre-bot commented Jul 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AilinKid left a comment

Choose a reason for hiding this comment

Uh oh!

AilinKid commented Nov 29, 2022

Uh oh!

ti-chi-bot commented Nov 29, 2022

Uh oh!

fixdb left a comment

Choose a reason for hiding this comment

Uh oh!

sre-bot commented Nov 29, 2022

TiDB MergeCI notify

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

winoros commented Jul 11, 2022 •

edited

Loading

ti-chi-bot commented Jul 11, 2022 •

edited

Loading

sre-bot commented Jul 13, 2022 •

edited

Loading