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 the fail when we compare multi fields in the subquery #21699

Merged
merged 5 commits into from
Dec 15, 2020

Conversation

Reminiscent
Copy link
Contributor

@Reminiscent Reminiscent commented Dec 14, 2020

What problem does this PR solve?

Issue Number: close #13551 close #21674

Problem Summary:
When we use (SELECT c1, c2 FROM t2 LIMIT 1) = ANY (SELECT c1, c2 FROM t1) in the where statement, the expressionRewrite.ctxStack will store the result value of SELECT c1, c2 FROM t2 LIMIT 1. So in issue#21674, when the value of c1 is zero, the plan will be convert to tableDual.

What is changed and how it works?

So, we should set expressionRewrite.asScalar = true like handleInSubquery function. It will use the parent expression's last column from the schema to represents whether the condition is matched(https://github.com/pingcap/tidb/blob/master/planner/core/expression_rewriter.go#L558-L561).

Related changes

  • PR to update pingcap/docs/pingcap/docs-cn:
  • Need to cherry-pick to the release branch

Check List

Tests

  • Unit test
  • Integration test

Release note

  • fix the fail when we compare multi fields in the subquery

@Reminiscent Reminiscent requested a review from a team as a code owner December 14, 2020 07:24
@Reminiscent Reminiscent requested review from XuHuaiyu and removed request for a team December 14, 2020 07:24
@Reminiscent
Copy link
Contributor Author

/cc @lzmhhh123

@ti-srebot ti-srebot requested a review from lzmhhh123 December 14, 2020 07:25
@Reminiscent
Copy link
Contributor Author

/cc @winoros

@ti-srebot ti-srebot requested a review from winoros December 14, 2020 07:26
@Reminiscent
Copy link
Contributor Author

/run-all-tests

@Reminiscent
Copy link
Contributor Author

/label sig/planner, type/bug-fix

@ti-srebot ti-srebot added sig/planner SIG: Planner type/bugfix This PR fixes a bug. labels Dec 14, 2020
Copy link
Contributor

@lzmhhh123 lzmhhh123 left a comment

Choose a reason for hiding this comment

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

LGTM

@ti-srebot ti-srebot added the status/LGT1 Indicates that a PR has LGTM 1. label Dec 14, 2020
@ichn-hu ichn-hu mentioned this pull request Dec 14, 2020
@@ -538,6 +539,7 @@ func (er *expressionRewriter) handleCompareSubquery(ctx context.Context, v *ast.
} else if v.Op == opcode.NE {
if v.All {
// `a != all(subq)` will be rewriten as `a not in (subq)`.
er.asScalar = true
Copy link
Contributor

Choose a reason for hiding this comment

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

The test case does not cover this?

@Reminiscent
Copy link
Contributor Author

/run-all-tests

@Reminiscent Reminiscent requested a review from XuHuaiyu December 15, 2020 06:45
@Reminiscent
Copy link
Contributor Author

@XuHuaiyu Update.PTAL!

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

@ti-srebot ti-srebot removed the status/LGT1 Indicates that a PR has LGTM 1. label Dec 15, 2020
@ti-srebot ti-srebot added the status/LGT2 Indicates that a PR has LGTM 2. label Dec 15, 2020
@XuHuaiyu
Copy link
Contributor

/merge

@ti-srebot ti-srebot added the status/can-merge Indicates a PR has been approved by a committer. label Dec 15, 2020
@ti-srebot
Copy link
Contributor

Your auto merge job has been accepted, waiting for:

  • 21078
  • 21690
  • 21499

@ti-srebot
Copy link
Contributor

/run-all-tests

@ti-srebot ti-srebot merged commit e01a142 into pingcap:master Dec 15, 2020
ti-srebot pushed a commit to ti-srebot/tidb that referenced this pull request Dec 15, 2020
@ti-srebot
Copy link
Contributor

cherry pick to release-4.0 in PR #21808

qw4990 pushed a commit that referenced this pull request Jan 26, 2021
@Reminiscent Reminiscent deleted the issue#21674 branch August 5, 2021 01:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
sig/planner SIG: Planner status/can-merge Indicates a PR has been approved by a committer. status/LGT2 Indicates that a PR has LGTM 2. type/bugfix This PR fixes a bug.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Wrong result when we compare field with zero value in subquery Compare multi fields with subquery fail
4 participants