Re: pgsql: Add more SQL/JSON constructor functions - Mailing list pgsql-hackers
From | Amit Langote |
---|---|
Subject | Re: pgsql: Add more SQL/JSON constructor functions |
Date | |
Msg-id | CA+HiwqEjUNWZRkepadDc3xqf7kBppDSodOVHCfhR-84DbdY=Cg@mail.gmail.com Whole thread Raw |
In response to | Re: pgsql: Add more SQL/JSON constructor functions (jian he <jian.universality@gmail.com>) |
List | pgsql-hackers |
On Thu, Aug 22, 2024 at 11:02 jian he <jian.universality@gmail.com> wrote:
On Tue, Jul 30, 2024 at 12:59 PM Amit Langote <amitlangote09@gmail.com> wrote:
>
> Hi,
>
> On Fri, Jul 26, 2024 at 11:19 PM jian he <jian.universality@gmail.com> wrote:
> > On Fri, Jul 26, 2024 at 4:53 PM Amit Langote <amitlangote09@gmail.com> wrote:
> > >
> > >
> > > Pushed 0003-0005 ahead of 0001-0002. Will try to push them over the
> > > weekend. Rebased for now.
>
> Pushed them now.
>
> > {
> > ...
> > /*
> > * For expression nodes that support soft errors. Should be set to NULL
> > * before calling ExecInitExprRec() if the caller wants errors thrown.
> > */
> > ErrorSaveContext *escontext;
> > } ExprState;
> >
> > i believe by default makeNode will set escontext to NULL.
> > So the comment should be, if you want to catch the soft errors, make
> > sure the escontext pointing to an allocated ErrorSaveContext.
> > or maybe just say, default is NULL.
> >
> > Otherwise, the original comment's meaning feels like: we need to
> > explicitly set it to NULL
> > for certain operations, which I believe is false?
>
> OK, I'll look into updating this.
>
> > struct
> > {
> > Oid targettype;
> > int32 targettypmod;
> > bool omit_quotes;
> > bool exists_coerce;
> > bool exists_cast_to_int;
> > bool check_domain;
> > void *json_coercion_cache;
> > ErrorSaveContext *escontext;
> > } jsonexpr_coercion;
> > do we need to comment that "check_domain" is only used for JSON_EXISTS_OP?
>
> I've renamed it to exists_check_domain and added a comment that
> exists_* fields are relevant only for JSON_EXISTS_OP.
>
> > json_behavior_type:
> > ERROR_P { $$ = JSON_BEHAVIOR_ERROR; }
> > | NULL_P { $$ = JSON_BEHAVIOR_NULL; }
> > | TRUE_P { $$ = JSON_BEHAVIOR_TRUE; }
> > | FALSE_P { $$ = JSON_BEHAVIOR_FALSE; }
> > | UNKNOWN { $$ = JSON_BEHAVIOR_UNKNOWN; }
> > | EMPTY_P ARRAY { $$ = JSON_BEHAVIOR_EMPTY_ARRAY; }
> > | EMPTY_P OBJECT_P { $$ = JSON_BEHAVIOR_EMPTY_OBJECT; }
> > /* non-standard, for Oracle compatibility only */
> > | EMPTY_P { $$ = JSON_BEHAVIOR_EMPTY_ARRAY; }
> > ;
> >
> > EMPTY_P behaves the same as EMPTY_P ARRAY
> > so for function GetJsonBehaviorConst, the following "case
> > JSON_BEHAVIOR_EMPTY:" is wrong?
> >
> > case JSON_BEHAVIOR_NULL:
> > case JSON_BEHAVIOR_UNKNOWN:
> > case JSON_BEHAVIOR_EMPTY:
> > val = (Datum) 0;
> > isnull = true;
> > typid = INT4OID;
> > len = sizeof(int32);
> > isbyval = true;
> > break;
> >
> > also src/backend/utils/adt/ruleutils.c
> > if (jexpr->on_error->btype != JSON_BEHAVIOR_EMPTY)
> > get_json_behavior(jexpr->on_error, context, "ERROR");
>
> Something like the attached makes sense? While this meaningfully
> changes the deparsing output, there is no behavior change for
> JsonTable top-level path execution. That's because the behavior when
> there's an error in the execution of the top-level path is to throw it
> or return an empty set, which is handled in jsonpath_exec.c, not
> execExprInterp.c.
>
hi amit.
seems you forgot to attach the patch?
Yeah, I had planned to look at this after my vacation earlier this month, but I context switched into working on another project and lost track of this. I’ll make some time next week to fix whatever remains go be fixed here. Thanks for the reminder.
pgsql-hackers by date: