public inbox for [email protected]
help / color / mirror / Atom feedFrom: jian he <[email protected]>
To: Alexandra Wang <[email protected]>
Cc: Nikita Malakhov <[email protected]>
Cc: Vik Fearing <[email protected]>
Cc: Mark Dilger <[email protected]>
Cc: Matheus Alcantara <[email protected]>
Cc: Peter Eisentraut <[email protected]>
Cc: Andrew Dunstan <[email protected]>
Cc: Nikita Glukhov <[email protected]>
Cc: PostgreSQL Hackers <[email protected]>
Cc: David E. Wheeler <[email protected]>
Subject: Re: SQL:2023 JSON simplified accessor support
Date: Wed, 25 Jun 2025 13:56:51 +0800
Message-ID: <CACJufxFAaJ3=X7wnGmS0857ia8+-iwDzxhua9X8Qnh_CVB1V1A@mail.gmail.com> (raw)
In-Reply-To: <CACJufxEt5bL-xYojenA7x1Fq=-DfhW3KGH=3Zz9TjJ1k95=uuQ@mail.gmail.com>
References: <[email protected]>
<[email protected]>
<CAK98qZ2QGcyJrJAFv9wjY6S8yP9dUVnmG9Gb4OXuzuMMuM1Z5Q@mail.gmail.com>
<[email protected]>
<[email protected]>
<CAK98qZ3Ly6PhRwCVmMKJBba5oHVF9k370MMT2b_gep-SuQfRtg@mail.gmail.com>
<CAK98qZ1za8XgOLY+2hQMPGoxVYFyh=dDkM2dZPVeJK4J6poyvA@mail.gmail.com>
<CAK98qZ0EfrPcv3ZwGdeDiLPEpXYfHOEc8S2D=OCPJGcyWy5d-g@mail.gmail.com>
<CAK98qZ1rZaVNy6ViangQom4iinZVH7=ebqhTsPxMQbN5ZtE9XQ@mail.gmail.com>
<CAFY6G8cY5SUG5L-0ryVpom6HynE49p8-XQ59qkaEgnZZJ-c4Rg@mail.gmail.com>
<CAFY6G8eUGcU3A4AHprgYbSOAOj7+WGhGxS_YP0wd2+aCpZTiNg@mail.gmail.com>
<CAK98qZ14uKKRVFg4ibzMfReYaZD6Byxq8nYnvNNtXNjCfcd8kQ@mail.gmail.com>
<CAHgHdKtL9nNaKXGCLt9gWugVzYWKhoBDQ7NESUwdCBty8kFK-A@mail.gmail.com>
<CAK98qZ1nz6ZZhQqTOCNwRguZE5GsBLW5BQT_k=s7AA6gc2CN_g@mail.gmail.com>
<[email protected]>
<CAN-LCVM6A9z6AzxsEX-vx9=3XgEUU6+zjUqejQw8LfjaRY5P1A@mail.gmail.com>
<CAK98qZ2Pmf6ZSChLq+CEPKJ_8jSa0gFTNJWJTcWbCziDpqa=CA@mail.gmail.com>
<CACJufxEt5bL-xYojenA7x1Fq=-DfhW3KGH=3Zz9TjJ1k95=uuQ@mail.gmail.com>
hi.
in src/backend/catalog/sql_features.txt
should we mark any T860, T861, T862, T863, T864
items as YES?
typedef struct SubscriptingRef
{
/* expressions that evaluate to upper container indexes */
List *refupperindexpr;
}
SubscriptingRef.refupperindexpr meaning changed,
So the above comments also need to be changed?
v11-0004-Extract-coerce_jsonpath_subscript.patch
+ /*
+ * We known from can_coerce_type that coercion will succeed, so
+ * coerce_type could be used. Note the implicit coercion context, which is
+ * required to handle subscripts of different types, similar to overloaded
+ * functions.
+ */
+ subExpr = coerce_type(pstate,
+ subExpr, subExprType,
+ targetType, -1,
+ COERCION_IMPLICIT,
+ COERCE_IMPLICIT_CAST,
+ -1);
+ if (subExpr == NULL)
+ ereport(ERROR,
+ (errcode(ERRCODE_DATATYPE_MISMATCH),
+ errmsg("jsonb subscript must have text type"),
the targetType can be "integer", then the error message
errmsg("jsonb subscript must have text type") would be wrong?
Also this error handling is not necessary.
since we can_coerce_type already tell us that coercion will succeed.
Also, do we really put v11-0004 as a separate patch?
in gram.y we have:
if (!IsA($5, A_Const) ||
castNode(A_Const, $5)->val.node.type != T_String)
ereport(ERROR,
errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
errmsg("only string constants are
supported in JSON_TABLE path specification"),
so simply, in make_jsonpath_item_expr we can
expr = transformExpr(pstate, expr, pstate->p_expr_kind);
if (!IsA(expr, Const) || ((Const *) expr)->consttype != INT4OID)
ereport(ERROR,
errcode(ERRCODE_DATATYPE_MISMATCH),
errmsg("only integer constants are supported in jsonb
simplified accessor subscripting"),
parser_errposition(pstate, exprLocation(expr)));
because I think the current error message "got type: unknown" is not good.
select ('123'::jsonb).a['1'];
ERROR: jsonb simplified accessor supports subscripting in type: INT4,
got type: unknown
then we don't need two "ereport(ERROR" within make_jsonpath_item_expr
we can also Assert (cnst->constisnull) is false.
see gram.y:16976
I saw you introduce the word "AST", for example
"Converts jsonpath AST into jsonpath value in binary."
I am not sure that is fine.
in jsonb_subscript_make_jsonpath we have:
+ foreach(lc, *indirection)
+ {
+
+ if (IsA(accessor, String))
+ ....
+ else if (IsA(accessor, A_Star))
+ ....
+ else if (IsA(accessor, A_Indices))
+ ....
+ else
+ break;
Is the last else branch unreliable? since indirection only support for
String, A_Star, A_Indices, we already have Assert in jsonb_check_jsonpath_needed
to ensure that.
+ *indirection = list_delete_first_n(*indirection, pathlen);
also this is not necessary,
because pathlen will be the same length as list *indirection in current design.
Please check the attached minor refactoring based on v11-0001 to v11-0006
Attachments:
[application/octet-stream] v11-0001-misc-refactoring-based-on-v11_01_to_06.no-cfbot (3.7K, ../CACJufxFAaJ3=X7wnGmS0857ia8+-iwDzxhua9X8Qnh_CVB1V1A@mail.gmail.com/2-v11-0001-misc-refactoring-based-on-v11_01_to_06.no-cfbot)
download
view thread (68+ messages) latest in thread
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected]
Subject: Re: SQL:2023 JSON simplified accessor support
In-Reply-To: <CACJufxFAaJ3=X7wnGmS0857ia8+-iwDzxhua9X8Qnh_CVB1V1A@mail.gmail.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox