planner: column pruning must reserve at least one column #54778

winoros · 2024-07-19T18:37:08Z

What problem does this PR solve?

Issue Number: close #54777

Problem Summary:

What changed and how does it work?

Column pruning must reserve at least one column for correctness.

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

winoros · 2024-07-19T18:49:54Z

pkg/planner/core/logical_projection.go

+	allPruned := true
+	for i, b := range used {
+		if b || expression.ExprHasSetVarOrSleep(p.Exprs[i]) {
+			// Set to true to avoid the ExprHasSetVarOrSleep be called multiple times.
+			used[i] = true
+			allPruned = false
+			break
+		}
+	}
+	if allPruned {
+		_, ok := p.Children()[0].(*LogicalTableDual)
+		if ok {
+			// If the child is dual. The proj should not be eliminated.
+			return p, nil
+		}
+	}


For the Proj->Dual.

winoros · 2024-07-19T18:51:01Z

pkg/planner/core/rule_column_pruning.go

@@ -155,7 +177,7 @@ func (ds *DataSource) PruneColumns(parentUsedCols []*expression.Column, opt *opt
 	// Current DataSource operator contains all the filters on this table, and the columns used by these filters are always included
 	// in the output schema. Even if they are not needed by DataSource's parent operator. Thus add a projection here to prune useless columns
 	// Limit to MPP tasks, because TiKV can't benefit from this now(projection can't be pushed down to TiKV now).
-	if !addOneHandle && ds.Schema().Len() > len(parentUsedCols) && ds.SCtx().GetSessionVars().IsMPPEnforced() && ds.TableInfo.TiFlashReplica != nil {
+	if !addOneHandle && ds.Schema().Len() > len(parentUsedCols) && len(parentUsedCols) > 0 && ds.SCtx().GetSessionVars().IsMPPEnforced() && ds.TableInfo.TiFlashReplica != nil {


We should not do the project if no column is required from its parent.

winoros · 2024-07-19T18:51:44Z

pkg/planner/core/rule_result_reorder.go

@@ -98,7 +101,7 @@ func (rs *resultReorder) injectSort(lp base.LogicalPlan) base.LogicalPlan {

 func (*resultReorder) isInputOrderKeeper(lp base.LogicalPlan) bool {
 	switch lp.(type) {
-	case *LogicalSelection, *LogicalProjection, *LogicalLimit:
+	case *LogicalSelection, *LogicalProjection, *LogicalLimit, *LogicalTableDual:


If we don't add this, we'll get Proj->Sort->Dual for Proj->Dual.

codecov · 2024-07-19T19:11:11Z

Codecov Report

Attention: Patch coverage is 87.23404% with 6 lines in your changes missing coverage. Please review.

Project coverage is 73.2215%. Comparing base (c517ffb) to head (99e44b7).
Report is 1 commits behind head on master.

Additional details and impacted files

@@               Coverage Diff                @@
##             master     #54778        +/-   ##
================================================
+ Coverage   72.5717%   73.2215%   +0.6497%     
================================================
  Files          1565       1573         +8     
  Lines        440170     447658      +7488     
================================================
+ Hits         319439     327782      +8343     
+ Misses       100937      99175      -1762     
- Partials      19794      20701       +907

Flag	Coverage Δ
integration	`25.1736% <29.7872%> (?)`
unit	`71.6175% <87.2340%> (+0.0034%)`	⬆️

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

Components	Coverage Δ
dumpling	`52.9567% <ø> (ø)`
parser	`∅ <ø> (∅)`
br	`46.1229% <ø> (+0.3007%)`	⬆️

elsa0520

LGTM

pkg/planner/core/logical_projection.go

pkg/planner/core/logical_join.go

ti-chi-bot · 2024-07-22T19:44:16Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: elsa0520, time-and-fate

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:

~~pkg/planner/OWNERS~~ [elsa0520,time-and-fate]

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-07-22T19:44:18Z

[LGTM Timeline notifier]

Timeline:

2024-07-22 02:06:59.410089111 +0000 UTC m=+838041.401030576: ☑️ agreed by elsa0520.
2024-07-22 19:44:18.061853783 +0000 UTC m=+901480.052795254: ☑️ agreed by time-and-fate.

hawkingrei · 2024-07-23T05:55:53Z

/retest

hawkingrei · 2024-07-23T09:19:32Z

/retest

ti-chi-bot · 2024-07-25T08:53:53Z

PR needs rebase.

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.

winoros · 2024-07-29T17:35:31Z

/merge

ti-chi-bot · 2024-07-29T17:35:33Z

@winoros: We have migrated to builtin LGTM and approve plugins for reviewing.

👉 Please use /approve when you want approve this pull request.

The changes announcement: LGTM plugin changes

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.

close pingcap#54777

planner: column pruning must reserve at least one column

10fa10b

ti-chi-bot bot added release-note-none Denotes a PR that doesn't merit a release note. sig/planner SIG: Planner size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jul 19, 2024

winoros commented Jul 19, 2024

View reviewed changes

elsa0520 approved these changes Jul 22, 2024

View reviewed changes

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

time-and-fate reviewed Jul 22, 2024

View reviewed changes

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

time-and-fate reviewed Jul 22, 2024

View reviewed changes

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

winoros added 3 commits July 23, 2024 01:07

Merge branch 'master' into strict-no-col

c7ab595

remove addConstOneForEmptyProjection

2a57310

address comments

2dda21e

time-and-fate approved these changes Jul 22, 2024

View reviewed changes

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

ti-chi-bot bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 24, 2024

Merge branch 'master' into strict-no-col

e0faf21

ti-chi-bot bot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Jul 24, 2024

winoros added 2 commits July 29, 2024 23:43

Merge remote-tracking branch 'origin/master' into strict-no-col

b3b5896

fix the case the correlated column should be pruned

99e44b7

winoros force-pushed the strict-no-col branch from 50f386f to 99e44b7 Compare July 29, 2024 16:03

winoros removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 29, 2024

ti-chi-bot bot merged commit 60c7e61 into pingcap:master Jul 29, 2024
23 checks passed

winoros deleted the strict-no-col branch July 29, 2024 18:07

hawkingrei pushed a commit to hawkingrei/tidb that referenced this pull request Aug 1, 2024

planner: column pruning must reserve at least one column (pingcap#54778)

f0f47b1

close pingcap#54777

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

planner: column pruning must reserve at least one column #54778

planner: column pruning must reserve at least one column #54778

winoros commented Jul 19, 2024

winoros Jul 19, 2024

winoros Jul 19, 2024

winoros Jul 19, 2024

codecov bot commented Jul 19, 2024 •

edited

Loading

elsa0520 left a comment

ti-chi-bot bot commented Jul 22, 2024

ti-chi-bot bot commented Jul 22, 2024

hawkingrei commented Jul 23, 2024

hawkingrei commented Jul 23, 2024

ti-chi-bot bot commented Jul 25, 2024

winoros commented Jul 29, 2024

ti-chi-bot bot commented Jul 29, 2024

planner: column pruning must reserve at least one column #54778

planner: column pruning must reserve at least one column #54778

Conversation

winoros commented Jul 19, 2024

What problem does this PR solve?

What changed and how does it work?

Check List

Release note

winoros Jul 19, 2024

Choose a reason for hiding this comment

winoros Jul 19, 2024

Choose a reason for hiding this comment

winoros Jul 19, 2024

Choose a reason for hiding this comment

codecov bot commented Jul 19, 2024 • edited Loading

Codecov Report

elsa0520 left a comment

Choose a reason for hiding this comment

ti-chi-bot bot commented Jul 22, 2024

ti-chi-bot bot commented Jul 22, 2024

[LGTM Timeline notifier]

hawkingrei commented Jul 23, 2024

hawkingrei commented Jul 23, 2024

ti-chi-bot bot commented Jul 25, 2024

winoros commented Jul 29, 2024

ti-chi-bot bot commented Jul 29, 2024

codecov bot commented Jul 19, 2024 •

edited

Loading