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

executor: specially handle empty input for apply's outer child aggregate (#20544) #21137

Merged
merged 6 commits into from
Jan 29, 2021

Conversation

ti-srebot
Copy link
Contributor

@ti-srebot ti-srebot commented Nov 19, 2020

cherry-pick #20544 and #21902 to release-4.0


What problem does this PR solve?

Issue Number: close #13749

Problem Summary:

Wrong query result for Apply operator when its outer child is an aggregate, and the input for the aggregate is empty.

What is changed and how it works?

What's Changed:

If the input for any aggregate in the outer executor tree is empty, we believe this outer row has no match in the apply, since inner child of apply can only reference columns from base outer tables(i.e, aggregate result cannot be referenced in the subquery). Exceptions are cases where outer child contains Join / Union executors, we specially handle them.

Related changes

  • Need to cherry-pick to the release branch

Check List

Tests

  • Unit test

Side effects

N/A

Release note

  • Fix incorrect result for some queries containing dependent subquery and aggregate functions.

@ti-srebot
Copy link
Contributor Author

/run-all-tests

@ti-srebot
Copy link
Contributor Author

@eurekaka you're already a collaborator in bot's repo.

@eurekaka
Copy link
Contributor

/run-all-tests

@lzmhhh123
Copy link
Contributor

@eurekaka Please fix the test.

@eurekaka
Copy link
Contributor

/run-check_dev

@eurekaka
Copy link
Contributor

/run-all-tests tidb-test=pr/ 1138

1 similar comment
@eurekaka
Copy link
Contributor

/run-all-tests tidb-test=pr/ 1138

@eurekaka
Copy link
Contributor

/run-all-tests tidb-test=pr/1138

@eurekaka
Copy link
Contributor

/run-integration-compatibility-test tidb-test=pr/1138

@eurekaka
Copy link
Contributor

/run-integration-compatibility-test tidb-test=pr/1138

@eurekaka
Copy link
Contributor

/run-all-tests tidb-test=pr/1138

@eurekaka
Copy link
Contributor

/run-all-tests tidb-test=pr/1138

@zz-jason
Copy link
Member

/merge

@ti-srebot
Copy link
Contributor Author

@zz-jason Oops! This PR requires at least 2 LGTMs to merge. The current number of LGTM is 0

@qw4990
Copy link
Contributor

qw4990 commented Jan 27, 2021

PTAL @eurekaka Please resolve conflicts.

@eurekaka
Copy link
Contributor

@eurekaka
Copy link
Contributor

/run-all-tests tidb-test=pr/1138

@eurekaka
Copy link
Contributor

/run-all-tests tidb-test=pr/1138

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

@ti-srebot ti-srebot added the status/LGT1 Indicates that a PR has LGTM 1. label Jan 29, 2021
Copy link
Contributor

@qw4990 qw4990 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 status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Jan 29, 2021
@qw4990
Copy link
Contributor

qw4990 commented Jan 29, 2021

/run-all-tests

@qw4990 qw4990 merged commit 71b8151 into pingcap:release-4.0 Jan 29, 2021
@eurekaka eurekaka deleted the release-4.0-2e918a2e8fb9 branch January 29, 2021 06:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants