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
