Interval code refactoring patch (Was: Re: [HACKERS] Patch for ISO-8601-Interval Input and output.)
Tom Lane wrote: ...failure case ... interval 'P-1Y-2M3DT-4H-5M-6'; This isn't the result I'd expect, and AFAICS the ISO spec does *not* allow any unit markers to be omitted in the format with designators. Yes, this is true. I see you already made the change. Tom Lane wrote: Applied with nontrivial revisions --- I fear I probably broke your third patch again :-(. No problem. It wasn't hard to update. Attached is an updated patch (as well as being updated on my website; but since it applies to HEAD it's as easy to get here). The bulk of the changes are in regression tests where rounding of fractional seconds was changed as discussed up-thread back in Sep. Seems I should also submit one more patch that merge the newest DecodeInterval, EncodeInterval and related functions into /ecpg/pgtypeslib/interval.c? And beyond that, there's still some eccentricities with the interval code (why's interval '1 year 1 year' ok but interval '1 second 1 second' not) but I don't know if I'd do more harm or good trying to look at those. cleanup.patch.gz Description: GNU Zip compressed data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Patch for ISO-8601-Interval Input and output.
Brendan Jurd wrote: On Sat, Nov 8, 2008 at 2:19 AM, Ron Mayer [EMAIL PROTECTED] wrote: Hmmm... Certainly what I had in datatype.sgml was wrong, but I'm now thinking 5.5.4.2.1 and 5.5.4.2.2 would be the most clear? Sorry, I don't understand what you mean by 5.5.4.2.1. In the spec Ah! That 5.5.4.2.1 comes from apparently an old Oct 2000 draft version of the spec titled ISO/FDIS 8601. (For now you can see it here: http://0ape.com/postgres_interval_patches/ISO-FDIS-8601.pdf ) I'll fix all the links to point to the 2004 spec. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Patch for ISO-8601-Interval Input and output.
On Tue, Nov 11, 2008 at 2:36 AM, Ron Mayer [EMAIL PROTECTED] wrote: I updated my web site[1] with the latest version of this patch. I'm just testing this latest version out now. I get the expected result from 'P0001', but oddly enough if I specify only the year and month, it pukes: postgres=# select interval 'P0001-01'; ERROR: invalid input syntax for type interval: P0001-01 LINE 1: select interval 'P0001-01'; I'm attaching a patch to clean up a few more code style issues and fix broken spec references within C code comments in datetime.c. Cheers, BJ iso8601_interval-3.diff Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Patch for ISO-8601-Interval Input and output.
Ron Mayer wrote: Ah! That 5.5.4.2.1 comes from apparently an old Oct 2000 draft version of the spec titled ISO/FDIS 8601. (For now you can see it here: http://0ape.com/postgres_interval_patches/ISO-FDIS-8601.pdf ) I'll fix all the links to point to the 2004 spec. I updated my web site[1] with the latest version of this patch. Main differences since last time * Merged with the IntervalStyle patch as it was checked into CVS. * Fixed references to consistently refer to the same version of the ISO 8601 spec (ISO 8601:2004(E)) [1] http://0ape.com/postgres_interval_patches/ PS: I realize that this patch makes datetime.c a bit longer that it needs to be; and that some of the functions added in this patch can be used by the other interval styles as well. patch 3 that can be found on the same HTML page does this refactoring. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Patch for ISO-8601-Interval Input and output.
On Tue, Nov 11, 2008 at 5:51 AM, R Mayer [EMAIL PROTECTED] wrote: Applied and pushed to the website http://0ape.com/postgres_interval_patches/ This latest version works as expected and I don't detect any other issues with the code or documentation ... seems I've run out of things to gripe about! I'm ready to sign off on this patch now and move on to the final cleanup patch. I'll update the commitfest to show this one as ready for committer. Cheers, BJ -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Patch for ISO-8601-Interval Input and output.
Brendan Jurd [EMAIL PROTECTED] writes: I'm ready to sign off on this patch now and move on to the final cleanup patch. I'll update the commitfest to show this one as ready for committer. OK, I'll pick this one up now. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Patch for ISO-8601-Interval Input and output.
Brendan Jurd wrote: On Tue, Nov 11, 2008 at 2:36 AM, Ron Mayer I get the expected result from 'P0001', but oddly enough if I specify only the year and month, it pukes: postgres=# select interval 'P0001-01'; Indeed. Thanks again. I've fixed this and added regression tests to check the handling of optional fields of the alternative format which my patch has been so very bad at handling. I'm attaching a patch to clean up a few more code style issues and fix broken spec references within C code comments in datetime.c. Applied and pushed to the website http://0ape.com/postgres_interval_patches/ -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Patch for ISO-8601-Interval Input and output.
R Mayer [EMAIL PROTECTED] writes: Applied and pushed to the website http://0ape.com/postgres_interval_patches/ I ran into an interesting failure case: regression=# select interval 'P-1Y-2M3DT-4H-5M-6'; interval --- P-1Y-2M3DT-10H-5M (1 row) This isn't the result I'd expect, and AFAICS the ISO spec does *not* allow any unit markers to be omitted in the format with designators. I think the problem is that the code will accept a value as being alternative format even when it's already read some format-with-designator fields. I propose adding a flag to remember that we've seen a field in the current part (date or time) and rejecting an apparent alternative-format input if the flag is set. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Patch for ISO-8601-Interval Input and output.
R Mayer [EMAIL PROTECTED] writes: Applied and pushed to the website http://0ape.com/postgres_interval_patches/ Applied with nontrivial revisions --- I fear I probably broke your third patch again :-(. There were still a number of parsing bugs, and I also didn't like the lack of error checking around the strtod() call ... regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Patch for ISO-8601-Interval Input and output.
On Sat, Nov 8, 2008 at 2:19 AM, Ron Mayer [EMAIL PROTECTED] wrote: Hmmm... Certainly what I had in datatype.sgml was wrong, but I'm now thinking 5.5.4.2.1 and 5.5.4.2.2 would be the most clear? Sorry, I don't understand what you mean by 5.5.4.2.1. In the spec you linked to, clause 5 Date and time format representations doesn't have any numbered subsections at all. It's just a half-page saying, basically, that if applications want to interchange information about datetime formats, they can. Much like the ents, spec authors don't like to say anything unless it's worth taking a very long time to say. So, to the best of my knowledge, there is no 5.5.4.2.1. There is no 5.5. Originally I assumed that when you wrote 5.5.4.2.1, you meant 4.4.4.2.1. However, when I looked closer I noticed that this section is about a textual representation of the format, not about the format itself. Therefore I suggested 4.4.3.2, which does specify the format. Cheers, BJ -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Patch for ISO-8601-Interval Input and output.
On Fri, Nov 7, 2008 at 3:35 AM, Ron Mayer [EMAIL PROTECTED] wrote: I think I updated the web site and git now, and 'P-00-01' is now accepted. It might be useful if someone double checked my reading of the spec, tho. Hi Ron, I've tested out your latest revision and read the spec more closely, and pretty much everything works as expected. 'P-00-01' yields 1 day, and various other combinations like 'P-01-00' and 'P-01' work correctly too. I agree with your interpretation of the spec, it clearly says that 'T' can be omitted when there are no time components. This should apply equally to both the designators format and the alternative format. The examples in Annex B confirm this. I did run into one potential bug: postgres=# select interval 'P0001'; interval -- 1 day Whereas, I expected to get '1 year', since the format allows you to omit lower-order components from right-to-left: P0001-01-01 = 1 year 1 month 1 day P0001-01 = 1 year 1 month P0001 = should be 1 year? On the documentation front, I have a few final cleanups to suggest (patch attached). * After giving the spec a closer look, I thought that 4.4.3.2 and 4.4.3.3 were the proper spec references to use for the two formats. * I removed the identation from a programlisting element. These elements are left at the start of the line to prevent the initial spacing from being interpreted as part of the listing itself. * I made Interval Output a level 3 section again, and this time I corrected that structural issue in my earlier patch. It's now contained within the level 2 section. * There was a sentence in the docs about the 'T' separator being mandatory in the alternative format. I deleted it. * Changed format with time-unit designators to just format with designators, as that is how the spec refers to it. * Some tabs had crept into the indentation, and there were a few indentation errors in the new tables. I corrected those (the tabs may have been my fault; I sometimes forget to change my vi settings when switching between C code and SGML). Cheers, BJ iso8601_interval-2.diff.bz2 Description: BZip2 compressed data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Patch for ISO-8601-Interval Input and output.
Brendan Jurd wrote: On Fri, Nov 7, 2008 at 3:35 AM, Ron Mayer [EMAIL PROTECTED] wrote: I think I updated the web site and git now, and 'P-00-01' is now accepted. It might be useful if someone double checked my reading of the spec, tho. I've tested out your latest revision and read the spec more closely, and pretty much everything works as expected. ... I agree with your interpretation of the spec, it clearly says that 'T' can be omitted when there are no time components. ... The examples in Annex B confirm this. Cool. Thanks. I did run into one potential bug: postgres=# select interval 'P0001'; ... Whereas, I expected to get '1 year', since the format allows you to omit lower-order components from right-to-left: P0001-01-01 = 1 year 1 month 1 day P0001-01 = 1 year 1 month P0001 = should be 1 year? Indeed, that's right. Thanks for catching another one. I just checked in (to my git) a patch that I believe fixes it. regression=# select interval 'P0001',interval 'P0001',interval 'PT01'; interval | interval | interval --+--+-- 1 year | 1 year | 01:00:00 (1 row) On the documentation front, I have a few final cleanups to suggest (patch attached). * After giving the spec a closer look, I thought that 4.4.3.2 and 4.4.3.3 were the proper spec references to use for the two formats. Hmmm... Certainly what I had in datatype.sgml was wrong, but I'm now thinking 5.5.4.2.1 and 5.5.4.2.2 would be the most clear? Totally agree with the rest of your docs changes and applied those. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Patch for ISO-8601-Interval Input and output.
Brendan Jurd wrote: I've applied them with a couple minor changes. * If ISO 8601 5.5.3.1.d's statement The designator T shall be absent if all of the time components are absent. also applies to 5.5.4.2.2; then I think the 'T' needed to be inside the optional tags, so I moved it there. The link to the spec's below[1]. Hmm, okay. When I was running my tests in psql I came away with the impression that the T was required in the alternative format. I might be mistaken. I'll run some further tests a little later on. Indeed that's a bug in my code; where I was sometimes requiring the 'T' (in the ISO8601 alternative format) and sometimes not (in the ISO8601 format from 5.5.4.2.1). Below's a test case. If I read the spec[1] right both of those should mean 1 day. I'll update git and post a new patch now. If people think I read the specs wrong, I'll undo this change and fix the docs. == [2]lt:/home/ramayer/proj/pg% ./psql regression psql (8.4devel) Type help for help. regression=# select interval 'P1D'; interval -- 1 day (1 row) regression=# select interval 'P-00-01'; ERROR: invalid input syntax for type interval: P-00-01 LINE 1: select interval 'P-00-01'; ^ == [1] http://isotc.iso.org/livelink/livelink/4021199/ISO_8601_2004_E.zip?func=doc.Fetchnodeid=4021199 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Patch for ISO-8601-Interval Input and output.
Ron Mayer wrote: Brendan Jurd wrote: 'T' ... optional Indeed that's a bug in my code; where I was sometimes requiring the 'T' (in the ISO8601 alternative format) and sometimes not (in the ISO8601 format from 5.5.4.2.1). Below's a test case. If I read the spec[1] right both of those should mean 1 day. I'll update git and post a new patch now. If people think I read the specs wrong, I'll undo this change and fix the docs. I think I updated the web site and git now, and 'P-00-01' is now accepted. It might be useful if someone double checked my reading of the spec, tho. [1] http://isotc.iso.org/livelink/livelink/4021199/ISO_8601_2004_E.zip?func=doc.Fetchnodeid=4021199 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Patch for ISO-8601-Interval Input and output.
On Thu, Oct 2, 2008 at 9:31 PM, Ron Mayer [EMAIL PROTECTED] wrote: Ron Mayer wrote: This patch (that works on top of the IntervalStyle patch I posted earlier today) adds support for ISO8601 standard[0] Time Interval Durations of the format with designators (section 4.4.4.2.1). The other ISO 8601 types of intervals deal with start and end points, so this one seemed most relevant. Hi Ron, Reviewing this patch now; I'm working from the 'iso8601' branch in your git repository. Compile, regression tests and my own ad hoc tests in psql all worked without a hitch. As I was reading through the new documentation introduced by the patch, I thought of various improvements and decided to try them out for myself. In the end, rather than try to explain all of my changes in detail, I thought I'd post a patch of my own (against your branch) and accompany it with a few explanatory notes. The patch includes some minor changes to the code style in src/backend/utils/adt/datetime.c, similar to my recommendations for the IntervalStyle patch. As for the documentation, I ended up making quite a few changes. Explanations and rationales to follow. ISO section numbers -- in a couple of places, the documentation referred to the wrong parts of ISO 8601. I found references to 5.5.4.2.1 and 5.5.4.2.2, whereas in both cases the first two digits should be '4'. Interval Input syntax -- I thought that the paragraph explaining the ISO input syntax was not verbose enough. One paragraph didn't seem sufficient to explain the syntaxes involved. In particular, the original paragraph didn't mention that units could be omitted or reordered in the designators format. I expanded a bit more on how the syntaxes worked, using the same pseudogrammar style used to explain the postgres interval format, and included a table showing the available time-unit designators. Interval Output section structure -- the new section for Interval Output was added at level 2, right below Date/Time Output. I felt this was uncomfortably assymetric with the Input section, which has Date/Time Input as a level 2 section, with Interval Input as a level 3 underneath it. I demoted Interval Output to a level 3 and moved it underneath Date/Time Output. The rest of my doc changes were adding extra linkage, and some minor wording and consistency tweaks. Most of these changes are of a highly subjective nature. I think they are improvements, but someone else might prefer the way it was before I got my hands dirty. Please consider the changes in my patch a set of suggestions for making the documentation on this feature a little easier to digest. You're welcome to take or leave them as you see fit. Cheers, BJ iso8601_interval-1.diff.bz2 Description: BZip2 compressed data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Patch for ISO-8601-Interval Input and output.
Brendan Jurd wrote: Reviewing this patch now; I'm working from the 'iso8601' branch in ... I thought I'd post a patch of my own (against your branch) and accompany it with a few explanatory notes. Wow thanks! That's very helpful (though it might have been more fair to your time if you just kicked it back to me saying rewrite the docs so they make sense)! I've applied them with a couple minor changes. * If ISO 8601 5.5.3.1.d's statement The designator T shall be absent if all of the time components are absent. also applies to 5.5.4.2.2; then I think the 'T' needed to be inside the optional tags, so I moved it there. The link to the spec's below[1]. * There was a sect2 that the patch changed to a sect3, and with that change I get an error: openjade:datatype.sgml:2306:31:E: document type does not allow element SECT3 here so I changed it back to a sect2. You can see my changes to your patch on gitweb here: http://tinyurl.com/6crks6 and see how they got applied after your patch here http://tinyurl.com/685hla I think I've updated my website, my git, and the cleanup patch to include these changes now. Most of these changes are of a highly subjective nature. I think they are improvements, but someone else might prefer the way it was before I got my hands dirty. Please consider the changes in my patch a set of suggestions for making the documentation on this feature a little easier to digest. You're welcome to take or leave them as you see fit. They're clearly improvements. I'm a total novice when it comes to writing docs. [1] http://isotc.iso.org/livelink/livelink/4021199/ISO_8601_2004_E.zip?func=doc.Fetchnodeid=4021199 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Patch for ISO-8601-Interval Input and output.
On Thu, Nov 6, 2008 at 3:36 AM, Ron Mayer [EMAIL PROTECTED] wrote: Wow thanks! That's very helpful (though it might have been more fair to your time if you just kicked it back to me saying rewrite the docs so they make sense)! Maybe ... but I figured it would take more time to fully explain what I meant by rewrite the docs in an email than it would to actually rewrite them. =) I've applied them with a couple minor changes. * If ISO 8601 5.5.3.1.d's statement The designator T shall be absent if all of the time components are absent. also applies to 5.5.4.2.2; then I think the 'T' needed to be inside the optional tags, so I moved it there. The link to the spec's below[1]. Hmm, okay. When I was running my tests in psql I came away with the impression that the T was required in the alternative format. I might be mistaken. I'll run some further tests a little later on. * There was a sect2 that the patch changed to a sect3, and with that change I get an error: openjade:datatype.sgml:2306:31:E: document type does not allow element SECT3 here so I changed it back to a sect2. Ah, the sect3 needs to go _inside_ the sect2, whereas I originally had just changed the tags and left it in the same position (after the sect2). I fixed this in my working copy but I must have forgotten to produce a fresh patch after doing so. Cheers, BJ -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
