Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

planner: change more conditions that are always false to dual | tidb-test=pr/2466 #59199

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

planner: change more conditions that are always false to dual | tidb-test=pr/2466 #59199

wants to merge 2 commits into from
+141 −87

Conversation

hawkingrei
Copy link
Member

@hawkingrei hawkingrei commented Jan 24, 2025 •
edited
Loading

What problem does this PR solve?

Issue Number: close #51446
Problem Summary:

What changed and how does it work?

During the logic optimization phase, we rely on Conds2TableDual to transform conditions that are always false into dual ones. However, the previous implementation was overly simplistic, so we have added some new judgments.

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

  • Affects user behaviors
  • Contains syntax changes
  • Contains variable changes
  • Contains experimental features
  • Changes MySQL compatibility

Release note

Please refer to Release Notes Language Style Guide to write a quality release note.

None

@ti-chi-bot ti-chi-bot bot added do-not-merge/needs-linked-issue do-not-merge/needs-tests-checked 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 Jan 24, 2025
@hawkingrei hawkingrei changed the title planner: change more conditions that are always false to dual | tidb-test=pr/2466 planner: create new rules for changing more conditions that are always false to dual | tidb-test=pr/2466 Jan 24, 2025
elsa0520
elsa0520 reviewed Jan 24, 2025
View reviewed changes
Copy link
Contributor

@elsa0520 elsa0520 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please change the title of PR.
This is not create a new rule. Just put the evaluate expression in the predicate push down rule

@hawkingrei hawkingrei changed the title planner: create new rules for changing more conditions that are always false to dual | tidb-test=pr/2466 planner: change more conditions that are always false to dual | tidb-test=pr/2466 Jan 24, 2025
elsa0520
elsa0520 reviewed Jan 24, 2025
View reviewed changes
pkg/planner/core/operator/logicalop/logical_join.go Outdated
@@ -1870,9 +1871,18 @@ func deriveNotNullExpr(ctx base.PlanContext, expr expression.Expression, schema

// Conds2TableDual builds a LogicalTableDual if cond is constant false or null.
func Conds2TableDual(p base.LogicalPlan, conds []expression.Expression) base.LogicalPlan {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems that this function does not belong to logical_join. It shouldn't belong to any logical operator.
It should belong to expression utils.
Could we move it to right place ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have create new file and move it into new file.

@ti-chi-bot ti-chi-bot bot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jan 26, 2025
@ti-chi-bot ti-chi-bot
Copy link

ti-chi-bot bot commented Jan 26, 2025

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please ask for approval from hawkingrei, ensuring that each of them provides their approval before proceeding. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

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 ti-chi-bot bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jan 26, 2025
@codecov Codecov
Copy link

codecov bot commented Jan 26, 2025 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 73.4730%. Comparing base (1dad768) to head (0ac6d80).
Report is 1 commits behind head on master.

Additional details and impacted files 
@@               Coverage Diff                @@
##             master     #59199        +/-   ##
================================================
+ Coverage   73.0325%   73.4730%   +0.4405%     
================================================
  Files          1689       1690         +1     
  Lines        466944     467005        +61     
================================================
+ Hits         341021     343123      +2102     
+ Misses       104947     102929      -2018     
+ Partials      20976      20953        -23
Flag Coverage Δ
integration 42.8142% <92.3076%> (?)
unit 72.2347% <100.0000%> (+0.0123%) ⬆️

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

Components Coverage Δ
dumpling 52.6910% <ø> (ø)
parser ∅ <ø> (∅)
br 45.3417% <ø> (+0.0201%) ⬆️

@hawkingrei
update
740d77b 
Signed-off-by: Weizhen Wang <[email protected]>
@hawkingrei
update
0ac6d80 
Signed-off-by: Weizhen Wang <[email protected]>
AilinKid
AilinKid reviewed Jan 26, 2025
View reviewed changes
pkg/planner/core/operator/logicalop/expression_util.go
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how about putting it into constraint pkg

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@elsa0520 elsa0520 elsa0520 left review comments

@AilinKid AilinKid AilinKid left review comments

Assignees
No one assigned
Labels
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.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Do not need to push down ColumnRef <compare_operator> null which is always false
3 participants
@hawkingrei @AilinKid @elsa0520