expression: add builtin json_keys #7776

Kingwl · 2018-09-25T06:33:33Z

What problem does this PR solve?

a part of #7546

What is changed and how it works?

Check List

Tests

Unit test

Code changes

Has exported function/method change

sre-bot · 2018-09-25T06:33:36Z

Hi contributor, thanks for your PR.

This patch needs to be approved by someone of admins. They should reply with "/ok-to-test" to accept this PR for running test automatically.

zz-jason · 2018-09-25T06:51:44Z

types/json/binary.go

+// GetKeys gets the keys of the object
+func (bj BinaryJSON) GetKeys() BinaryJSON {
+	count := bj.getElemCount()
+	ret := make([]BinaryJSON, 0, count-1)


should it be count rather than count-1?

winoros · 2018-09-26T06:33:20Z

expression/builtin_json.go

+	if isNull || err != nil {
+		return res, isNull, errors.Trace(err)
+	}
+	if res.Type() != "OBJECT" {


Could we use Typecode?

mccxj · 2018-09-27T02:08:04Z

expression/builtin_json.go

+		return res, isNull, errors.Trace(err)
+	}
+	if res.TypeCode != json.TypeCodeObject {
+		return res, isNull, json.ErrInvalidJSONData


isNull -> true

mccxj · 2018-09-27T02:10:06Z

expression/builtin_json.go

+			return res, true, json.ErrInvalidJSONPathWildcard
+		}
+		var exists bool
+		res, exists = res.Extract([]json.PathExpression{pathExpr})


merge line 812 and 813

zz-jason · 2018-09-27T02:15:47Z

@Kingwl please merge master and resolve conflicts.

XuHuaiyu · 2018-09-27T03:15:31Z

expression/builtin_json.go

+	if len(b.args) == 2 {
+		path, isNull, err := b.args[1].EvalString(b.ctx, row)
+		if isNull || err != nil {
+			return res, isNull, errors.Trace(err)


Can we directly return err here? @lysu

XuHuaiyu · 2018-09-27T03:19:08Z

expression/builtin_json_test.go

@@ -423,3 +423,72 @@ func (s *testEvaluatorSuite) TestJSONContainsPath(c *C) {
 		}
 	}
 }
+
+func (s *testEvaluatorSuite) TestJSONKeys(c *C) {


We may add a test for this what mentioned in the MySQL manual:
If the top-level value has nested subobjects, the return value does not include keys from those subobjects

seems already existed

You mean line 526?

XuHuaiyu · 2018-09-27T03:22:48Z

expression/builtin_json.go

+		return nil, errors.Trace(err)
+	}
+	argTps := []types.EvalType{types.ETJson}
+	if len(args) == 2 {


We'd better implement 2 function signatures:

builtinJSONKeysSig
builtinJSONKeys2ArgsSig

We can take substringFunctionClass as example.

XuHuaiyu · 2018-09-29T03:37:47Z

expression/builtin_json.go

+		sig = &builtinJSONKeysSig{bf}
+	case 2:
+		sig = &builtinJSONKeys2ArgsSig{bf}
+	default:


This would never happen since we checked it in line 772.

XuHuaiyu · 2018-09-29T03:40:22Z

executor/show.go

@@ -351,19 +351,19 @@ func (e *ShowExec) fetchShowIndex() error {
 				subPart = col.Length
 			}
 			e.appendRow([]interface{}{
-				tb.Meta().Name.O,       // Table


circle ci failed:

gofmt (simplify) executor/show.go infoschema/tables.go parser/misc.go Makefile:94: recipe for target 'fmt' failed

These files should not be modified.

XuHuaiyu · 2018-09-29T03:41:29Z

expression/builtin_json.go

+		// Should never happens.
+		return nil, errors.Errorf("JSON_KEYS invalid arg length, expect 1 or 2 but got: %v", len(args))
+	}
+	sig.setPbCode(tipb.ScalarFuncSig_JsonKeysSig)


Seems builtinJSONKeysSig and builtinJSONKeys2ArgsSig should be set different PbCode.

@Kingwl PTAL this comment.

XuHuaiyu · 2018-09-29T03:42:21Z

expression/builtin_json_test.go

@@ -423,3 +423,72 @@ func (s *testEvaluatorSuite) TestJSONContainsPath(c *C) {
 		}
 	}
 }
+
+func (s *testEvaluatorSuite) TestJSONKeys(c *C) {


You mean line 526?

Kingwl · 2018-10-17T09:12:28Z

blocked by #7893 for now

ngaut · 2018-10-18T09:07:22Z

PTAL

zz-jason

LGTM

zz-jason · 2018-10-18T12:32:54Z

@XuHuaiyu PTAL

XuHuaiyu

LGTM

XuHuaiyu · 2018-10-18T12:43:39Z

/run-all-tests

Added in pingcap/tidb#7776

bugfix fixed pingcap#7518 expression: MySQL compatible current_user function (pingcap#7801) plan: propagate constant over outer join (pingcap#7794) - extract `outerCol = const` from join conditions and filter conditions, substitute `outerCol` in join conditions with `const`; - extract `outerCol = innerCol` from join conditions, derive new join conditions based on this column equal condition and `outerCol` related expressions in join conditions and filter conditions; util/timeutil: fix data race caused by forgetting set stats lease to 0 (pingcap#7901) stats: handle ddl event for partition table (pingcap#7903) plan: implement Operand and Pattern of cascades planner. (pingcap#7910) planner: not convert to TableDual if empty range is derived from deferred constants (pingcap#7808) plan: move projEliminate behind aggEliminate (pingcap#7909) admin: fix admin check table bug of byte compare (pingcap#7887) * admin: remove reflect deepEqual stats: fix panic caused by empty histogram (pingcap#7912) plan: fix panic caused by empty schema of LogicalTableDual (pingcap#7906) * fix drop view if exist error (pingcap#7833) executor: refine `explain analyze` (pingcap#7888) executor: add an variable to compatible with MySQL insert for OGG (pingcap#7863) expression: maintain `DeferredExpr` in aggressive constant folding. (pingcap#7915) stats: fix histogram boundaries overflow error (pingcap#7883) ddl:support the definition of `null` change to `not null` using `alter table` (pingcap#7771) * ddl:support the definition of null change to not null using alter table ddl: add check when create table with foreign key. (pingcap#7885) * ddl: add check when create table with foreign key planner: eliminate if null on non null column (pingcap#7924) executor: fix a bug in point get (pingcap#7934) planner, executor: refine ColumnPrune for LogicalUnionAll (pingcap#7930) executor: fix panic when limit is too large (pingcap#7936) ddl: add TiDB version to metrics (pingcap#7902) stats: limit the length of sample values (pingcap#7931) vendor: update tipb (pingcap#7893) planner: support the Group and GroupExpr for the cascades planner (pingcap#7917) store/tikv: log more information when other err occurs (pingcap#7948) types: fix date time parse (pingcap#7933) ddl: just print error message when ddl job is normal to calcel, to eliminate noisy log (pingcap#7875) stats: update delta info for partition table (pingcap#7947) explaintest: add explain test for partition pruning (pingcap#7505) util: move disjoint set to util package (pingcap#7950) util: add PreAlloc4Row and Insert for Chunk and List (pingcap#7916) executor: add the slow log for commit (pingcap#7951) expression: add builtin json_keys (pingcap#7776) privilege: add USAGE in `show grants` for mysql compatibility (pingcap#7955) ddl: fix invailid ddl job panic (pingcap#7940) *: move ast.NewValueExpr to standalone parser_driver package (pingcap#7952) Make the ast package get rid of the dependency of types.Datum server: allow cors http request (pingcap#7939) *: move `Statement` and `RecordSet` from ast to sqlexec package (pingcap#7970) pr suggestion update executor/aggfuncs: split unit tests to corresponding file (pingcap#7993) store/tikv: fix typo (pingcap#7990) executor, planner: clone proj schema for different children in buildProj4Union (pingcap#7999) executor: let information_schema be the first database in ShowDatabases (pingcap#7938) stats: use local feedback for partition table (pingcap#7963) executor: add unit test for aggfuncs (pingcap#7966) server: add log for binary execute statement (pingcap#7987) admin: refine admin check decoder (pingcap#7862) executor: improve wide table insert & update performance (pingcap#7935) ddl: fix reassigned partition id in `truncate table` does not take effect (pingcap#7919) fix reassigned partition id in truncate table does not take effect add changelog for 2.1.0 rc4 (pingcap#8020) *: make parser package dependency as small as possible (pingcap#7989) parser: support `:=` in the `set` syntax (pingcap#8018) According to MySQL document, `set` use the = assignment operator, but the := assignment operator is also permitted stats: garbage collect stats for partition table (pingcap#7962) docs: add the proposal for the column pool (pingcap#7988) expression: refine built-in func truncate to support uint arg (pingcap#8000) stats: support show stats for partition table (pingcap#8023) stats: update error rate for partition table (pingcap#8022) stats: fix estimation for out of range point queries (pingcap#8015) *: move parser to a separate repository (pingcap#8036) executor: fix wrong result when index join on union scan. (pingcap#8031) Do not modify Plan of dataReaderBuilder directly, because it would impact next batch of outer rows, as well as other concurrent inner workers. Instead, build a local child builder to store the child plan. planner: fix a panic of a cached prepared statement with IndexScan (pingcap#8017) *: fix the issue of executing DDL after executing SQL failure in txn (pingcap#8044) * ddl, executor: fix the issue of executing DDL after executing SQL failure in txn add unit test remove debug info add like evaluator case sensitive test ddl, domain: make schema correct after canceling jobs (pingcap#7997) unit test fix code format proposal: maintaining histograms in plan. (pingcap#7605) support _tidb_rowid for table scan range (pingcap#8047) var rename fix

Added in pingcap/tidb#7776

zz-jason added type/enhancement The issue or PR belongs to an enhancement. contribution This PR is from a community contributor. component/expression labels Sep 25, 2018

zz-jason reviewed Sep 25, 2018

View reviewed changes

Kingwl force-pushed the json_keys branch 2 times, most recently from ec6d629 to fb0643d Compare September 25, 2018 08:02

winoros reviewed Sep 26, 2018

View reviewed changes

Kingwl force-pushed the json_keys branch from fb0643d to bb7e162 Compare September 26, 2018 06:39

mccxj reviewed Sep 27, 2018

View reviewed changes

XuHuaiyu reviewed Sep 27, 2018

View reviewed changes

XuHuaiyu reviewed Sep 29, 2018

View reviewed changes

Kingwl force-pushed the json_keys branch from 81d2cca to 9a81296 Compare September 29, 2018 09:37

update json_keys

8e93d83

Kingwl force-pushed the json_keys branch from 9a81296 to 8e93d83 Compare September 29, 2018 10:05

mccxj mentioned this pull request Oct 8, 2018

Support remaining JSON functions from MySQL 5.7 #7546

Closed

28 tasks

Kingwl mentioned this pull request Oct 12, 2018

expression: add json_arrayagg and json_objectagg #7824

Closed

Kingwl added 3 commits October 15, 2018 09:53

Merge branch 'master' into json_keys

e4bd37d

fix pb code for json keys

46b87f8

Merge branch 'master' into json_keys

b14db17

Kingwl added 2 commits October 18, 2018 16:10

Merge branch 'master' into json_keys

177df6e

Merge branch 'master' into json_keys

1090428

zz-jason reviewed Oct 18, 2018

View reviewed changes

zz-jason added the status/LGT1 Indicates that a PR has LGTM 1. label Oct 18, 2018

Merge branch 'master' into json_keys

9245d17

XuHuaiyu approved these changes Oct 18, 2018

View reviewed changes

Merge branch 'master' into json_keys

9ce3940

XuHuaiyu added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Oct 18, 2018

Merge branch 'master' into json_keys

347b61f

zz-jason added the status/all tests passed label Oct 18, 2018

winoros merged commit 708611d into pingcap:master Oct 18, 2018

Kingwl deleted the json_keys branch October 18, 2018 16:02

morgo added a commit to pingcap/docs that referenced this pull request Oct 19, 2018

sql: Added JSON_KEYS function

18bb9bd

Added in pingcap/tidb#7776

morgo mentioned this pull request Oct 19, 2018

sql: Add JSON_KEYS function pingcap/docs#680

Merged

morgo added a commit to pingcap/docs that referenced this pull request Oct 29, 2018

sql: Added JSON_KEYS function (#680)

567ff4f

Added in pingcap/tidb#7776

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

expression: add builtin json_keys #7776

expression: add builtin json_keys #7776

Kingwl commented Sep 25, 2018

sre-bot commented Sep 25, 2018

zz-jason Sep 25, 2018

Kingwl Sep 25, 2018

winoros Sep 26, 2018

mccxj Sep 27, 2018

mccxj Sep 27, 2018

zz-jason commented Sep 27, 2018

XuHuaiyu Sep 27, 2018

lysu Sep 28, 2018

XuHuaiyu Sep 27, 2018

Kingwl Sep 28, 2018

XuHuaiyu Sep 29, 2018

XuHuaiyu Sep 27, 2018

XuHuaiyu Sep 29, 2018

XuHuaiyu Sep 29, 2018

XuHuaiyu Sep 29, 2018

zz-jason Oct 9, 2018

XuHuaiyu Sep 29, 2018

Kingwl commented Oct 17, 2018

ngaut commented Oct 18, 2018

zz-jason left a comment

zz-jason commented Oct 18, 2018

XuHuaiyu left a comment

XuHuaiyu commented Oct 18, 2018

expression: add builtin json_keys #7776

expression: add builtin json_keys #7776

Conversation

Kingwl commented Sep 25, 2018

What problem does this PR solve?

What is changed and how it works?

Check List

sre-bot commented Sep 25, 2018

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

zz-jason commented Sep 27, 2018

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

Kingwl commented Oct 17, 2018

ngaut commented Oct 18, 2018

zz-jason left a comment

Choose a reason for hiding this comment

zz-jason commented Oct 18, 2018

XuHuaiyu left a comment

Choose a reason for hiding this comment

XuHuaiyu commented Oct 18, 2018