jira 2227 : support json fuction - JSON_OBJECT_FIELD_TEXT #833

andyyangcn · 2016-11-14T03:24:57Z

add json parser code and a new json fucntion : JSON_OBJECT_FIELD_TEXT('json string', 'key').

Traf-Jenkins · 2016-11-14T03:26:01Z

Can one of the admins verify this patch?

Traf-Jenkins · 2016-11-14T03:27:01Z

Can one of the admins verify this patch?

Traf-Jenkins · 2016-11-14T03:31:01Z

Can one of the admins verify this patch?

Traf-Jenkins · 2016-11-14T03:32:01Z

Can one of the admins verify this patch?

moscowgentalman · 2016-11-14T04:24:52Z

jenkins, test

moscowgentalman · 2016-11-14T05:18:11Z

@svarnau , please help to take a look

svarnau · 2016-11-14T06:16:56Z

jenkins, add user

Traf-Jenkins · 2016-11-14T06:21:13Z

Check Test Started: https://jenkins.esgyn.com/job/Check-PR-master/1355/

Traf-Jenkins · 2016-11-14T06:22:18Z

Test Failed. https://jenkins.esgyn.com/job/Check-PR-master/1355/

moscowgentalman

this is a big check-in, will need to take some time to review the code. Some initial comments embedded.

moscowgentalman · 2016-11-14T07:21:08Z

core/sql/common/json.cpp

@@ -0,0 +1,1139 @@
+#include "stringinfo.h"


Must add license info at the header.

moscowgentalman · 2016-11-14T07:27:20Z

core/sql/bin/SqlciErrors.txt

@@ -3925,3 +3925,16 @@ $3~String1.
 30047 ZZZZZ 99999 BEGINNER MAJOR DBADMIN NAR details. HexdRow: $0~String0 ErrNum: $1~Int1 ObjectName: $2~TableName PartitionName: $3~String3 FileNum: $4~Int4 RecNum: $5~Int5
 30048 ZZZZZ 99999  BEGINNER MAJOR DBADMIN Fast Load failed. Number of error rows($0~Int0) inserted into the exception table exceeded the max allowed($1~Int1).
 30049 ZZZZZ 99999  BEGINNER MAJOR DBADMIN Fast Load succeeded. Number of error rows inserted into the exception table is $0~Int0
+
+32001 ZZZZZ 99999 BEGINNER MAJOR DBADMIN Token is invalid.
+32002 ZZZZZ 99999 BEGINNER MAJOR DBADMIN JSON value is invalid.


if possible, considering use 8000~8999 range for function run-time error?
I am not very sure here, need @zellerh, @DaveBirdsall to review as well.

Yes, agree. Maybe something like 8970-8982.

moscowgentalman · 2016-11-14T07:28:08Z

core/sql/common/json.h

@@ -0,0 +1,124 @@
+#ifndef JSON_H


license header is required

Yes, and also please try to follow the naming conventions for source file names. For example, this file in the common directory should probably be called ComJSON.h or similar. That should make it much easier to find. Otherwise, if someone would come across a name like json.cpp or json.h, they would assume that it is not part of Trafodion and/or they would have to do quite a bit of searching to figure out where the file comes from. Overall, I think the idea of putting the JSON-related code into the common directory is a very good idea.

zellerh · 2016-11-14T21:58:56Z

core/sql/common/stringinfo.cpp

+
+    if (str->data != NULL)
+        free(str->data);
+    str->data = (char *) malloc(newlen);


Sorry, I haven't reviewed all of the code. Is there a chance that we will introduce memory leaks? Would it make sense to pass a Trafodion heap pointer (NAMemory *) to a StringInfo object, so we can control the memory allocation better?

zellerh · 2016-11-14T21:59:46Z

core/sql/common/json.h

+ * to doing a pure parse with no side-effects, and is therefore exactly
+ * what the json input routines do.
+ *
+ * The 'fname' and 'token' strings passed to these actions are palloc'd.


The comments talk about palloc, but we really use malloc in this code. See another comment about whether malloc is the right thing to use.

Postgresql uses palloc function to allocate memories from memory pool. So, when I porting the code, I remove that part and use malloc as a replace. For postgresql, developer doesn't need to care about when to release the memory, because when the request is completed, all memory allocated in the request will be release. I'm not sure if Trafodion has the same mechanism to manage memory, if yes, I can use that to replace palloc.

zellerh · 2016-11-14T22:27:52Z

Much of this code is a copy of the PostgreSQL source code. You cannot strip off the PostgreSQL license header and copyright from these files. I'm not quite sure how this should be handled, but my guess would be that you need to leave the PostgreSQL license header in the file and add the Apache License header as well. Also, you may need to add an entry to the RAT_README file in the top-level directory. See also http://www.apache.org/legal/resolved.html, this states that it is ok to include PostgreSQL code in an Apache project. Maybe @gtapper, our new Release Manager, can help you with the details. Our previous release manager, @robertamarton, has a lot of experience with this.

zellerh · 2016-11-14T22:34:51Z

core/sql/common/stringinfo.h

+    int			cursor;
+} StringInfoData;
+
+typedef StringInfoData *StringInfo;


"StringInfo" is a very generic term and this struct is used for a very specialized purpose here. It would be good to rename it to make it more clear what it is for. For example: ComJSONStringInfo would make it more clear that this is part of the common directory and used for JSON strings. Also, it is not common in Trafodion to typedef a pointer to a regular name, and this will probably confuse people as well. Is it possible to use the Trafodion strings NAString or NAText instead? NAText is basically the same as std::string. Just a question, I realize that it may be too much work to do this.

Absolutely right. Such code would also have to be documented in licenses/lic-server-src that is used to build the LICENSE file that goes out with the released code.

moscowgentalman · 2016-11-15T05:29:23Z

core/sql/parser/sqlparser.y

@@ -33735,7 +33741,6 @@ nonreserved_func_word:  TOK_ABS
                      | TOK_FLOOR
                      | TOK_FN
                      | TOK_GREATEST
-                      | TOK_GROUPING_ID


should not remove GROUPING_ID, must be accidental?

merge mistake. add it back.

Traf-Jenkins · 2016-11-15T06:39:14Z

New Check Test Started: https://jenkins.esgyn.com/job/Check-PR-master/1358/

Traf-Jenkins · 2016-11-15T07:51:12Z

Test Failed. https://jenkins.esgyn.com/job/Check-PR-master/1358/

Traf-Jenkins · 2016-11-15T07:51:30Z

Previous Test Aborted. New Check Test Started: https://jenkins.esgyn.com/job/Check-PR-master/1359/

Traf-Jenkins · 2016-11-15T07:51:52Z

Test Failed. https://jenkins.esgyn.com/job/Check-PR-master/1359/

Traf-Jenkins · 2016-11-15T08:09:09Z

New Check Test Started: https://jenkins.esgyn.com/job/Check-PR-master/1360/

Traf-Jenkins · 2016-11-15T08:09:34Z

Test Failed. https://jenkins.esgyn.com/job/Check-PR-master/1360/

Traf-Jenkins · 2016-11-17T06:27:13Z

New Check Test Started: https://jenkins.esgyn.com/job/Check-PR-master/1368/

Traf-Jenkins · 2016-11-17T09:19:14Z

Test Passed. https://jenkins.esgyn.com/job/Check-PR-master/1368/

moscowgentalman · 2016-11-17T15:55:58Z

core/sql/common/ComJSONFuncs.cpp

+        int			len = _state->lex->prev_token_terminator - start;
+
+        //_state->tresult = cstring_to_text_with_len(start, len);
+        _state->tresult = (char *)malloc(len + 1);


not clear where this malloc memory will be free?

It will be free in ex_function_json_object_field_text::eval().

if (rltStr != NULL) { Lng32 rltLen = str_len(rltStr)+1; str_cpy_all(op_data[0], rltStr, rltLen); free(rltStr); // If result is a varchar, store the length of substring // in the varlen indicator. if (getOperand(0)->getVCIndicatorLength() > 0) getOperand(0)->setVarLength(rltLen, op_data[-MAX_OPERANDS]); }

moscowgentalman · 2016-11-18T02:53:10Z

overall looks good to me. +1 from me.
There are two following tasks for this though, it is OK to finish them in following check-ins instead of this one:

Since this impact the document, please file a JIRA to track the task of update SQL Reference Manual about this new function.
Add some test case, so others will be better understanding how this works.

andyyangcn · 2016-11-18T05:48:05Z

A new jira is created to track the task for updating documents.
TRAFODION-2360

moscowgentalman · 2016-11-23T14:01:25Z

@svarnau , @gtapper do you have concerns from license point of view of merging this PR?

svarnau

rat exclude may not be needed.
license file addition looks good.

svarnau · 2016-11-23T16:33:21Z

.rat-excludes

+ComJSON.cpp
+ComJSONFuncs.cpp
+ComJSONStringInfo.h
+ComJSONStringInfo.cpp


Not sure if these are needed here, with the copyright restored and the ASF statements added, but I did not try running RAT without to verify.
Would like a better comment here than BSD license - specify PostgresSQL.

have removed json files from .rat-excludes

svarnau · 2016-11-23T16:39:44Z

In good shape from license file view.

update from apache

robertamarton · 2016-11-25T19:36:35Z

There has been a lot of email recently on general incubator and legal lists. The JSon license has been clarified, here is a recap from Apache legal:

>>> o No new project, sub-project or codebase, which has not >>> used JSON licensed jars (or similar), are allowed to use >>> them. In other words, if you haven't been using them, you >>> aren't allowed to start. It is Cat-X. >>> >>> o If you have been using it, and have done so in a *release*, >>> AND there has been NO pushback from your community/eco-system, >>> you have a temporary exclusion from the Cat-X classification thru >>> April 30, 2017. At that point in time, ANY and ALL usage >>> of these JSON licensed artifacts are DISALLOWED. You must >>> either find a suitably licensed replacement, or do without. >>> There will be NO exceptions. >>> >>> o Any situation not covered by the above is an implicit >>> DISALLOWAL of usage. >>> >>> Also please note that in the 2nd situation (where a temporary >>> exclusion has been granted), you MUST ensure that NOTICE explicitly >>> notifies the end-user that a JSON licensed artifact exists. They >>> may not be aware of it up to now, and that MUST be addressed. >>> >>> If there are any questions, please ask on the ***@***.******@***.***> >>> list. >>> >>> -- >>> Jim Jagielski >>> VP Legal Affairs

I notice that the license for the include files you are BSD which should be fine. I don’t believe you are using any JSON jar files so perhaps the license change does not affect us or Postgres. However, INAL and cannot state positively that we are okay. Should we ask for a clarification from the legal list? Roberta From: Liu Ming [mailto:notifications@github.com] Sent: Wednesday, November 23, 2016 6:01 AM To: apache/incubator-trafodion <incubator-trafodion@noreply.github.com> Cc: Roberta Marton <roberta.marton@esgyn.com>; Mention <mention@noreply.github.com> Subject: Re: [apache/incubator-trafodion] jira 2227 : support json fuction - JSON_OBJECT_FIELD_TEXT (#833) @svarnau<https://github.com/svarnau> , @gtapper<https://github.com/gtapper> do you have concerns from license point of view of merging this PR? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub<#833 (comment)>, or mute the thread<https://github.com/notifications/unsubscribe-auth/AMa1INImMLs26aPIoaQ6v-BjyrPaGiRCks5rBEc2gaJpZM4Kw61P>.

Traf-Jenkins · 2016-11-28T06:09:13Z

Check Test Started: https://jenkins.esgyn.com/job/Check-PR-master/1409/

Traf-Jenkins · 2016-11-28T09:01:58Z

Test Passed. https://jenkins.esgyn.com/job/Check-PR-master/1409/

moscowgentalman · 2016-11-29T12:53:49Z

@andyyangcn , you need to fix the conflicts

Do a 'git pull' and fix the merge issue, and commit again.

sync

Traf-Jenkins · 2016-12-05T07:45:14Z

New Check Test Started: https://jenkins.esgyn.com/job/Check-PR-master/1433/

Traf-Jenkins · 2016-12-05T10:41:09Z

Test Passed. https://jenkins.esgyn.com/job/Check-PR-master/1433/

jira 2227

8a49f90

moscowgentalman reviewed Nov 14, 2016

View reviewed changes

zellerh reviewed Nov 14, 2016

View reviewed changes

moscowgentalman reviewed Nov 15, 2016

View reviewed changes

unknown added 2 commits November 15, 2016 14:20

jira 2227

4bb6282

jira 2227

4b806d1

jira 2227 update license file

1c25e39

jira 2227 update sqlparser.y

190915b

jira 2227 - update license file

580df31

moscowgentalman reviewed Nov 17, 2016

View reviewed changes

svarnau reviewed Nov 23, 2016

View reviewed changes

Merge pull request #1 from apache/master

2b3d98b

update from apache

remove json files from .rat-excludes

aef4f93

andyyangcn and others added 2 commits December 5, 2016 15:17

Merge pull request #3 from apache/master

691ba74

sync

fix conflicts

7dbfdb3

asfgit merged commit 7dbfdb3 into apache:master Dec 6, 2016

jira 2227 : support json fuction - JSON_OBJECT_FIELD_TEXT #833

jira 2227 : support json fuction - JSON_OBJECT_FIELD_TEXT #833

Conversation

andyyangcn commented Nov 14, 2016

Traf-Jenkins commented Nov 14, 2016

Traf-Jenkins commented Nov 14, 2016

Traf-Jenkins commented Nov 14, 2016

Traf-Jenkins commented Nov 14, 2016

moscowgentalman commented Nov 14, 2016

moscowgentalman commented Nov 14, 2016

svarnau commented Nov 14, 2016

Traf-Jenkins commented Nov 14, 2016

Traf-Jenkins commented Nov 14, 2016

moscowgentalman 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

zellerh Nov 14, 2016 • edited

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

zellerh commented Nov 14, 2016

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

Traf-Jenkins commented Nov 15, 2016

Traf-Jenkins commented Nov 15, 2016

Traf-Jenkins commented Nov 15, 2016

Traf-Jenkins commented Nov 15, 2016

Traf-Jenkins commented Nov 15, 2016

Traf-Jenkins commented Nov 15, 2016

Traf-Jenkins commented Nov 17, 2016

Traf-Jenkins commented Nov 17, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

moscowgentalman commented Nov 18, 2016 • edited

andyyangcn commented Nov 18, 2016

moscowgentalman commented Nov 23, 2016

svarnau left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

svarnau commented Nov 23, 2016

robertamarton commented Nov 25, 2016 via email

Traf-Jenkins commented Nov 28, 2016

Traf-Jenkins commented Nov 28, 2016

moscowgentalman commented Nov 29, 2016

Traf-Jenkins commented Dec 5, 2016

Traf-Jenkins commented Dec 5, 2016

zellerh Nov 14, 2016 •

edited

moscowgentalman commented Nov 18, 2016 •

edited