planner: add optimizer trace framework for logicalOptimize #29559

Yisaer · 2021-11-08T08:53:32Z

What problem does this PR solve?

ref #29661

Add optimize trace framework for logicalOptimize. In this pr, we do the following work.

use stmtCtx.EnableOptimizeTrace to indicate whether the statement should be enabled optimizer trace
add optimize trace common framework into logical optimize processing.

Issue Number: close #xxx

Problem Summary:

What is changed and how it works?

Check List

Tests

Unit test

Release note

None

ti-chi-bot · 2021-11-08T08:53:33Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

AilinKid
rebelice

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.

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

planner/core/optimizer.go

rebelice

rest LGTM

planner/core/optimizer.go

rebelice · 2021-11-11T03:47:19Z

planner/core/rule_aggregation_elimination.go

@@ -179,25 +179,25 @@ func wrapCastFunction(ctx sessionctx.Context, arg expression.Expression, targetT
 	return expression.BuildCastFunction(ctx, arg, targetTp)
 }

-func (a *aggregationEliminator) optimize(ctx context.Context, p LogicalPlan) (LogicalPlan, error) {
+func (a *aggregationEliminator) optimize(ctx context.Context, p LogicalPlan, opts ...logicalOptimizeOption) (*logicalOptimizeReturn, error) {
 	newChildren := make([]LogicalPlan, 0, len(p.Children()))
 	for _, child := range p.Children() {
 		newChild, err := a.optimize(ctx, child)


Do we need to add opts here?

currently we only implement framework for rule.optimize. In future we will implement specific logical code for each rule during optimize. Thus no need to add opts here for now.

planner/core/rule_decorrelate.go

AilinKid

Rest LGTM

AilinKid · 2021-11-11T07:54:38Z

planner/core/plan.go

@@ -377,6 +381,15 @@ func (p *baseLogicalPlan) ExplainInfo() string {
 	return ""
 }

+// buildLogicalPlanTrace implements LogicalPlan
+func (p *baseLogicalPlan) buildLogicalPlanTrace() tracing.LogicalPlanTrace {


need pointer here? func (p *baseLogicalPlan) buildLogicalPlanTrace() *tracing.LogicalPlanTrace

AilinKid · 2021-11-11T08:03:34Z

planner/core/plan.go

@@ -377,6 +381,15 @@ func (p *baseLogicalPlan) ExplainInfo() string {
 	return ""
 }

+// buildLogicalPlanTrace implements LogicalPlan
+func (p *baseLogicalPlan) buildLogicalPlanTrace() tracing.LogicalPlanTrace {
+	planTrace := tracing.LogicalPlanTrace{ID: p.ID(), TP: p.TP()}


explainInfo will be filled next?

yes, explainInfo will be implemented by specific logical operator.

AilinKid · 2021-11-11T09:31:36Z

/merge

ti-chi-bot · 2021-11-11T09:31:39Z

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

Commit hash: 7a7aaa242575391a7b8892f1e1a3dc2691c3962d

Yisaer · 2021-11-11T09:44:06Z

/merge

Yisaer · 2021-11-11T10:30:20Z

/hold

planner/core/optimizer.go

chrysan · 2021-11-11T10:39:39Z

planner/core/optimizer.go

+		// The order of flags is same as the order of optRule in the list.
+		// We use a bitmask to record which opt rules should be used. If the i-th bit is 1, it means we should
+		// apply i-th optimizing rule.
+		if flag&(1<<uint(i)) == 0 || isLogicalRuleDisabled(rule) {


This part is duplicated with the original one, you can align. Please think about minimizing changes to original codes to make tracer work.

chrysan · 2021-11-11T10:41:05Z

planner/core/optimizer.go

 // logicalOptRule means a logical optimizing rule, which contains decorrelate, ppd, column pruning, etc.
 type logicalOptRule interface {
-	optimize(context.Context, LogicalPlan) (LogicalPlan, error)
+	optimize(context.Context, LogicalPlan, ...logicalOptimizeOption) (*logicalOptimizeReturn, error)


Please add one rule implementation to show how this framework works.

updated in aggregationEliminateChecker.

ti-chi-bot · 2021-11-11T10:41:18Z

@chrysan: Request changes is only allowed for the reviewers in list.

In response to this:

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 ti-community-infra/tichi repository.

Yisaer · 2021-11-11T10:50:19Z

/hold

Signed-off-by: yisaer <[email protected]>

Yisaer · 2021-11-12T03:18:44Z

/hold cancel

Yisaer · 2021-11-12T03:18:58Z

/merge

ti-chi-bot · 2021-11-12T03:19:01Z

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

Commit hash: dbb28ee

chrysan · 2021-11-12T03:27:44Z

planner/core/optimizer.go

+	return op
+}
+
+func (op *logicalOptimizeOp) appendRuleTracerBeforeRuleOptimize(name string, before LogicalPlan) {


If you want logicalOptimizeOp to be generic not only for tracer, you'd better set the func name more generic without word "tracer".

Yisaer · 2021-11-12T03:28:52Z

/hold

Signed-off-by: yisaer <[email protected]>

Yisaer · 2021-11-12T03:40:51Z

/hold cancel

Yisaer · 2021-11-12T03:40:57Z

/merge

ti-chi-bot · 2021-11-12T03:40:59Z

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

Commit hash: 10ae26b

ti-chi-bot · 2021-11-12T03:41:10Z

@Yisaer: Your PR was out of date, I have automatically updated it for you.

At the same time I will also trigger all tests for you:

/run-all-tests

If the CI test fails, you just re-trigger the test that failed and the bot will merge the PR for you after the CI passes.

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 ti-community-infra/tichi repository.

Yisaer · 2021-11-12T04:05:20Z

/run_check_dev_2

Yisaer · 2021-11-12T04:05:58Z

/run_check_dev

Yisaer · 2021-11-12T04:06:18Z

/run_all_tests

Yisaer · 2021-11-12T04:06:40Z

/run-all-tests

Yisaer · 2021-11-12T04:14:31Z

/run-check_dev_2

Yisaer · 2021-11-12T04:23:55Z

/run-check_dev_2

ti-chi-bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. release-note-none Denotes a PR that doesn't merit a release note. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Nov 8, 2021

Yisaer marked this pull request as ready for review November 10, 2021 07:25

ti-chi-bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 10, 2021

Yisaer requested review from time-and-fate and rebelice November 10, 2021 07:37

Yisaer mentioned this pull request Nov 10, 2021

Tracking for Optimize Trace #29661

Open

19 tasks

rebelice reviewed Nov 10, 2021

View reviewed changes

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

rebelice reviewed Nov 11, 2021

View reviewed changes

rebelice approved these changes Nov 11, 2021

View reviewed changes

ti-chi-bot added the status/LGT1 Indicates that a PR has LGTM 1. label Nov 11, 2021

AilinKid changed the title ~~planner: add optimize trace framework for logicalOptimize~~ planner: add optimizer trace framework for logicalOptimize Nov 11, 2021

AilinKid reviewed Nov 11, 2021

View reviewed changes

Yisaer force-pushed the support_dump_plan branch from 125d053 to 7a7aaa2 Compare November 11, 2021 09:20

AilinKid approved these changes Nov 11, 2021

View reviewed changes

ti-chi-bot added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Nov 11, 2021

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

ti-chi-bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 11, 2021

chrysan suggested changes Nov 11, 2021

View reviewed changes

Yisaer force-pushed the support_dump_plan branch from 8632757 to 4340387 Compare November 11, 2021 11:42

ti-chi-bot removed the status/can-merge Indicates a PR has been approved by a committer. label Nov 11, 2021

address the comment

dbb28ee

Signed-off-by: yisaer <[email protected]>

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 Nov 11, 2021

ti-chi-bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 12, 2021

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

chrysan reviewed Nov 12, 2021

View reviewed changes

ti-chi-bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 12, 2021

address the comment

10ae26b

Signed-off-by: yisaer <[email protected]>

Yisaer force-pushed the support_dump_plan branch from 3cff75f to 10ae26b Compare November 12, 2021 03:40

ti-chi-bot removed the status/can-merge Indicates a PR has been approved by a committer. label Nov 12, 2021

ti-chi-bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 12, 2021

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

Merge branch 'master' into support_dump_plan

926c89d

Merge branch 'master' into support_dump_plan

e8c5d37

ti-chi-bot merged commit a4bd02e into pingcap:master Nov 12, 2021

Yisaer added the sig/planner SIG: Planner label Jan 14, 2022

planner: add optimizer trace framework for logicalOptimize #29559

planner: add optimizer trace framework for logicalOptimize #29559

Conversation

Yisaer commented Nov 8, 2021 • edited Loading

What problem does this PR solve?

What is changed and how it works?

Check List

Release note

ti-chi-bot commented Nov 8, 2021 • edited Loading

rebelice left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AilinKid 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

AilinKid commented Nov 11, 2021

ti-chi-bot commented Nov 11, 2021

Yisaer commented Nov 11, 2021

Yisaer commented Nov 11, 2021

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

ti-chi-bot commented Nov 11, 2021

Yisaer commented Nov 11, 2021

Yisaer commented Nov 12, 2021

Yisaer commented Nov 12, 2021

ti-chi-bot commented Nov 12, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Yisaer commented Nov 12, 2021

Yisaer commented Nov 12, 2021

Yisaer commented Nov 12, 2021

ti-chi-bot commented Nov 12, 2021

ti-chi-bot commented Nov 12, 2021

Yisaer commented Nov 12, 2021

Yisaer commented Nov 12, 2021

Yisaer commented Nov 12, 2021

Yisaer commented Nov 12, 2021

Yisaer commented Nov 12, 2021

Yisaer commented Nov 12, 2021

Yisaer commented Nov 8, 2021 •

edited

Loading

ti-chi-bot commented Nov 8, 2021 •

edited

Loading