> <v16-0007-Implement-jsonb-wildcard-member-accessor.patch><v16-0001-Allow-transformation-of-only-a-sublist-of-subscr.patch><v16-0004-Extract-coerce_jsonpath_subscript.patch><v16-0006-Implement-Jsonb-subscripting-with-slicing.patch><v16-0005-Implement-read-only-dot-notation-for-jsonb.patch><v16-0003-Export-jsonPathFromParseResult.patch><v16-0002-Allow-Generic-Type-Subscripting-to-Accept-Dot-No.patch>


A few more comment for v16.

1 - 0002
```
--- a/contrib/hstore/hstore_subs.c
+++ b/contrib/hstore/hstore_subs.c

-       if (isSlice || list_length(*indirection) != 1)
+       if (ai->is_slice || list_length(*indirection) != 1)
```

We should put list_length() check before ai->is_slace. Because when indirection 
length is greater than 1, it take a single check; other it may need to check 
the both conditions.

2 - 0002 - also in hstore_subs.c 
```
   *indirection = NIL;
```

We should do “list_delete_first_n(indirection, 1)”.

3 - 0003
```
+Datum
+jsonPathFromParseResult(JsonPathParseResult *jsonpath, int estimated_len,
+                                               struct Node *escontext)
+{
```

`jsonpath` is not changed in this function, so it should be defined as a const: 
“const JsonPathParseResult *jsonpath”. 

4 - 0004
```
+               Oid                     targets[2] = {INT4OID, TEXTOID};
+
+               /*
+                * Jsonb can handle multiple subscript types, but cases when a
+                * subscript could be coerced to multiple target types must be
+                * avoided, similar to overloaded functions. It could be 
possibly
+                * extend with jsonpath in the future.
+                */
+               for (int i = 0; i < 2; i++)
```

This is a nit optimization. We can do:

Oid targets[] = {INT4OID, TEXTOID};
For (int I = 0; I < sizeof(targets) / sizeof(targets[0]); I ++) 

This way lets the compiler to decide length of “targets”. So that avoids hard 
code “2” in “for”. If we need to add more elements to “targets”, we can just 
add it without updating “for”.

Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/




Reply via email to