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.

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

plan: return table dual when filter is false/null #7756

Merged
merged 7 commits into from
Sep 25, 2018

Conversation

eurekaka
Copy link
Contributor

@eurekaka eurekaka commented Sep 20, 2018

What problem does this PR solve?

Fix #7728

What is changed and how it works?

Add false/null check after constant propagation

Check List

Tests

  • Unit test

@eurekaka eurekaka added type/enhancement The issue or PR belongs to an enhancement. status/WIP sig/planner SIG: Planner labels Sep 20, 2018
@eurekaka
Copy link
Contributor Author

/run-all-tests

Copy link
Member

@winoros winoros left a comment

Choose a reason for hiding this comment

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

lgtm

@winoros winoros added the status/LGT1 Indicates that a PR has LGTM 1. label Sep 21, 2018
@@ -34,6 +34,12 @@ func addSelection(p LogicalPlan, child LogicalPlan, conditions []expression.Expr
return
}
conditions = expression.PropagateConstant(p.context(), conditions)
// Return table dual when filter is constant false or null
Copy link
Contributor

Choose a reason for hiding this comment

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

add a . at the end of this comment.

@@ -55,6 +61,11 @@ func (p *LogicalSelection) PredicatePushDown(predicates []expression.Expression)
retConditions, child := p.children[0].PredicatePushDown(append(p.Conditions, predicates...))
if len(retConditions) > 0 {
p.Conditions = expression.PropagateConstant(p.ctx, retConditions)
// Return table dual when filter is constant false or null
Copy link
Contributor

Choose a reason for hiding this comment

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

ditto

@@ -375,3 +392,21 @@ func deriveOtherConditions(p *LogicalJoin, deriveLeft bool, deriveRight bool) (l
}
return
}

// conds2TableDual build a LogicalTableDual if cond is constant false or null
Copy link
Contributor

Choose a reason for hiding this comment

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

s/ build/ builds

return nil
}
sc := p.context().GetSessionVars().StmtCtx
if isTrue, err := con.Value.ToBool(sc); (err == nil && isTrue == 0) || con.Value.IsNull() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Will it be possible that err != nil && con.Value.IsNull()

Copy link
Member

Choose a reason for hiding this comment

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

It is possible, but the constant is a NULL value, the err should be ignored.

Copy link
Member

@zz-jason zz-jason left a comment

Choose a reason for hiding this comment

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

LGTM

@eurekaka eurekaka merged commit 0001d2a into pingcap:master Sep 25, 2018
@eurekaka eurekaka deleted the join_2_dual branch September 25, 2018 02:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
sig/planner SIG: Planner status/LGT1 Indicates that a PR has LGTM 1. type/enhancement The issue or PR belongs to an enhancement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants