planner: fix the issue where TiDB generates multiple plandigests for 'IN (...)'. #47216

King-Dylan · 2023-09-24T14:29:25Z

What problem does this PR solve?

Issue Number: close #33559
Problem Summary:
queries with the same plan have different plan_digest in information_schema.statement_summary

What is changed and how it works?

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.

Supports generating the same plan digest for SQL IN operations with different numbers of arguments.

sre-bot · 2023-09-24T14:29:32Z

All committers have signed the CLA.

ti-chi-bot · 2023-09-24T14:29:35Z

Welcome @King-Dylan!

It looks like this is your first PR to pingcap/tidb 🎉.

I'm the bot to help you request reviewers, add labels and more, See available commands.

We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to pingcap/tidb. 😃

ti-chi-bot · 2023-09-24T14:29:35Z

Hi @King-Dylan. Thanks for your PR.

I'm waiting for a pingcap member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

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.

tiprow · 2023-09-24T14:30:02Z

Hi @King-Dylan. 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.

fixdb

This patch only works for IN list where it is in predicate, but will it work for IN list in Projection? e.g.

select a in (1, 2, 3) from t;

…n inlist'

fixdb

Can we also add test case for projection?

jackysp · 2023-10-03T18:46:30Z

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.You have signed the CLA already but the status is still pending? Let us recheck it.

Please sign it @King-Dylan

expression/explain.go

time-and-fate · 2023-10-08T10:00:04Z

expression/explain.go

+		}
+	default:
+		if expr.FuncName.L == ast.In {
+			buffer.WriteString("...")


What if there is only one constant or if there are columns in the arguments? What's the expected behavior?

I think if the plan hasn't changed, we don't need to worry about how many arguments are used. What do you think about? I can add more logical if you think it's necessary.

I think we can say this is proper and expected behavior. And I'm also OK with this idea.
What I'm concerned about is that it's not fully consistent with the behavior of SQL digest. Essentially there isn't an overall design for their behaviors.

expression/explain.go

time-and-fate · 2023-10-10T08:45:41Z

expression/explain.go

+		}
+	default:
+		if expr.FuncName.L == ast.In {
+			buffer.WriteString("...")


I think we can say this is proper and expected behavior. And I'm also OK with this idea.
What I'm concerned about is that it's not fully consistent with the behavior of SQL digest. Essentially there isn't an overall design for their behaviors.

ti-chi-bot · 2023-10-13T07:08:58Z

[LGTM Timeline notifier]

Timeline:

2023-10-12 07:36:14.516107442 +0000 UTC m=+1296972.103217588: ☑️ agreed by time-and-fate.
2023-10-13 07:08:57.531890132 +0000 UTC m=+1381735.119000292: ☑️ agreed by chrysan.

codecov · 2023-10-13T07:25:27Z

Codecov Report

Merging #47216 (3b19c71) into master (cc12200) will increase coverage by 1.3477%.
Report is 14 commits behind head on master.
The diff coverage is 77.0833%.

Additional details and impacted files

@@               Coverage Diff                @@
##             master     #47216        +/-   ##
================================================
+ Coverage   71.5920%   72.9398%   +1.3477%     
================================================
  Files          1400       1423        +23     
  Lines        405894     413929      +8035     
================================================
+ Hits         290588     301919     +11331     
+ Misses        95488      93118      -2370     
+ Partials      19818      18892       -926

Flag	Coverage Δ
integration	`42.7726% <20.8333%> (?)`
unit	`71.7399% <77.0833%> (+0.1478%)`	⬆️

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

Components	Coverage Δ
dumpling	`55.5260% <ø> (+1.4757%)`	⬆️
parser	`∅ <ø> (∅)`
br	`49.3025% <ø> (-3.5954%)`	⬇️

ti-chi-bot · 2023-10-23T06:35:21Z

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

In response to this:

/retest

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.

tiprow · 2023-10-23T06:35:38Z

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

In response to this:

/retest

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.

hawkingrei · 2023-10-23T09:53:08Z

/ok-to-test

King-Dylan · 2023-10-23T12:27:51Z

/test tiprow_fast_test

ti-chi-bot · 2023-10-23T12:28:11Z

@King-Dylan: 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 tiprow_fast_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.

tiprow · 2023-10-23T12:28:22Z

@King-Dylan: 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 tiprow_fast_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.

King-Dylan · 2023-10-23T12:42:35Z

/retest

King-Dylan · 2023-10-31T07:56:12Z

/retest

King-Dylan · 2023-10-31T08:41:17Z

/retest

easonn7 · 2023-11-01T03:50:22Z

/approve

A reasonable optimization for tidb_ignore_inlist_plan_digest is to exclude inlists from plan digests in most cases.

ti-chi-bot · 2023-11-01T03:50:30Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: AilinKid, chrysan, easonn7, time-and-fate, XuHuaiyu

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~~ [AilinKid,XuHuaiyu,chrysan,time-and-fate]
~~pkg/expression/OWNERS~~ [XuHuaiyu]
~~pkg/sessionctx/variable/OWNERS~~ [easonn7]

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

King-Dylan · 2023-11-01T05:38:24Z

/retest

fix the issue where TiDB generates multiple plandigests for 'IN (...)'.

e11e914

ti-chi-bot bot added do-not-merge/invalid-title release-note-none Denotes a PR that doesn't merit a release note. labels Sep 24, 2023

ti-chi-bot bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. needs-ok-to-test Indicates a PR created by contributors and need ORG member send '/ok-to-test' to start testing. labels Sep 24, 2023

King-Dylan changed the title ~~fix the issue where TiDB generates multiple plandigests for 'IN (...)'.~~ planner: fix the issue where TiDB generates multiple plandigests for 'IN (...)'. Sep 24, 2023

ti-chi-bot bot removed the do-not-merge/invalid-title label Sep 24, 2023

fixdb reviewed Sep 25, 2023

View reviewed changes

King-Dylan and others added 2 commits September 25, 2023 21:00

Merge branch 'pingcap:master' into DylanKing-branch

f9752c5

fix the issue where TiDB generates multiple plandigests for projectio…

afa078f

…n inlist'

King-Dylan requested a review from fixdb September 26, 2023 04:40

fixdb reviewed Sep 26, 2023

View reviewed changes

Update TestPlanDigest4InList

36073f2

King-Dylan requested a review from fixdb October 2, 2023 02:01

jackysp reviewed Oct 3, 2023

View reviewed changes

expression/explain.go Outdated Show resolved Hide resolved

time-and-fate reviewed Oct 8, 2023

View reviewed changes

expression/explain.go Outdated Show resolved Hide resolved

time-and-fate reviewed Oct 8, 2023

View reviewed changes

Delete useless code

e1c1e4a

King-Dylan requested review from jackysp and time-and-fate October 9, 2023 12:59

Add a switch to control the behavior and prevent compatibility issues.

6cfae76

King-Dylan requested a review from a team as a code owner October 10, 2023 07:55

time-and-fate reviewed Oct 10, 2023

View reviewed changes

King-Dylan added 2 commits October 10, 2023 23:54

Reduced repetition logic

cc42eb5

min update

3de28a3

ti-chi-bot bot removed the needs-1-more-lgtm Indicates a PR needs 1 more LGTM. label Oct 13, 2023

resolve conflict

5f19128

ti-chi-bot bot added ok-to-test Indicates a PR is ready to be tested. and removed needs-ok-to-test Indicates a PR created by contributors and need ORG member send '/ok-to-test' to start testing. labels Oct 23, 2023

King-Dylan added 3 commits October 23, 2023 18:23

update bazel

5e6912b

fix test

9140534

fix tiprow_fast_test

2e25700

fix test

c86989c

King-Dylan and others added 4 commits October 23, 2023 21:31

Update BUILD.bazel

cfc6d29

update bazel

b3a511b

Merge branch 'master' into DylanKing-branch

79f5a73

update bazel

3b19c71

AilinKid approved these changes Oct 31, 2023

View reviewed changes

XuHuaiyu approved these changes Oct 31, 2023

View reviewed changes

ti-chi-bot bot added the approved label Nov 1, 2023

ti-chi-bot bot merged commit 89945f5 into pingcap:master Nov 1, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

planner: fix the issue where TiDB generates multiple plandigests for 'IN (...)'. #47216

planner: fix the issue where TiDB generates multiple plandigests for 'IN (...)'. #47216

King-Dylan commented Sep 24, 2023 •

edited

Loading

sre-bot commented Sep 24, 2023 •

edited

Loading

ti-chi-bot bot commented Sep 24, 2023

ti-chi-bot bot commented Sep 24, 2023

tiprow bot commented Sep 24, 2023

fixdb left a comment

fixdb left a comment

jackysp commented Oct 3, 2023

time-and-fate Oct 8, 2023

King-Dylan Oct 9, 2023

time-and-fate Oct 10, 2023 •

edited

Loading

time-and-fate Oct 10, 2023 •

edited

Loading

ti-chi-bot bot commented Oct 13, 2023

codecov bot commented Oct 13, 2023 •

edited

Loading

ti-chi-bot bot commented Oct 23, 2023

tiprow bot commented Oct 23, 2023

hawkingrei commented Oct 23, 2023

King-Dylan commented Oct 23, 2023

ti-chi-bot bot commented Oct 23, 2023

tiprow bot commented Oct 23, 2023

King-Dylan commented Oct 23, 2023

King-Dylan commented Oct 31, 2023

King-Dylan commented Oct 31, 2023

easonn7 commented Nov 1, 2023

ti-chi-bot bot commented Nov 1, 2023

King-Dylan commented Nov 1, 2023

planner: fix the issue where TiDB generates multiple plandigests for 'IN (...)'. #47216

planner: fix the issue where TiDB generates multiple plandigests for 'IN (...)'. #47216

Conversation

King-Dylan commented Sep 24, 2023 • edited Loading

What problem does this PR solve?

What is changed and how it works?

Check List

Release note

sre-bot commented Sep 24, 2023 • edited Loading

ti-chi-bot bot commented Sep 24, 2023

ti-chi-bot bot commented Sep 24, 2023

tiprow bot commented Sep 24, 2023

fixdb left a comment

Choose a reason for hiding this comment

fixdb left a comment

Choose a reason for hiding this comment

jackysp commented Oct 3, 2023

time-and-fate Oct 8, 2023

Choose a reason for hiding this comment

King-Dylan Oct 9, 2023

Choose a reason for hiding this comment

time-and-fate Oct 10, 2023 • edited Loading

Choose a reason for hiding this comment

time-and-fate Oct 10, 2023 • edited Loading

Choose a reason for hiding this comment

ti-chi-bot bot commented Oct 13, 2023

[LGTM Timeline notifier]

codecov bot commented Oct 13, 2023 • edited Loading

Codecov Report

ti-chi-bot bot commented Oct 23, 2023

tiprow bot commented Oct 23, 2023

hawkingrei commented Oct 23, 2023

King-Dylan commented Oct 23, 2023

ti-chi-bot bot commented Oct 23, 2023

tiprow bot commented Oct 23, 2023

King-Dylan commented Oct 23, 2023

King-Dylan commented Oct 31, 2023

King-Dylan commented Oct 31, 2023

easonn7 commented Nov 1, 2023

ti-chi-bot bot commented Nov 1, 2023

King-Dylan commented Nov 1, 2023

King-Dylan commented Sep 24, 2023 •

edited

Loading

sre-bot commented Sep 24, 2023 •

edited

Loading

time-and-fate Oct 10, 2023 •

edited

Loading

time-and-fate Oct 10, 2023 •

edited

Loading

codecov bot commented Oct 13, 2023 •

edited

Loading