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/core: force first column used if no columns required (#9125) #9754

Closed
wants to merge 1 commit into from

Conversation

jarvys
Copy link
Contributor

@jarvys jarvys commented Mar 16, 2019

What problem does this PR solve?

Fix issue #9125

What is changed and how it works?

For sql like select count(1) from (select count(1) from t), no column used for the inner Aggregation. TiKV's response will be empty if the inner Aggregation is pushed down. So force some column to be used for the inner Aggregation.

Check List

Tests

  • Unit test (a new test case TestIssue9125 in cbo_test.go)

Code changes

  • None

Side effects

  • None

Related changes

  • None

@codecov
Copy link

codecov bot commented Mar 16, 2019

Codecov Report

Merging #9754 into master will decrease coverage by 0.0242%.
The diff coverage is 100%.

@@               Coverage Diff               @@
##            master      #9754        +/-   ##
===============================================
- Coverage   67.191%   67.1668%   -0.0243%     
===============================================
  Files          381        381                
  Lines        79835      79837         +2     
===============================================
- Hits         53642      53624        -18     
- Misses       21408      21422        +14     
- Partials      4785       4791         +6

@morgo morgo added contribution This PR is from a community contributor. sig/planner SIG: Planner labels Mar 17, 2019
@eurekaka eurekaka requested a review from winoros March 18, 2019 02:52
// For sql like `select count(1) from (select count(1) from t)`, no column used.
// tikv's response will be empty if the inner aggr is pushed down.
// We should force some column to be used.
used[0] = true
Copy link
Contributor

Choose a reason for hiding this comment

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

This fix looks pretty ad-hoc, IMHO a better approach would be extracting columns from aggregation(constant).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are right -_-. I had the same feeling when I tried to fix this issue. Thanks for your feedback and advice.

@winoros
Copy link
Member

winoros commented Jul 23, 2019

Thanks for your contribution.
But we have to close this one since #9125 was closed.
Very sorry for your inconvenience.

@winoros winoros closed this Jul 23, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
contribution This PR is from a community contributor. sig/planner SIG: Planner
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants