Re: Support for jsonpath .datetime() method - Mailing list pgsql-hackers

The following review has been posted through the commitfest application:
make installcheck-world:  tested, passed
Implements feature:       tested, passed
Spec compliant:           tested, passed
Documentation:            not tested

Hi,

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.

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.

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.

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.

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

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.

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.

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.

The new status of this patch is: Waiting on Author