planner, executor: allow build root task type of expand operator and implement expand executor #54536

AilinKid · 2024-07-10T04:25:38Z

What problem does this PR solve?

Issue Number: close #42631

Problem Summary:

What changed and how does it work?

Check List

Tests

Unit test
Integration test
Manual test (add detailed scripts or steps below)
No need to test
- I checked and no code files have been changed.

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.

None

tiprow · 2024-07-10T04:25:56Z

Hi @AilinKid. Thanks for your PR.

PRs from untrusted users cannot be marked as trusted with /ok-to-test in this repo meaning untrusted PR authors can never trigger tests themselves. Collaborators can still trigger tests on the PR using /test all.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

codecov · 2024-07-10T04:54:21Z

Codecov Report

Attention: Patch coverage is 78.26087% with 30 lines in your changes missing coverage. Please review.

Project coverage is 56.8239%. Comparing base (5e872b9) to head (bb827e4).
Report is 77 commits behind head on master.

Additional details and impacted files

@@                Coverage Diff                @@
##             master     #54536         +/-   ##
=================================================
- Coverage   72.7920%   56.8239%   -15.9681%     
=================================================
  Files          1551       1682        +131     
  Lines        436754     628114     +191360     
=================================================
+ Hits         317922     356919      +38997     
- Misses        99241     247552     +148311     
- Partials      19591      23643       +4052

Flag	Coverage Δ
integration	`37.2439% <78.2608%> (?)`
unit	`71.7376% <39.8550%> (-0.0869%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Components	Coverage Δ
dumpling	`52.9656% <ø> (ø)`
parser	`∅ <ø> (∅)`
br	`52.0237% <ø> (+6.1880%)`	⬆️

Signed-off-by: AilinKid <314806019@qq.com>

pkg/config/config.go

pkg/executor/expand.go

elsa0520

LGTM

elsa0520 · 2024-07-23T08:25:41Z

pkg/planner/core/exhaust_physical_plans.go

 	// for property.RootTaskType and property.MppTaskType with no partition option, we can give an MPP Expand.
-	if p.SCtx().GetSessionVars().IsMPPAllowed() {
+	canPushToTiFlash := p.CanPushToCop(kv.TiFlash)


"CanPushToCop" This function name is strange. Cop is a protocol name but TiFlash already deprecated TiFlashCop and only keep the TiFlash[MPP]

yes, borrowed from old logic, i think it should be renamed as p.CanPushToEngine, since your argument is engine type, the name will be more accurate.

elsa0520 · 2024-07-23T08:30:08Z

pkg/planner/core/exhaust_physical_plans.go

-	return nil, true, nil
+	// for property.RootTaskType, we can give a TiDB Expand.
+	{
+		taskTypes := []property.TaskType{property.CopSingleReadTaskType, property.CopMultiReadTaskType, property.RootTaskType}


Do we need to keep all taskTypes in here? Because there is only expand operator in tidb, so why we still need to keep copsingle and copmulti?

when we come here, the prop asks for a root expand execution, while for its children prop, there is no limitation here.

Expand(Root) + IndexReader( from single cop prop) + IndexRangeScan Expand(Root) + IndexLookupReader (from multi cop prop) + IndexRangeScan + TiDBRowIDScan Expand(Root) + OtherRootOp(Root) (from root prop) + OtherOps Expand(Root) + OtherMppOps (Mpp) (from mpp prop) + OtherOps

the four cases here are all reasonable

pkg/planner/core/task.go

Co-authored-by: Elsa <111482174+elsa0520@users.noreply.github.com>

Signed-off-by: AilinKid <314806019@qq.com>

windtalker

LGTM

ti-chi-bot · 2024-07-24T09:18:34Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: elsa0520, windtalker

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

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [elsa0520,windtalker]
~~pkg/planner/OWNERS~~ [elsa0520]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

ti-chi-bot · 2024-07-24T09:18:37Z

[LGTM Timeline notifier]

Timeline:

2024-07-19 06:01:07.612707969 +0000 UTC m=+592889.603649438: ☑️ agreed by elsa0520.
2024-07-24 09:18:36.151411523 +0000 UTC m=+1036738.142352977: ☑️ agreed by windtalker.

tiprow · 2024-07-24T09:21:10Z

@AilinKid: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
fast_test_tiprow	`bb827e4`	link	true	`/test fast_test_tiprow`

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.

AilinKid · 2024-07-24T09:46:01Z

/retest-required

tiprow · 2024-07-24T09:46:22Z

@AilinKid: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.

In response to this:

/retest-required

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

ti-chi-bot bot added do-not-merge/needs-linked-issue release-note-none Denotes a PR that doesn't merit a release note. sig/planner SIG: Planner size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jul 10, 2024

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

AilinKid and others added 9 commits July 16, 2024 16:09

allow build root task type of expand operator

3427e3b

Signed-off-by: AilinKid <314806019@qq.com>

.

eece7e0

Signed-off-by: AilinKid <314806019@qq.com>

port mysql rollup test

309e8bd

Signed-off-by: AilinKid <314806019@qq.com>

Delete pkg/executor/test/expandtest/expand_test.go

e3cb0e6

Delete pkg/executor/test/expandtest/BUILD.bazel

016839b

Delete pkg/executor/test/expandtest/main_test.go

28da893

.

c5dba2a

Signed-off-by: AilinKid <314806019@qq.com>

/

8519655

Signed-off-by: AilinKid <314806019@qq.com>

.

cd77044

Signed-off-by: AilinKid <314806019@qq.com>

AilinKid force-pushed the expand_exec branch from bef2a35 to cd77044 Compare July 16, 2024 08:25

windtalker reviewed Jul 18, 2024

View reviewed changes

pkg/config/config.go Outdated Show resolved Hide resolved

pkg/executor/expand.go Show resolved Hide resolved

pkg/executor/expand.go Show resolved Hide resolved

elsa0520 approved these changes Jul 19, 2024

View reviewed changes

ti-chi-bot bot added the needs-1-more-lgtm Indicates a PR needs 1 more LGTM. label Jul 19, 2024

elsa0520 self-requested a review July 19, 2024 06:01

elsa0520 reviewed Jul 23, 2024

View reviewed changes

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

AilinKid and others added 2 commits July 24, 2024 15:28

Update pkg/planner/core/task.go

82dcb4b

Co-authored-by: Elsa <111482174+elsa0520@users.noreply.github.com>

.

bb827e4

Signed-off-by: AilinKid <314806019@qq.com>

ti-chi-bot bot added the approved label Jul 24, 2024

windtalker approved these changes Jul 24, 2024

View reviewed changes

ti-chi-bot bot added lgtm and removed needs-1-more-lgtm Indicates a PR needs 1 more LGTM. labels Jul 24, 2024

ti-chi-bot bot merged commit cbe807f into pingcap:master Jul 24, 2024
22 of 23 checks passed

hawkingrei mentioned this pull request Sep 27, 2024

Can't find column with constant expression #56218

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

planner, executor: allow build root task type of expand operator and implement expand executor #54536

planner, executor: allow build root task type of expand operator and implement expand executor #54536

AilinKid commented Jul 10, 2024 •

edited

Loading

tiprow bot commented Jul 10, 2024

codecov bot commented Jul 10, 2024 •

edited

Loading

elsa0520 left a comment

elsa0520 Jul 23, 2024

AilinKid Jul 24, 2024 •

edited

Loading

elsa0520 Jul 23, 2024

AilinKid Jul 24, 2024

windtalker left a comment

ti-chi-bot bot commented Jul 24, 2024

ti-chi-bot bot commented Jul 24, 2024

tiprow bot commented Jul 24, 2024

AilinKid commented Jul 24, 2024

tiprow bot commented Jul 24, 2024

planner, executor: allow build root task type of expand operator and implement expand executor #54536

planner, executor: allow build root task type of expand operator and implement expand executor #54536

Conversation

AilinKid commented Jul 10, 2024 • edited Loading

What problem does this PR solve?

What changed and how does it work?

Check List

Release note

tiprow bot commented Jul 10, 2024

codecov bot commented Jul 10, 2024 • edited Loading

Codecov Report

elsa0520 left a comment

Choose a reason for hiding this comment

elsa0520 Jul 23, 2024

Choose a reason for hiding this comment

AilinKid Jul 24, 2024 • edited Loading

Choose a reason for hiding this comment

elsa0520 Jul 23, 2024

Choose a reason for hiding this comment

AilinKid Jul 24, 2024

Choose a reason for hiding this comment

windtalker left a comment

Choose a reason for hiding this comment

ti-chi-bot bot commented Jul 24, 2024

ti-chi-bot bot commented Jul 24, 2024

[LGTM Timeline notifier]

tiprow bot commented Jul 24, 2024

AilinKid commented Jul 24, 2024

tiprow bot commented Jul 24, 2024

AilinKid commented Jul 10, 2024 •

edited

Loading

codecov bot commented Jul 10, 2024 •

edited

Loading

AilinKid Jul 24, 2024 •

edited

Loading