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

plan: fix concat in group statement #7448

Merged
merged 6 commits into from
Aug 21, 2018
Merged

plan: fix concat in group statement #7448

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
20946ba
fix issue 7331
Aug 21, 2018
2e6120f
add comment
Aug 21, 2018
5179fc9
add comment
Aug 21, 2018
2e054e3
change comment
Aug 21, 2018
bc19f43
Merge branch 'master' into fix7331
Aug 21, 2018
da60051
Merge branch 'master' into fix7331
XuHuaiyu Aug 21, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions plan/logical_plan_builder.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -854,6 +854,13 @@ func matchField(f *ast.SelectField, col *ast.ColumnNameExpr, ignoreAsName bool)
if f.AsName.L == "" || ignoreAsName {
if curCol, isCol := f.Expr.(*ast.ColumnNameExpr); isCol {
return curCol.Name.Name.L == col.Name.Name.L
} else if _, isFunc := f.Expr.(*ast.FuncCallExpr); isFunc {
Copy link
Member

Choose a reason for hiding this comment

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

it's better to add a comment here to explain why we add this check and what issue it fixes.

Copy link
Author

Choose a reason for hiding this comment

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

ok

// Fix issue 7331
// If there are some function calls in SelectField, we check if
// ColumnNameExpr in GroupByClause matches one of these function calls.
// Example: select concat(k1,k2) from t group by `concat(k1,k2)`,
// `concat(k1,k2)` matches with function call concat(k1, k2).
return strings.ToLower(f.Text()) == col.Name.Name.L
}
// a expression without as name can't be matched.
return false
Expand Down
8 changes: 8 additions & 0 deletions plan/logical_plan_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1171,6 +1171,14 @@ func (s *testPlanSuite) TestValidate(c *C) {
sql: "select a from t having sum(avg(a))",
err: ErrInvalidGroupFuncUse,
},
{
sql: "select concat(c_str, d_str) from t group by `concat(c_str, d_str)`",
err: nil,
},
{
sql: "select concat(c_str, d_str) from t group by `concat(c_str,d_str)`",
Copy link
Member

Choose a reason for hiding this comment

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

This should not result in an error.

Copy link
Member

Choose a reason for hiding this comment

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

this is the result of mysql:

MySQL(localhost:3306) > select concat(k1, k2) from t group by concat(k1,k2);
+----------------+
| concat(k1, k2) |
+----------------+
| 12             |
+----------------+
1 row in set (0.01 sec)

Copy link
Author

Choose a reason for hiding this comment

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

mysql> select concat(k1, k2) from t group by `concat(k1,k2)`;
ERROR 1054 (42S22): Unknown column 'concat(k1,k2)' in 'group statement'

`concat(k1,k2)` is different from concat(k1,k2).

Copy link
Member

Choose a reason for hiding this comment

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

😂 Got it

Copy link
Contributor

Choose a reason for hiding this comment

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

It'll be better to add the test case select concat(k1, k2) from t group by concat(k1,k2); here.

Copy link
Author

Choose a reason for hiding this comment

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

@XuHuaiyu TiDB always support this case.😂

err: ErrUnknownColumn,
},
}
for _, tt := range tests {
sql := tt.sql
Expand Down