Re: Invalid to_date patterns (was: [PATCHES] [GENERAL] ISO week dates)

On 2/17/07, Alvaro Herrera <alvherre(at)commandprompt(dot)com> wrote:
> Bruce Momjian escribió:
>
> > Maybe now would be an appropriate time to discuss the open questions in
> > the submitting email:
>
> > > Brendan Jurd wrote:
> > > > I'd also like to raise the topic of how conversion from text to ISO
> > > > week dates should be handled, where the user has specified a bogus
> > > > mixture of fields. Existing code basically ignores these issues; for
> > > > example, if a user were to call to_date('1998-01-01 2454050',
> > > > 'YYYY-MM-DD J') the function returns 2006-01-01, a result of setting
> > > > the year field from YYYY, then overwriting year, month and day with
> > > > the values from the Julian date in J, then setting the month and day
> > > > normally from MM and DD.
> > > >
> > > > 2006-01-01 is not a valid representation of either of the values the
> > > > user specified. Now you might say "ask a silly question, get a silly
> > > > answer"; the user shouldn't send nonsense arguments to to_date and
> > > > expect a sensible result. But perhaps the right way to respond to a
> > > > broken timestamp definition is to throw an error, rather than behave
> > > > as though everything has gone to plan, and return something which is
> > > > not correct.
> > > >
> > > > The same situation can arise if the user mixes ISO and Gregorian data;
> > > > how should Postgres deal with something like to_date('2006-250',
> > > > 'IYYY-DDD')? The current behaviour in my patch is actually to assume
> > > > that the user meant to say 'IYYY-IDDD', since "the 250th Gregorian day
> > > > of the ISO year 2006" is total gibberish. But perhaps it should be
> > > > throwing an error message.
>
> My thinking is that erroneous patterns should throw an error, and not
> try to second-guess the user. (IIRC this was being discussed in some
> other thread not long ago).

It seems to me there are basically two different responses to the
problem of invalid patterns. One is to reject all patterns which
potentially under- or over-constrain the date value, and the other is
to only reject those patterns which, when applied to the given date
string, actually cause a conflict.

For example, on the surface the pattern 'YYYY-MM-DD J' would appear to
be invalid, because it specifies the date using both the Gregorian and
Julian conventions. You could argue that the whole idea of using a
pattern like this is bogus, and reject the pattern as soon as it is
parsed.

On the other hand, if a user called to_date('2007-02-17 2454149',
'YYYY-MM-DD J'), and you attempted to resolve the pattern you would
find that the Julian date and the Gregorian date agree perfectly with
each other, and there is no reason to reject the conversion.

My gut reaction at first was to go with the former approach. It's
programmatically more simple, and it's easier to explain in
documentation/error messages. But then it occurred to me that one of
the use cases for to_date is slurping date information out of textual
reports which may contain redundant date information. If a user
wanted to parse something like "2007-02-17 Q1", he would probably try
'YYYY-MM-DD "Q"Q', even though this pattern is logically
over-constraining. Would it be fair to throw an error in such a case?

Please let me know what you think.

BJ

On Sat, Feb 17, 2007 at 02:41:32PM +1100, Brendan Jurd wrote:
> My gut reaction at first was to go with the former approach. It's
> programmatically more simple, and it's easier to explain in
> documentation/error messages. But then it occurred to me that one of
> the use cases for to_date is slurping date information out of textual
> reports which may contain redundant date information. If a user
> wanted to parse something like "2007-02-17 Q1", he would probably try
> 'YYYY-MM-DD "Q"Q', even though this pattern is logically
> over-constraining. Would it be fair to throw an error in such a case?

If that's the use case, it would seem to me reasonable to be able to
mark fields for parsing but to not use them in the final calculation,
like the * modifier for scanf in C.

Other than that I'd follow whatever Oracle does, that seem to be the
trend with those functions.

Have a nice day,
--
Martijn van Oosterhout <kleptog(at)svana(dot)org> http://svana.org/kleptog/
> From each according to his ability. To each according to his ability to litigate.

On 2/17/07, Martijn van Oosterhout <kleptog(at)svana(dot)org> wrote:
> On Sat, Feb 17, 2007 at 02:41:32PM +1100, Brendan Jurd wrote:
> > My gut reaction at first was to go with the former approach. It's
> > programmatically more simple, and it's easier to explain in
> > documentation/error messages. But then it occurred to me that one of
> > the use cases for to_date is slurping date information out of textual
> > reports which may contain redundant date information. If a user
> > wanted to parse something like "2007-02-17 Q1", he would probably try
> > 'YYYY-MM-DD "Q"Q', even though this pattern is logically
> > over-constraining. Would it be fair to throw an error in such a case?
>
> If that's the use case, it would seem to me reasonable to be able to
> mark fields for parsing but to not use them in the final calculation,
> like the * modifier for scanf in C.
>
> Other than that I'd follow whatever Oracle does, that seem to be the
> trend with those functions.

I just looked through the Oracle documentation, and it is
conspicuously silent on the topic of invalid format patterns. Much
like ours in fact.

I like your suggestion of the pattern modifier. So if a user did try
to format with 'YYYY-MM-DD "Q"Q', we would throw an error telling them
that the pattern is over-constraining, and they can use this pattern
modifier (* or whatever) to single out the non-normative fields.

Anybody else want to weigh in on this?

On 2/17/07, Brendan Jurd <direvus(at)gmail(dot)com> wrote:
>
> I just looked through the Oracle documentation, and it is
> conspicuously silent on the topic of invalid format patterns. Much
> like ours in fact.

On the case of the format: YYYY-MM-DD J, if J is the same date as YYYY-MM-DD
then Oracle appears to silently ignore it. But if J is not the same date as
YYYY-MM-DD then Oracle throws an error:

SQL> select to_date('2007-02-17 2454149', 'YYYY-MM-DD J') from dual;

TO_DATE('
---------
17-FEB-07

SQL> select to_date('2007-02-17 2454145', 'YYYY-MM-DD J') from dual;
select to_date('2007-02-17 2454145', 'YYYY-MM-DD J') from dual
*
ERROR at line 1:
ORA-01834: day of month conflicts with Julian date

I like your suggestion of the pattern modifier. So if a user did try
> to format with 'YYYY-MM-DD "Q"Q', we would throw an error telling them
> that the pattern is over-constraining, and they can use this pattern
> modifier (* or whatever) to single out the non-normative fields.

As for 'Q', Oracle doesn't appear to support this in terms of TO_DATE, I
believe it is for TO_CHAR only.

SQL> select to_date('2007-02-17 1', 'YYYY-MM-DD Q') from dual;
select to_date('2007-02-17 1', 'YYYY-MM-DD Q') from dual
*
ERROR at line 1:
ORA-01820: format code cannot appear in date input format

SQL> select to_date('1', 'Q') from dual;
select to_date('1', 'Q') from dual
*
ERROR at line 1:
ORA-01820: format code cannot appear in date input format

--
Chad
http://www.postgresqlforums.com/

Because this patch was not completed, I have added it to the TODO list:

* Fix to_date()-related functions to consistently issue errors

http://archives.postgresql.org/pgsql-hackers/2007-02/msg00915.php

---------------------------------------------------------------------------

Brendan Jurd wrote:
> On 2/17/07, Alvaro Herrera <alvherre(at)commandprompt(dot)com> wrote:
> > Bruce Momjian escribi?:
> >
> > > Maybe now would be an appropriate time to discuss the open questions in
> > > the submitting email:
> >
> > > > Brendan Jurd wrote:
> > > > > I'd also like to raise the topic of how conversion from text to ISO
> > > > > week dates should be handled, where the user has specified a bogus
> > > > > mixture of fields. Existing code basically ignores these issues; for
> > > > > example, if a user were to call to_date('1998-01-01 2454050',
> > > > > 'YYYY-MM-DD J') the function returns 2006-01-01, a result of setting
> > > > > the year field from YYYY, then overwriting year, month and day with
> > > > > the values from the Julian date in J, then setting the month and day
> > > > > normally from MM and DD.
> > > > >
> > > > > 2006-01-01 is not a valid representation of either of the values the
> > > > > user specified. Now you might say "ask a silly question, get a silly
> > > > > answer"; the user shouldn't send nonsense arguments to to_date and
> > > > > expect a sensible result. But perhaps the right way to respond to a
> > > > > broken timestamp definition is to throw an error, rather than behave
> > > > > as though everything has gone to plan, and return something which is
> > > > > not correct.
> > > > >
> > > > > The same situation can arise if the user mixes ISO and Gregorian data;
> > > > > how should Postgres deal with something like to_date('2006-250',
> > > > > 'IYYY-DDD')? The current behaviour in my patch is actually to assume
> > > > > that the user meant to say 'IYYY-IDDD', since "the 250th Gregorian day
> > > > > of the ISO year 2006" is total gibberish. But perhaps it should be
> > > > > throwing an error message.
> >
> > My thinking is that erroneous patterns should throw an error, and not
> > try to second-guess the user. (IIRC this was being discussed in some
> > other thread not long ago).
>
> It seems to me there are basically two different responses to the
> problem of invalid patterns. One is to reject all patterns which
> potentially under- or over-constrain the date value, and the other is
> to only reject those patterns which, when applied to the given date
> string, actually cause a conflict.
>
> For example, on the surface the pattern 'YYYY-MM-DD J' would appear to
> be invalid, because it specifies the date using both the Gregorian and
> Julian conventions. You could argue that the whole idea of using a
> pattern like this is bogus, and reject the pattern as soon as it is
> parsed.
>
> On the other hand, if a user called to_date('2007-02-17 2454149',
> 'YYYY-MM-DD J'), and you attempted to resolve the pattern you would
> find that the Julian date and the Gregorian date agree perfectly with
> each other, and there is no reason to reject the conversion.
>
> My gut reaction at first was to go with the former approach. It's
> programmatically more simple, and it's easier to explain in
> documentation/error messages. But then it occurred to me that one of
> the use cases for to_date is slurping date information out of textual
> reports which may contain redundant date information. If a user
> wanted to parse something like "2007-02-17 Q1", he would probably try
> 'YYYY-MM-DD "Q"Q', even though this pattern is logically
> over-constraining. Would it be fair to throw an error in such a case?
>
> Please let me know what you think.
>
> BJ

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://www.enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

On 4/3/07, Bruce Momjian <bruce(at)momjian(dot)us> wrote:
>
> Because this patch was not completed, I have added it to the TODO list:
>
> * Fix to_date()-related functions to consistently issue errors
>
> http://archives.postgresql.org/pgsql-hackers/2007-02/msg00915.php

I'm now taking another run at this issue. Here's what I've got in mind.

There are three distinct conventions for specifying a date that we
consider in Postgres. These are
* Julian day,
* ISO week date, and
* Standard Gregorian.

Within an ISO week date, you can identify a date using either
* year, week and day-of-week, or
* year and day-of-year.

Likewise within a Gregorian date, you can identify a date using
* year, month and day-of-month,
* year, month, week-of-month and day-of-week (extremely weird, but there it is)
* year, week, and day-of-week, or
* year and day-of-year.

Chad Wagner mentioned that Oracle will allow a combination of Julian
and Gregorian formats so long as both formats yield the same date. If
we're going to stick with the theme of imitating Oracle, I propose the
following:

* No mixing of Gregorian and ISO fields permitted. If the format
string contains both Gregorian and ISO normative fields in any
sequence or combination, we throw an ERRCODE_INVALID_DATETIME_FORMAT
and reject the query.
* Either Gregorian or ISO format strings may include a Julian date
field, as long as the results are in agreement. If the results
disagree, we reject the query.
* Purely non-normative fields (like "Q") are completely and silently
disregarded.
* A Gregorian or ISO format may be over-constraining as long as all
values are in agreement. If there are any conflicts we reject the
query.

So, for example, we would reject something like "YYYY-IDDD" out of
hand because it combines the ISO and Gregorian conventions, making it
impossible to ascertain what the user really wants to do.

We would allow YYYY-MM-DD J as long as the result for the YYYY-MM-DD
part matches the result for the J part.

We would also allow something like YYYY-MM-DD D as long as the results
of YYYY-MM-DD and D matched. So to_date('2007-07-18 4', 'YYYY-MM-DD
D') would successfully return the date 18 July 2007, but if you tried
to_date('2007-07-18 5', 'YYYY-MM-DD D') you would get an error.

If there are no objections I'd be happy to cook a patch up.

"Brendan Jurd" <direvus(at)gmail(dot)com> writes:
>> * Fix to_date()-related functions to consistently issue errors
>> http://archives.postgresql.org/pgsql-hackers/2007-02/msg00915.php

> I'm now taking another run at this issue. Here's what I've got in mind.

This is all good but I think that self-inconsistent format strings are
not really the main source of to_date problems. Most of the complaints
I've seen arise from to_date plowing ahead to deliver a ridiculous
answer when the input data string doesn't match the format. I'd like to
see the code try a little harder to validate the input data.

regards, tom lane

On 7/18/07, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> This is all good but I think that self-inconsistent format strings are
> not really the main source of to_date problems. Most of the complaints
> I've seen arise from to_date plowing ahead to deliver a ridiculous
> answer when the input data string doesn't match the format. I'd like to
> see the code try a little harder to validate the input data.
>

Agreed, but so far it doesn't look like there's much overlap between
the parsing and validating code and the stuff I'm playing with.

I'll keep an eye out for any opportunities to improve on this, but it
might end up being a separate TODO.

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

I'm picking this one up again, since my refactor of formatting.c has
been applied, and I seem to be done playing with psql for the time
being =)

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.7 (GNU/Linux)
Comment: http://getfiregpg.org

iD8DBQFIFOAe5YBsbHkuyV0RAjHtAJ41opoNgu8M4jYTz9wsR2YGQNnDJQCgqNM0
RKNzCRnHUFwyNjSB3O3k0c8=
=andX
-----END PGP SIGNATURE-----

On Wed, Jul 18, 2007 at 10:00 AM, Brendan Jurd <direvus(at)gmail(dot)com> wrote:
> On 7/18/07, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> > This is all good but I think that self-inconsistent format strings are
> > not really the main source of to_date problems. Most of the complaints
> > I've seen arise from to_date plowing ahead to deliver a ridiculous
> > answer when the input data string doesn't match the format. I'd like to
> > see the code try a little harder to validate the input data.
>
> Agreed, but so far it doesn't look like there's much overlap between
> the parsing and validating code and the stuff I'm playing with.
>
> I'll keep an eye out for any opportunities to improve on this, but it
> might end up being a separate TODO.
>

Actually, it turns out that there was an opportunity to improve on
validation. At the moment I'm experimenting with using strtol to
harvest integer values from the input string, instead of sscanf. So
far the results are promising. I've got the code throwing errors for
a few different kinds of bogus input:

* String is too short
* Value is out of range
* Value isn't an integer at all

I'm also getting rid of some more code duplication in do_to_timestamp.
I hope to have something worth posting to -patches in the near
future.

Cheers,
BJ