add Extract and Unquote functions for JSON. #3353

hicqu · 2017-05-26T15:01:57Z

Add two functions for JSON: extract and unquote.

extract unquote merge set

this PR add extract and unquote.

hicqu · 2017-05-26T15:06:49Z

this PR only add two functions for JSON: extract and unquote. PTAL, thanks.

shenli · 2017-05-26T15:09:55Z

util/types/json/json_test.go

-
-	j6, err := ParseFromString(`true`)
-	c.Assert(err, IsNil)
+	j1 := parseFromStringPanic(`{"a": "b"}`)


You can move this into function_test.go

shenli · 2017-05-26T15:21:17Z

util/types/json/functions.go

+// Extract receives several path expressions as arguments, matches them in j, and returns:
+//  ret: target JSON matched any path expressions. maybe autowrapped as an array.
+//  found: true if any path expressions matched.
+func (j JSON) Extract(pathExprList ...string) (ret JSON, found bool, err error) {


We do not need to process the pathExprList string list for every json object.
For example select json_extract(json_col, '$.a', '$.c') from test_json. We should preprocess the '$.a', '$.c' to some structure and apply it to each row from test_json table.
So I think we should define a structure to represent the processed pathExpr list and pass the preprocessed pathExpr list to Json.Extrac() function.

c4pt0r · 2017-05-27T01:45:33Z

util/types/json/functions.go

+		ret = elemList[0]
+	} else {
+		ret.typeCode = typeCodeArray
+		for _, elem := range elemList {


why not use copy(dst, src)

copy needs dst has same len as src. so, I prefer dst = append(dst, src...).

hicqu · 2017-05-27T09:53:06Z

now we call Extract with a PathExpression which can be reused multi times. PTAL, thanks!

shenli · 2017-05-29T00:24:04Z

util/types/json/path_expr.go

+	flags pathExpressionFlag
+}
+
+func validateJSONPathExpr(pathExpr string) (pe PathExpression, err error) {


parseJSONPathExpr is a better name.

tiancaiamao · 2017-05-31T03:26:03Z

util/types/json/functions.go

+// Extract receives several path expressions as arguments, matches them in j, and returns:
+//  ret: target JSON matched any path expressions. maybe autowrapped as an array.
+//  found: true if any path expressions matched.
+func (j JSON) Extract(pathExprList []PathExpression) (ret JSON, found bool) {


How to distinguish the returned array is one of matched path, or a wrapped array?
Does it matters?

Good question. I will test select json_extract('{"a": [1, 2]}', '$.a') and select json_extract('{"a": [1, 2]}', "$.a[0]", "$.a[1]").

these two statements have same result value on MySQL 5.7, so it seems we cannot distinguish them, and we don't need, either.

tiancaiamao · 2017-05-31T03:28:30Z

util/types/json/functions.go

+func (j JSON) Unquote() string {
+	switch j.typeCode {
+	case typeCodeString:
+		return j.str


j.String() return j.str when it's typeCodeString ?

for example,

select json_unquote(`"hello, world"`); -- should return `hello, world` -- but j.String() will return a json marshal string, -- in this case will be `"hello, world"`

so I use j.str instead of j.String.

tiancaiamao · 2017-05-31T03:42:40Z

util/types/json/functions.go

+	var currentLeg = pathExpr.legs[0]
+	pathExpr.legs = pathExpr.legs[1:]
+	if currentLeg.isArrayIndex && j.typeCode == typeCodeArray {
+		if currentLeg.arrayIndex == -1 {


-1 means match all? maybe you can define constant for it.

tiancaiamao · 2017-05-31T03:44:06Z

util/types/json/functions.go

+		} else if currentLeg.arrayIndex < len(j.array) {
+			childRet := extract(j.array[currentLeg.arrayIndex], pathExpr)
+			ret = append(ret, childRet...)
+		}


Will arrayIndex < 0 and arrayIndex != -1 ?

no, it won't.

hicqu · 2017-05-31T05:04:49Z

address comments. PTAL, thanks.

XuHuaiyu · 2017-05-31T04:54:02Z

util/types/json/functions.go

+	}
+	if len(elemList) == 0 {
+		found = false
+	} else if len(pathExprList) == 1 && len(elemList) == 1 {


It seems len(elemList) will always be 1 here ?

if pathExpr contains any asterisks, len(elemList) won't be 1 even if len(pathExprList) equals to 1.

XuHuaiyu · 2017-05-31T05:10:33Z

util/types/json/path_expr.go

+	start        int  // start offset of the leg in raw string, inclusive.
+	end          int  // end offset of the leg in raw string, exclusive.
+	isArrayIndex bool // the leg is an array index or not.
+	arrayIndex   int  // if isArrayIndex is true, the value shoud be parsed into here.


s/shoud/should

XuHuaiyu · 2017-05-31T05:22:36Z

util/types/json/path_expr.go

+		arrayLocation ::= '[' (non-negative-integer | '*') ']'
+		keyName ::= ECMAScript-identifier | ECMAScript-string-literal
+
+	And some implemetion limits in MySQL 5.7:


s/implemetion/implementation

XuHuaiyu · 2017-05-31T05:31:57Z

util/types/json/path_expr.go

+	pathExpressionContainsDoubleAsterisk pathExpressionFlag = 0x02
+)
+
+func (pef pathExpressionFlag) containsAnyAsterisk() bool {


comment for this function.
It seems this function is not used in this PR.

yes, it's prepared for another function: json_set/json_insert and json_replace.

XuHuaiyu · 2017-05-31T06:31:35Z

util/types/json/path_expr.go

+	pe.flags = pathExpressionFlag(0)
+
+	// lastEnd and currentStart is for checking all characters between two legs are blank or not.
+	var lastEnd = -1


lastEnd, currentStart := -1, -1

XuHuaiyu · 2017-05-31T06:32:23Z

util/types/json/path_expr.go

+	pe.legs = make([]pathLeg, 0, len(indices))
+	pe.flags = pathExpressionFlag(0)
+
+	// lastEnd and currentStart is for checking all characters between two legs are blank or not.


If possible, may be an example here will be more explicit.

XuHuaiyu · 2017-05-31T06:32:52Z

util/types/json/path_expr.go

+	for _, indice := range indices {
+		currentStart = indice[0]
+		if lastEnd > 0 && currentStart != lastEnd {
+			// We have already remove all blank characters.


s/ remove/ removed

XuHuaiyu · 2017-05-31T06:45:08Z

util/types/json/path_expr.go

+	flags pathExpressionFlag
+}
+
+// ParseJSONPathExpr parses a JSON path expression. Returns a PathExpression


an example here may be better.

XuHuaiyu · 2017-05-31T06:47:49Z

util/types/json/path_expr.go

+
+// pathLeg is only used by PathExpression.
+type pathLeg struct {
+	start        int  // start offset of the leg in raw string, inclusive.


can we store the string value of leg called member directly?

I have considered that. I think we should avoid alloc extra space on heap. what do you think?

@coocood , which is better do you think?

@hicqu Slicing the string from raw doesn't allocate memory on the heap.

XuHuaiyu · 2017-05-31T06:55:14Z

util/types/json/path_expr.go

+	start        int  // start offset of the leg in raw string, inclusive.
+	end          int  // end offset of the leg in raw string, exclusive.
+	isArrayIndex bool // the leg is an array index or not.
+	arrayIndex   int  // if isArrayIndex is true, the value shoud be parsed into here.


I'm not sure will this be better, we can define a constant like notArrayIndex which is set to 2.
thus we can save the storage spaces of isArrayIndex.
arrayIndex == -2 ==> isArrayIndex is false
arrayIndex == -1 ==> isArrayIndex == true && arrayIndexAsterisk
arrayIndex >= 0 ==> isArrayIndex == true

tiancaiamao · 2017-05-31T09:24:00Z

util/types/json/json_test.go

@@ -27,19 +28,28 @@ func TestT(t *testing.T) {
 	TestingT(t)
 }

+func parseFromStringPanic(s string) JSON {


parseFromStringPanic is not a good name, mustParseFromString would confirm to Go idiom.

CLAassistant · 2017-05-31T13:02:12Z

All committers have signed the CLA.

shenli · 2017-05-31T15:05:35Z

@XuHuaiyu @coocood PTAL

shenli · 2017-05-31T15:29:27Z

/cc @AndreMouche

shenli · 2017-05-31T15:45:22Z

util/types/json/path_expr.go

+
+// ParseJSONPathExpr parses a JSON path expression. Returns a PathExpression
+// object which can be used in JSON_EXTRACT, JSON_SET and so on.
+func ParseJSONPathExpr(pathExpr string) (pe PathExpression, err error) {


Could you add a few comments in the function? It is not easy to read the code.

and support double asterisk.

hicqu · 2017-06-01T07:01:06Z

now we can support double asterisk in path expression.
@coocood @XuHuaiyu @shenli , PTAL again, thanks.

XuHuaiyu

Rest LGTM.

XuHuaiyu · 2017-06-01T08:01:59Z

util/types/json/path_expr.go

+
+type pathLegType byte
+
+const (


add comments for these three types.

XuHuaiyu · 2017-06-01T08:04:18Z

util/types/json/path_expr.go

@@ -80,9 +88,28 @@ type PathExpression struct {
 	flags pathExpressionFlag
 }

+func (pe PathExpression) popOneLeg() (pathLeg, PathExpression) {


comment for this function.

XuHuaiyu · 2017-06-01T08:54:22Z

LGTM

coocood · 2017-06-01T09:05:39Z

util/types/json/functions.go

+
+// unquoteString recognizes the escape sequences shown in:
+// https://dev.mysql.com/doc/refman/5.7/en/json-modification-functions.html#json-unquote-character-escape-sequences
+func unquoteString(s string) (string, error) {


Need to consider UTF8 characters?

Maybe this function can be used?

tidb/util/stringutil/string_util.go

Line 50 in 26d54c8

func UnquoteChar(s string, quote byte) (value []byte, tail string, err error) {

coocood · 2017-06-01T09:10:17Z

util/types/json/functions_test.go

+	j5 := mustParseFromString(`null`)
+	j6 := mustParseFromString(`true`)
+	var jList = []struct {
+		In  JSON


Use string as In, then parse in the loop is more clear.

coocood · 2017-06-01T09:12:31Z

util/types/json/functions_test.go

+
+func (s *testJSONSuite) TestJSONUnquote(c *C) {
+	var tests = []struct {
+		j        JSON


use string type for j is easier to read.

coocood · 2017-06-01T09:23:49Z

util/types/json/json_test.go

+	jStringSmall := mustParseFromString(`"hello"`)
+	jArrayLarge := mustParseFromString(`["a", "c"]`)
+	jArraySmall := mustParseFromString(`["a", "b"]`)
+	jObject := mustParseFromString(`{"a": "b"}`)


OK, this case, the variable name is more meaningful than the string value.

…qupeng/json-functions

hicqu · 2017-06-01T10:11:02Z

address comments. @coocood , PTAL again, thanks.

coocood · 2017-06-01T11:51:57Z

LGTM

hicqu added 3 commits May 26, 2017 17:21

support functions:

debac63

extract unquote merge set

init

d58d95c

add functions one by one.

307b974

this PR add extract and unquote.

hicqu changed the title ~~Qupeng/json functions~~ add Extract and Unquote functions for JSON. May 26, 2017

hicqu added document-store priority/P1 The issue has P1 priority. labels May 26, 2017

hicqu requested review from AndreMouche, shenli, tiancaiamao and XuHuaiyu May 26, 2017 15:06

shenli reviewed May 26, 2017

View reviewed changes

c4pt0r reviewed May 27, 2017

View reviewed changes

hicqu added 5 commits May 27, 2017 13:51

fix json path expr.

a4b2857

improve code.

8f48d03

improve path_expr.

a478fe7

add flags into PathExpression.

8423c01

fix json_extract

e45591a

shenli reviewed May 29, 2017

View reviewed changes

tiancaiamao reviewed May 31, 2017

View reviewed changes

address comments.

6f47e2a

fix for ci

5520fe1

XuHuaiyu reviewed May 31, 2017

View reviewed changes

address comments.

725b0b8

tiancaiamao reviewed May 31, 2017

View reviewed changes

hicqu added 4 commits May 31, 2017 20:40

address comments.

9ff1d42

address comments.

aff5ee6

address comments

d18073a

address comments.

11465d1

shenli reviewed May 31, 2017

View reviewed changes

address comments.

b49429b

and support double asterisk.

XuHuaiyu reviewed Jun 1, 2017

View reviewed changes

address comments.

b504ec6

hicqu added 2 commits June 1, 2017 16:57

Merge branch 'master' into qupeng/json-functions

11da2b1

Merge branch 'master' into qupeng/json-functions

5057d7c

coocood reviewed Jun 1, 2017

View reviewed changes

hicqu added 4 commits June 1, 2017 18:04

address comments

2042f1c

Merge branch 'qupeng/json-functions' of github.com:pingcap/tidb into …

af0ca13

…qupeng/json-functions

address comments

9e04b3c

address comments

4dddaa8

hicqu added 2 commits June 1, 2017 19:27

remove useless variables.

cb4cd7a

address comments.

67e3677

Merge branch 'master' into qupeng/json-functions

5678eec

coocood approved these changes Jun 2, 2017

View reviewed changes

coocood merged commit 01dd725 into master Jun 2, 2017

coocood deleted the qupeng/json-functions branch June 2, 2017 02:30

hhkbp2 mentioned this pull request Jun 18, 2017

util/codec: add extract, unquote function for json doc tikv/tikv#1940

Closed

add Extract and Unquote functions for JSON. #3353

add Extract and Unquote functions for JSON. #3353

Conversation

hicqu commented May 26, 2017 • edited by shenli Loading

hicqu commented May 26, 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

hicqu commented May 27, 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

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

hicqu commented May 31, 2017

Choose a reason for hiding this comment

hicqu May 31, 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

hicqu May 31, 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

Choose a reason for hiding this comment

hicqu May 31, 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

CLAassistant commented May 31, 2017 • edited Loading

shenli commented May 31, 2017

shenli commented May 31, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hicqu commented Jun 1, 2017

XuHuaiyu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

XuHuaiyu commented Jun 1, 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

hicqu commented Jun 1, 2017

coocood commented Jun 1, 2017

hicqu commented May 26, 2017 •

edited by shenli

Loading

hicqu May 31, 2017 •

edited

Loading

hicqu May 31, 2017 •

edited

Loading

hicqu May 31, 2017 •

edited

Loading

CLAassistant commented May 31, 2017 •

edited

Loading