Fix parsing of CAST() statements by dbussink · Pull Request #10512 · vitessio/vitess

dbussink · 2022-06-15T08:39:00Z

CAST() was treated as an alias for CONVERT() but with slightly different syntax.

This is also described in the documentation at https://dev.mysql.com/doc/refman/8.0/en/cast-functions.html, specifically:

With CAST(expr AS type) syntax, the CAST() function takes an expression of any type and produces a result value of the specified type. This operation may also be expressed as CONVERT(expr, type), which is equivalent. If expr is NULL, CAST() returns NULL.

This is wrong sadly. CAST() is not equivalent to CONVERT(), specifically in the context of a CREATE TABLE. For JSON keys, the ARRAY attribute is possible on a CAST(), but that is not accepted for a CONVERT().

The difference in parsing also shows in MySQL:

mysql> select convert(json_keys(c), char(64) array) from t;
ERROR 1064 (42000): You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near 'array) from t' at line 1
mysql> select cast(json_keys(c) as char(64) array) from t;
ERROR 1235 (42000): This version of MySQL doesn't yet support 'Use of CAST( .. AS .. ARRAY) outside of functional index in CREATE(non-SELECT)/ALTER TABLE or in general expressions'

Here the first statement can't be parsed at all. The second is properly parsed, but ARRAY is not allowed in the context of CAST() in this situation (and only in a CREATE TABLE).

This means we should really treat these as two separate expressions and don't store them both in the same structure. The change here creates a separate CAST structure, removes the ARRAY option from CONVERT and updates the grammar and all tests accordingly.

Related Issue(s)

Found as part of work related to #10203

Checklist

"Backport me!" label has been added if this change should be backported
Tests were added or are not required
Documentation was added or is not required

CAST() was treated as an alias for CONVERT() but with slightly different syntax. This is also described in the documentation at https://dev.mysql.com/doc/refman/8.0/en/cast-functions.html, specifically: With CAST(expr AS type syntax, the CAST() function takes an expression of any type and produces a result value of the specified type. This operation may also be expressed as CONVERT(expr, type), which is equivalent. If expr is NULL, CAST() returns NULL. This is wrong sadly. CAST() is not equivalent to CONVERT(), specifically in the context of a CREATE TABLE. For JSON keys, the ARRAY attribute is possible on a CAST(), but that is not accepted for a CONVERT(). The difference in parsing also shows in MySQL: ``` mysql> select convert(json_keys(c), char(64) array) from t; ERROR 1064 (42000): You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near 'array) from t' at line 1 mysql> select cast(json_keys(c) as char(64) array) from t; ERROR 1235 (42000): This version of MySQL doesn't yet support 'Use of CAST( .. AS .. ARRAY) outside of functional index in CREATE(non-SELECT)/ALTER TABLE or in general expressions' ``` Here the first statement can't be parsed at all. The second is properly parsed, but ARRAY is not allowed in the context of CAST() in this situation (and only in a CREATE TABLE). This means we should really treat these as two separate expressions and don't store them both in the same structure. The change here creates a separate CAST structure, removes the ARRAY option from CONVERT and updates the grammar and all tests accordingly. Signed-off-by: Dirkjan Bussink <d.bussink@gmail.com>

github-actions · 2022-06-15T08:39:27Z

Signed-off-by: Dirkjan Bussink <d.bussink@gmail.com>

dbussink · 2022-06-15T09:02:17Z

go/vt/vtgate/planbuilder/horizon_planning.go

-			if aggr, ok := a.Func.(sqlparser.AggrFunc); ok {
-				collID = ctx.SemTable.CollationForExpr(aggr.GetArg())
-			}
+			collID := ctx.SemTable.CollationForExpr(a.Func.GetArg())


Linter mentioned that this was a useless type check since it's already an AggrFunc so I cleaned that up.

GuptaManan100

LGTM, but let's ask one more @vitessio/query-serving member to view this

systay · 2022-06-15T11:44:01Z

go/vt/vtgate/planbuilder/horizon_planning.go

-		unary, ok := expr.(*sqlparser.ConvertExpr)
-		if ok && sqlparser.IsColName(unary.Expr) {
+		switch unary := expr.(type) {
+		case *sqlparser.CastExpr:


in the old code we were doing IsColName(unary.Expr) before accepting the type, but that was lost here. was that intentional?

@systay Ugh, missed this comment, that wasn't intentional. Let me fix that up.

systay

LGTM!

Signed-off-by: Vicent Marti <vmg@strn.cat>

vmg

The evalengine code was needlessly duplicated. Fixed that myself.

This was accidentally removed in vitessio#10512 but it shouldn't have been. Signed-off-by: Dirkjan Bussink <d.bussink@gmail.com>

This was accidentally removed in #10512 but it shouldn't have been. Signed-off-by: Dirkjan Bussink <d.bussink@gmail.com>

* Fix parsing of CAST() statements CAST() was treated as an alias for CONVERT() but with slightly different syntax. This is also described in the documentation at https://dev.mysql.com/doc/refman/8.0/en/cast-functions.html, specifically: With CAST(expr AS type syntax, the CAST() function takes an expression of any type and produces a result value of the specified type. This operation may also be expressed as CONVERT(expr, type), which is equivalent. If expr is NULL, CAST() returns NULL. This is wrong sadly. CAST() is not equivalent to CONVERT(), specifically in the context of a CREATE TABLE. For JSON keys, the ARRAY attribute is possible on a CAST(), but that is not accepted for a CONVERT(). The difference in parsing also shows in MySQL: ``` mysql> select convert(json_keys(c), char(64) array) from t; ERROR 1064 (42000): You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near 'array) from t' at line 1 mysql> select cast(json_keys(c) as char(64) array) from t; ERROR 1235 (42000): This version of MySQL doesn't yet support 'Use of CAST( .. AS .. ARRAY) outside of functional index in CREATE(non-SELECT)/ALTER TABLE or in general expressions' ``` Here the first statement can't be parsed at all. The second is properly parsed, but ARRAY is not allowed in the context of CAST() in this situation (and only in a CREATE TABLE). This means we should really treat these as two separate expressions and don't store them both in the same structure. The change here creates a separate CAST structure, removes the ARRAY option from CONVERT and updates the grammar and all tests accordingly. Signed-off-by: Dirkjan Bussink <d.bussink@gmail.com> * Handle new cast expression in evalengine and planbuilder Signed-off-by: Dirkjan Bussink <d.bussink@gmail.com> * evalengine: do not duplicate CAST/CONVERT translation Signed-off-by: Vicent Marti <vmg@strn.cat> Co-authored-by: Vicent Marti <vmg@strn.cat> Signed-off-by: Manan Gupta <manan@planetscale.com>

This was accidentally removed in vitessio#10512 but it shouldn't have been. Signed-off-by: Dirkjan Bussink <d.bussink@gmail.com>

* Fix parsing of CAST() statements CAST() was treated as an alias for CONVERT() but with slightly different syntax. This is also described in the documentation at https://dev.mysql.com/doc/refman/8.0/en/cast-functions.html, specifically: With CAST(expr AS type syntax, the CAST() function takes an expression of any type and produces a result value of the specified type. This operation may also be expressed as CONVERT(expr, type), which is equivalent. If expr is NULL, CAST() returns NULL. This is wrong sadly. CAST() is not equivalent to CONVERT(), specifically in the context of a CREATE TABLE. For JSON keys, the ARRAY attribute is possible on a CAST(), but that is not accepted for a CONVERT(). The difference in parsing also shows in MySQL: ``` mysql> select convert(json_keys(c), char(64) array) from t; ERROR 1064 (42000): You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near 'array) from t' at line 1 mysql> select cast(json_keys(c) as char(64) array) from t; ERROR 1235 (42000): This version of MySQL doesn't yet support 'Use of CAST( .. AS .. ARRAY) outside of functional index in CREATE(non-SELECT)/ALTER TABLE or in general expressions' ``` Here the first statement can't be parsed at all. The second is properly parsed, but ARRAY is not allowed in the context of CAST() in this situation (and only in a CREATE TABLE). This means we should really treat these as two separate expressions and don't store them both in the same structure. The change here creates a separate CAST structure, removes the ARRAY option from CONVERT and updates the grammar and all tests accordingly. Signed-off-by: Dirkjan Bussink <d.bussink@gmail.com> * Handle new cast expression in evalengine and planbuilder Signed-off-by: Dirkjan Bussink <d.bussink@gmail.com> * evalengine: do not duplicate CAST/CONVERT translation Signed-off-by: Vicent Marti <vmg@strn.cat> Co-authored-by: Vicent Marti <vmg@strn.cat> Signed-off-by: Dirkjan Bussink <d.bussink@gmail.com> Co-authored-by: Vicent Marti <vmg@strn.cat>

…10514 (#10517) * Fix parsing of CAST() statements (#10512) * Fix parsing of CAST() statements CAST() was treated as an alias for CONVERT() but with slightly different syntax. This is also described in the documentation at https://dev.mysql.com/doc/refman/8.0/en/cast-functions.html, specifically: With CAST(expr AS type syntax, the CAST() function takes an expression of any type and produces a result value of the specified type. This operation may also be expressed as CONVERT(expr, type), which is equivalent. If expr is NULL, CAST() returns NULL. This is wrong sadly. CAST() is not equivalent to CONVERT(), specifically in the context of a CREATE TABLE. For JSON keys, the ARRAY attribute is possible on a CAST(), but that is not accepted for a CONVERT(). The difference in parsing also shows in MySQL: ``` mysql> select convert(json_keys(c), char(64) array) from t; ERROR 1064 (42000): You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near 'array) from t' at line 1 mysql> select cast(json_keys(c) as char(64) array) from t; ERROR 1235 (42000): This version of MySQL doesn't yet support 'Use of CAST( .. AS .. ARRAY) outside of functional index in CREATE(non-SELECT)/ALTER TABLE or in general expressions' ``` Here the first statement can't be parsed at all. The second is properly parsed, but ARRAY is not allowed in the context of CAST() in this situation (and only in a CREATE TABLE). This means we should really treat these as two separate expressions and don't store them both in the same structure. The change here creates a separate CAST structure, removes the ARRAY option from CONVERT and updates the grammar and all tests accordingly. Signed-off-by: Dirkjan Bussink <d.bussink@gmail.com> * Handle new cast expression in evalengine and planbuilder Signed-off-by: Dirkjan Bussink <d.bussink@gmail.com> * evalengine: do not duplicate CAST/CONVERT translation Signed-off-by: Vicent Marti <vmg@strn.cat> Co-authored-by: Vicent Marti <vmg@strn.cat> Signed-off-by: Manan Gupta <manan@planetscale.com> * Add back unary single column expression check (#10514) This was accidentally removed in #10512 but it shouldn't have been. Signed-off-by: Dirkjan Bussink <d.bussink@gmail.com> Co-authored-by: Dirkjan Bussink <d.bussink@gmail.com> Co-authored-by: Vicent Marti <vmg@strn.cat>

dbussink added Type: Bug Backport me! labels Jun 15, 2022

dbussink requested review from GuptaManan100, harshit-gangal and systay as code owners June 15, 2022 08:39

dbussink added Component: Query Serving Skip Upgrade Downgrade labels Jun 15, 2022

Handle new cast expression in evalengine and planbuilder

c34a81d

Signed-off-by: Dirkjan Bussink <d.bussink@gmail.com>

dbussink requested a review from frouioui as a code owner June 15, 2022 09:01

dbussink commented Jun 15, 2022

View reviewed changes

GuptaManan100 reviewed Jun 15, 2022

View reviewed changes

systay reviewed Jun 15, 2022

View reviewed changes

systay approved these changes Jun 15, 2022

View reviewed changes

evalengine: do not duplicate CAST/CONVERT translation

5313291

Signed-off-by: Vicent Marti <vmg@strn.cat>

vmg approved these changes Jun 15, 2022

View reviewed changes

vmg merged commit 425feff into vitessio:main Jun 15, 2022

vmg deleted the dbussink/fix-cast-parsing branch June 15, 2022 13:48

dbussink added a commit to planetscale/vitess that referenced this pull request Jun 15, 2022

Add back unary single column expression check

033331a

This was accidentally removed in vitessio#10512 but it shouldn't have been. Signed-off-by: Dirkjan Bussink <d.bussink@gmail.com>

dbussink mentioned this pull request Jun 15, 2022

Add back unary single column expression check #10514

Merged

3 tasks

deepthi pushed a commit that referenced this pull request Jun 15, 2022

Add back unary single column expression check (#10514)

9c63aa6

This was accidentally removed in #10512 but it shouldn't have been. Signed-off-by: Dirkjan Bussink <d.bussink@gmail.com>

GuptaManan100 mentioned this pull request Jun 16, 2022

Backport to release-14: Fix parsing of CAST() statements #10512 and #10514 #10517

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix parsing of CAST() statements#10512

Fix parsing of CAST() statements#10512
vmg merged 3 commits intovitessio:mainfrom
planetscale:dbussink/fix-cast-parsing

dbussink commented Jun 15, 2022

Uh oh!

github-actions bot commented Jun 15, 2022

Uh oh!

dbussink Jun 15, 2022

Uh oh!

GuptaManan100 left a comment

Uh oh!

systay Jun 15, 2022

Uh oh!

dbussink Jun 15, 2022

Uh oh!

systay left a comment

Uh oh!

vmg left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

dbussink commented Jun 15, 2022

Related Issue(s)

Checklist

Uh oh!

github-actions bot commented Jun 15, 2022

Review Checklist

General

Bug fixes

Non-trivial changes

New/Existing features

Backward compatibility

Uh oh!

dbussink Jun 15, 2022

Choose a reason for hiding this comment

Uh oh!

GuptaManan100 left a comment

Choose a reason for hiding this comment

Uh oh!

systay Jun 15, 2022

Choose a reason for hiding this comment

Uh oh!

dbussink Jun 15, 2022

Choose a reason for hiding this comment

Uh oh!

systay left a comment

Choose a reason for hiding this comment

Uh oh!

vmg left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants