Re: jsonpath

Attached 23rd version of the patches.


On 05.01.2019 3:11, Alexander Korotkov wrote:
> On Tue, Dec 4, 2018 at 2:23 AM Nikita Glukhov <n.gluhov@postgrespro.ru> wrote:
>> 2) We define both DCH_FF# and DCH_ff#, but we never ever use the
>> lower-case version. Heck, it's not mentioned even in DCH_keywords, which
>> does this:
>>
>>    ...
>>    {"FF1", 3, DCH_FF1, false, FROM_CHAR_DATE_NONE},  /* F */
>>    ...
>>    {"ff1", 3, DCH_FF1, false, FROM_CHAR_DATE_NONE},  /* F */
>>    ...
>>
>> Compare that to DCH_DAY, DCH_Day and DCH_day, mapped to "DAY", "Day" and
>> "day".
>>
>> Yes, "ff#" are mapped to DCH_FF# like "mi" is mapped DCH_MI.
>>
>> "Day", "day" are not mapped to DCH_DAY because they determine letter case in the
>> output, but "ff1" and "FF#" output contains only digits.
> Right, DCH_poz is also offset in DCH_keywords array.  So, if array has
> an entry for "ff1" then enum should have a DCH_ff1 member in the same
> position.
>
> I got some other questions regarding this patchset.
>
> 1) Why do we parse FF7-FF9 if we're not supporting them anyway?
> Without defining FF7-FF9 we can also don't throw errors for them
> everywhere.  That would save us some code lines.
FF7-FF9 were removed.
> 2) + DCH_to_char_fsec("%01d", in->fsec / INT64CONST(100000));
> Why do we use INT64CONST() here and in the similar places assuming
> that fsec is only uint32?
Fixed.

> 3) wrapItem() is unused in
> 0002-Jsonpath-engine-and-operators-v21.patch, but used in
> 0006-Jsonpath-syntax-extensions-v21.patch.  Please, move it to
> 0006-Jsonpath-syntax-extensions-v21.patch?
wraptItem() was moved into patch #6.

> 4) I also got these couple of warning during compilation.
>
> jsonpath_exec.c:1485:1: warning: unused function
> 'recursiveExecuteNested' [-Wunused-function]
> recursiveExecuteNested(JsonPathExecContext *cxt, JsonPathItem *jsp,
> ^
> 1 warning generated.
> jsonpath_scan.l:444:6: warning: implicit declaration of function
> 'jsonpath_yyparse' is invalid in C99 [-Wimplicit-function-declaration]
>          if (jsonpath_yyparse((void*)&parseresult) != 0)
>              ^
> 1 warning generated.
>
> Perhaps recursiveExecuteNested() is unsed in this patchset.  It's
> probably used by some subsequent SQL/JSON-related patchset.  So,
> please, move it there.

recursiveExecuteNested() was removed from this patch set.

Prototype for jsonpath_yyparse() should be in the Bison-generated file
src/include/adt/jsonpath_gram.h.

> 5) I think each usage of PG_TRY()/PG_CATCH() deserves comment
> describing why it's safe to use without subtransaction.  In fact we
> should just state that no function called inside performs data
> modification.
Comments were added.

-- 
Nikita Glukhov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company