executor: fix pointGet and batchPointGet for global index #47514

L-maple · 2023-10-10T07:57:37Z

What problem does this PR solve?

Issue Number: close #47513

Problem 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.

None

ti-chi-bot · 2023-10-10T07:57:47Z

Hi @L-maple. 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-10-10T07:57:58Z

Hi @L-maple. 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.

L-maple · 2023-10-10T07:58:38Z

/cc @Defined2014 @mjonss

mjonss · 2023-10-16T13:32:05Z

/ok-to-test

mjonss · 2023-10-16T13:49:48Z

@L-maple Hi, we just merged a PR to change the code layout in the repository, 47123, could you please merge latest master and resolve possible conflicts?

mjonss · 2023-10-16T13:50:06Z

/test all

mjonss

Have you considered to separate the two issues into separate PRs, they seems unrelated?

planner/core/point_get_plan.go

L-maple · 2023-10-17T13:10:34Z

Have you considered to separate the two issues into separate PRs, they seems unrelated?

OK, I will do it tomorrow.

codecov · 2023-10-17T13:21:41Z

Codecov Report

Merging #47514 (2cc45da) into master (eff1156) will increase coverage by 0.1496%.
Report is 34 commits behind head on master.
The diff coverage is 49.1228%.

Additional details and impacted files

@@               Coverage Diff                @@
##             master     #47514        +/-   ##
================================================
+ Coverage   70.9388%   71.0885%   +0.1496%     
================================================
  Files          1368       1430        +62     
  Lines        396702     423119     +26417     
================================================
+ Hits         281416     300789     +19373     
- Misses        95593     103363      +7770     
+ Partials      19693      18967       -726

Flag	Coverage Δ
integration	`44.0204% <49.1228%> (?)`
unit	`70.9388% <ø> (-0.0001%)`	⬇️

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

Components	Coverage Δ
dumpling	`53.9663% <ø> (ø)`
parser	`∅ <ø> (∅)`
br	`46.0222% <ø> (-6.8811%)`	⬇️

L-maple · 2023-11-06T03:59:17Z

/retest

L-maple · 2023-11-10T07:28:36Z

/cc @mjonss @Defined2014

mjonss

LGTM, just some minor suggestions.

pkg/planner/core/point_get_plan.go

L-maple · 2023-11-27T11:50:33Z

@Defined2014 Hi, This PR has been reviewed for a long time, maybe I should push the process.

L-maple · 2023-11-27T11:51:00Z

/retest

L-maple · 2023-12-15T08:32:22Z

@Defined2014 @tiancaiamao Hi, this PR is opened for a long time, is there any problems?

pkg/executor/point_get_test.go

pkg/planner/core/initialize.go

Defined2014 · 2023-12-15T09:41:57Z

pkg/planner/core/point_get_plan.go

@@ -1157,6 +1160,66 @@ func tryPointGetPlan(ctx sessionctx.Context, selStmt *ast.SelectStmt, check bool
 	return nil
 }

+func checkGlobalIndexForPointGetPlan(ctx sessionctx.Context, tblName *ast.TableName, tblAlias model.CIStr, fields []*ast.SelectField, pairs []nameValuePair, check bool) *PointGetPlan {


Why not just use code in tryPointGetPlan? Many lines are same as tryPointGetPlan function.

Maybe we only need L1167-L1181.

How about this one? @L-maple

Defined2014

Need update tests/integrationtest/r/executor/partition/global_index.result by

cd tests/integrationtest/
./run-tests.sh -r executor/partition/global_index

pkg/planner/core/point_get_plan.go

ti-chi-bot · 2023-12-28T12:18:52Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Defined2014, mjonss

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~~ [Defined2014,mjonss]

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-12-28T12:18:56Z

[LGTM Timeline notifier]

Timeline:

2023-11-14 13:01:02.320383072 +0000 UTC m=+4167659.907493217: ☑️ agreed by mjonss.
2023-12-28 12:18:55.901153374 +0000 UTC m=+1741026.938380301: ☑️ agreed by Defined2014.

ti-chi-bot · 2023-12-28T12:49:12Z

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

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

ti-chi-bot bot requested review from Defined2014 and mjonss October 10, 2023 07:58

L-maple force-pushed the feature/fix_pointget_for_global_index branch from 60338f3 to b3f5dde Compare October 11, 2023 04:04

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 Oct 11, 2023

L-maple force-pushed the feature/fix_pointget_for_global_index branch from b3f5dde to 5bbeac1 Compare October 11, 2023 08:35

Defined2014 removed the do-not-merge/needs-triage-completed label Oct 16, 2023

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

L-maple force-pushed the feature/fix_pointget_for_global_index branch from 5bbeac1 to c35276a Compare October 16, 2023 11:36

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 16, 2023

mjonss reviewed Oct 16, 2023

View reviewed changes

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

L-maple force-pushed the feature/fix_pointget_for_global_index branch from c35276a to 4f04e62 Compare October 17, 2023 13:07

L-maple force-pushed the feature/fix_pointget_for_global_index branch from 4f04e62 to 01d6362 Compare October 18, 2023 08:48

ti-chi-bot bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Oct 18, 2023

L-maple force-pushed the feature/fix_pointget_for_global_index branch from 01d6362 to d36d7d4 Compare October 18, 2023 09:53

mjonss approved these changes Nov 14, 2023

View reviewed changes

pkg/planner/core/point_get_plan.go Outdated Show resolved Hide resolved

pkg/planner/core/point_get_plan.go Outdated Show resolved Hide resolved

pkg/planner/core/point_get_plan.go Outdated Show resolved Hide resolved

ti-chi-bot bot added needs-1-more-lgtm Indicates a PR needs 1 more LGTM. approved labels Nov 14, 2023

L-maple force-pushed the feature/fix_pointget_for_global_index branch from 9b17cca to 21e0e50 Compare November 15, 2023 02:24

Defined2014 reviewed Dec 15, 2023

View reviewed changes

L-maple force-pushed the feature/fix_pointget_for_global_index branch from 21e0e50 to 83a0aba Compare December 26, 2023 03:41

L-maple requested a review from Defined2014 December 26, 2023 06:27

Defined2014 reviewed Dec 26, 2023

View reviewed changes

executor: fix pointGet and batchPointGet for global index

7f771b4

L-maple force-pushed the feature/fix_pointget_for_global_index branch from 83a0aba to 7f771b4 Compare December 28, 2023 10:22

L-maple requested a review from Defined2014 December 28, 2023 10:25

Defined2014 reviewed Dec 28, 2023

View reviewed changes

pkg/planner/core/point_get_plan.go Show resolved Hide resolved

Update pkg/planner/core/point_get_plan.go

afd99ed

Defined2014 reviewed Dec 28, 2023

View reviewed changes

pkg/planner/core/point_get_plan.go Outdated Show resolved Hide resolved

Update pkg/planner/core/point_get_plan.go

2cc45da

Defined2014 approved these changes Dec 28, 2023

View reviewed changes

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

ti-chi-bot bot merged commit 933e713 into pingcap:master Dec 28, 2023
18 checks passed

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

This is an automated cherry-pick of pingcap#47514

61bd605

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

ti-chi-bot mentioned this pull request Dec 28, 2023

executor: fix pointGet and batchPointGet for global index (#47514) #49892

Closed

13 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

executor: fix pointGet and batchPointGet for global index #47514

executor: fix pointGet and batchPointGet for global index #47514

L-maple commented Oct 10, 2023 •

edited

Loading

ti-chi-bot bot commented Oct 10, 2023

tiprow bot commented Oct 10, 2023

L-maple commented Oct 10, 2023

mjonss commented Oct 16, 2023

mjonss commented Oct 16, 2023

mjonss commented Oct 16, 2023

mjonss left a comment

L-maple commented Oct 17, 2023

codecov bot commented Oct 17, 2023 •

edited

Loading

L-maple commented Nov 6, 2023

L-maple commented Nov 10, 2023

mjonss left a comment

L-maple commented Nov 27, 2023

L-maple commented Nov 27, 2023

L-maple commented Dec 15, 2023

Defined2014 Dec 15, 2023

Defined2014 Dec 26, 2023

Defined2014 left a comment •

edited

Loading

ti-chi-bot bot commented Dec 28, 2023

ti-chi-bot bot commented Dec 28, 2023

ti-chi-bot commented Dec 28, 2023

executor: fix pointGet and batchPointGet for global index #47514

executor: fix pointGet and batchPointGet for global index #47514

Conversation

L-maple commented Oct 10, 2023 • edited Loading

What problem does this PR solve?

What is changed and how it works?

Check List

Release note

ti-chi-bot bot commented Oct 10, 2023

tiprow bot commented Oct 10, 2023

L-maple commented Oct 10, 2023

mjonss commented Oct 16, 2023

mjonss commented Oct 16, 2023

mjonss commented Oct 16, 2023

mjonss left a comment

Choose a reason for hiding this comment

L-maple commented Oct 17, 2023

codecov bot commented Oct 17, 2023 • edited Loading

Codecov Report

L-maple commented Nov 6, 2023

L-maple commented Nov 10, 2023

mjonss left a comment

Choose a reason for hiding this comment

L-maple commented Nov 27, 2023

L-maple commented Nov 27, 2023

L-maple commented Dec 15, 2023

Defined2014 Dec 15, 2023

Choose a reason for hiding this comment

Defined2014 Dec 26, 2023

Choose a reason for hiding this comment

Defined2014 left a comment • edited Loading

Choose a reason for hiding this comment

ti-chi-bot bot commented Dec 28, 2023

ti-chi-bot bot commented Dec 28, 2023

[LGTM Timeline notifier]

ti-chi-bot commented Dec 28, 2023

L-maple commented Oct 10, 2023 •

edited

Loading

codecov bot commented Oct 17, 2023 •

edited

Loading

Defined2014 left a comment •

edited

Loading