planner: fix ExtractCorrelatedCols method in PhysicalTableScan #51205

Lloyd-Pottiger · 2024-02-20T15:40:31Z

What problem does this PR solve?

Issue Number: close #51204

Problem Summary:

What changed and how does it work?

Before, we do not try to extract correlated column in LateMaterializationFilterCondition. So when the correlated columns only in LateMaterializationFilterCondition, exprs failed to fill in correlated column with constant value during Plan2Pb, and it leads to wrong query result.

Now, in ExtractCorrelatedCols method, we will also try to extract correlated column in LateMaterializationFilterCondition.

Also fix other methods.

Check List

Currently, integration test do not support TiFlash, so I will add tests in https://github.com/pingcap/tiflash

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.

Fix wrong query result of Apply operator when there are correlated column in push down conditions

Signed-off-by: Lloyd-Pottiger <yan1579196623@gmail.com>

tiprow · 2024-02-20T15:40:46Z

Hi @Lloyd-Pottiger. 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.

JaySon-Huang · 2024-02-20T15:54:30Z

please update the "Problem Summary" and "Release note"

Lloyd-Pottiger · 2024-02-20T16:07:03Z

please update the "Problem Summary" and "Release note"

done

codecov · 2024-02-20T16:07:07Z

Codecov Report

Merging #51205 (a83fd24) into master (a465300) will decrease coverage by 16.4706%.
Report is 23 commits behind head on master.
The diff coverage is 50.0000%.

Additional details and impacted files

@@                Coverage Diff                @@
##             master     #51205         +/-   ##
=================================================
- Coverage   70.6268%   54.1562%   -16.4706%     
=================================================
  Files          1467       1576        +109     
  Lines        434809     605291     +170482     
=================================================
+ Hits         307092     327803      +20711     
- Misses       108517     254591     +146074     
- Partials      19200      22897       +3697

Flag	Coverage Δ
integration	`20.9225% <0.0000%> (?)`
unit	`70.3229% <50.0000%> (-0.1090%)`	⬇️

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

Components	Coverage Δ
dumpling	`53.9957% <ø> (-0.0313%)`	⬇️
parser	`∅ <ø> (∅)`
br	`50.4113% <ø> (+4.5428%)`	⬆️

windtalker

LGTM

hawkingrei · 2024-02-21T02:04:25Z

pkg/planner/core/physical_plans.go

@@ -935,13 +936,16 @@ func (ts *PhysicalTableScan) Clone() (PhysicalPlan, error) {

 // ExtractCorrelatedCols implements PhysicalPlan interface.
 func (ts *PhysicalTableScan) ExtractCorrelatedCols() []*expression.CorrelatedColumn {
-	corCols := make([]*expression.CorrelatedColumn, 0, len(ts.AccessCondition)+len(ts.filterCondition))
+	corCols := make([]*expression.CorrelatedColumn, 0, len(ts.AccessCondition)+len(ts.filterCondition)+len(ts.LateMaterializationFilterCondition))


Can you add the tests for it?

SeaRise

LGTM

SeaRise · 2024-02-21T03:13:16Z

pkg/planner/core/physical_plans.go

@@ -935,13 +936,16 @@ func (ts *PhysicalTableScan) Clone() (PhysicalPlan, error) {

 // ExtractCorrelatedCols implements PhysicalPlan interface.
 func (ts *PhysicalTableScan) ExtractCorrelatedCols() []*expression.CorrelatedColumn {
-	corCols := make([]*expression.CorrelatedColumn, 0, len(ts.AccessCondition)+len(ts.filterCondition))
+	corCols := make([]*expression.CorrelatedColumn, 0, len(ts.AccessCondition)+len(ts.filterCondition)+len(ts.LateMaterializationFilterCondition))


Will CorrelatedCols be explicit in explan plan?
If so maybe a ut can be added.

SeaRise · 2024-02-21T03:13:29Z

/hold

ti-chi-bot · 2024-02-21T03:13:32Z

[LGTM Timeline notifier]

Timeline:

2024-02-21 01:16:37.928351322 +0000 UTC m=+406286.675974432: ☑️ agreed by windtalker.
2024-02-21 03:13:31.381374051 +0000 UTC m=+413300.128997162: ☑️ agreed by SeaRise.

…y-pick #51205) (#51208) close #51204

Signed-off-by: Lloyd-Pottiger <yan1579196623@gmail.com>

Lloyd-Pottiger · 2024-02-21T06:22:40Z

/retest

tiprow · 2024-02-21T06:23:02Z

@Lloyd-Pottiger: 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.

fixdb · 2024-02-21T07:17:52Z

pkg/planner/core/physical_plan_test.go

@@ -437,3 +437,57 @@ func TestPhysicalPlanMemoryTrace(t *testing.T) {
 	pp.MPPPartitionCols = append(pp.MPPPartitionCols, &property.MPPPartitionColumn{})
 	require.Greater(t, pp.MemoryUsage(), size)
 }
+
+func TestPhysicalTableScanExtractCorrelatedCols(t *testing.T) {


Can we just use json output file to compare the whole physical plan? It is hard to see what is the purpose of this test case.

I can add some comments.

It is only used to test ExtractCorrelatedCols, compare the whole plan do not meet our needs.

Signed-off-by: Lloyd-Pottiger <yan1579196623@gmail.com>

Lloyd-Pottiger · 2024-02-21T09:08:26Z

/unhold

fixdb

+1

ti-chi-bot · 2024-02-21T09:13:22Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: fixdb, SeaRise, windtalker

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~~ [fixdb,windtalker]

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-02-21T09:49:12Z

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

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

ti-chi-bot · 2024-02-21T09:49:52Z

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

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

… (#51227) close #51204

… (#51228) close #51204

…d in PhysicalTableScan (pingcap#887) * planner: fix ExtractCorrelatedCols method in PhysicalTableScan (pingcap#51205) (pingcap#51227) close pingcap#51204 * make check Signed-off-by: zzm <zhouzemin@pingcap.com> * gctuner: make TestIssue48741 more stable (pingcap#49430) --------- Signed-off-by: zzm <zhouzemin@pingcap.com> Co-authored-by: Ti Chi Robot <ti-community-prow-bot@tidb.io> Co-authored-by: Hangjie Mo <mohangjie1995@gmail.com>

fix

43b9010

Signed-off-by: Lloyd-Pottiger <yan1579196623@gmail.com>

ti-chi-bot bot added do-not-merge/needs-triage-completed do-not-merge/needs-tests-checked release-note-none Denotes a PR that doesn't merit a release note. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Feb 20, 2024

ti-chi-bot bot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-none Denotes a PR that doesn't merit a release note. labels Feb 20, 2024

ti-chi-bot added 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 Feb 21, 2024

windtalker approved these changes Feb 21, 2024

View reviewed changes

ti-chi-bot bot added approved needs-1-more-lgtm Indicates a PR needs 1 more LGTM. and removed do-not-merge/needs-triage-completed labels Feb 21, 2024

hawkingrei reviewed Feb 21, 2024

View reviewed changes

hawkingrei mentioned this pull request Feb 21, 2024

planner: fix ExtractCorrelatedCols method in PhysicalTableScan (cherry-pick #51205) #51208

Merged

13 tasks

SeaRise approved these changes Feb 21, 2024

View reviewed changes

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

ti-chi-bot bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 21, 2024

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

planner: fix ExtractCorrelatedCols method in PhysicalTableScan (cherr…

074d6d1

…y-pick #51205) (#51208) close #51204

add ut

e357cfc

Signed-off-by: Lloyd-Pottiger <yan1579196623@gmail.com>

ti-chi-bot bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Feb 21, 2024

rename

f2f321b

Signed-off-by: Lloyd-Pottiger <yan1579196623@gmail.com>

fixdb reviewed Feb 21, 2024

View reviewed changes

Lloyd-Pottiger added 2 commits February 21, 2024 16:56

refine

ece9b22

Signed-off-by: Lloyd-Pottiger <yan1579196623@gmail.com>

refine

a83fd24

Signed-off-by: Lloyd-Pottiger <yan1579196623@gmail.com>

ti-chi-bot bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 21, 2024

fixdb approved these changes Feb 21, 2024

View reviewed changes

ti-chi-bot bot removed the do-not-merge/needs-tests-checked label Feb 21, 2024

ti-chi-bot bot merged commit a0e0969 into pingcap:master Feb 21, 2024
21 checks passed

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

planner: fix ExtractCorrelatedCols method in PhysicalTableScan (#51205) #51227

Merged

13 tasks

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

This is an automated cherry-pick of pingcap#51205

228e00e

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

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

This is an automated cherry-pick of pingcap#51205

06285f7

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

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

planner: fix ExtractCorrelatedCols method in PhysicalTableScan (#51205) #51228

Merged

13 tasks

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

planner: fix ExtractCorrelatedCols method in PhysicalTableScan (#51205)…

99e14aa

… (#51227) close #51204

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

planner: fix ExtractCorrelatedCols method in PhysicalTableScan (#51205)…

1fdeeb7

… (#51228) close #51204

Lloyd-Pottiger deleted the fix-lm-cor branch February 22, 2024 12:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

planner: fix ExtractCorrelatedCols method in PhysicalTableScan #51205

planner: fix ExtractCorrelatedCols method in PhysicalTableScan #51205

Lloyd-Pottiger commented Feb 20, 2024 •

edited

Loading

tiprow bot commented Feb 20, 2024

JaySon-Huang commented Feb 20, 2024

Lloyd-Pottiger commented Feb 20, 2024

codecov bot commented Feb 20, 2024 •

edited

Loading

windtalker left a comment

hawkingrei Feb 21, 2024

Lloyd-Pottiger Feb 21, 2024

Lloyd-Pottiger Feb 21, 2024

SeaRise left a comment

SeaRise Feb 21, 2024

Lloyd-Pottiger Feb 21, 2024

SeaRise commented Feb 21, 2024

ti-chi-bot bot commented Feb 21, 2024

Lloyd-Pottiger commented Feb 21, 2024

tiprow bot commented Feb 21, 2024

fixdb Feb 21, 2024

Lloyd-Pottiger Feb 21, 2024

Lloyd-Pottiger commented Feb 21, 2024

fixdb left a comment

ti-chi-bot bot commented Feb 21, 2024

ti-chi-bot commented Feb 21, 2024

ti-chi-bot commented Feb 21, 2024

planner: fix ExtractCorrelatedCols method in PhysicalTableScan #51205

planner: fix ExtractCorrelatedCols method in PhysicalTableScan #51205

Conversation

Lloyd-Pottiger commented Feb 20, 2024 • edited Loading

What problem does this PR solve?

What changed and how does it work?

Check List

Release note

tiprow bot commented Feb 20, 2024

JaySon-Huang commented Feb 20, 2024

Lloyd-Pottiger commented Feb 20, 2024

codecov bot commented Feb 20, 2024 • edited Loading

Codecov Report

windtalker left a comment

Choose a reason for hiding this comment

hawkingrei Feb 21, 2024

Choose a reason for hiding this comment

Lloyd-Pottiger Feb 21, 2024

Choose a reason for hiding this comment

Lloyd-Pottiger Feb 21, 2024

Choose a reason for hiding this comment

SeaRise left a comment

Choose a reason for hiding this comment

SeaRise Feb 21, 2024

Choose a reason for hiding this comment

Lloyd-Pottiger Feb 21, 2024

Choose a reason for hiding this comment

SeaRise commented Feb 21, 2024

ti-chi-bot bot commented Feb 21, 2024

[LGTM Timeline notifier]

Lloyd-Pottiger commented Feb 21, 2024

tiprow bot commented Feb 21, 2024

fixdb Feb 21, 2024

Choose a reason for hiding this comment

Lloyd-Pottiger Feb 21, 2024

Choose a reason for hiding this comment

Lloyd-Pottiger commented Feb 21, 2024

fixdb left a comment

Choose a reason for hiding this comment

ti-chi-bot bot commented Feb 21, 2024

ti-chi-bot commented Feb 21, 2024

ti-chi-bot commented Feb 21, 2024

Lloyd-Pottiger commented Feb 20, 2024 •

edited

Loading

codecov bot commented Feb 20, 2024 •

edited

Loading