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

v5.0.1-pre Inconsistent select count(1) #24149

Closed
mahjonp opened this issue Apr 20, 2021 · 6 comments · Fixed by #24180
Closed

v5.0.1-pre Inconsistent select count(1) #24149

mahjonp opened this issue Apr 20, 2021 · 6 comments · Fixed by #24180
Assignees
Labels
severity/critical sig/planner SIG: Planner type/bug The issue is confirmed as a bug.

Comments

@mahjonp
Copy link
Contributor

mahjonp commented Apr 20, 2021

Bug Report

Please answer these questions before submitting your issue. Thanks!

1. Minimal reproduce step (Required)

drop table if exists t1;
create table t1 (c1 int primary key, c2 int, c3 int, index c2 (c2));
select count(1) from (select count(1) from (select * from t1 where c3 = 100) k) k2;

2. What did you expect to see? (Required)

+----------+
| count(1) |
+----------+
| 1        |
+----------+

3. What did you see instead (Required)

+----------+
| count(1) |
+----------+
| 0        |
+----------+

4. What is your TiDB version? (Required)

 Release Version: v5.0.1
Edition: Community
Git Commit Hash: 4141f837ad4655a567119db743b7b752a95f5aa2
Git Branch: heads/refs/tags/v5.0.1
UTC Build Time: 2021-04-17 04:28:49
GoVersion: go1.13
Race Enabled: false
TiKV Min Version: v3.0.0-60965b006877ca7234adaced7890d7b029ed1306
Check Table Before Drop: false 
@mahjonp mahjonp added the type/bug The issue is confirmed as a bug. label Apr 20, 2021
@lzmhhh123
Copy link
Contributor

Can't reproduce on the master branch.

@lzmhhh123
Copy link
Contributor

lzmhhh123 commented Apr 20, 2021

It cause by
commit: f5cac30
#24024
#24093
@guo-shaoge PTAL.

@guo-shaoge
Copy link
Collaborator

guo-shaoge commented Apr 20, 2021

Caused by this PR .

HashAgg_11's func should be count, but we got firstrow. Also we should have a Agg which pushed to TiKV.

mysql> explain select count(1) from (select count(1) from (select * from t1)k) k1;
+--------------------------------+----------+-----------+---------------+--------------------------------+
| id                             | estRows  | task      | access object | operator info                  |
+--------------------------------+----------+-----------+---------------+--------------------------------+
| StreamAgg_9                    | 1.00     | root      |               | funcs:count(1)->Column#5       |
| └─Projection_10                | 1.00     | root      |               | 1->Column#11                   |
|   └─HashAgg_11                 | 1.00     | root      |               | funcs:firstrow(1)->Column#12   |
|     └─Projection_13            | 10000.00 | root      |               | 1->Column#13                   |
|       └─TableReader_15         | 10000.00 | root      |               | data:TableFullScan_14          |
|         └─TableFullScan_14     | 10000.00 | cop[tikv] | table:t1      | keep order:false, stats:pseudo |
+--------------------------------+----------+-----------+---------------+--------------------------------+

@guo-shaoge
Copy link
Collaborator

guo-shaoge commented Apr 21, 2021

Caused by this PR .

HashAgg_11's func should be count, but we got firstrow. Also we should have a Agg which pushed to TiKV.

mysql> explain select count(1) from (select count(1) from (select * from t1)k) k1;
+--------------------------------+----------+-----------+---------------+--------------------------------+
| id                             | estRows  | task      | access object | operator info                  |
+--------------------------------+----------+-----------+---------------+--------------------------------+
| StreamAgg_9                    | 1.00     | root      |               | funcs:count(1)->Column#5       |
| └─Projection_10                | 1.00     | root      |               | 1->Column#11                   |
|   └─HashAgg_11                 | 1.00     | root      |               | funcs:firstrow(1)->Column#12   |
|     └─Projection_13            | 10000.00 | root      |               | 1->Column#13                   |
|       └─TableReader_15         | 10000.00 | root      |               | data:TableFullScan_14          |
|         └─TableFullScan_14     | 10000.00 | cop[tikv] | table:t1      | keep order:false, stats:pseudo |
+--------------------------------+----------+-----------+---------------+--------------------------------+

Why we got firstrow instead of count in HashAgg_11 : Projection_10 doesn't use any column in HashAgg_11, so all agg func in it will be pruned, but we will add firstrow to keep things all right.

Why we have wrong result: The result of query select count(1) from (select count(1) from (select * from t1 where c3 = 100) k) k2 should be 1 instead of 0. Because the agg func in HashAgg_11 is firstrow, and because t1 is empty ,so HashAgg_11's result is nil, so StreamAgg_9's result is 0.

Suggested fix : We can change HashAgg_11's func from firstrow to count, which looks more reasonable.

Why the query's result will be ok if firstrow is replaced by count: When build aggregate executor, executor will have a default val(check here ), if not all agg funcs are firstrow.

@zz-jason
Copy link
Member

Projection_10 doesn't use any column in HashAgg_11

Seems unreasonable, Projection_10 is expected to use the result of count(1) from HashAgg_11.

@ti-srebot
Copy link
Contributor

Please edit this comment or add a new comment to complete the following information

Not a bug

  1. Remove the 'type/bug' label
  2. Add notes to indicate why it is not a bug

Duplicate bug

  1. Add the 'type/duplicate' label
  2. Add the link to the original bug

Bug

Note: Make Sure that 'component', and 'severity' labels are added
Example for how to fill out the template: #20100

1. Root Cause Analysis (RCA) (optional)

2. Symptom (optional)

3. All Trigger Conditions (optional)

4. Workaround (optional)

5. Affected versions

6. Fixed versions

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
severity/critical sig/planner SIG: Planner type/bug The issue is confirmed as a bug.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants