planner, CTE: Fix default inline CTE which contains agg or window function and refactor inline CTE strategy | tidb-test=pr/2239 #48188

elsa0520 · 2023-11-01T10:50:26Z

What problem does this PR solve?

Issue Number: close #47711
close #47603

Problem Summary:

What is changed and how it works?

Forbidden CTE inline when it contains or indirectly contains agg or window and is referenced by recursive part of CTE.
The fake recursive CTE can be identified and judged as non-recursive CTE. (mark as recursive but query has no recursive part)
Refactor the CTE inline strategy

Refactor
Unified inline strategy logic in CTE used.

preprocessor and buildCTE() : collect CTE info such as consumerCount, forceInlineByHintOrVar, containAggOrWindow
tryBuildCTE() : Combine the declaration of CTE and the use of CTE to jointly determine whether a CTE can be inlined.

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

…ction and refactor inline cte stratagy

tiprow · 2023-11-01T10:50:41Z

Hi @elsa0520. 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/test-infra repository.

elsa0520 · 2023-11-01T10:56:43Z

/test all

tiprow · 2023-11-01T10:57:06Z

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

In response to this:

/test all

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/test-infra repository.

elsa0520 · 2023-11-01T11:00:48Z

/cherry-pick release-7.5

ti-chi-bot · 2023-11-01T11:00:51Z

@elsa0520: once the present PR merges, I will cherry-pick it on top of release-7.5 in the new PR and assign it to you.

In response to this:

/cherry-pick release-7.5

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.

codecov · 2023-11-01T11:10:58Z

Codecov Report

Merging #48188 (02f3f47) into master (2b18f64) will increase coverage by 1.2360%.
Report is 92 commits behind head on master.
The diff coverage is 91.4285%.

Additional details and impacted files

@@               Coverage Diff                @@
##             master     #48188        +/-   ##
================================================
+ Coverage   71.5890%   72.8250%   +1.2360%     
================================================
  Files          1400       1425        +25     
  Lines        405903     413333      +7430     
================================================
+ Hits         290582     301010     +10428     
+ Misses        95511      93396      -2115     
+ Partials      19810      18927       -883

Flag	Coverage Δ
integration	`43.2572% <74.2857%> (?)`
unit	`71.3875% <91.4285%> (-0.2016%)`	⬇️

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

Components	Coverage Δ
dumpling	`53.9874% <ø> (-0.0629%)`	⬇️
parser	`∅ <ø> (∅)`
br	`48.7908% <ø> (-4.1012%)`	⬇️

time-and-fate

We need some test cases.
Another bad case:

explain
with a as (select count(*) as id from dual),
b as (select 2 as bb from a),
c as (
  with recursive tmp as (select 1 as res from dual union all select res+1 from tmp,b where res+1 < bb) 
  select * from tmp
)
select * from c;

For the new bad case, I think it's caused by the maintenance of buildingRecursivePartForCTE, and the current implementation doesn't process the setting and resetting properly.

time-and-fate · 2023-11-01T15:15:01Z

pkg/planner/core/planbuilder.go

+	// isInline will determine whether it can be inlined when **CTE is used**
 	isInline bool


I think we can remove this field now.
Because it's no longer a property of a cte, it's checked every time we use the cte.

This property should be record in cte because the tryToBuildSequence use it later ~

But it seems the isInline now is not correct anymore according to the rationale of this PR. 🤔
If we want to know if it's expected by the user to inline this CTE there, we should use the new forceInlineByHintOrVar. If we want to know if it really can be inlined, we should go through the new checking logic introduced in this PR.

In tryToBuildSequence, it looks like isInline actually means "whether this CTE is really inlined in the most recent buildWith call for this CTE".

If my understanding is correct, I think we at least need some comments to clarify this.

pkg/planner/core/logical_plan_builder.go

time-and-fate · 2023-11-01T15:58:07Z

Also, there are a lot of failures in the CI, please take a look.

1. with in setoprselectlist should be record in ctestack 2. the fake recursive cte also can be inlined. such as mark as recursive but has no recursive part in query

elsa0520 · 2023-11-07T09:07:03Z

We need some test cases.

Another bad case:
explain
with a as (select count(*) as id from dual),
b as (select 2 as bb from a),
c as (
  with recursive tmp as (select 1 as res from dual union all select res+1 from tmp,b where res+1 < bb) 
  select * from tmp
)
select * from c;
For the new bad case, I think it's caused by the maintenance of buildingRecursivePartForCTE, and the current implementation doesn't process the setting and resetting properly.

Cover this case and add more tests ~

elsa0520 · 2023-11-08T02:29:12Z

/test unit-test

ti-chi-bot · 2023-11-08T02:29:15Z

@elsa0520: The specified target(s) for /test were not found.
The following commands are available to trigger required jobs:

/test build
/test check-dev
/test check-dev2
/test mysql-test
/test unit-test

The following commands are available to trigger optional jobs:

/test canary-notify-when-compatibility-sections-changed
/test pingcap/tidb/canary_ghpr_unit_test
/test pull-br-integration-test
/test pull-common-test
/test pull-e2e-test
/test pull-integration-common-test
/test pull-integration-copr-test
/test pull-integration-ddl-test
/test pull-integration-jdbc-test
/test pull-integration-mysql-test
/test pull-integration-nodejs-test
/test pull-mysql-client-test
/test pull-sqllogic-test
/test pull-tiflash-test

Use /test all to run the following jobs that were automatically triggered:

pingcap/tidb/ghpr_build
pingcap/tidb/ghpr_check
pingcap/tidb/ghpr_check2
pingcap/tidb/ghpr_mysql_test
pingcap/tidb/ghpr_unit_test

In response to this:

/test unit-test

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/test-infra repository.

elsa0520 · 2023-11-08T02:29:21Z

/test check_dev_2

ti-chi-bot · 2023-11-08T02:29:24Z

@elsa0520: The specified target(s) for /test were not found.
The following commands are available to trigger required jobs:

/test build
/test check-dev
/test check-dev2
/test mysql-test
/test unit-test

The following commands are available to trigger optional jobs:

/test canary-notify-when-compatibility-sections-changed
/test pingcap/tidb/canary_ghpr_unit_test
/test pull-br-integration-test
/test pull-common-test
/test pull-e2e-test
/test pull-integration-common-test
/test pull-integration-copr-test
/test pull-integration-ddl-test
/test pull-integration-jdbc-test
/test pull-integration-mysql-test
/test pull-integration-nodejs-test
/test pull-mysql-client-test
/test pull-sqllogic-test
/test pull-tiflash-test

Use /test all to run the following jobs that were automatically triggered:

pingcap/tidb/ghpr_build
pingcap/tidb/ghpr_check
pingcap/tidb/ghpr_check2
pingcap/tidb/ghpr_mysql_test
pingcap/tidb/ghpr_unit_test

In response to this:

/test check_dev_2

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/test-infra repository.

elsa0520 · 2023-11-08T05:01:10Z

/test mysql-test

tiprow · 2023-11-08T05:01:33Z

@elsa0520: The specified target(s) for /test were not found.
The following commands are available to trigger required jobs:

/test tiprow_fast_test

Use /test all to run all jobs.

In response to this:

/test mysql-test

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/test-infra repository.

elsa0520 · 2023-11-08T05:15:17Z

/test mysql-test

tiprow · 2023-11-08T05:15:39Z

@elsa0520: The specified target(s) for /test were not found.
The following commands are available to trigger required jobs:

/test tiprow_fast_test

Use /test all to run all jobs.

In response to this:

/test mysql-test

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/test-infra repository.

elsa0520 · 2023-11-08T05:59:26Z

/test mysql-test

tiprow · 2023-11-08T05:59:48Z

@elsa0520: The specified target(s) for /test were not found.
The following commands are available to trigger required jobs:

/test tiprow_fast_test

Use /test all to run all jobs.

In response to this:

/test mysql-test

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/test-infra repository.

elsa0520 · 2023-11-08T06:46:53Z

/test mysql-test

tiprow · 2023-11-08T06:47:17Z

@elsa0520: The specified target(s) for /test were not found.
The following commands are available to trigger required jobs:

/test tiprow_fast_test

Use /test all to run all jobs.

In response to this:

/test mysql-test

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/test-infra repository.

YangKeao · 2023-11-08T06:58:39Z

Will also close #47603

YangKeao

LGTM

ti-chi-bot · 2023-11-08T07:27:04Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: qw4990, YangKeao

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~~ [YangKeao,qw4990]

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 · 2023-11-08T07:27:09Z

[LGTM Timeline notifier]

Timeline:

2023-11-08 03:41:31.112034241 +0000 UTC m=+3615688.699144388: ☑️ agreed by qw4990.
2023-11-08 07:27:08.673658156 +0000 UTC m=+3629226.260768303: ☑️ agreed by YangKeao.

YangKeao · 2023-11-08T07:42:40Z

/retest

elsa0520 · 2023-11-08T08:27:44Z

/test mysql-test

tiprow · 2023-11-08T08:28:06Z

@elsa0520: The specified target(s) for /test were not found.
The following commands are available to trigger required jobs:

/test tiprow_fast_test

Use /test all to run all jobs.

In response to this:

/test mysql-test

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/test-infra repository.

ti-chi-bot · 2023-11-08T11:10:28Z

In response to a cherrypick label: new pull request created to branch release-7.1: #48436.

Signed-off-by: ti-chi-bot <ti-community-prow-bot@tidb.io>

ti-chi-bot · 2023-11-08T11:11:09Z

In response to a cherrypick label: new pull request created to branch release-7.5: #48437.

ti-chi-bot · 2023-11-08T11:11:53Z

In response to a cherrypick label: new pull request created to branch release-6.5: #48438.

Signed-off-by: ti-chi-bot <ti-community-prow-bot@tidb.io>

planner, cte: Fix default inline cte which contains agg or window fun…

c73e382

…ction and refactor inline cte stratagy

time-and-fate reviewed Nov 1, 2023

View reviewed changes

fix

6e791c5

1. with in setoprselectlist should be record in ctestack 2. the fake recursive cte also can be inlined. such as mark as recursive but has no recursive part in query

ti-chi-bot bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Nov 7, 2023

the cte indirectly contains agg also cannot be inline in recursive part

061da2f

ti-chi-bot 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 7, 2023

ti-chi-bot bot removed the do-not-merge/needs-tests-checked label Nov 7, 2023

qw4990 self-requested a review November 7, 2023 09:36

elsa0520 added 2 commits November 7, 2023 18:30

fix ut

dc53bd8

fix ut

0ad15f0

YangKeao mentioned this pull request Nov 8, 2023

planner: don't inline CTE when it's consumed by a recursive CTE #48406

Closed

8 tasks

ti-chi-bot bot added do-not-merge/needs-triage-completed and removed do-not-merge/needs-triage-completed labels Nov 8, 2023

YangKeao approved these changes Nov 8, 2023

View reviewed changes

ti-chi-bot bot added lgtm and removed needs-1-more-lgtm Indicates a PR needs 1 more LGTM. labels Nov 8, 2023

use compute inline function instead

02f3f47

ti-chi-bot bot merged commit c131521 into pingcap:master Nov 8, 2023
11 of 16 checks passed

ti-chi-bot mentioned this pull request Nov 8, 2023

planner, CTE: Fix default inline CTE which contains agg or window function and refactor inline CTE strategy | tidb-test=pr/2241 #48436

Merged

13 tasks

ti-chi-bot pushed a commit to ti-chi-bot/tidb that referenced this pull request Nov 8, 2023

This is an automated cherry-pick of pingcap#48188

1fb3746

Signed-off-by: ti-chi-bot <ti-community-prow-bot@tidb.io>

ti-chi-bot mentioned this pull request Nov 8, 2023

planner, CTE: Fix default inline CTE which contains agg or window function and refactor inline CTE strategy | tidb-test=pr/2240 #48437

Merged

13 tasks

ti-chi-bot mentioned this pull request Nov 8, 2023

planner, CTE: Fix default inline CTE which contains agg or window function and refactor inline CTE strategy | tidb-test=pr/2242 #48438

Merged

13 tasks

ti-chi-bot pushed a commit to ti-chi-bot/tidb that referenced this pull request Nov 8, 2023

This is an automated cherry-pick of pingcap#48188

751e602

Signed-off-by: ti-chi-bot <ti-community-prow-bot@tidb.io>

		// isInline will determine whether it can be inlined when CTE is used
		isInline bool

planner, CTE: Fix default inline CTE which contains agg or window function and refactor inline CTE strategy | tidb-test=pr/2239 #48188

planner, CTE: Fix default inline CTE which contains agg or window function and refactor inline CTE strategy | tidb-test=pr/2239 #48188

Conversation

elsa0520 commented Nov 1, 2023 • edited by YangKeao Loading

What problem does this PR solve?

What is changed and how it works?

Check List

Release note

tiprow bot commented Nov 1, 2023

elsa0520 commented Nov 1, 2023

tiprow bot commented Nov 1, 2023

elsa0520 commented Nov 1, 2023

ti-chi-bot commented Nov 1, 2023

codecov bot commented Nov 1, 2023 • edited Loading

Codecov Report

time-and-fate left a comment • edited Loading

Choose a reason for hiding this comment

time-and-fate Nov 1, 2023

Choose a reason for hiding this comment

elsa0520 Nov 7, 2023

Choose a reason for hiding this comment

time-and-fate Nov 8, 2023

Choose a reason for hiding this comment

time-and-fate Nov 8, 2023

Choose a reason for hiding this comment

time-and-fate Nov 8, 2023

Choose a reason for hiding this comment

time-and-fate commented Nov 1, 2023

elsa0520 commented Nov 7, 2023

elsa0520 commented Nov 8, 2023

ti-chi-bot bot commented Nov 8, 2023

elsa0520 commented Nov 8, 2023

ti-chi-bot bot commented Nov 8, 2023

elsa0520 commented Nov 8, 2023

tiprow bot commented Nov 8, 2023

elsa0520 commented Nov 8, 2023

tiprow bot commented Nov 8, 2023

elsa0520 commented Nov 8, 2023

tiprow bot commented Nov 8, 2023

elsa0520 commented Nov 8, 2023

tiprow bot commented Nov 8, 2023

YangKeao commented Nov 8, 2023

YangKeao left a comment

Choose a reason for hiding this comment

ti-chi-bot bot commented Nov 8, 2023

ti-chi-bot bot commented Nov 8, 2023

[LGTM Timeline notifier]

YangKeao commented Nov 8, 2023

elsa0520 commented Nov 8, 2023

tiprow bot commented Nov 8, 2023

ti-chi-bot commented Nov 8, 2023

ti-chi-bot commented Nov 8, 2023

ti-chi-bot commented Nov 8, 2023

elsa0520 commented Nov 1, 2023 •

edited by YangKeao

Loading

codecov bot commented Nov 1, 2023 •

edited

Loading

time-and-fate left a comment •

edited

Loading