plan: detect table alias duplication for 'JOIN' #6716
Conversation
plan/errors.go
Outdated
| @@ -45,6 +45,7 @@ const ( | |||
| codeDupFieldName = mysql.ErrDupFieldName | |||
| codeNonUpdatableTable = mysql.ErrNonUpdatableTable | |||
| codeInternal = mysql.ErrInternal | |||
| codeNonUniqTable = terror.ErrCode(mysql.ErrNonuniqTable) | |||
There was a problem hiding this comment.
No need to wrap the terror.ErrCode()?
There was a problem hiding this comment.
No need indeed. will fix
plan/errors.go
Outdated
| @@ -75,6 +76,7 @@ var ( | |||
| ErrDupFieldName = terror.ClassOptimizer.New(codeDupFieldName, mysql.MySQLErrName[mysql.ErrDupFieldName]) | |||
| ErrNonUpdatableTable = terror.ClassOptimizer.New(codeNonUpdatableTable, mysql.MySQLErrName[mysql.ErrNonUpdatableTable]) | |||
| ErrInternal = terror.ClassOptimizer.New(codeInternal, mysql.MySQLErrName[mysql.ErrInternal]) | |||
| ErrNonUniqTable = terror.ClassOptimizer.New(codeNonUniqTable, "Not unique table/alias: '%s'") | |||
There was a problem hiding this comment.
Can we use mysql.MySQLErrName[mysql. ErrNonuniqTable]?
plan/preprocess.go
Outdated
| p.isParentJoin = true | ||
| } | ||
|
|
||
| func isTableAliasDuplicate(node ast.ResultSetNode, tableAliases map[string]interface{}) error { |
There was a problem hiding this comment.
How about s/isTableAliasDuplicate/checkDuplicatedTableAlias/, and making it a member function of p?
There was a problem hiding this comment.
isTableAliasDuplicate is an utility function, I name it follow the other utility functions in preprocess.go such as isConstraintKeyTp, isInvalidDefaultValue, isIncorrectName. I think it's better to keep this name and make it out of p
| if len(ts.AsName.L) != 0 && exists { | ||
| return ErrNonUniqTable.GenByArgs(ts.AsName) | ||
| } | ||
| tableAliases[ts.AsName.L] = nil |
There was a problem hiding this comment.
I think we should only add ts.AsName.L into tableAliases only when len(ts.AsName.L) != 0?
There was a problem hiding this comment.
It doesn't matter because there's nothing done if len(ts.AsName.L) == 0
| case *ast.Join: | ||
| if len(p.tableAliasInJoin) > 0 { | ||
| p.tableAliasInJoin = p.tableAliasInJoin[:len(p.tableAliasInJoin)-1] | ||
| } |
There was a problem hiding this comment.
isParentJoin works only in function Enter, there's no need to clear it in Leave
plan/preprocess.go
Outdated
| @@ -45,6 +45,9 @@ type preprocessor struct { | |||
| inPrepare bool | |||
| // inCreateOrDropTable is true when visiting create/drop table statement. | |||
| inCreateOrDropTable bool | |||
|
|
|||
| tableAliasInJoin []map[string]interface{} | |||
| isParentJoin bool | |||
There was a problem hiding this comment.
How about s/isParentJoin/parentIsJoin/?
plan/preprocess.go
Outdated
| @@ -45,6 +45,9 @@ type preprocessor struct { | |||
| inPrepare bool | |||
| // inCreateOrDropTable is true when visiting create/drop table statement. | |||
| inCreateOrDropTable bool | |||
|
|
|||
| tableAliasInJoin []map[string]interface{} | |||
There was a problem hiding this comment.
add a comment about this stack?
|
LGTM |
|
@tiancaiamao PTAL |
|
/run-all-tests |
|
|
||
| // tableAliasInJoin is a stack that keeps the table alias names for joins. | ||
| // len(tableAliasInJoin) may bigger than 1 because the left/right child of join may be subquery that contains `JOIN` | ||
| tableAliasInJoin []map[string]interface{} |
There was a problem hiding this comment.
I wonder that is it really necessary to be a slice of map ?
|
LGTM |
tiancaiamao
added
status/LGT2
fix #3908