planner: fix index-out-of-range error when checking only_full_group_by #23844
Conversation
ti-chi-bot
added
the
size/S
|
[REVIEW NOTIFICATION] This pull request has been approved by:
To complete the pull request process, please ask the reviewers in the list to review by filling The full list of commands accepted by this bot can be found here. Reviewer can indicate their review by writing |
ti-chi-bot
added
status/LGT2
|
/merge |
|
This pull request has been accepted and is ready to merge. Commit hash: 82e5bd4
|
ti-chi-bot
added
the
status/can-merge
|
|
/needs-cherry-pick-5.0 |
|
/cherrypick release-5.0 |
|
/cherry-pick release-5.0 |
|
/label needs-cherry-pick-5.0 |
|
/run-cherry-picker |
|
cherry pick to release-5.0 in PR #24016 |
Signed-off-by: ti-srebot <ti-srebot@pingcap.com>
| @@ -2676,7 +2676,7 @@ func checkColFuncDepend( | |||
| Name: colInfo.Name, | |||
| } | |||
| pIdx, err := expression.FindFieldName(p.OutputNames(), pkName) | |||
| if err != nil { | |||
| if err != nil || pIdx < 0 { | |||
There was a problem hiding this comment.
Better to add a comment to explain when pIdx will be less than 0.
There was a problem hiding this comment.
Here is a case when pIdx will be less than 0.
CREATE TABLE `BB` (
`pk` int(11) NOT NULL AUTO_INCREMENT,
`col_int_not_null` int NOT NULL,
PRIMARY KEY (`pk`)
);
SELECT OUTR . col2 AS X
FROM
BB AS OUTR2
INNER JOIN
(SELECT col_int_not_null AS col1,
pk AS col2
FROM BB) AS OUTR ON OUTR2.col_int_not_null = OUTR.col1
GROUP BY OUTR2.col_int_not_null;
When we checkOnlyFullGroupBy for the SELECT statement and enter checkColFuncDepend, pkName.Table is OUTR which is an alias, while pkName.Name is pk which is a original name. Hence expression.FindFieldName will fail and pIdx will be less than 0. Currently, when we meet pIdx < 0, we directly regard primaryFuncDepend as false and jump out. This way is easy to implement but makes only-full-group-by checker not smart enough. Later we will refactor only-full-group-by checker and resolve the inconsistency between the alias table name and the original column name.
There was a problem hiding this comment.
I mean, you should add comment in the code
There was a problem hiding this comment.
The comment has been added into code by #25769
|
/label needs-cherry-pick-4.0 |
|
/run-cherry-picker |
|
cherry pick to release-4.0 in PR #30645 |
Signed-off-by: ti-srebot <ti-srebot@pingcap.com>
What problem does this PR solve?
Issue Number: close #23839
Problem Summary:
TiDB throws index-out-of-range error when checking only_full_group_by for a complex sql.
What is changed and how it works?
What's Changed & How it Works:
In function
checkColFuncDepend, ifexpression.FindFieldNamereturns-1, i.e., it doesn't find the column name, we just jump out of loop.However, the problem is not entirely solved. Currently TiDB thinks the sql in #23839 violates only_full_group_by rule but MySQL doesn't think so. As #22301 (comment) says,
only-full-group-by checkerwill be refactored in the future.Related changes
pingcap/docs/pingcap/docs-cn:Check List
Tests
Release note