Fix formatting for function expressions and booleans #10255
Conversation
These do not change casing in MySQL so they should not be canonicalized to uppercase in the canonical uppercase form either. This also will lead to spurious incorrect diffs with schemadiff potentially. Signed-off-by: Dirkjan Bussink <d.bussink@gmail.com>
dbussink
requested review from
harshit-gangal,
systay and
GuptaManan100
as code owners
Review ChecklistHello reviewers! 👋 Please follow this checklist when reviewing this Pull Request. General
Bug fixes
Non-trivial changes
New/Existing features
Backward compatibility
|
| @@ -1310,7 +1310,7 @@ func (node *FuncExpr) Format(buf *TrackedBuffer) { | |||
| if containEscapableChars(funcName, NoAt) { | |||
| writeEscapedString(buf, funcName) | |||
| } else { | |||
| buf.literal(funcName) | |||
| buf.WriteString(funcName) | |||
There was a problem hiding this comment.
This would mean that function name won't be capitalised? Is that what we want? Don't we need them to be in the same case in canonical form for schema-diff?
There was a problem hiding this comment.
@GuptaManan100 The thing is that MySQL doesn't capitalize it either in it's canonical form:
mysql> create table t1 (b binary(16) default (UUID_TO_BIN(UUID(), TRUE)));
Query OK, 0 rows affected (0.01 sec)
mysql> show create table t1;
+-------+--------------------------------------------------------------------------------------------------------------------------------------------+
| Table | Create Table |
+-------+--------------------------------------------------------------------------------------------------------------------------------------------+
| t1 | CREATE TABLE `t1` (
`b` binary(16) DEFAULT (uuid_to_bin(uuid(),true))
) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_0900_ai_ci |
+-------+--------------------------------------------------------------------------------------------------------------------------------------------+
1 row in set (0.00 sec)
mysql>
As you can see above, it explicit is in lowercase. The same applies to true as well. Always upcasing it here explicitly causes problems for schemadiff that not doing solves.
There was a problem hiding this comment.
Hmm okay, I am thinking what if a user gave a query like create table t1 (b binary(16) default (UuiD_TO_BiN(UUID(), TRUE))); Should we also lower-case the function name?
There was a problem hiding this comment.
Because this is the output I see from the parser right now (on main) -
create table t1 (
b binary(16) default (UuiD_TO_BiN(UUID(), true))
)There was a problem hiding this comment.
@GuptaManan100 I thought about that, but that isn't something that so far has been done in the parser in general. For example, if you look at how collations and charsets are treated, those are not canonicalized into forced lowercase either.
So I've followed that existing situation here to at least fix the immediate problem. The issue right now is that schemas fed from MySQL itself are changing. While the case you mention can also be a problem, it's much less a practical problem since schemadiff is normally always fed schemas from MySQL, specifically from SHOW CREATE TABLE.
If the preference is to force lowercase things for stuff like this, I think we can do that, but I'd prefer if we can take a separate pass at that since it applies to a lot more cases than just these fixes here (like collations and charsets).
These do not change casing in MySQL so they should not be canonicalized to uppercase in the canonical uppercase form either.
This also will lead to spurious incorrect diffs with schemadiff potentially.
Related Issue(s)
Found when working on #10203
Checklist