parser, planner: add query block level `no_decorrelate` hint #37633

time-and-fate · 2022-09-06T02:50:29Z

What problem does this PR solve?

Issue Number: close #37789

What is changed and how it works?

Implement query block level no_decorrelate() hint.

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.

None

…o-decorrelate

ti-chi-bot · 2022-09-06T02:50:30Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

Reminiscent
chrysan

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.

…o-decorrelate

Reminiscent · 2022-09-16T03:29:17Z

planner/core/testdata/plan_suite_in.json

+
+      "SELECT ta.NAME FROM ta WHERE EXISTS (select 1 from tb where ta.code = tb.code and tb.NAME LIKE 'chad9%') AND (select max(id) from tc where ta.name=tc.name and tc.name like 'chad99%') > 100 and (select max(id) from td where ta.id=td.id and td.name like 'chad999%') > 100",
+      "SELECT ta.NAME FROM ta WHERE EXISTS (select /*+ semi_join_rewrite() */ 1 from tb where ta.code = tb.code and tb.NAME LIKE 'chad9%') AND (select /*+ no_decorrelate() */ max(id) from tc where ta.name=tc.name and tc.name like 'chad99%') > 100 and (select /*+ no_decorrelate() */ max(id) from td where ta.id=td.id and td.name like 'chad999%') > 100",
+      "SELECT ta.NAME FROM ta WHERE EXISTS (select /*+ no_decorrelate() */ 1 from tb where ta.code = tb.code and tb.NAME LIKE 'chad9%') AND (select /*+ no_decorrelate() */ max(id) from tc where ta.name=tc.name and tc.name like 'chad99%') > 100 and (select /*+ no_decorrelate() */ max(id) from td where ta.id=td.id and td.name like 'chad999%') > 100"


Please add some test cases about USE_TOJA hint.
Besides, please use these hint in the same query block to see what happened if these hint are conflicts.

Should we add these test cases about the TiFlash and partition tables(Both static and dynamic mode)?

Currently, USE_TOJA and NO_DECORRELATE won't interfere with each other. USE_TOJA handles non-correlated IN subquery, and NO_DECORRELATE handles correlated subquery.
And USE_TOJA is a query level hint instead of a query block level one.

I personally think we don't need to add such cases.
Because decorrelation-related logic happens very early in the whole optimization process, it's before any partition-related rewrites and any physical operator.
I think they won't interfere with each other. If tiflash and partitioned table related logic work well with Apply, there should be no other issues.

Reminiscent · 2022-09-16T03:34:41Z

planner/core/expression_rewriter.go

@@ -318,7 +318,7 @@ func (er *expressionRewriter) constructBinaryOpFunction(l expression.Expression,

 // buildSubquery translates the subquery ast to plan.
 // Currently, only the EXIST can apply the rewrite hint(rewrite the semi join to inner join with aggregation).
-func (er *expressionRewriter) buildSubquery(ctx context.Context, subq *ast.SubqueryExpr, rewriteHintCanTakeEffect bool) (np LogicalPlan, hasSemiJoinRewriteHint bool, err error) {


Please update the comments for this function.

Reminiscent · 2022-09-16T03:36:09Z

planner/core/expression_rewriter.go

+	oldSubQCtx := er.b.subQueryCtx
+	er.b.subQueryCtx = subqueryCtx
+	oldHintFlags := er.b.subQueryHintFlags
+	er.b.subQueryHintFlags = 0


What's the meaning for this part?

As L331 said, first, we save the current values of subQueryCtx and subQueryHintFlags, then set them to values we want to pass to the subquery, after we're done in this function, we restore the values to the saved values.

Reminiscent · 2022-09-16T03:44:46Z

planner/core/logical_plan_builder.go

+			b.subQueryHintFlags |= HintFlagSemiJoinRewrite
+		case HintNoDecorrelate:
+			if b.subQueryCtx == notHandlingSubquery {
+				b.ctx.GetSessionVars().StmtCtx.AppendWarning(ErrInternal.GenWithStack("NO_DECORRELATE() is inapplicable because it's not in an IN subquery, an EXISTS subquery, an ANY/ALL/SOME subquery or a scalar subquery."))


Should continue here?

Updated.
It actually doesn't matter, I added it to be consistent with the code above.

Reminiscent · 2022-09-16T03:48:53Z

planner/core/expression_rewriter.go

 	}
 	// Pop the handle map generated by the subquery.
 	er.b.handleHelper.popMap()
-	return np, er.b.hasValidSemiJoinHint, nil
+	return np, er.b.subQueryHintFlags, nil


It seems we always return 0 for the return value hintFlags uint64? Because we set er.b.subQueryHintFlags = 0 in line335 and return always execute before the defer function in line337-341.

No. You can see the two examples to see golang's behavior:
https://go.dev/play/p/jg7rfs2Qc6f
https://go.dev/play/p/Cb7L2I03tvC

It's indeed a bit confusing, I updated the code to make it more clear.

planner/core/expression_rewriter.go

…o-decorrelate

Reminiscent · 2022-09-19T12:03:07Z

/merge

ti-chi-bot · 2022-09-19T12:03:11Z

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

Commit hash: b8d980d

time-and-fate · 2022-09-19T12:14:20Z

/hold

time-and-fate · 2022-09-19T12:14:57Z

As discussed before, let's merge this PR after release-6.3 branching.

time-and-fate · 2022-09-20T06:38:13Z

/unhold

sre-bot · 2022-09-20T10:52:14Z

TiDB MergeCI notify

✅ Well Done! New fixed [1] after this pr merged.

CI Name	Result	Duration	Compare with Parent commit
idc-jenkins-ci-tidb/common-test	🔴 failed 1, success 10, total 11	52 min	Existing failure
idc-jenkins-ci-tidb/integration-ddl-test	🔴 failed 1, success 5, total 6	23 min	Existing failure
idc-jenkins-ci-tidb/tics-test	🔴 failed 1, success 0, total 1	4 min 40 sec	Existing failure
idc-jenkins-ci-tidb/integration-common-test	✅ all 17 tests passed	8 min 45 sec	Fixed
idc-jenkins-ci/integration-cdc-test	🟢 all 37 tests passed	26 min	Existing passed
idc-jenkins-ci-tidb/sqllogic-test-2	🟢 all 28 tests passed	4 min 33 sec	Existing passed
idc-jenkins-ci-tidb/sqllogic-test-1	🟢 all 26 tests passed	4 min 17 sec	Existing passed
idc-jenkins-ci-tidb/mybatis-test	🟢 all 1 tests passed	3 min 10 sec	Existing passed
idc-jenkins-ci-tidb/integration-compatibility-test	🟢 all 1 tests passed	2 min 55 sec	Existing passed
idc-jenkins-ci-tidb/plugin-test	🟢 build success, plugin test success	4min	Existing passed

…#37633) close pingcap#37789

time-and-fate added 2 commits September 6, 2022 10:46

add

f3924ee

Merge remote-tracking branch 'upstream/master' into s10-block-level-n…

b1eaeff

…o-decorrelate

ti-chi-bot added release-note-none Denotes a PR that doesn't merit a release note. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Sep 6, 2022

time-and-fate added 4 commits September 6, 2022 11:23

add comments

b835401

add tests

2326a67

Merge remote-tracking branch 'upstream/master' into s10-block-level-n…

ac6300a

…o-decorrelate

add comments and fix tests

7903b76

time-and-fate marked this pull request as ready for review September 13, 2022 15:14

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 Sep 13, 2022

time-and-fate added 3 commits September 15, 2022 16:53

handle conflicts with semi join rewrite and add tests

86b754a

Merge remote-tracking branch 'upstream/master' into s10-block-level-n…

db70fac

…o-decorrelate

update comments

0df2287

Reminiscent reviewed Sep 16, 2022

View reviewed changes

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

Reminiscent reviewed Sep 16, 2022

View reviewed changes

planner/core/expression_rewriter.go Show resolved Hide resolved

Reminiscent reviewed Sep 16, 2022

View reviewed changes

planner/core/expression_rewriter.go Show resolved Hide resolved

Reminiscent reviewed Sep 16, 2022

View reviewed changes

planner/core/expression_rewriter.go Show resolved Hide resolved

time-and-fate added 4 commits September 19, 2022 15:08

update comments and make code clearer

80c8c5e

Merge remote-tracking branch 'upstream/master' into s10-block-level-n…

a8bed54

…o-decorrelate

update

f741256

update test result

b8d980d

chrysan approved these changes Sep 19, 2022

View reviewed changes

ti-chi-bot added the status/LGT1 Indicates that a PR has LGTM 1. label Sep 19, 2022

Reminiscent approved these changes Sep 19, 2022

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 Sep 19, 2022

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

Merge branch 'master' into s10-block-level-no-decorrelate

7646d8a

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

time-and-fate mentioned this pull request Sep 19, 2022

optimizer-hints, correlated-subquery-optimization: add docs for no_decorrelate hint pingcap/docs-cn#11340

Merged

12 tasks

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

Merge branch 'master' into s10-block-level-no-decorrelate

e23f1e5

ti-chi-bot merged commit df82263 into pingcap:master Sep 20, 2022

time-and-fate added a commit to time-and-fate/tidb that referenced this pull request Sep 22, 2022

parser, planner: add query block level no_decorrelate hint (pingcap…

2d735c1

…#37633) close pingcap#37789

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

parser, planner: add query block level `no_decorrelate` hint #37633

parser, planner: add query block level `no_decorrelate` hint #37633

time-and-fate commented Sep 6, 2022 •

edited

Loading

ti-chi-bot commented Sep 6, 2022 •

edited

Loading

Reminiscent Sep 16, 2022

Reminiscent Sep 16, 2022

time-and-fate Sep 19, 2022

time-and-fate Sep 19, 2022

Reminiscent Sep 16, 2022

time-and-fate Sep 19, 2022

Reminiscent Sep 16, 2022

time-and-fate Sep 19, 2022

Reminiscent Sep 16, 2022

time-and-fate Sep 19, 2022

Reminiscent Sep 16, 2022

time-and-fate Sep 19, 2022

time-and-fate Sep 19, 2022

Reminiscent commented Sep 19, 2022

ti-chi-bot commented Sep 19, 2022

time-and-fate commented Sep 19, 2022

time-and-fate commented Sep 19, 2022

time-and-fate commented Sep 20, 2022

sre-bot commented Sep 20, 2022

parser, planner: add query block level no_decorrelate hint #37633

parser, planner: add query block level no_decorrelate hint #37633

Conversation

time-and-fate commented Sep 6, 2022 • edited Loading

What problem does this PR solve?

What is changed and how it works?

Check List

Release note

ti-chi-bot commented Sep 6, 2022 • edited Loading

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

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

Reminiscent commented Sep 19, 2022

ti-chi-bot commented Sep 19, 2022

time-and-fate commented Sep 19, 2022

time-and-fate commented Sep 19, 2022

time-and-fate commented Sep 20, 2022

sre-bot commented Sep 20, 2022

TiDB MergeCI notify

parser, planner: add query block level `no_decorrelate` hint #37633

parser, planner: add query block level `no_decorrelate` hint #37633

time-and-fate commented Sep 6, 2022 •

edited

Loading

ti-chi-bot commented Sep 6, 2022 •

edited

Loading