Re: Support for jsonpath .datetime() method

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