From: | Chao Li <li(dot)evan(dot)chao(at)gmail(dot)com> |
---|---|
To: | Alexandra Wang <alexandra(dot)wang(dot)oss(at)gmail(dot)com> |
Cc: | jian he <jian(dot)universality(at)gmail(dot)com>, Nikita Malakhov <hukutoc(at)gmail(dot)com>, Vik Fearing <vik(at)postgresfriends(dot)org>, Mark Dilger <mark(dot)dilger(at)enterprisedb(dot)com>, Matheus Alcantara <matheusssilv97(at)gmail(dot)com>, Peter Eisentraut <peter(at)eisentraut(dot)org>, Andrew Dunstan <andrew(at)dunslane(dot)net>, Nikita Glukhov <glukhov(dot)n(dot)a(at)gmail(dot)com>, PostgreSQL Hackers <pgsql-hackers(at)postgresql(dot)org>, "David E(dot) Wheeler" <david(at)justatheory(dot)com> |
Subject: | Re: SQL:2023 JSON simplified accessor support |
Date: | 2025-09-03 07:11:03 |
Message-ID: | EF15A5C3-7F07-4A42-85F5-290C7568E1CA@gmail.com |
Views: | Whole Thread | Raw Message | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
> On Sep 3, 2025, at 10:16, Alexandra Wang <alexandra(dot)wang(dot)oss(at)gmail(dot)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.
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.
Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/
From | Date | Subject | |
---|---|---|---|
Next Message | Steven Niu | 2025-09-03 07:22:00 | 回复: Fix segfault while accessing half-initialized hash table in pgstat_shmem.c |
Previous Message | Michael Paquier | 2025-09-03 07:10:01 | Re: Fix segfault while accessing half-initialized hash table in pgstat_shmem.c |