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

expression, session: handle CASE WHEN specially when folding constant during outerJoin Simplification #11105

Merged
merged 22 commits into from
Jul 24, 2019
Merged

expression, session: handle CASE WHEN specially when folding constant during outerJoin Simplification #11105

merged 22 commits into from
Jul 24, 2019

Conversation

lovewin99
Copy link
Contributor

What problem does this PR solve?

fix issue:#11102 Unexcepted result in SELECT ... CASE WHEN ... ELSE NULL...

What is changed and how it works?

Check List

Tests

  • Unit test

lovewin99 added 2 commits July 5, 2019 19:27
@lovewin99
fix issue:pingcap#11102
7b00dbb 
                                                            Unexcepted result in `SELECT ... CASE WHEN ... ELSE NULL...`
@lovewin99
fix issue:pingcap#11102
f933936 
                                                            Unexcepted result in `SELECT ... CASE WHEN ... ELSE NULL...`
@codecov
Copy link

codecov bot commented Jul 6, 2019 •
edited
Loading

Codecov Report

Merging #11105 into master will not change coverage.
The diff coverage is n/a.

@@             Coverage Diff             @@
##             master     #11105   +/-   ##
===========================================
  Coverage   81.4185%   81.4185%           
===========================================
  Files           424        424           
  Lines         90795      90795           
===========================================
  Hits          73924      73924           
  Misses        11571      11571           
  Partials       5300       5300

@shenli shenli added contribution This PR is from a community contributor. type/bugfix This PR fixes a bug. labels Jul 8, 2019
@XuHuaiyu XuHuaiyu changed the title expression: fix issue:#11102 Unexcepted result in SELECT ... CASE WHEN ... ELSE NULL... expression: fix unexcepted result in SELECT ... CASE WHEN ... ELSE NULL... Jul 8, 2019
@XuHuaiyu XuHuaiyu added sig/planner SIG: Planner and removed component/expression labels Jul 8, 2019
@XuHuaiyu
Copy link
Contributor

XuHuaiyu commented Jul 8, 2019

/run-all-tests

1 similar comment
@lovewin99
Copy link
Contributor Author

/run-all-tests

@lovewin99
Copy link
Contributor Author

/run-integration-common-test
/run-common-test
/run-unit-test

@lovewin99
Copy link
Contributor Author

/run-all-tests

@mahjonp
Copy link
Contributor

mahjonp commented Jul 8, 2019

/rebuild

@lovewin99
Copy link
Contributor Author

/run-all-tests

@XuHuaiyu
Copy link
Contributor

XuHuaiyu commented Jul 8, 2019

A simpler sql to re-produce this issue:

tidb> select t1.id from kankan1 t1 left join kankan2 t2 on t1.id = t2.id where (case  when t1.name='b' then 'case2' when t1.name='a' then 'case1' else NULL end) = 'case1';
+------+
| id   |
+------+
|    2 |
+------+

The root cause for this bug is that:
(case when t1.name='b' then 'case2' when t1.name='a' then 'case1' else NULL end) = 'case1' is regarded as a null-reject condition since it returns false after constant folding. Then left outer join is simplified to inner join wrongly.

@XuHuaiyu
Copy link
Contributor

XuHuaiyu commented Jul 8, 2019

I wonder that can we put case-when in specialFoldHandler instead of forbidding it.

lovewin99 added 2 commits July 9, 2019 13:50
@lovewin99
fix issue:pingcap#11102
760d28a 
                                                            Unexcepted result in `SELECT ... CASE WHEN ... ELSE NULL...`
@lovewin99
fix issue:pingcap#11102
c7300dc 
                                                            Unexcepted result in `SELECT ... CASE WHEN ... ELSE NULL...`
@lovewin99
Copy link
Contributor Author

lovewin99 commented Jul 9, 2019 •
edited
Loading

I wonder that can we put case-when in specialFoldHandler instead of forbidding it.

@XuHuaiyu Good advice, I have modified it.

@lovewin99
Copy link
Contributor Author

/rebuild

1 similar comment
@lovewin99
Copy link
Contributor Author

/rebuild

winkyao
winkyao reviewed Jul 10, 2019
View reviewed changes
expression/constant_fold.go Outdated
isDeferredConst = isDeferredConst || isDeferred1
}

if l%2 == 1 && isFirstCondition {
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this mean the else expr? Please add some comments.

@lovewin99
fix issue:pingcap#11102
33565a0 
                                                            Unexcepted result in `SELECT ... CASE WHEN ... ELSE NULL...`
winkyao
winkyao reviewed Jul 12, 2019
View reviewed changes
Copy link
Contributor

@winkyao winkyao 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 the status/LGT2 Indicates that a PR has LGTM 2. label Jul 24, 2019
@zz-jason
Copy link
Member

/run-all-tests

@lovewin99
Copy link
Contributor Author

/run-all-tests

@zz-jason
Copy link
Member

/run-sqllogic-test-1

@lovewin99
Copy link
Contributor Author

/run-sqllogic-test-1

@lovewin99
Copy link
Contributor Author

/run-all-tests

@lovewin99
Copy link
Contributor Author

Repaired test. PTAL @zz-jason @XuHuaiyu

zz-jason
zz-jason approved these changes Jul 24, 2019
View reviewed changes
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/can-merge Indicates a PR has been approved by a committer. status/all tests passed and removed status/LGT2 Indicates that a PR has LGTM 2. labels Jul 24, 2019
@sre-bot
Copy link
Contributor

sre-bot commented Jul 24, 2019

/run-all-tests

@sre-bot
Copy link
Contributor

sre-bot commented Jul 24, 2019

success

@sre-bot sre-bot merged commit c95d42a into pingcap:master Jul 24, 2019
@sre-bot
Copy link
Contributor

sre-bot commented Jul 24, 2019

cherry pick to release-2.1 failed

@sre-bot
Copy link
Contributor

sre-bot commented Jul 24, 2019

cherry pick to release-3.0 failed

lovewin99 added a commit to lovewin99/tidb that referenced this pull request Jul 25, 2019
@lovewin99
expression, session: handle CASE WHEN specially when folding consta…
876f1db 
…nt during outerJoin Simplification (pingcap#11105)

Test pass, auto merge by Bot
@lovewin99 lovewin99 mentioned this pull request Jul 25, 2019
Merged
lovewin99 added a commit to lovewin99/tidb that referenced this pull request Jul 25, 2019
@lovewin99
expression, session: handle CASE WHEN specially when folding consta…
37ef7f9 
…nt during outerJoin Simplification (pingcap#11105)

Test pass, auto merge by Bot
@lovewin99 lovewin99 mentioned this pull request Jul 25, 2019
Merged
sre-bot pushed a commit that referenced this pull request Jul 26, 2019
@lovewin99 @sre-bot
expression, session: handle CASE WHEN specially when folding consta…
64b4ffc 
…nt during outerJoin Simplification (#11105) (#11438)

All tests passed, auto merged by Bot
@lovewin99 lovewin99 deleted the master-3.0-issue branch July 30, 2019 06:37
This was referenced Aug 3, 2019
Closed
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@XuHuaiyu XuHuaiyu XuHuaiyu left review comments

@winkyao winkyao winkyao left review comments

@zz-jason zz-jason zz-jason approved these changes

Assignees
No one assigned
Labels
contribution This PR is from a community contributor. sig/planner SIG: Planner status/can-merge Indicates a PR has been approved by a committer. type/bugfix This PR fixes a bug.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@lovewin99 @XuHuaiyu @mahjonp @zimulala @zz-jason @sre-bot @winkyao @shenli