Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

planner, CTE, view: Fix default inline CTE which contains orderby/limit/distinct and inside of view | tidb-test=pr/2415 #56609

Merged
merged 8 commits into from
Oct 15, 2024

Conversation

elsa0520
Copy link
Contributor

@elsa0520 elsa0520 commented Oct 12, 2024

What problem does this PR solve?

Issue Number: close #56603, close #56582

Problem Summary:

What changed and how does it work?

  1. If the CTE contain orderby/limit/distinct and CTE is referenced by another CTE recursive part, it cannot be inlined . Related issue Parsing CTE error with "doesn't yet support 'ORDER BY / LIMIT / SELECT DISTINCT in recursive query block" #56603
  2. If the CTE inside of view, the consumerCount of CTE cannot be updated because the view skip the preprocessor phase in during optimizer, it also cannot judge the inline or not. So the CTE inside of view will always be not inlined. Related issue The CTE inside of view can't determine whether it can be inlined #56582

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

  • Affects user behaviors
  • Contains syntax changes
  • Contains variable changes
  • Contains experimental features
  • Changes MySQL compatibility

Release note

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

修复错误的 inline CTE,如果 CTE 包含 orderby/limit/distinct 子句并且被另外一个 CTE 的 recursive part 所引用。
同时去除对 view 中的 CTE 进行是否默认 inline 的优化。

@ti-chi-bot ti-chi-bot bot added the release-note Denotes a PR that will be considered when it comes time to generate release notes. label Oct 12, 2024
@elsa0520 elsa0520 requested a review from AilinKid October 12, 2024 08:04
@ti-chi-bot ti-chi-bot bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. sig/planner SIG: Planner labels Oct 12, 2024
Copy link

tiprow bot commented Oct 12, 2024

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-sigs/prow repository.

@elsa0520 elsa0520 added affects-6.5 This bug affects the 6.5.x(LTS) versions. affects-7.1 This bug affects the 7.1.x(LTS) versions. affects-7.5 This bug affects the 7.5.x(LTS) versions. affects-8.1 This bug affects the 8.1.x(LTS) versions. labels Oct 12, 2024
@hawkingrei
Copy link
Member

/ok-to-test

@ti-chi-bot ti-chi-bot bot added the ok-to-test Indicates a PR is ready to be tested. label Oct 12, 2024
Copy link

codecov bot commented Oct 12, 2024

Codecov Report

Attention: Patch coverage is 96.29630% with 1 line in your changes missing coverage. Please review.

Project coverage is 56.4312%. Comparing base (10647c9) to head (8ed01e0).
Report is 19 commits behind head on master.

Additional details and impacted files
@@                Coverage Diff                @@
##             master     #56609         +/-   ##
=================================================
- Coverage   73.3366%   56.4312%   -16.9054%     
=================================================
  Files          1629       1753        +124     
  Lines        449954     631627     +181673     
=================================================
+ Hits         329981     356435      +26454     
- Misses        99734     251155     +151421     
- Partials      20239      24037       +3798     
Flag Coverage Δ
integration 36.9076% <96.2963%> (?)
unit 72.5715% <88.8888%> (+0.0966%) ⬆️

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

Components Coverage Δ
dumpling 52.9478% <ø> (ø)
parser ∅ <ø> (∅)
br 52.4232% <ø> (+6.8341%) ⬆️

@elsa0520
Copy link
Contributor Author

/test mysql-test

Copy link

tiprow bot commented Oct 12, 2024

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

  • /test fast_test_tiprow
  • /test fast_test_tiprow_ddlargsv1
  • /test tidb_parser_test

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

  • fast_test_tiprow
  • tidb_parser_test

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-sigs/prow repository.

@ti-chi-bot ti-chi-bot bot added approved needs-1-more-lgtm Indicates a PR needs 1 more LGTM. labels Oct 12, 2024
Copy link
Contributor

@AilinKid AilinKid left a comment

Choose a reason for hiding this comment

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

REST LGTM

if cte.forceInlineByHintOrVar {
b.ctx.GetSessionVars().StmtCtx.AppendWarning(plannererrors.ErrCTERecursiveForbidsAggregation.FastGenByArgs(cte.def.Name))
}
} else if cte.consumerCount > 1 {
cte.isInline = false
Copy link
Contributor

Choose a reason for hiding this comment

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

cte.isInline is the only the variable declared here and used here, we can use a temporary variable rather than a member variable inside cte?

Copy link
Contributor

Choose a reason for hiding this comment

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

which means this function will return a bool back to the caller

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is used by buildSequence fuction so I cannot just return a bool to the caller

func (b *PlanBuilder) tryToBuildSequence(ctes []*cteInfo, p base.LogicalPlan) base.LogicalPlan {
	if !b.ctx.GetSessionVars().EnableMPPSharedCTEExecution {
		return p
	}
	for i := len(ctes) - 1; i >= 0; i-- {
		if !ctes[i].nonRecursive {
			return p
		}
		if ctes[i].isInline || ctes[i].cteClass == nil {
			ctes = append(ctes[:i], ctes[i+1:]...)
		}
	}

@elsa0520 elsa0520 changed the title planner, CTE, view: Fix default inline CTE which contains orderby/limit/distinct and inside of view planner, CTE, view: Fix default inline CTE which contains orderby/limit/distinct and inside of view | mysql-test=pr/2415 Oct 14, 2024
@elsa0520 elsa0520 changed the title planner, CTE, view: Fix default inline CTE which contains orderby/limit/distinct and inside of view | mysql-test=pr/2415 planner, CTE, view: Fix default inline CTE which contains orderby/limit/distinct and inside of view | tidb-test=pr/2415 Oct 14, 2024
@elsa0520
Copy link
Contributor Author

/test mysql-test

Copy link

ti-chi-bot bot commented Oct 14, 2024

@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 pull-br-integration-test
  • /test pull-integration-ddl-test
  • /test pull-lightning-integration-test
  • /test pull-mysql-client-test
  • /test pull-unit-test-ddlv1
  • /test unit-test

The following commands are available to trigger optional jobs:

  • /test pingcap/tidb/canary_ghpr_unit_test
  • /test pull-common-test
  • /test pull-e2e-test
  • /test pull-integration-common-test
  • /test pull-integration-copr-test
  • /test pull-integration-e2e-test
  • /test pull-integration-jdbc-test
  • /test pull-integration-mysql-test
  • /test pull-integration-nodejs-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
  • pingcap/tidb/pull_integration_ddl_test
  • pingcap/tidb/pull_mysql_client_test

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.

Copy link

tiprow bot commented Oct 14, 2024

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

  • /test fast_test_tiprow
  • /test fast_test_tiprow_ddlargsv1
  • /test tidb_parser_test

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

  • fast_test_tiprow
  • tidb_parser_test

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-sigs/prow repository.

@elsa0520
Copy link
Contributor Author

/test unit-test

Copy link

tiprow bot commented Oct 14, 2024

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

  • /test fast_test_tiprow
  • /test fast_test_tiprow_ddlargsv1
  • /test tidb_parser_test

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

  • fast_test_tiprow
  • tidb_parser_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-sigs/prow repository.

@elsa0520
Copy link
Contributor Author

/test mysql-test

Copy link

tiprow bot commented Oct 14, 2024

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

  • /test fast_test_tiprow
  • /test fast_test_tiprow_ddlargsv1
  • /test tidb_parser_test

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

  • fast_test_tiprow
  • tidb_parser_test

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-sigs/prow repository.

@elsa0520 elsa0520 changed the title planner, CTE, view: Fix default inline CTE which contains orderby/limit/distinct and inside of view | tidb-test=pr/2415 planner, CTE, view: Fix default inline CTE which contains orderby/limit/distinct and inside of view Oct 14, 2024
@elsa0520
Copy link
Contributor Author

/test mysql-test

Copy link

tiprow bot commented Oct 14, 2024

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

  • /test fast_test_tiprow
  • /test fast_test_tiprow_ddlargsv1
  • /test tidb_parser_test

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

  • fast_test_tiprow
  • tidb_parser_test

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-sigs/prow repository.

Copy link

tiprow bot commented Oct 15, 2024

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

  • /test fast_test_tiprow
  • /test fast_test_tiprow_ddlargsv1
  • /test tidb_parser_test

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

  • fast_test_tiprow
  • tidb_parser_test

In response to this:

/test check-dev2

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 ti-chi-bot bot merged commit fa723c3 into pingcap:master Oct 15, 2024
23 checks passed
@elsa0520
Copy link
Contributor Author

/cherry-pick release-6.5-20241009-v6.5.10

@ti-chi-bot
Copy link
Member

@elsa0520: new pull request created to branch release-6.5-20241009-v6.5.10: #56666.

In response to this:

/cherry-pick release-6.5-20241009-v6.5.10

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.

@elsa0520
Copy link
Contributor Author

/cherry-pick release-6.5

@ti-chi-bot
Copy link
Member

@elsa0520: new pull request created to branch release-6.5: #56670.

In response to this:

/cherry-pick release-6.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.

ti-chi-bot pushed a commit to ti-chi-bot/tidb that referenced this pull request Oct 16, 2024
Signed-off-by: ti-chi-bot <ti-community-prow-bot@tidb.io>
ti-chi-bot bot pushed a commit that referenced this pull request Oct 16, 2024
…it/distinct and inside of view | tidb-test=e1d0c1e615f749e7139f5be95bc4a2b8cedb7380 (#56609) (#56666)

close #56582, close #56603
@ti-chi-bot ti-chi-bot bot added needs-cherry-pick-release-8.1 Should cherry pick this PR to release-8.1 branch. needs-cherry-pick-release-7.5 Should cherry pick this PR to release-7.5 branch. needs-cherry-pick-release-7.1 Should cherry pick this PR to release-7.1 branch. labels Oct 17, 2024
ti-chi-bot pushed a commit to ti-chi-bot/tidb that referenced this pull request Oct 17, 2024
Signed-off-by: ti-chi-bot <ti-community-prow-bot@tidb.io>
@ti-chi-bot
Copy link
Member

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

@ti-chi-bot
Copy link
Member

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

ti-chi-bot pushed a commit to ti-chi-bot/tidb that referenced this pull request Oct 17, 2024
Signed-off-by: ti-chi-bot <ti-community-prow-bot@tidb.io>
@ti-chi-bot
Copy link
Member

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

ti-chi-bot pushed a commit to ti-chi-bot/tidb that referenced this pull request Oct 17, 2024
Signed-off-by: ti-chi-bot <ti-community-prow-bot@tidb.io>
ti-chi-bot bot pushed a commit that referenced this pull request Nov 7, 2024
…it/distinct and inside of view | tidb-test=pr/2422 (#56609) (#56696)

close #56582, close #56603
ti-chi-bot bot pushed a commit that referenced this pull request Dec 4, 2024
…it/distinct and inside of view | tidb-test=pr/2438 (#56609) (#56695)

close #56582, close #56603
@elsa0520
Copy link
Contributor Author

/cherry-pick release-6.5-20241213-v6.5.11

@ti-chi-bot
Copy link
Member

@elsa0520: new pull request created to branch release-6.5-20241213-v6.5.11: #58314.

In response to this:

/cherry-pick release-6.5-20241213-v6.5.11

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.

@bb7133
Copy link
Member

bb7133 commented Dec 17, 2024

Hi @elsa0520 , thanks for this fix. Please notice that we always use English on Github, please try to avoid Chinese for the release note.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
affects-6.5 This bug affects the 6.5.x(LTS) versions. affects-7.1 This bug affects the 7.1.x(LTS) versions. affects-7.5 This bug affects the 7.5.x(LTS) versions. affects-8.1 This bug affects the 8.1.x(LTS) versions. approved lgtm needs-cherry-pick-release-7.1 Should cherry pick this PR to release-7.1 branch. needs-cherry-pick-release-7.5 Should cherry pick this PR to release-7.5 branch. needs-cherry-pick-release-8.1 Should cherry pick this PR to release-8.1 branch. ok-to-test Indicates a PR is ready to be tested. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/planner SIG: Planner size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
6 participants