Re: Support for jsonpath .datetime() method

On 7/16/19 6:41 AM, Alexander Korotkov wrote:
> Hi!
>
> Thank you for the review!
>
> Revised version of patch is attached.
>
> On Mon, Jul 15, 2019 at 3:57 PM Anastasia Lubennikova
> <lubennikovaav(at)gmail(dot)com> wrote:
>> In general, the feature looks good. It is consistent with the standard and the code around.
>> It definitely needs more documentation - datetime() and new jsonb_path_*_tz() functions are not documented.
> Documentation is added for both jsonpath .datetime() method and SQL
> jsonb_path_*_tz() functions.
>
>> Here are also minor questions on implementation and code style:
>>
>> 1) + case jbvDatetime:
>> elog(ERROR, "unexpected jbvBinary value");
>> We should use separate error message for jvbDatetime here.
> Fixed.
>
>> 2) + *jentry = JENTRY_ISSTRING | len;
>> Here we can avoid using JENTRY_ISSTRING since it defined to 0x0.
>> I propose to do so to be consistent with jbvString case.
> Fixed.
>
>> 3)
>> + * Default time-zone for tz types is specified with 'tzname'. If 'tzname' is
>> + * NULL and the input string does not contain zone components then "missing tz"
>> + * error is thrown.
>> + */
>> +Datum
>> +parse_datetime(text *date_txt, text *fmt, bool strict, Oid *typid,
>> + int32 *typmod, int *tz)
>>
>> The comment about 'tzname' is outdated.
> Fixed.
>
>> 4) Some typos:
>>
>> + * Convinience macros for error handling
>>> * Convenience macros for error handling
>> + * Two macros below helps handling errors in functions, which takes
>>> * Two macros below help to handle errors in functions, which take
> Fixed.
>
>> 5) + * RETURN_ERROR() macro intended to wrap ereport() calls. When have_error
>> + * argument is provided, then instead of ereport'ing we set *have_error flag
>>
>> have_error is not a macro argument, so I suggest to rephrase this comment.
>>
>> Shouldn't we pass have_error explicitly?
>> In case someone will change the name of the variable, this macro will work incorrectly.
> Comment about RETURN_ERROR() is updated. RETURN_ERROR() is just
> file-wide macro. So I think in this case it's ok to pass *have_error
> flag implicitly for the sake of brevity.
>
>> 6) * When no argument is supplied, first fitting ISO format is selected.
>> + /* Try to recognize one of ISO formats. */
>> + static const char *fmt_str[] =
>> + {
>> + "yyyy-mm-dd HH24:MI:SS TZH:TZM",
>> + "yyyy-mm-dd HH24:MI:SS TZH",
>> + "yyyy-mm-dd HH24:MI:SS",
>> + "yyyy-mm-dd",
>> + "HH24:MI:SS TZH:TZM",
>> + "HH24:MI:SS TZH",
>> + "HH24:MI:SS"
>> + };
>>
>> How do we choose the order of formats to check? Is it in standard?
>> Anyway, I think this struct needs a comment that explains that changing of order can affect end-user.
> Yes, standard defines which order we should try datetime types (and
> corresponding ISO formats). I've updated respectively array, its
> comment and docs.
>
>> 7) + if (res == jperNotFound)
>> + RETURN_ERROR(ereport(ERROR,
>> + (errcode(ERRCODE_INVALID_ARGUMENT_FOR_JSON_DATETIME_FUNCTION),
>> + errmsg("invalid argument for SQL/JSON datetime function"),
>> + errdetail("unrecognized datetime format"),
>> + errhint("use datetime template argument for explicit format specification"))));
>>
>> The hint is confusing. If I understand correctly, no-arg datetime function supports all formats,
>> so if parsing failed, it must be an invalid argument and providing format explicitly won't help.
> Custom format string may define format not enumerated in fmt_str[].
> For instance, imagine "dd.mm.yyyy". In some cases custom format
> string can fix the error. So, ISTM hint is OK.
>
> I'm setting this back to "Needs review" waiting for either you or
> Peter Eisentraut provide additional review.
>
> ------
> Alexander Korotkov
> Postgres Professional: http://www.postgrespro.com
> The Russian Postgres Company

Hi Alexander,

I had look at the added docs and would like to suggest a couple of
changes. Please see the attached patches with my my edits for func.sgml
and some of the comments.

Looks like we also need to change the following entry in
features-unsupported.sgml, and probably move it to features-supported.sgml?

 <row>
  <entry>T832</entry>
  <entry></entry>
  <entry>SQL/JSON path language: item method</entry>
  <entry>datetime() not yet implemented</entry>
 </row>

--
Liudmila Mantrova
Technical writer at Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company