-
Notifications
You must be signed in to change notification settings - Fork 88
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add SELECT FOR JSON AUTO support in Babelfish #2243
Add SELECT FOR JSON AUTO support in Babelfish #2243
Conversation
Signed-off-by: Jake Owen <owjco@amazon.com>
Signed-off-by: Jake Owen <owjco@amazon.com>
Signed-off-by: Jake Owen <owjco@amazon.com>
Signed-off-by: Jake Owen <owjco@amazon.com>
Pull Request Test Coverage Report for Build 7561344918Warning: This coverage report may be inaccurate.We've detected an issue with your CI configuration that might affect the accuracy of this pull request's coverage report.
💛 - Coveralls |
Signed-off-by: Jake Owen <owjco@amazon.com>
Signed-off-by: Jake Owen <owjco@amazon.com>
Signed-off-by: Jake Owen <owjco@amazon.com>
Signed-off-by: Jake Owen <owjco@amazon.com>
TargetEntry* te = (TargetEntry*) lc->ptr_value; | ||
if(te && strncmp(te->resname, "json", 4) == 0 && te->expr != NULL && ((Expr*) te->expr)->type == T_FuncExpr) { | ||
List* args = ((FuncExpr*) te->expr)->args; | ||
if(args != NULL && ((Node*) ((ListCell*) args->elements)->ptr_value)->type == T_Aggref) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you mean need to get the first elements of List arg here ?
I think better to write linitial ? or proper function from pg_list.h
// Handle Views with an alias | ||
SubLink* sl = (SubLink*) te->expr; | ||
if(((Node*) sl->subselect)->type == T_Query) | ||
checkForJsonAuto((Query*) sl->subselect); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there any else in here ?
Signed-off-by: Jake Owen <owjco@amazon.com>
Signed-off-by: Jake Owen <owjco@amazon.com>
Signed-off-by: Jake Owen <owjco@amazon.com>
@@ -865,6 +875,9 @@ pltsql_pre_parse_analyze(ParseState *pstate, RawStmt *parseTree) | |||
static void | |||
pltsql_post_parse_analyze(ParseState *pstate, Query *query, JumbleState *jstate) | |||
{ | |||
if(!checkForJsonAuto(query)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The place to put query tree walker should be after tsql dialect check
@@ -865,6 +875,9 @@ pltsql_pre_parse_analyze(ParseState *pstate, RawStmt *parseTree) | |||
static void | |||
pltsql_post_parse_analyze(ParseState *pstate, Query *query, JumbleState *jstate) | |||
{ | |||
if(!checkForJsonAuto(query)) | |||
(void) query_tree_walker(query, check_json_auto_walker, (void *) pstate, 0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In here we should call check_json_auto_walker instead of query_tree_walker to avoid one more stack level.
forjson_table **tableInfoArr; | ||
if(target) { | ||
ListCell* lc = list_nth_cell(target, 0); | ||
if(lc != NULL && ((Node*) lfirst(lc))->type == T_TargetEntry) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use nodeTag(node) == T_TargetEntry to make the code more readable
|
||
// Modify query to be of the form "JSONAUTOALIAS.[nest_level].[table_alias]" | ||
rtable = (List*) query->rtable; | ||
if(rtable != NULL) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What if it's == NULL, in such case we should throw exception ? If so , pls use assert
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Otherwise we'll have to write some else clause to cover those
JsonbPair *rowPairs; | ||
if(currDepth == maxDepth) { | ||
jsonbArray->val.array.nElems++; | ||
jsonbArray->val.array.elems = (JsonbValue *) repalloc(jsonbArray->val.array.elems, sizeof(JsonbValue) * (jsonbArray->val.array.nElems)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why we realloc some space in here ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to allocate space for the new array element being added to jsonbArray
if(args != NULL && ((Node*) linitial(args))->type == T_Aggref) { | ||
Aggref* agg = linitial_node(Aggref, args); | ||
List* aggargs = agg->args; | ||
if(aggargs != NULL && list_nth_cell(aggargs, 1) != NULL && ((Node*) lsecond(aggargs))->type == T_TargetEntry) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
list_nth_cell(aggargs, 1) != NULL
--> should be
list_length(aggargs) > 1 ?
} | ||
|
||
for(int i = 0; i < subq->targetList->length; i++) { | ||
TargetEntry* te = castNode(TargetEntry, lfirst(list_nth_cell(subq->targetList, i))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we're for loop into targetList, why not using foreach( lc , targetlist ) ?
Signed-off-by: Jake Owen <owjco@amazon.com>
Signed-off-by: Jake Owen <owjco@amazon.com>
Signed-off-by: Jake Owen <owjco@amazon.com>
Signed-off-by: Jake Owen <owjco@amazon.com>
ctequery = (Query*) cte->ctequery; | ||
foreach(lc2, ctequery->rtable) { | ||
subqRte = castNode(RangeTblEntry, lfirst(lc2)); | ||
if(subqRte->rtekind == RTE_RELATION) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pls add a
assert(subqRte->rtekind == RTE_RELATION)
in here to make sure we don't miss anything in future
CREATE TRIGGER forjson_vu_trigger_1 on forjson_auto_vu_t_users for insert as | ||
begin | ||
select U.Id AS "users.userid", | ||
U.firstname as "firstname", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should add one more test case that inside the trigger like this :
with cte as (
select * from inserted
)
select * from cte as json auto
@@ -215,6 +215,7 @@ forjson | |||
forjson-datatypes | |||
forjson-subquery | |||
forjson-nesting | |||
forjsonauto |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we already support for json auto syntax, then we should add this test case into other Mvu schedule files as well.
Signed-off-by: Jake Owen <owjco@amazon.com>
Signed-off-by: Jake Owen <owjco@amazon.com>
e5bb2ec
into
babelfish-for-postgresql:BABEL_4_X_DEV
…ql#2243) This change adds SELECT FOR JSON AUTO support to Babelfish which nests JSON objects based on the structure of the Select statement. Task: BABEL-3668 Signed-off-by: Jake Owen <owjco@amazon.com>
…ql#2243) This change adds SELECT FOR JSON AUTO support to Babelfish which nests JSON objects based on the structure of the Select statement. Task: BABEL-3668 Signed-off-by: Jake Owen <owjco@amazon.com>
…ql#2243) (babelfish-for-postgresql#2270) This change adds SELECT FOR JSON AUTO support to Babelfish which nests JSON objects based on the structure of the Select statement. Task: BABEL-3668 Signed-off-by: Jake Owen <owjco@amazon.com>
Description
This change adds
SELECT FOR JSON AUTO
support to Babelfish which nests JSON objects based on the structure of the Select statement.Issues Resolved
BABEL-3668
Test Scenarios Covered
Minor version upgrade tests -
NA
Major version upgrade tests -
Added to upgrade scripts
Performance tests -
NA
Tooling impact -
NA
Client tests -
NA
Check List
By submitting this pull request, I confirm that my contribution is under the terms of the Apache 2.0 and PostgreSQL licenses, and grant any person obtaining a copy of the contribution permission to relicense all or a portion of my contribution to the PostgreSQL License solely to contribute all or a portion of my contribution to the PostgreSQL open source project.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.