Re: SQL:2023 JSON simplified accessor support - Mailing list pgsql-hackers
| From | Chao Li |
|---|---|
| Subject | Re: SQL:2023 JSON simplified accessor support |
| Date | |
| Msg-id | EF15A5C3-7F07-4A42-85F5-290C7568E1CA@gmail.com Whole thread Raw |
| In response to | Re: SQL:2023 JSON simplified accessor support (Alexandra Wang <alexandra.wang.oss@gmail.com>) |
| List | pgsql-hackers |
On Sep 3, 2025, at 10:16, Alexandra Wang <alexandra.wang.oss@gmail.com> wrote:<v15-0007-Implement-jsonb-wildcard-member-accessor.patch><v15-0001-Allow-transformation-of-only-a-sublist-of-subscr.patch><v15-0004-Extract-coerce_jsonpath_subscript.patch><v15-0006-Implement-Jsonb-subscripting-with-slicing.patch><v15-0005-Implement-read-only-dot-notation-for-jsonb.patch><v15-0003-Export-jsonPathFromParseResult.patch><v15-0002-Allow-Generic-Type-Subscripting-to-Accept-Dot-No.patch>
I have a few more other small comments:
1 - 0002
```
diff --git a/src/backend/parser/parse_expr.c b/src/backend/parser/parse_expr.c
index 6e8fd42c612..ff104c95311 100644
--- a/src/backend/parser/parse_expr.c
+++ b/src/backend/parser/parse_expr.c
@@ -441,8 +441,9 @@ transformIndirection(ParseState *pstate, A_Indirection *ind)
ListCell *i;
/*
- * We have to split any field-selection operations apart from
- * subscripting. Adjacent A_Indices nodes have to be treated as a single
+ * Combine field names and subscripts into a single indirection list, as
+ * some subscripting containers, such as jsonb, support field access using
+ * dot notation. Adjacent A_Indices nodes have to be treated as a single
* multidimensional subscript operation.
*/
foreach(i, ind->indirection)
@@ -460,19 +461,43 @@ transformIndirection(ParseState *pstate, A_Indirection *ind)
}
else
{
- Node *newresult;
-
Assert(IsA(n, String));
+ subscripts = lappend(subscripts, n);
+ }
+ }
```
I raised this comment in my previous email, I guess you missed this one.
With this change, the 3 clauses are quite similar, the if-elseif-else can be simplified as:
If (!IsA(n, A_Indices) && !Is_A(n, A_Start))
Assert(IsA(n, String));
subscripts = lappend(subscripts, n)
2 - 0001
```
diff --git a/src/include/nodes/subscripting.h b/src/include/nodes/subscripting.h
index 234e8ad8012..5d576af346f 100644
--- a/src/include/nodes/subscripting.h
+++ b/src/include/nodes/subscripting.h
@@ -71,6 +71,11 @@ struct SubscriptExecSteps;
* does not care to support slicing, it can just throw an error if isSlice.)
* See array_subscript_transform() for sample code.
*
+ * The transform method receives a pointer to a list of raw indirections.
+ * This allows the method to parse a sublist of the indirections (typically
+ * the prefix) and modify the original list in place, enabling the caller to
+ * either process the remaining indirections differently or raise an error.
+ *
* The transform method is also responsible for identifying the result type
```
This is nit comment about the wording. “This allows the method to parse a sublist..." sounds like more from the patch perspective. It’s more suitable for git commit comments, but code comment, I feel it’s better to be more general, for example:
* The transform method receives a pointer to a list of raw indirections.
* It can parse a sublist (typically the prefix) of these indirections and
* modify the original list in place, allowing the caller to either handle
* the remaining indirections differently or raise an error.
* It can parse a sublist (typically the prefix) of these indirections and
* modify the original list in place, allowing the caller to either handle
* the remaining indirections differently or raise an error.
3 - 0005
```
diff --git a/src/backend/executor/execExpr.c b/src/backend/executor/execExpr.c
index f1569879b52..eac31b097e4 100644
--- a/src/backend/executor/execExpr.c
+++ b/src/backend/executor/execExpr.c
@@ -3320,50 +3320,54 @@ ExecInitSubscriptingRef(ExprEvalStep *scratch, SubscriptingRef *sbsref,
state->steps_len - 1);
}
+ /* When slicing, individual subscript bounds can be omitted */
+ if (!e)
+ {
+ sbsrefstate->upperprovided[i] = false;
+ sbsrefstate->upperindexnull[i] = true;
+ }
+ else {
+ sbsrefstate->upperprovided[i] = true;
```
This is also a nit comment about code format. “{" should be put to the next line of “else” according to other existing code.
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/
HighGo Software Co., Ltd.
https://www.highgo.com/
pgsql-hackers by date: