util/codec: add unquote function for json doc #1948

hhkbp2 · 2017-06-20T11:46:24Z

Hi,

This PR adds unquote function for json document type. It's one part of the PR #1940, rewritten to include
pingcap/tidb#3504
pingcap/tidb#3507

PTAL @AndreMouche @lishihai9017 @andelf @siddontang

hhkbp2 · 2017-06-20T11:49:11Z

PTAL @hicqu

AndreMouche · 2017-06-20T13:59:44Z

src/util/codec/mysql/json/functions.rs

+    pub fn unquote(&self) -> Result<String> {
+        match *self {
+            Json::String(ref s) => unquote_string(s),
+            _ => Ok(format!("{:?}", self)),


use self.to_string()

AndreMouche · 2017-06-20T14:03:58Z

src/util/codec/mysql/json/functions.rs

+            };
+            match c {
+                '"' => ret.push('"'),
+                'b' => ret.push('\x08'),


Would it more easy to understand if we make \x08 to a constant?

AndreMouche · 2017-06-20T14:13:47Z

src/util/codec/mysql/json/functions.rs

+                    let utf8 = try!(decode_escaped_unicode(&unicode));
+                    ret.push(utf8);
+                }
+                _ => ret.push(c),


It seems we should push a \\ according tidb https://github.com/pingcap/tidb/blob/master/util/types/json/functions.go#L122 ?

It's a bug in TiDB. Confirmed with @hicqu already.

fixed in pingcap/tidb#3521.

AndreMouche · 2017-06-20T14:15:54Z

src/util/codec/mysql/json/functions.rs

+// https://dev.mysql.com/doc/refman/5.7/en/json-modification-functions.html#
+// json-unquote-character-escape-sequences
+pub fn unquote_string(s: &str) -> Result<String> {
+    let mut ret = String::with_capacity(s.len());


Why we do not use Vec<u8> just like tidb do https://github.com/pingcap/tidb/blob/master/util/types/json/functions.go#L89?

If we use raw u8 but not utf8 char here, there is an issue involved if there is a utf8 char in Json::String(String). For example, the utf8 恜苏 contains binary 0x60 0x5C 0x82 0xCF. It should unquote to

mysql> SELECT JSON_UNQUOTE('"恜苏"'); +--------------------------------------+ | JSON_UNQUOTE('"恜苏"') | +--------------------------------------+ | 恜苏 | +--------------------------------------+

If the raw u8 array(same as string in Golang) is used to implement the unquote, the binary

0x60 0x5C 0x82 0xCF

would be converted to

0x60 0x82 0xCF

because 0x5C 0x82 is \ R is unescaped to R.

So the utf8 input is broken.

@hicqu PTAL

In tidb I use a byte buffer to store utf8 encoded bytes. Call String() on that we can get a correct string, because go default string encoding format is utf8.

AndreMouche

Rest LGTM

AndreMouche · 2017-06-21T06:49:48Z

src/util/codec/mysql/json/path_expr.rs

@@ -0,0 +1,260 @@
+// Copyright 2017 PingCAP, Inc.


Could we move this file to the PR #1949 which is the implementation for extract?

address comment.

hhkbp2 · 2017-06-21T07:05:16Z

PTAL @AndreMouche @lishihai9017 @hicqu

AndreMouche

LGTM， @hicqu @lishihai9017 @andelf PTAL

andelf · 2017-06-21T07:27:30Z

src/util/codec/mysql/json/functions.rs

+        if ch == '\\' {
+            let c = match chars.next() {
+                Some(c) => c,
+                None => return Err(box_err!("Missing a closing quotation mark in string")),


closing quotation mark seems misleading. since "\/bfnrt", "uXXXX are not something like )-closing.
Incomplete escape sequence may be better.

This message is coped from TiDB impl.
@hicqu We are gonna to change it according to the comment.

andelf · 2017-06-21T07:42:33Z

src/util/codec/mysql/json/functions.rs

+
+const CHAR_BACKSPACE: char = '\x08';
+const CHAR_HORIZONTAL_TAB: char = '\x09';
+const CHAR_LINE_FEED: char = '\x0A';


andelf · 2017-06-21T07:42:44Z

src/util/codec/mysql/json/functions.rs

+const CHAR_BACKSPACE: char = '\x08';
+const CHAR_HORIZONTAL_TAB: char = '\x09';
+const CHAR_LINE_FEED: char = '\x0A';
+const CHAR_FORM_FEED: char = '\x0C';


andelf · 2017-06-21T07:47:57Z

src/util/codec/mysql/json/functions.rs

+                'n' => ret.push(CHAR_LINE_FEED),
+                'r' => ret.push(CHAR_CARRIAGE_RETURN),
+                't' => ret.push(CHAR_HORIZONTAL_TAB),
+                '\\' => ret.push('\\'),


will we accept '/' as an escape char?

ref: http://www.json.org/
ref: mysql command line

No. This MySQL docs says it clearly what escape sequences are supported.
https://dev.mysql.com/doc/refman/5.7/en/json-modification-functions.html#function_json-unquote

andelf · 2017-06-21T08:33:32Z

mysql> SELECT JSON_UNQUOTE('"\\/"');
+-----------------------+
| JSON_UNQUOTE('"\\/"') |
+-----------------------+
| /                     |
+-----------------------+
1 row in set (0.00 sec)

shall we copy the undocumented part?

hicqu · 2017-06-21T08:53:46Z

src/util/codec/mysql/json/functions.rs

+                            None => return Err(box_err!("Invalid unicode: {}", unicode)),
+                        }
+                    }
+                    let utf8 = try!(decode_escaped_unicode(&unicode));


I think here we can't try!, because if it's \uBADF, we should raise the error instead of panic.

Oh, try!() will return an error but not panic.

yes, my fault.

hicqu · 2017-06-21T08:59:42Z

src/util/codec/mysql/json/functions.rs

+                't' => ret.push(CHAR_HORIZONTAL_TAB),
+                '\\' => ret.push('\\'),
+                'u' => {
+                    let mut unicode = String::with_capacity(ESCAPED_UNICODE_BYTES_SIZE);


here we can use:

if let Ok(unicode) = str::from_utf8(chars.as_str().as_bytes()[0:4]) { // logic here. } else { // error here. }

for avoid using String, which will alloc something on heap.

and it may fail, need length check?

The function takes std::char as operation unit, so that to avoid manipulate raw bytes(which could be encoded in utf8) directly. Is there any way to get a 4 char slice out of chars, or construct a slice on stack?

If char boundary is break, the function will fail (length 4 check fails or from_utf8 fails), both are logically ok.

yes, check chars.as_str().as_bytes().len() >= 4 is also needed.

hicqu · 2017-06-21T09:01:56Z

src/util/codec/mysql/json/functions.rs

+                                  ("\\\\", true, Some("\x5c")),
+                                  ("\\u597d", true, Some("好")),
+                                  ("0\\u597d0", true, Some("0好0")),
+                                  ("[", true, Some("[")),


add a \\a here, and it should be unquoted as a, which is compatible with MySQL document, but different from its implement.

mysql command line sucks.

mysql> SELECT JSON_UNQUOTE('"\\u597d"'); +---------------------------+ | JSON_UNQUOTE('"\\u597d"') | +---------------------------+ | 好 | +---------------------------+ 1 row in set (0.00 sec) mysql> SELECT JSON_UNQUOTE('"\\/"'); +-----------------------+ | JSON_UNQUOTE('"\\/"') | +-----------------------+ | / | +-----------------------+ 1 row in set (0.00 sec) mysql> SELECT JSON_UNQUOTE('"\\,"'); ERROR 3141 (22032): Invalid JSON text in argument 1 to function json_unquote: "Invalid escape character in string." at position 2. mysql> SELECT JSON_UNQUOTE('"\\a"'); ERROR 3141 (22032): Invalid JSON text in argument 1 to function json_unquote: "Invalid escape character in string." at position 2. mysql>

hhkbp2 · 2017-06-21T11:00:45Z

PTAL @hicqu @andelf

andelf · 2017-06-21T11:05:31Z

LGTM

AndreMouche · 2017-06-21T13:03:57Z

@hicqu PTAL

hicqu · 2017-06-22T03:30:32Z

LGTM.

hhkbp2 added 2 commits June 20, 2017 19:41

util/codec: add unquote function for json doc

b9ca46b

Merge branch 'master' into hhkbp2/jsondoc-add-functions

3df294d

hhkbp2 mentioned this pull request Jun 20, 2017

util/codec: add extract function for json doc #1949

Merged

AndreMouche reviewed Jun 20, 2017

View reviewed changes

AndreMouche reviewed Jun 21, 2017

View reviewed changes

hhkbp2 added 2 commits June 21, 2017 14:58

address comments

1b24d6e

move path_expr.rs to next PR

48ed656

AndreMouche reviewed Jun 21, 2017

View reviewed changes

andelf reviewed Jun 21, 2017

View reviewed changes

address comment

e6bd841

hicqu reviewed Jun 21, 2017

View reviewed changes

hhkbp2 added 3 commits June 21, 2017 18:21

address comment

f9e45bb

address comment

7122eb3

Merge branch 'master' into hhkbp2/jsondoc-add-functions

f592847

Merge branch 'master' into hhkbp2/jsondoc-add-functions

b9ac7f7

Merge branch 'master' into hhkbp2/jsondoc-add-functions

d83678e

hicqu approved these changes Jun 22, 2017

View reviewed changes

Merge branch 'master' into hhkbp2/jsondoc-add-functions

41f3767

AndreMouche merged commit 40ab236 into master Jun 22, 2017

AndreMouche deleted the hhkbp2/jsondoc-add-functions branch June 22, 2017 05:04

sre-bot added the contribution This PR is from a community contributor. label Dec 18, 2019

util/codec: add unquote function for json doc #1948

util/codec: add unquote function for json doc #1948

Conversation

hhkbp2 commented Jun 20, 2017 • edited Loading

hhkbp2 commented Jun 20, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hhkbp2 Jun 21, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AndreMouche left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hhkbp2 commented Jun 21, 2017

AndreMouche left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andelf commented Jun 21, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hicqu Jun 21, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hhkbp2 Jun 21, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hhkbp2 commented Jun 21, 2017

andelf commented Jun 21, 2017

AndreMouche commented Jun 21, 2017

hicqu commented Jun 22, 2017

hhkbp2 commented Jun 20, 2017 •

edited

Loading

hhkbp2 Jun 21, 2017 •

edited

Loading

hicqu Jun 21, 2017 •

edited

Loading

hhkbp2 Jun 21, 2017 •

edited

Loading