Skip to content

implement JSON_QUOTE()#2372

Merged
jycor merged 6 commits intomainfrom
james/quote
Mar 8, 2024
Merged

implement JSON_QUOTE()#2372
jycor merged 6 commits intomainfrom
james/quote

Conversation

@jycor
Copy link
Contributor

@jycor jycor commented Mar 7, 2024

@jycor jycor requested a review from fulghum March 7, 2024 08:34
Copy link
Contributor

@fulghum fulghum left a comment

Choose a reason for hiding this comment

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

Code looks good, but I'm not sure that we're matching MySQL's behavior for JSON_QUOTE, particularly around escape chars.

Comment on lines +67 to +68
arg: expression.NewLiteral(`\`, types.Text),
exp: `"\\"`,
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be an error, same as the test case for json_unquote?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure why json_unquote throws an error in the first place; I'm having trouble replicating in MySQL...

Comment on lines +63 to +64
arg: expression.NewLiteral(`"\t\u0032"`, types.Text),
exp: `"\"\\t\\u0032\""`,
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be escaping the backslashes in the control characters? It seems like MySQL doesn't return the same result. I'm not sure I totally understand what MySQL is doing here though... so I'm probably missing something. I don't get why it drops the \ in \u0032.

mysql> SELECT JSON_QUOTE('"\t\u0032"');
+--------------------------+
| JSON_QUOTE('"\t\u0032"') |
+--------------------------+
| "\"\tu0032\""            |
+--------------------------+

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe this has something to do with the shell vs string literals?
When compiling and running dolt sql, I get the same output:

tmp/main>  SELECT JSON_QUOTE('"\t\u0032"');
+--------------------------+
| JSON_QUOTE('"\t\u0032"') |
+--------------------------+
| "\"\tu0032\""            |
+--------------------------+
1 row in set (0.00 sec)

Comment on lines +71 to +72
arg: expression.NewLiteral(`\b\f\n\r\t\"`, types.Text),
exp: `"\\b\\f\\n\\r\\t\\\""`,
Copy link
Contributor

Choose a reason for hiding this comment

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

For this one, with MySQL, I'm getting:

mysql> select JSON_QUOTE('\b\f\n\r\t\"');
+----------------------------+
| JSON_QUOTE('\b\f\n\r\t\"') |
+----------------------------+
| "\bf\n\r\t\""              |
+----------------------------+

The MySQL docs call out that \f is a supported escape sequence, but it also says for non-supported escape sequences it ignores the \, which seems to be what it's doing here with \f.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same as above

tmp/main> select json_quote("\b\f\n\r\t\"");
+----------------------------+
| json_quote("\b\f\n\r\t\"") |
+----------------------------+
| "\bf\n\r\t\""              |
+----------------------------+
1 row in set (0.00 sec)

Copy link
Contributor

@fulghum fulghum left a comment

Choose a reason for hiding this comment

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

Thanks for confirming with the dolt sql examples. Looks good to me!

@jycor jycor merged commit e1a7036 into main Mar 8, 2024
@jycor jycor deleted the james/quote branch March 8, 2024 00:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants