planner: fix name ambiguous check when building natural join without filter #36012

AilinKid · 2022-07-07T08:12:40Z

Signed-off-by: AilinKid 314806019@qq.com

What problem does this PR solve?

Issue Number: close #32044

Problem Summary:

What is changed and how it works?

natural join without a filter may find their common columns from both sides first.
then, for every common column, we need to make sure that there is exactly one column matches the name for each side.

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.

planner: fix name ambiguous check when building natural join without filter

ti-chi-bot · 2022-07-07T08:12:42Z

[REVIEW NOTIFICATION]

This pull request has not been approved.

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.

sre-bot · 2022-07-07T08:24:19Z

Code Coverage Details: https://codecov.io/github/pingcap/tidb/commit/f6853c670b2190f4f99342629e9adb7db01056d4

time-and-fate · 2022-07-11T06:22:41Z

planner/core/logical_plan_builder.go

+		//  t1 natural join (t3 cross join t4)
+		// t3 and t4 may generate the same name column from cross join.
+		// for every common column of natural join, the name from right or left should be exactly one.
+		checkAmbiguous := func(lnames, rnames types.NameSlice) error {


I think we don't need to make it a separate function here because we don't reuse this logic.

time-and-fate · 2022-07-11T06:23:02Z

planner/core/logical_plan_builder.go

+		// t3 and t4 may generate the same name column from cross join.
+		// for every common column of natural join, the name from right or left should be exactly one.
+		checkAmbiguous := func(lnames, rnames types.NameSlice) error {
+			// step1: find the common names first


Where is "step2"? 🤣

xhebox · 2022-07-21T04:37:37Z

planner/core/logical_plan_builder.go

+			// record left map
+			if cnt, ok := lNameMap[name.ColName.L]; ok {
+				lNameMap[name.ColName.L] = cnt + 1
+			} else {
+				lNameMap[name.ColName.L] = 1


What about map["v"] = map["v"] + 1? If it is not in the map, the default value of int will be returned, i.e. 0.

Since int is not a pointer, I think there is no need to use v, ok := map... and check ok.

time-and-fate · 2022-07-21T09:11:12Z

planner/core/logical_plan_builder.go

+				if cnt > 1 {
+					return ErrAmbiguous.GenWithStackByArgs(name.ColName.L, "from clause")
+				}
+				commonNames = append(commonNames, name.ColName.L)


Seems we can also check "the right map" here, and don't need to maintain the commonNames.

AilinKid

/retest

tiprow · 2024-01-22T13:33:51Z

@AilinKid: 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.

codecov · 2024-01-26T09:12:44Z

Codecov Report

Merging #36012 (f2a6a04) into master (1760a26) will increase coverage by 2.1483%.
Report is 2 commits behind head on master.
The diff coverage is 100.0000%.

Additional details and impacted files

@@               Coverage Diff                @@
##             master     #36012        +/-   ##
================================================
+ Coverage   70.4861%   72.6344%   +2.1483%     
================================================
  Files          1463       1463                
  Lines        433217     433271        +54     
================================================
+ Hits         305358     314704      +9346     
+ Misses       108613      98699      -9914     
- Partials      19246      19868       +622

Flag	Coverage Δ
integration	`48.9156% <100.0000%> (?)`
unit	`70.2776% <55.5555%> (+0.0018%)`	⬆️

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

Components	Coverage Δ
dumpling	`54.2940% <ø> (ø)`
parser	`∅ <ø> (∅)`
br	`51.2310% <ø> (+5.5025%)`	⬆️

ti-chi-bot · 2024-01-28T12:44:20Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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~~ [hawkingrei,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 · 2024-01-28T12:44:24Z

[LGTM Timeline notifier]

Timeline:

2024-01-26 09:38:51.577154496 +0000 UTC m=+1126373.141452201: ☑️ agreed by hawkingrei.
2024-01-28 12:44:23.217689425 +0000 UTC m=+1310304.781987129: ☑️ agreed by qw4990.

tiprow · 2024-01-28T12:46:42Z

@AilinKid: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
tiprow_fast_test	d68fa5fbf2e5adebeb6d46ef85fe268c6a53af42	link	true	`/test tiprow_fast_test`

Full PR test history. Your PR dashboard.

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. I understand the commands that are listed here.

AilinKid

/retest

tiprow · 2024-01-28T14:53:06Z

@AilinKid: 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 · 2024-01-28T15:03:51Z

pkg/planner/core/logical_plan_builder.go

+			// check left map
+			if cnt, ok := lNameMap[name.ColName.L]; ok {
+				if cnt > 1 {
+					return ErrAmbiguous.GenWithStackByArgs(name.ColName.L, "from clause")


ErrAmbiguous shoud be plannererrors.ErrAmbiguous. ErrAmbiguous have been moved into pkg/util/dbterror/plannererrors.

Signed-off-by: AilinKid <314806019@qq.com>

ti-chi-bot · 2024-02-01T10:51:51Z

/cherry-pick release-7.5

ti-chi-bot · 2024-02-01T10:52:32Z

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

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

ti-chi-bot · 2024-02-01T10:53:16Z

@ti-chi-bot: new pull request could not be created: failed to create pull request against pingcap/tidb#release-7.5 from head ti-chi-bot:cherry-pick-36012-to-release-7.5: status code 422 not one of [201], body: {"message":"Validation Failed","errors":[{"resource":"PullRequest","code":"custom","message":"A pull request already exists for ti-chi-bot:cherry-pick-36012-to-release-7.5."}],"documentation_url":"https://docs.github.com/rest/pulls/pulls#create-a-pull-request"}

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.

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

ti-chi-bot · 2024-02-08T02:34:56Z

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

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

…filter (#36012) (#50904) close #32044

…filter (#36012) (#51068) close #32044

ti-chi-bot added release-note Denotes a PR that will be considered when it comes time to generate release notes. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jul 7, 2022

time-and-fate reviewed Jul 11, 2022

View reviewed changes

xhebox reviewed Jul 21, 2022

View reviewed changes

time-and-fate reviewed Jul 21, 2022

View reviewed changes

AilinKid commented Jan 22, 2024

View reviewed changes

AilinKid force-pushed the fix-natural-name-resolution-32044 branch from ca37aca to 56a6df7 Compare January 26, 2024 09:11

hawkingrei approved these changes Jan 26, 2024

View reviewed changes

ti-chi-bot bot added approved needs-1-more-lgtm Indicates a PR needs 1 more LGTM. labels Jan 26, 2024

qw4990 approved these changes Jan 28, 2024

View reviewed changes

ti-chi-bot bot added lgtm and removed needs-1-more-lgtm Indicates a PR needs 1 more LGTM. labels Jan 28, 2024

AilinKid commented Jan 28, 2024

View reviewed changes

hawkingrei reviewed Jan 28, 2024

View reviewed changes

AilinKid and others added 3 commits January 29, 2024 13:57

fix name ambiguous check when building natural join without fileter

fa5804f

Signed-off-by: AilinKid <314806019@qq.com>

address commnet

195dcf0

Signed-off-by: AilinKid <314806019@qq.com>

Delete executor/join_test.go

7cf8215

AilinKid added 2 commits January 29, 2024 13:57

fix tes t

1acdd9a

Signed-off-by: AilinKid <314806019@qq.com>

fix error prefix

f2a6a04

Signed-off-by: AilinKid <314806019@qq.com>

AilinKid force-pushed the fix-natural-name-resolution-32044 branch from d68fa5f to f2a6a04 Compare January 29, 2024 06:00

ti-chi-bot bot merged commit 41f56d1 into pingcap:master Jan 29, 2024
21 checks passed

ti-chi-bot added the needs-cherry-pick-release-7.5 Should cherry pick this PR to release-7.5 branch. label Feb 1, 2024

ti-chi-bot pushed a commit to ti-chi-bot/tidb that referenced this pull request Feb 1, 2024

This is an automated cherry-pick of pingcap#36012

47f536c

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

ti-chi-bot mentioned this pull request Feb 1, 2024

planner: fix name ambiguous check when building natural join without filter (#36012) #50904

Merged

12 tasks

ti-chi-bot pushed a commit to ti-chi-bot/tidb that referenced this pull request Feb 1, 2024

This is an automated cherry-pick of pingcap#36012

c399a39

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

ti-chi-bot added the needs-cherry-pick-release-7.1 Should cherry pick this PR to release-7.1 branch. label Feb 8, 2024

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

This is an automated cherry-pick of pingcap#36012

db721ec

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

ti-chi-bot mentioned this pull request Feb 8, 2024

planner: fix name ambiguous check when building natural join without filter (#36012) #51068

Merged

12 tasks

AilinKid added a commit to ti-chi-bot/tidb that referenced this pull request Feb 19, 2024

This is an automated cherry-pick of pingcap#36012

b672ffd

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

ti-chi-bot bot pushed a commit that referenced this pull request Feb 19, 2024

planner: fix name ambiguous check when building natural join without …

d686efd

…filter (#36012) (#50904) close #32044

ti-chi-bot bot pushed a commit that referenced this pull request Feb 27, 2024

planner: fix name ambiguous check when building natural join without …

295ab16

…filter (#36012) (#51068) close #32044

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

planner: fix name ambiguous check when building natural join without filter #36012

planner: fix name ambiguous check when building natural join without filter #36012

AilinKid commented Jul 7, 2022

ti-chi-bot commented Jul 7, 2022

sre-bot commented Jul 7, 2022 •

edited

Loading

time-and-fate Jul 11, 2022

AilinKid Jul 20, 2022

time-and-fate Jul 11, 2022

xhebox Jul 21, 2022

time-and-fate Jul 21, 2022

AilinKid left a comment

tiprow bot commented Jan 22, 2024

codecov bot commented Jan 26, 2024 •

edited

Loading

ti-chi-bot bot commented Jan 28, 2024

ti-chi-bot bot commented Jan 28, 2024

tiprow bot commented Jan 28, 2024

AilinKid left a comment

tiprow bot commented Jan 28, 2024

hawkingrei Jan 28, 2024

AilinKid Jan 29, 2024

ti-chi-bot commented Feb 1, 2024

ti-chi-bot commented Feb 1, 2024

ti-chi-bot commented Feb 1, 2024

ti-chi-bot commented Feb 8, 2024

planner: fix name ambiguous check when building natural join without filter #36012

planner: fix name ambiguous check when building natural join without filter #36012

Conversation

AilinKid commented Jul 7, 2022

What problem does this PR solve?

What is changed and how it works?

Check List

Release note

ti-chi-bot commented Jul 7, 2022

sre-bot commented Jul 7, 2022 • edited Loading

time-and-fate Jul 11, 2022

Choose a reason for hiding this comment

AilinKid Jul 20, 2022

Choose a reason for hiding this comment

time-and-fate Jul 11, 2022

Choose a reason for hiding this comment

xhebox Jul 21, 2022

Choose a reason for hiding this comment

time-and-fate Jul 21, 2022

Choose a reason for hiding this comment

AilinKid left a comment

Choose a reason for hiding this comment

tiprow bot commented Jan 22, 2024

codecov bot commented Jan 26, 2024 • edited Loading

Codecov Report

ti-chi-bot bot commented Jan 28, 2024

ti-chi-bot bot commented Jan 28, 2024

[LGTM Timeline notifier]

tiprow bot commented Jan 28, 2024

AilinKid left a comment

Choose a reason for hiding this comment

tiprow bot commented Jan 28, 2024

hawkingrei Jan 28, 2024

Choose a reason for hiding this comment

AilinKid Jan 29, 2024

Choose a reason for hiding this comment

ti-chi-bot commented Feb 1, 2024

ti-chi-bot commented Feb 1, 2024

ti-chi-bot commented Feb 1, 2024

ti-chi-bot commented Feb 8, 2024

sre-bot commented Jul 7, 2022 •

edited

Loading

codecov bot commented Jan 26, 2024 •

edited

Loading