planner: correct the generation of the field name #11324
Conversation
| fields = rs.Fields() | ||
| c.Assert(fields[0].Column.Name.L, Equals, "if(1,c,c)") | ||
| // It's a compatibility issue. Should be empty instead. | ||
| c.Assert(fields[0].ColumnAsName.L, Equals, "if(1,c,c)") |
There was a problem hiding this comment.
This comptibility issue is separated work. #11323
Codecov Report
@@ Coverage Diff @@
## master #11324 +/- ##
=========================================
Coverage ? 81.69%
=========================================
Files ? 424
Lines ? 92294
Branches ? 0
=========================================
Hits ? 75395
Misses ? 11596
Partials ? 5303 |
Codecov Report
@@ Coverage Diff @@
## master #11324 +/- ##
==============================================
- Coverage 81.45% 81.2767% -0.1733%
==============================================
Files 423 423
Lines 90771 90102 -669
==============================================
- Hits 73933 73232 -701
- Misses 11561 11573 +12
- Partials 5277 5297 +20 |
zz-jason
added
sig/planner
| } | ||
| if field.AsName.L != "" { | ||
| colName = field.AsName | ||
| func (b *PlanBuilder) buildProjectionFieldNameFromColumns(origField *ast.SelectField, colNameField *ast.ColumnNameExpr, c *expression.Column) (colName, origColName, tblName, origTblName, dbName model.CIStr) { |
There was a problem hiding this comment.
Can we set Schema and Table for ColumnNameExpr when building expression.Column from it? Thus we can remove c here which will make the code clearer.
There was a problem hiding this comment.
I prefer not to change the ast's content 😂
There was a problem hiding this comment.
It will not be changed.
Check https://github.com/pingcap/tidb/blob/master/planner/core/logical_plan_builder.go#L2119
There was a problem hiding this comment.
No, if there's no wild star. It's the original content.
There was a problem hiding this comment.
But we can create a new ast.ColumnNameExpr here?
https://github.com/pingcap/tidb/blob/master/planner/core/logical_plan_builder.go#L1889
There was a problem hiding this comment.
I think we don't need to change it for now. The logic of creating the field names will be rewritten in the following PRs.
|
/run-all-tests |
|
/run-common-test |
zz-jason
added
status/all tests passed
status/LGT1
|
cherry pick to release-2.1 failed |
|
cherry pick to release-3.0 in PR #11379 |
What problem does this PR solve?
fix #11313
Also, this will help us remove the
IsReferencedfield inexpression.Column.What is changed and how it works?
*expression.Columnmay be generated from a simplified expression.But one
ast.ColumnNameExprmust refer to a column, though it may be a correlated column.So check the ast instead of the expr.
Check List
Tests