Add SELECT FOR JSON AUTO support in Babelfish by Jakeowen1 · Pull Request #2243 · babelfish-for-postgresql/babelfish_extensions

Jakeowen1 · 2024-01-10T21:26:42Z

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

Use case based -

select U.Id AS "users.userid", O.productId AS "order.productId", O.Id AS "product.oid", P.price AS "product.price" FROM forjson_nesting_vu_t_users U JOIN forjson_nesting_vu_t_orders O ON (U.id = O.userid) JOIN forjson_nesting_vu_t_products P ON (P.id = O.productid) FOR JSON AUTO

[{"users.userid": 1, "o": [{"order.productId": 1, "product.oid": 2, "p": [{"product.price": "20"}, {"product.price": "20"}, {"product.price": "30"}, {"product.price": "30"}]}, {"order.productId": 1, "product.oid": 1, "p": [{"product.price": "20"}, {"product.price": "20"}, {"product.price": "30"}, {"product.price": "30"}]}]}]

Boundary conditions -

select U.Id AS "users.userid", O.productId AS "order.productId", O.Id AS "product.oid", P.price AS "product.price", S.totalSales AS "totalsales" FROM forjson_nesting_vu_t_users U JOIN forjson_nesting_vu_t_orders O ON (U.id = O.userid) JOIN forjson_nesting_vu_t_products P ON (P.id = O.productid) JOIN forjson_nesting_vu_t_sales S ON (P.price = S.price) FOR JSON AUTO

[{"users.userid": 1, "o": [{"order.productId": 1, "product.oid": 2, "p": [{"product.price": "20", "s": [{"totalsales": 50}, {"totalsales": 50}]}]}, {"order.productId": 1, "product.oid": 1, "p": [{"product.price": "30", "s": [{"totalsales": 100}, {"totalsales": 100}]}]}, {"order.productId": 1, "product.oid": 2, "p": [{"product.price": "30", "s": [{"totalsales": 100}, {"totalsales": 100}]}]}, {"order.productId": 1, "product.oid": 1, "p": [{"product.price": "20", "s": [{"totalsales": 50}, {"totalsales": 50}]}]}]}]

Arbitrary inputs -

INSERT INTO forjson_nesting_vu_t_sales VALUES (1, NULL, NULL), (2, NULL, NULL);
GO

select U.Id AS "users.userid", O.productId AS "order.productId", O.Id AS "product.oid", P.price AS "product.price", S.totalSales AS "totalsales" FROM forjson_nesting_vu_t_users U JOIN forjson_nesting_vu_t_orders O ON (U.id = O.userid) JOIN forjson_nesting_vu_t_products P ON (P.id = O.productid) JOIN forjson_nesting_vu_t_sales S ON (P.price = S.price) FOR JSON AUTO

[{"users.userid": 1, "o": [{"order.productId": 1, "product.oid": 2, "p": [{"product.price": "20", "s": [{"totalsales": 50}, {"totalsales": 50}]}]}, {"order.productId": 1, "product.oid": 1, "p": [{"product.price": "30", "s": [{"totalsales": 100}, {"totalsales": 100}]}]}, {"order.productId": 1, "product.oid": 2, "p": [{"product.price": "30", "s": [{"totalsales": 100}, {"totalsales": 100}]}]}, {"order.productId": 1, "product.oid": 1, "p": [{"product.price": "20", "s": [{"totalsales": 50}, {"totalsales": 50}]}]}]}]

Negative test cases -

1> select cast(null as datetime) for JSON AUTO
2> go
json                                                                                                                                                                                                                                                            
----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
[{}]                                                                                                                                                                                                                                                            

(1 rows affected)
1>

Minor version upgrade tests -
NA
Major version upgrade tests -
Added to upgrade scripts
Performance tests -
NA
Tooling impact -
NA
Client tests -
NA

Check List

Commits are signed per the DCO using --signoff

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.

Signed-off-by: Jake Owen <owjco@amazon.com>

coveralls · 2024-01-11T00:05:21Z

Pull Request Test Coverage Report for Build 7561344918

Warning: 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.
To ensure accuracy in future PRs, please see these guidelines.
A quick fix for this PR: rebase it; your next report should be accurate.

0 of 0 changed or added relevant lines in 0 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+0.1%) to 71.821%

Totals
Change from base Build 7472197412:	0.1%
Covered Lines:	39695
Relevant Lines:	55269

💛 - Coveralls

Signed-off-by: Jake Owen <owjco@amazon.com>

contrib/babelfishpg_tsql/src/pl_handler.c

forestkeeper · 2024-01-11T23:16:04Z

contrib/babelfishpg_tsql/src/pl_handler.c

+			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) {


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

contrib/babelfishpg_tsql/src/pl_handler.c

forestkeeper · 2024-01-11T23:18:58Z

contrib/babelfishpg_tsql/src/pl_handler.c

+				// Handle Views with an alias
+				SubLink* sl = (SubLink*) te->expr;
+				if(((Node*) sl->subselect)->type == T_Query)
+					checkForJsonAuto((Query*) sl->subselect);


Is there any else in here ?

contrib/babelfishpg_tsql/src/pl_handler.c

contrib/babelfishpg_tsql/src/tsql_for/forjson.c

Signed-off-by: Jake Owen <owjco@amazon.com>

forestkeeper · 2024-01-16T19:54:24Z

contrib/babelfishpg_tsql/src/pl_handler.c

 static void
 pltsql_post_parse_analyze(ParseState *pstate, Query *query, JumbleState *jstate)
 {
+	if(!checkForJsonAuto(query))


The place to put query tree walker should be after tsql dialect check

forestkeeper · 2024-01-16T19:58:33Z

contrib/babelfishpg_tsql/src/pl_handler.c

 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);


In here we should call check_json_auto_walker instead of query_tree_walker to avoid one more stack level.

forestkeeper · 2024-01-16T20:00:20Z

contrib/babelfishpg_tsql/src/pl_handler.c

+	forjson_table **tableInfoArr;
+	if(target) {
+		ListCell* lc = list_nth_cell(target, 0);
+		if(lc != NULL && ((Node*) lfirst(lc))->type == T_TargetEntry) {


Please use nodeTag(node) == T_TargetEntry to make the code more readable

forestkeeper · 2024-01-16T20:02:04Z

contrib/babelfishpg_tsql/src/pl_handler.c

+
+	// Modify query to be of the form "JSONAUTOALIAS.[nest_level].[table_alias]" 
+	rtable = (List*) query->rtable;
+	if(rtable != NULL) {


What if it's == NULL, in such case we should throw exception ? If so , pls use assert

Otherwise we'll have to write some else clause to cover those

contrib/babelfishpg_tsql/src/pl_handler.c

forestkeeper · 2024-01-16T20:08:56Z

contrib/babelfishpg_tsql/src/tsql_for/forjson.c

+	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));


Why we realloc some space in here ?

We need to allocate space for the new array element being added to jsonbArray

test/JDBC/expected/forjsonauto-vu-prepare.out

forestkeeper · 2024-01-16T21:39:35Z

contrib/babelfishpg_tsql/src/pl_handler.c

+				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) {


list_nth_cell(aggargs, 1) != NULL

--> should be

list_length(aggargs) > 1 ?

forestkeeper · 2024-01-16T21:41:21Z

contrib/babelfishpg_tsql/src/pl_handler.c

+					}
+
+					for(int i = 0; i < subq->targetList->length; i++) {
+						TargetEntry* te = castNode(TargetEntry, lfirst(list_nth_cell(subq->targetList, i)));


If we're for loop into targetList, why not using foreach( lc , targetlist ) ?

Signed-off-by: Jake Owen <owjco@amazon.com>

forestkeeper · 2024-01-17T19:06:50Z

contrib/babelfishpg_tsql/src/pl_handler.c

+					ctequery = (Query*) cte->ctequery;
+					foreach(lc2, ctequery->rtable) {
+						subqRte = castNode(RangeTblEntry, lfirst(lc2));
+						if(subqRte->rtekind == RTE_RELATION)


Pls add a

assert(subqRte->rtekind == RTE_RELATION)

in here to make sure we don't miss anything in future

forestkeeper · 2024-01-17T19:13:03Z

test/JDBC/expected/forjsonauto-vu-prepare.out

+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",


Should add one more test case that inside the trigger like this :

with cte as ( select * from inserted ) select * from cte as json auto

forestkeeper · 2024-01-17T19:29:00Z

test/JDBC/upgrade/latest/schedule

 forjson-datatypes
 forjson-subquery
 forjson-nesting
+forjsonauto


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>

…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>

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>

Jake Owen added 4 commits January 10, 2024 21:05

SELECT FOR JSON AUTO SUPPORT

46efdf4

Signed-off-by: Jake Owen <owjco@amazon.com>

Add cases to catch null values

aff4642

Signed-off-by: Jake Owen <owjco@amazon.com>

Add test files

dd94ee4

Signed-off-by: Jake Owen <owjco@amazon.com>

Fix auto detection

9a22757

Signed-off-by: Jake Owen <owjco@amazon.com>

Jake Owen added 4 commits January 11, 2024 00:20

Change test names to avoid conflict

383440e

Signed-off-by: Jake Owen <owjco@amazon.com>

Added functionality to prevent duplicates

853d26c

Signed-off-by: Jake Owen <owjco@amazon.com>

Added comments for clarity

6a0fcde

Signed-off-by: Jake Owen <owjco@amazon.com>

Remove duplicate code

a0f50a0

Signed-off-by: Jake Owen <owjco@amazon.com>

forestkeeper reviewed Jan 11, 2024

View reviewed changes

contrib/babelfishpg_tsql/src/pl_handler.c Show resolved Hide resolved

forestkeeper reviewed Jan 11, 2024

View reviewed changes

contrib/babelfishpg_tsql/src/pl_handler.c Outdated Show resolved Hide resolved