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

planner: fix wrong row compare logic in expression_rewriter #8241

Merged
merged 3 commits into from Nov 8, 2018
Merged

planner: fix wrong row compare logic in expression_rewriter #8241

merged 3 commits into from Nov 8, 2018

Conversation

imtbkcat
Copy link

@imtbkcat imtbkcat commented Nov 8, 2018

What problem does this PR solve?

When execute this SQL: SELECT (1,2,3) < (1,NULL, 3).
TiDB:

mysql> SELECT (1,2,3) < (1,    NULL, 3);
+---------------------------+
| (1,2,3) < (1,    NULL, 3) |
+---------------------------+
|                         0 |
+---------------------------+
1 row in set (0.00 sec)

MySQL:

mysql> SELECT (1,2,3) < (1,    NULL, 3);
+---------------------------+
| (1,2,3) < (1,    NULL, 3) |
+---------------------------+
|                      NULL |
+---------------------------+
1 row in set (0.00 sec)

Correct compare logic is comparing prefix of row until there is different digit between them.
If they has same prefix before NULL digit, it will return NULL.

What is changed and how it works?

adding check code before entering next recursion.

Check List

Tests

  • Unit test
  • Manual test

Side effects

  • Possible performance regression

Related changes

  • Need to cherry-pick to the release branch

@imtbkcat imtbkcat added type/bug-fix This PR fixes a bug. sig/planner SIG: Planner labels Nov 8, 2018
@@ -220,6 +220,24 @@ func (er *expressionRewriter) constructBinaryOpFunction(l expression.Expression,
if err != nil {
return nil, errors.Trace(err)
}
if evalexpr, ok := expr1.(*expression.Constant); ok {
_, isNull, err1 := evalexpr.EvalInt(er.ctx, chunk.Row{})
if err1 != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

if err1 != nil || isNil {
return expr1, err1
}

We do not need the errors.Trace now.

@@ -220,6 +220,24 @@ func (er *expressionRewriter) constructBinaryOpFunction(l expression.Expression,
if err != nil {
return nil, errors.Trace(err)
}
if evalexpr, ok := expr1.(*expression.Constant); ok {
_, isNull, err1 := evalexpr.EvalInt(er.ctx, chunk.Row{})
Copy link
Contributor

Choose a reason for hiding this comment

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

(er.ctx, nil) is enough.

@imtbkcat
Copy link
Author

imtbkcat commented Nov 8, 2018

@XuHuaiyu addressed

Copy link
Contributor

@XuHuaiyu XuHuaiyu left a comment

Choose a reason for hiding this comment

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

LGTM

@XuHuaiyu
Copy link
Contributor

XuHuaiyu commented Nov 8, 2018

/run-all-tests

@XuHuaiyu
Copy link
Contributor

XuHuaiyu commented Nov 8, 2018

PTAL @zz-jason

@imtbkcat imtbkcat added the status/LGT1 Indicates that a PR has LGTM 1. label Nov 8, 2018
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

@zz-jason zz-jason added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Nov 8, 2018
@imtbkcat imtbkcat merged commit 6c242ad into pingcap:master Nov 8, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
sig/planner SIG: Planner status/LGT2 Indicates that a PR has LGTM 2. type/bug-fix This PR fixes a bug.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants