Re: Support for jsonpath .datetime() method
On 25.09.2019 22:55, Alexander Korotkov wrote:
On Mon, Sep 23, 2019 at 10:05 PM Alexander Korotkov
wrote:
I've reordered the patchset. I moved the most debatable patch, which
introduces and RR and changes parsing of YYY, YY and Y to the
end. I think we have enough of time in this release cycle to decide
whether we want this.
Patches 0001-0005 looks quite mature for me. I'm going to push this
if no objections. After pushing them, I'm going to start discussion
related to RR, YY and friends in separate thread.
Pushed. Remaining patch is attached. I'm going to start the separate
thread with its detailed explanation.
Attached patch with refactoring of compareDatetime() according
to the complaints of Tom Lane in [1]:
* extracted four subroutines for type conversions
* extracted subroutine for error reporting
* added default cases to all switches
* have_error flag is expected to be not-NULL always
* fixed errhint() message style
[1] https://www.postgresql.org/message-id/32308.1569455803%40sss.pgh.pa.us
--
Nikita Glukhov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company
>From 81d8de2f1d0e0d4ec44729d3d2976b1e63834b14 Mon Sep 17 00:00:00 2001
From: Nikita Glukhov
Date: Thu, 26 Sep 2019 17:52:40 +0300
Subject: [PATCH] Refactor jsonpath's compareDatime()
---
src/backend/utils/adt/jsonpath_exec.c| 181 ++-
src/test/regress/expected/jsonb_jsonpath.out | 30 ++---
2 files changed, 110 insertions(+), 101 deletions(-)
diff --git a/src/backend/utils/adt/jsonpath_exec.c b/src/backend/utils/adt/jsonpath_exec.c
index a35f718..7e540e3 100644
--- a/src/backend/utils/adt/jsonpath_exec.c
+++ b/src/backend/utils/adt/jsonpath_exec.c
@@ -2298,7 +2298,7 @@ compareItems(int32 op, JsonbValue *jb1, JsonbValue *jb2, bool useTz)
break;
case jbvDatetime:
{
-bool have_error = false;
+bool have_error;
cmp = compareDatetime(jb1->val.datetime.value,
jb1->val.datetime.typid,
@@ -2571,15 +2571,72 @@ wrapItemsInArray(const JsonValueList *items)
return pushJsonbValue(, WJB_END_ARRAY, NULL);
}
+/* Check if the timezone required for casting from type1 to type2 is used */
+static void
+checkTimezoneIsUsedForCast(bool useTz, const char *type1, const char *type2)
+{
+ if (!useTz)
+ ereport(ERROR,
+(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+ errmsg("cannot convert value from %s to %s without timezone usage",
+ type1, type2),
+ errhint("Use *_tz() function for timezone support.")));
+}
+
+/* Convert date datum to timestamp datum */
+static Datum
+castDateToTimestamp(Datum dt, bool *have_error)
+{
+ Timestamp ts = date2timestamp_opt_error(DatumGetDateADT(dt), have_error);
+
+ return TimestampGetDatum(ts);
+}
+
+/* Convert date datum to timestamptz datum */
+static Datum
+castDateToTimestampTz(Datum date, bool useTz, bool *have_error)
+{
+ TimestampTz tstz;
+
+ checkTimezoneIsUsedForCast(useTz, "date", "timestamptz");
+ tstz = date2timestamptz_opt_error(DatumGetDateADT(date), have_error);
+
+ return TimestampTzGetDatum(tstz);
+}
+
+/* Convert time datum to timetz datum */
+static Datum
+castTimeToTimeTz(Datum time, bool useTz)
+{
+ checkTimezoneIsUsedForCast(useTz, "time", "timetz");
+
+ return DirectFunctionCall1(time_timetz, time);
+}
+
+/* Convert timestamp datum to timestamptz datum */
+static Datum
+castTimestampToTimestampTz(Timestamp ts, bool useTz, bool *have_error)
+{
+ TimestampTz tstz;
+
+ checkTimezoneIsUsedForCast(useTz, "timestamp", "timestamptz");
+ tstz = timestamp2timestamptz_opt_error(DatumGetTimestamp(ts), have_error);
+
+ return TimestampTzGetDatum(tstz);
+}
+
/*
* Cross-type comparison of two datetime SQL/JSON items. If items are
- * uncomparable, 'error' flag is set.
+ * uncomparable or there is an error during casting, 'have_error' flag is set.
+ * If the cast requires timezone and it is not used, then hard error is thrown.
*/
static int
compareDatetime(Datum val1, Oid typid1, Datum val2, Oid typid2,
bool useTz, bool *have_error)
{
- PGFunction cmpfunc = NULL;
+ PGFunction cmpfunc;
+
+ *have_error = false;
switch (typid1)
{
@@ -2588,35 +2645,26 @@ compareDatetime(Datum val1, Oid typid1, Datum val2, Oid typid2,
{
case DATEOID:
cmpfunc = date_cmp;
-
break;
case TIMESTAMPOID:
- val1 = TimestampGetDatum(date2timestamp_opt_error(DatumGetDateADT(val1), have_error));
- if (have_error && *have_error)
- return 0;
+ val1 = castDateToTimestamp(val1, have_error);
cmpfunc = timestamp_cmp;
-
break;
case TIMESTAMPTZOID:
- if (!useTz)
- ereport(ERROR,
-(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
- errmsg("cannot convert value from %s to %s without timezone usage",
- "date", "timestamptz"),
- errhint("use *_tz() function for timezone support")));
- val1 = TimestampTzGetDatum(date2timestamptz_opt_error(DatumGetDateADT(val1), have_error));
- if (have_error && *have_error)
Re: Support for jsonpath .datetime() method
On Mon, Sep 23, 2019 at 10:05 PM Alexander Korotkov wrote: > I've reordered the patchset. I moved the most debatable patch, which > introduces and RR and changes parsing of YYY, YY and Y to the > end. I think we have enough of time in this release cycle to decide > whether we want this. > > Patches 0001-0005 looks quite mature for me. I'm going to push this > if no objections. After pushing them, I'm going to start discussion > related to RR, YY and friends in separate thread. Pushed. Remaining patch is attached. I'm going to start the separate thread with its detailed explanation. -- Alexander Korotkov Postgres Professional: http://www.postgrespro.com The Russian Postgres Company 0001-Introduce--and-RR-revise-YYY-YY-and-Y-datetime-9.patch Description: Binary data
Re: Support for jsonpath .datetime() method
On Tue, Aug 13, 2019 at 12:08 AM Alexander Korotkov
wrote:
> On Thu, Aug 1, 2019 at 1:31 PM Thomas Munro wrote:
> > On Sat, Jul 27, 2019 at 2:43 AM Andrew Dunstan
> > wrote:
> > > On 7/23/19 6:48 PM, Nikita Glukhov wrote:
> > > > Some concrete pieces of review:
> > > >> +
> > > >> +FF1
> > > >> +decisecond (0-9)
> > > >> +
> > > >>
> > > >> Let's not use such weird terms as "deciseconds". We could say
> > > >> "fractional seconds, 1 digit" etc. or something like that.
> > > > And what about "tenths of seconds", "hundredths of seconds"?
> > >
> > > Yes, those are much better.
> >
> > I've moved this to the September CF, still in "Waiting on Author" state.
>
> I'd like to summarize differences between standard datetime parsing
> and our to_timestamp()/to_date().
Let me describe my proposal to overcome these differences.
> 1) Standard defines much less datetime template parts. Namely it defines:
> | YYY | YY | Y
> | RR
> MM
> DD
> DDD
> HH | HH12
> HH24
> MI
> SS
> S
> FF1 | FF2 | FF3 | FF4 | FF5 | FF6 | FF7 | FF8 | FF9
> A.M. | P.M.
> TZH
> TZM
>
> We support majority of them and much more.
Regarding non-contradicting template parts we can support them in
.datetime() method too. That would be our extension to standard. See
no problem here.
> Incompatibilities are:
> * (our name is S),
Since is not reserved, I'd propose to make an alias for S.
> * We don't support | RR,
> * Our handling of | YYY | YY | Y is different. What we have
> here is more like | RR in standard (Nikita explained that
> upthread [1]),
I'd like to make | YYY | YY | Y and | RR behavior standard
conforming in both to_timestamp()/to_date() and .datetime(). Handling
these template parts differently in different functions would be
confusing for users.
> * We don't support FF[1-9]. FF[1-6] are implemented in patch. We
> can't support FF[7-9], because our binary representation of timestamp
> datatype don't have enough of precision.
I propose to postpone implementation of FF[7-9]. We can support them
later once we have precise enough datatypes.
> 2) Standard defines only following delimiters: , ,
> , , , , , . And
> it requires strict matching of separators between template and input
> strings. We don't do so either in FX or non-FX mode.
>
> For instance, we allow both to_date('2019/12/31', '-MM-DD') and
> to_date('2019/12/31', 'FX-MM-DD'). But according to standard this
> date should be written only as '2019-12-31' to match given template
> string.
>
> 4) For non-delimited template parts standard requires matching to
> digit sequences of lengths between 1 and maximum number of characters
> of that template part. We don't always do so. For instance, we allow
> more than 4 digits to correspond to , more than 3 digits to
> correspond to YYY and so on.
>
> # select to_date('2019-12-31', 'YYY-MM-DD');
> to_date
>
> 2019-12-31
> (1 row)
In order to implement these I'd like to propose introduction of
special do_to_timestamp() flag, which would define standard conforming
parsing. This flag would be used in .datetime() jsonpath method.
Later we also should use it for CAST(... FORMAT ...) expression, which
should also do standard conforming parsing
> 3) Standard prescribes recognition of digits according to \p{Nd}
> regex. \p{Nd} matches to "a digit zero through nine in any script
> except ideographic scripts". As far as I remember, we currently do
> recognize only ASCII digits.
Support all unicode digit scripts would be cool for both
to_timestamp()/to_date() and standard parsing. However, I think this
could be postponed. Personally I didn't meet non-ascii digits in
databases yet. If needed one can implement this later, shouldn't be
hard.
If no objections, Nikita and me will work on revised patchset based on
this proposal.
--
Alexander Korotkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company
Re: Support for jsonpath .datetime() method
On Thu, Aug 1, 2019 at 1:31 PM Thomas Munro wrote:
> On Sat, Jul 27, 2019 at 2:43 AM Andrew Dunstan
> wrote:
> > On 7/23/19 6:48 PM, Nikita Glukhov wrote:
> > > Some concrete pieces of review:
> > >> +
> > >> +FF1
> > >> +decisecond (0-9)
> > >> +
> > >>
> > >> Let's not use such weird terms as "deciseconds". We could say
> > >> "fractional seconds, 1 digit" etc. or something like that.
> > > And what about "tenths of seconds", "hundredths of seconds"?
> >
> > Yes, those are much better.
>
> I've moved this to the September CF, still in "Waiting on Author" state.
I'd like to summarize differences between standard datetime parsing
and our to_timestamp()/to_date().
1) Standard defines much less datetime template parts. Namely it defines:
| YYY | YY | Y
| RR
MM
DD
DDD
HH | HH12
HH24
MI
SS
S
FF1 | FF2 | FF3 | FF4 | FF5 | FF6 | FF7 | FF8 | FF9
A.M. | P.M.
TZH
TZM
We support majority of them and much more. Incompatibilities are:
* (our name is S),
* We don't support | RR,
* Our handling of | YYY | YY | Y is different. What we have
here is more like | RR in standard (Nikita explained that
upthread [1]),
* We don't support FF[1-9]. FF[1-6] are implemented in patch. We
can't support FF[7-9], because our binary representation of timestamp
datatype don't have enough of precision.
2) Standard defines only following delimiters: , ,
, , , , , . And
it requires strict matching of separators between template and input
strings. We don't do so either in FX or non-FX mode.
For instance, we allow both to_date('2019/12/31', '-MM-DD') and
to_date('2019/12/31', 'FX-MM-DD'). But according to standard this
date should be written only as '2019-12-31' to match given template
string.
3) Standard prescribes recognition of digits according to \p{Nd}
regex. \p{Nd} matches to "a digit zero through nine in any script
except ideographic scripts". As far as I remember, we currently do
recognize only ASCII digits.
4) For non-delimited template parts standard requires matching to
digit sequences of lengths between 1 and maximum number of characters
of that template part. We don't always do so. For instance, we allow
more than 4 digits to correspond to , more than 3 digits to
correspond to YYY and so on.
# select to_date('2019-12-31', 'YYY-MM-DD');
to_date
2019-12-31
(1 row)
Links.
1.
https://www.postgresql.org/message-id/d6efab15-f3a4-40d6-8ddb-6fd8f64cbc08%40postgrespro.ru
--
Alexander Korotkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company
Re: Support for jsonpath .datetime() method
On Sat, Jul 27, 2019 at 2:43 AM Andrew Dunstan wrote: > On 7/23/19 6:48 PM, Nikita Glukhov wrote: > > Some concrete pieces of review: > >> + > >> +FF1 > >> +decisecond (0-9) > >> + > >> > >> Let's not use such weird terms as "deciseconds". We could say > >> "fractional seconds, 1 digit" etc. or something like that. > > And what about "tenths of seconds", "hundredths of seconds"? > > Yes, those are much better. I've moved this to the September CF, still in "Waiting on Author" state. -- Thomas Munro https://enterprisedb.com
Re: Support for jsonpath .datetime() method
On 7/23/19 6:48 PM, Nikita Glukhov wrote: > Some concrete pieces of review: >> + >> +FF1 >> +decisecond (0-9) >> + >> >> Let's not use such weird terms as "deciseconds". We could say >> "fractional seconds, 1 digit" etc. or something like that. > And what about "tenths of seconds", "hundredths of seconds"? Yes, those are much better. cheers andrew -- Andrew Dunstanhttps://www.2ndQuadrant.com PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: Support for jsonpath .datetime() method
On 7/24/19 4:25 PM, Peter Eisentraut wrote:
> On 2019-07-24 00:48, Nikita Glukhov wrote:
>> It seems that our YY works like RR should:
>>
>> SELECT to_date('69', 'YY');
>> to_date
>>
>> 2069-01-01
>> (1 row)
>>
>> SELECT to_date('70', 'YY');
>> to_date
>>
>> 1970-01-01
>> (1 row)
>>
>> But by the standard first two digits of current year should be used in YY.
> Is this behavior even documented anywhere in our documentation? I
> couldn't find it. What's the exact specification of what it does in
> these cases?
>
>> So it's unclear what we should do:
>> - implement YY and RR strictly following the standard only in .datetime()
>> - fix YY implementation in to_date()/to_timestamp() and implement RR
>> - use our non-standard templates in .datetime()
> I think we definitely should try to use the same template system in both
> the general functions and in .datetime().
Agreed. It's too hard to maintain otherwise.
> This might involve some
> compromises between existing behavior, Oracle behavior, SQL standard.
> So far I'm not worried: If you're using two-digit years like above,
> you're playing with fire anyway. Also some of the other cases like
> dealing with trailing spaces are probably acceptable as slight
> incompatibilities or extensions.
My instict wouyld be to move as close as possible to the standard,
especially if the current behaviour isn't documented.
>
> We should collect a list of test cases that illustrate the differences
> and then work out how to deal with them.
>
Agreed.
cheers
andrew
--
Andrew Dunstanhttps://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: Support for jsonpath .datetime() method
On 2019-07-24 00:48, Nikita Glukhov wrote:
> It seems that our YY works like RR should:
>
> SELECT to_date('69', 'YY');
> to_date
>
> 2069-01-01
> (1 row)
>
> SELECT to_date('70', 'YY');
> to_date
>
> 1970-01-01
> (1 row)
>
> But by the standard first two digits of current year should be used in YY.
Is this behavior even documented anywhere in our documentation? I
couldn't find it. What's the exact specification of what it does in
these cases?
> So it's unclear what we should do:
> - implement YY and RR strictly following the standard only in .datetime()
> - fix YY implementation in to_date()/to_timestamp() and implement RR
> - use our non-standard templates in .datetime()
I think we definitely should try to use the same template system in both
the general functions and in .datetime(). This might involve some
compromises between existing behavior, Oracle behavior, SQL standard.
So far I'm not worried: If you're using two-digit years like above,
you're playing with fire anyway. Also some of the other cases like
dealing with trailing spaces are probably acceptable as slight
incompatibilities or extensions.
We should collect a list of test cases that illustrate the differences
and then work out how to deal with them.
--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: Support for jsonpath .datetime() method
On Wed, Jul 24, 2019 at 1:50 AM Nikita Glukhov wrote: > So it's unclear what we should do: > - implement YY and RR strictly following the standard only in .datetime() > - fix YY implementation in to_date()/to_timestamp() and implement RR > - use our non-standard templates in .datetime() Also it appears that according to standard .datetime() should treat spaces and delimiters differently than our to_date()/to_timestamp(). It requires strict matching of spaces and delimiters in input and format strings. We don't have such behavior in both non-FX and FX modes. Also, standard doesn't define FX mode at all. This rules cover jsonpath .datetime() method and CAST(... FORMAT ...) – new cast clause defined by standard. So, I think due to reasons of compatibility it doesn't worth trying to make behavior of our to_date()/to_timestamp() to fit requirements for jsonpath .datetime() and CAST(... FORMAT ...). I propose to leave this functions as is (maybe add new patterns), but introduce another datetime parsing mode, which would fit to the standard. Opinions? -- Alexander Korotkov Postgres Professional: http://www.postgrespro.com The Russian Postgres Company
Re: Support for jsonpath .datetime() method
On 23.07.2019 16:44, Peter Eisentraut wrote:
I think the best way forward here is to focus first on patch 0002 and
get the additional format templates in, independent of any surrounding
JSON functionality.
In particular, remove parse_datetime() and all the related API changes,
then it becomes much simpler.
The codes FF1..FF6 that you added appear to be correct, but reading the
spec I find there is more missing, specifically
- and RR
It seems that our YY works like RR should:
SELECT to_date('69', 'YY');
to_date
2069-01-01
(1 row)
SELECT to_date('70', 'YY');
to_date
1970-01-01
(1 row)
But by the standard first two digits of current year should be used in YY.
Oracle follows the standard but its implementation has the different
rounding algorithm:
SELECT TO_CHAR(TO_DATE('99', 'YY'), '') from dual;
2099
SELECT TO_CHAR(TO_DATE('49', 'RR'), '') from dual;
2049
SELECT TO_CHAR(TO_DATE('50', 'RR'), '') from dual;
1950
So it's unclear what we should do:
- implement YY and RR strictly following the standard only in .datetime()
- fix YY implementation in to_date()/to_timestamp() and implement RR
- use our non-standard templates in .datetime()
- S (currently only is supported, but that's not standard)
S template can be easily added as alias to .
Also in some cases we allow timestamps with seven digits of fractional
precision, so perhaps FF7 should be supported as well. I'm not quite
sure about the details here. You tests only cover 6 and 9 digits. It
would be good to cover 7 and perhaps 8 as well, since those are the
boundary cases.
FF7-FF9 weer present in earlier versions of the jsonpath patches, but they
had been removed (see [1]) because they were not completely supported due
to the limited precision of timestamp.
Some concrete pieces of review:
+
+FF1
+decisecond (0-9)
+
Let's not use such weird terms as "deciseconds". We could say
"fractional seconds, 1 digit" etc. or something like that.
And what about "tenths of seconds", "hundredths of seconds"?
+/* Return flags for DCH_from_char() */
+#define DCH_DATED 0x01
+#define DCH_TIMED 0x02
+#define DCH_ZONED 0x04
I think you mean do_to_timestamp() here. These terms "dated" etc. are
from the SQL standard text, but they should be explained somewhere for
the readers of the code.
[1]
https://www.postgresql.org/message-id/885de241-5a51-29c8-a6b3-f1dda22aba13%40postgrespro.ru
--
Nikita Glukhov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company
Re: Support for jsonpath .datetime() method
I think the best way forward here is to focus first on patch 0002 and get the additional format templates in, independent of any surrounding JSON functionality. In particular, remove parse_datetime() and all the related API changes, then it becomes much simpler. The codes FF1..FF6 that you added appear to be correct, but reading the spec I find there is more missing, specifically - and RR - S (currently only is supported, but that's not standard) Also in some cases we allow timestamps with seven digits of fractional precision, so perhaps FF7 should be supported as well. I'm not quite sure about the details here. You tests only cover 6 and 9 digits. It would be good to cover 7 and perhaps 8 as well, since those are the boundary cases. Some concrete pieces of review: + +FF1 +decisecond (0-9) + Let's not use such weird terms as "deciseconds". We could say "fractional seconds, 1 digit" etc. or something like that. +/* Return flags for DCH_from_char() */ +#define DCH_DATED 0x01 +#define DCH_TIMED 0x02 +#define DCH_ZONED 0x04 I think you mean do_to_timestamp() here. These terms "dated" etc. are from the SQL standard text, but they should be explained somewhere for the readers of the code. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: Support for jsonpath .datetime() method
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[] =
+{
+"-mm-dd HH24:MI:SS TZH:TZM",
+"-mm-dd HH24:MI:SS TZH",
+"-mm-dd HH24:MI:SS",
+"-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
