Thread: [HACKERS] Add Roman numeral conversion to to_number
Adds to the to_number() function the ability to convert Roman numerals
to a number. This feature is on the formatting.c TODO list. It is not
currently implemented in either Oracle, MSSQL or MySQL so gives
PostgreSQL an edge :-)
==Usage==
Call: to_number(numerals, 'RN') or to_number(numerals, 'rn').
Example: to_number('MMMXIII', 'RN') returns 3013. to_number('xiv',
'rn') returns 14.
The function is case insensitive for the numerals. It accepts a mix of
cases and treats them the same. So to_number ('MCI, 'rn') and
to_number ('McI', 'RN') both return 1101. The format mask must however
be either 'RN' or 'rn'. If there are other elements in the mask, those
other elements will be ignored. So to_number('MMM', 'FMRN') returns
3000.
Whitespace before the numerals is ignored.
==Validation==
The new function roman_to_int() in formatting.c performs the
conversion. It strictly validates the numerals based on the following
Roman-to-Arabic conversion rules:
1. The power-of-ten numerals (I, X, C, M) can be repeated up to three
times in a row. The beginning-with-5 numerals (V, L, D) can each
appear only once.
2. Subtraction from a power-of-ten numeral cannot occur if a
beginning-with-5 numeral appears later.
3. Subtraction cannot occur if the smaller numeral is less than a
tenth of the greater numeral (so IX is valid, but IC is invalid).
4. There cannot be two subtractions in a row.
5. A beginning-with-5 numeral cannot subtract.
If any of these rules are violated, an error is raised.
==Testing==
This has been tested on a Windows build of the master branch with
MinGW. The included regression tests positively test every value from
1 to 3999 (the Roman numeral max value) by calling the existing
to_char() function to get the Roman value, then converting it back to
an Arabic value. There are also negative tests for each invalid code
path and some positive mixed-case tests.
Documentation is updated to include this new feature.
==References==
http://sierra.nmsu.edu/morandi/coursematerials/RomanNumerals.html
Documents the strict Roman numeral standard.
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Attachment
On Thu, Aug 3, 2017 at 9:25 AM, Oliver Ford <ojford@gmail.com> wrote:
> Adds to the to_number() function the ability to convert Roman numerals
> to a number. This feature is on the formatting.c TODO list. It is not
> currently implemented in either Oracle, MSSQL or MySQL so gives
> PostgreSQL an edge :-)
I kind of put my head in my hands when I saw this. I'm not really
sure it's worth complicating the code for something that has so little
practical utility, but maybe other people will feel differently. I
can't deny the profound advantages associated with having a leg up on
Oracle.
The error reporting is a little wonky, although maybe no wonkier than
anything else about these conversion routines.
rhaas=# select to_number('q', 'rn');
ERROR: invalid character "q"
(hmm, no position)
rhaas=# select to_number('dd', 'rn');
ERROR: invalid character "D" at position 1
(now i get a position, but it's not really the right position; and the
problem isn't really that the character is invalid but that you don't
like me including it twice, and I said 'd' not 'D')
rhaas=# select to_number('à', 'rn');
ERROR: invalid character "?"
(eh?)
How much call is there for a format that can only represent values up to 3999?
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
On Thu, Aug 03, 2017 at 01:45:02PM -0400, Robert Haas wrote:
> On Thu, Aug 3, 2017 at 9:25 AM, Oliver Ford <ojford@gmail.com> wrote:
> > Adds to the to_number() function the ability to convert Roman numerals
> > to a number. This feature is on the formatting.c TODO list. It is not
> > currently implemented in either Oracle, MSSQL or MySQL so gives
> > PostgreSQL an edge :-)
>
> I kind of put my head in my hands when I saw this. I'm not really
> sure it's worth complicating the code for something that has so little
> practical utility, but maybe other people will feel differently. I
> can't deny the profound advantages associated with having a leg up on
> Oracle.
>
> The error reporting is a little wonky, although maybe no wonkier than
> anything else about these conversion routines.
>
> rhaas=# select to_number('q', 'rn');
> ERROR: invalid character "q"
>
> (hmm, no position)
>
> rhaas=# select to_number('dd', 'rn');
> ERROR: invalid character "D" at position 1
>
> (now i get a position, but it's not really the right position; and the
> problem isn't really that the character is invalid but that you don't
> like me including it twice, and I said 'd' not 'D')
>
> rhaas=# select to_number('à', 'rn');
> ERROR: invalid character "?"
>
> (eh?)
>
> How much call is there for a format that can only represent values up to 3999?
There are ways to represent much larger numbers, possibly bigger than
INT_MAX. https://en.wikipedia.org/wiki/Roman_numerals#Large_numbers
https://en.wikipedia.org/wiki/Numerals_in_Unicode#Roman_numerals
As nearly as I can tell, this patch is late by 124 days.
Best,
David.
--
David Fetter <david(at)fetter(dot)org> http://fetter.org/
Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter
Skype: davidfetter XMPP: david(dot)fetter(at)gmail(dot)com
Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate
On Thursday, 3 August 2017, Robert Haas <robertmhaas@gmail.com> wrote:
On Thu, Aug 3, 2017 at 9:25 AM, Oliver Ford <ojford@gmail.com> wrote:
> Adds to the to_number() function the ability to convert Roman numerals
> to a number. This feature is on the formatting.c TODO list. It is not
> currently implemented in either Oracle, MSSQL or MySQL so gives
> PostgreSQL an edge :-)
I kind of put my head in my hands when I saw this. I'm not really
sure it's worth complicating the code for something that has so little
practical utility, but maybe other people will feel differently. I
can't deny the profound advantages associated with having a leg up on
Oracle.
The formatting.c file specifies it as a TODO, so I thought implementing it would be worthwhile. As there is a to_roman conversion having it the other way is good for completeness.
The error reporting is a little wonky, although maybe no wonkier than
anything else about these conversion routines.
rhaas=# select to_number('q', 'rn');
ERROR: invalid character "q"
(hmm, no position)
rhaas=# select to_number('dd', 'rn');
ERROR: invalid character "D" at position 1
(now i get a position, but it's not really the right position; and the
problem isn't really that the character is invalid but that you don't
like me including it twice, and I said 'd' not 'D')
rhaas=# select to_number('à', 'rn');
ERROR: invalid character "?"
(eh?)
How much call is there for a format that can only represent values up to 3999?
The existing int_to_roman code goes up to 3999 so this patch is consistent with that. I could extend both to handle Unicode values for large numbers?
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
On Thu, Aug 3, 2017 at 2:54 PM, Oliver Ford <ojford@gmail.com> wrote: > The formatting.c file specifies it as a TODO, so I thought implementing it > would be worthwhile. As there is a to_roman conversion having it the other > way is good for completeness. Huh, didn't know about that. Well, I'm not direly opposed to this or anything, just not sure that it's worth spending a lot of time on. > The existing int_to_roman code goes up to 3999 so this patch is consistent > with that. I could extend both to handle Unicode values for large numbers? Well, following what the existing code does is usually a good place to start -- whether you want to try to extend it is up to you. I'm not very interested in Roman numeral handling personally, so you might want to wait for some opinions from others. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On 8/3/17 13:45, Robert Haas wrote: > On Thu, Aug 3, 2017 at 9:25 AM, Oliver Ford <ojford@gmail.com> wrote: >> Adds to the to_number() function the ability to convert Roman numerals >> to a number. This feature is on the formatting.c TODO list. It is not >> currently implemented in either Oracle, MSSQL or MySQL so gives >> PostgreSQL an edge :-) > I kind of put my head in my hands when I saw this. I'm not really > sure it's worth complicating the code for something that has so little > practical utility, but maybe other people will feel differently. I can't get excited about it. to_number() and such usually mirror the Oracle implementation, so having something that is explicitly not in Oracle goes a bit against its mission. One of the more interesting features of to_number/to_char is that it has a bunch of facilities for formatting decimal points, leading/trailing zeros, filling in spaces and signs, and so on. None of that applies naturally to Roman numerals, so there isn't a strong case for including that into these functions, when a separate function or module could do. There are probably a bunch of Perl or Python modules that can be employed for this. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Mon, Aug 14, 2017 at 2:48 PM, Peter Eisentraut <peter.eisentraut@2ndquadrant.com> wrote: > On 8/3/17 13:45, Robert Haas wrote: >> On Thu, Aug 3, 2017 at 9:25 AM, Oliver Ford <ojford@gmail.com> wrote: >>> Adds to the to_number() function the ability to convert Roman numerals >>> to a number. This feature is on the formatting.c TODO list. It is not >>> currently implemented in either Oracle, MSSQL or MySQL so gives >>> PostgreSQL an edge :-) >> I kind of put my head in my hands when I saw this. I'm not really >> sure it's worth complicating the code for something that has so little >> practical utility, but maybe other people will feel differently. > > I can't get excited about it. to_number() and such usually mirror the > Oracle implementation, so having something that is explicitly not in > Oracle goes a bit against its mission. > > One of the more interesting features of to_number/to_char is that it has > a bunch of facilities for formatting decimal points, leading/trailing > zeros, filling in spaces and signs, and so on. None of that applies > naturally to Roman numerals, so there isn't a strong case for including > that into these functions, when a separate function or module could do. Well, doesn't that also apply to scientific notation (EEEE)? 'RN' is documented as an accepted formatting string, and nowhere does it mention that it only works for input. So we ought to allow for it to be fixed or at least document that it does not work. It's nothing but a curio obviously, but it's kind of cool IMO. merlin
The following review has been posted through the commitfest application:
make installcheck-world: tested, passed
Implements feature: tested, passed
Spec compliant: not tested
Documentation: not tested
Code looks fine, but one niggly complaint at line 146 of the patch file ("while (*cp) {"). A K&R style brace slipped
in,which doesn't match the formatting of the file.
Given that this is providing new formatting options, there should be new tests added that validate the options and
errorhandling.
There's also the "do we want this?" debate from the discussion thread that still needs to be resolved. (I don't have an
opinioneither way.)
I'm sending this back to the author to address the first two issues.
The new status of this patch is: Waiting on Author
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
I'll fix the brace, but there are two other patches in the first email for tests and docs. For some reason the commitfest app didn't pick them up.
On Friday, 15 September 2017, Doug Doole <DougDoole@gmail.com> wrote:
On Friday, 15 September 2017, Doug Doole <DougDoole@gmail.com> wrote:
The following review has been posted through the commitfest application:
make installcheck-world: tested, passed
Implements feature: tested, passed
Spec compliant: not tested
Documentation: not tested
Code looks fine, but one niggly complaint at line 146 of the patch file ("while (*cp) {"). A K&R style brace slipped in, which doesn't match the formatting of the file.
Given that this is providing new formatting options, there should be new tests added that validate the options and error handling.
There's also the "do we want this?" debate from the discussion thread that still needs to be resolved. (I don't have an opinion either way.)
I'm sending this back to the author to address the first two issues.
The new status of this patch is: Waiting on Author
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Ah. Sorry I missed them - I'll give them a look. (Won't be able to get to it until Saturday though.)
On Thu, Sep 14, 2017 at 10:06 PM Oliver Ford <ojford@gmail.com> wrote:
I'll fix the brace, but there are two other patches in the first email for tests and docs. For some reason the commitfest app didn't pick them up.
On Friday, 15 September 2017, Doug Doole <DougDoole@gmail.com> wrote:The following review has been posted through the commitfest application:
make installcheck-world: tested, passed
Implements feature: tested, passed
Spec compliant: not tested
Documentation: not tested
Code looks fine, but one niggly complaint at line 146 of the patch file ("while (*cp) {"). A K&R style brace slipped in, which doesn't match the formatting of the file.
Given that this is providing new formatting options, there should be new tests added that validate the options and error handling.
There's also the "do we want this?" debate from the discussion thread that still needs to be resolved. (I don't have an opinion either way.)
I'm sending this back to the author to address the first two issues.
The new status of this patch is: Waiting on Author
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Oliver, I took a look at your tests and they look thorough to me.
One recommendation, instead of having 3999 separate selects to test every legal roman numeral, why not just do something like this:
do $$
declare
i int;
rn text;
rn_val int;
begin
for i in 1..3999 loop
rn := trim(to_char(i, 'rn'));
rn_val := to_number(rn, 'rn');
if (i <> rn_val) then
raise notice 'Mismatch: i=% rn=% rn_val=%', i, rn, rn_val;
end if;
end loop;
raise notice 'Tested roman numerals 1..3999';
end;
$$;
It's a lot easier to maintain than separate selects.
I finally figured out why docs weren't building on my machine so I was able to take a look at your doc patch too. I think it's fine.
Reading it did suggest a couple extra negative tests to confirm that when 'rn' is specified, all other elements are ignored:
select to_number('vii7', 'rn9');
select to_number('7vii', '9rn');
On 3 Aug 2017 16:29, "Oliver Ford" <ojford@gmail.com> wrote:
Adds to the to_number() function the ability to convert Roman numerals
to a number. This feature is on the formatting.c TODO list. It is not
currently implemented in either Oracle, MSSQL or MySQL so gives
PostgreSQL an edge :-)
I see use of this in full text search as a dictionary. It's useful for indexing and searching historical documents. Probably, better to have as contrib.
==Usage==
Call: to_number(numerals, 'RN') or to_number(numerals, 'rn').
Example: to_number('MMMXIII', 'RN') returns 3013. to_number('xiv',
'rn') returns 14.
The function is case insensitive for the numerals. It accepts a mix of
cases and treats them the same. So to_number ('MCI, 'rn') and
to_number ('McI', 'RN') both return 1101. The format mask must however
be either 'RN' or 'rn'. If there are other elements in the mask, those
other elements will be ignored. So to_number('MMM', 'FMRN') returns
3000.
Whitespace before the numerals is ignored.
==Validation==
The new function roman_to_int() in formatting.c performs the
conversion. It strictly validates the numerals based on the following
Roman-to-Arabic conversion rules:
1. The power-of-ten numerals (I, X, C, M) can be repeated up to three
times in a row. The beginning-with-5 numerals (V, L, D) can each
appear only once.
2. Subtraction from a power-of-ten numeral cannot occur if a
beginning-with-5 numeral appears later.
3. Subtraction cannot occur if the smaller numeral is less than a
tenth of the greater numeral (so IX is valid, but IC is invalid).
4. There cannot be two subtractions in a row.
5. A beginning-with-5 numeral cannot subtract.
If any of these rules are violated, an error is raised.
==Testing==
This has been tested on a Windows build of the master branch with
MinGW. The included regression tests positively test every value from
1 to 3999 (the Roman numeral max value) by calling the existing
to_char() function to get the Roman value, then converting it back to
an Arabic value. There are also negative tests for each invalid code
path and some positive mixed-case tests.
Documentation is updated to include this new feature.
==References==
http://sierra.nmsu.edu/morandi/coursematerials/ RomanNumerals.html
Documents the strict Roman numeral standard.
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Sat, Sep 16, 2017 at 10:42:49PM +0000, Douglas Doole wrote: > Oliver, I took a look at your tests and they look thorough to me. > > One recommendation, instead of having 3999 separate selects to test every > legal roman numeral, why not just do something like this: > > do $$ > declare > i int; > rn text; > rn_val int; > begin > for i in 1..3999 loop > rn := trim(to_char(i, 'rn')); > rn_val := to_number(rn, 'rn'); > if (i <> rn_val) then > raise notice 'Mismatch: i=% rn=% rn_val=%', i, rn, rn_val; > end if; > end loop; > raise notice 'Tested roman numerals 1..3999'; > end; > $$; > > It's a lot easier to maintain than separate selects. Why not just one SELECT, as in: SELECT i, to_char(i, 'rn'), to_number(to_char(i, 'rn'), 'rn'); FROM generate_series(1,3999) i Best, David. -- David Fetter <david(at)fetter(dot)org> http://fetter.org/ Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter Skype: davidfetter XMPP: david(dot)fetter(at)gmail(dot)com Remember to vote! Consider donating to Postgres: http://www.postgresql.org/about/donate -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
On Sun, Sep 17, 2017 at 6:43 PM, David Fetter <david@fetter.org> wrote:
On Sat, Sep 16, 2017 at 10:42:49PM +0000, Douglas Doole wrote:
> Oliver, I took a look at your tests and they look thorough to me.
>
> One recommendation, instead of having 3999 separate selects to test every
> legal roman numeral, why not just do something like this:
>
> do $$
> declare
> i int;
> rn text;
> rn_val int;
> begin
> for i in 1..3999 loop
> rn := trim(to_char(i, 'rn'));
> rn_val := to_number(rn, 'rn');
> if (i <> rn_val) then
> raise notice 'Mismatch: i=% rn=% rn_val=%', i, rn, rn_val;
> end if;
> end loop;
> raise notice 'Tested roman numerals 1..3999';
> end;
> $$;
>
> It's a lot easier to maintain than separate selects.
Why not just one SELECT, as in:
SELECT i, to_char(i, 'rn'), to_number(to_char(i, 'rn'), 'rn');
FROM generate_series(1,3999) i
Question: What is our definition of a legal Roman numeral?
For example sometimes IXX appears in the corpus to refer to 19 even though our standardised notation would be XIX.
Best,
David.
--
David Fetter <david(at)fetter(dot)org> http://fetter.org/
Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter
Skype: davidfetter XMPP: david(dot)fetter(at)gmail(dot)com
Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Best Regards,
Chris Travers
Database Administrator
Saarbrücker Straße 37a, 10405 Berlin
Re: Peter Eisentraut 2017-08-14 <b1dc5f08-47dc-8a43-3147-372bba169fc3@2ndquadrant.com> > There are probably a bunch of Perl or Python modules that can be > employed for this. https://github.com/ChristophBerg/postgresql-numeral Christoph -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
On Sun, Sep 17, 2017 at 12:29 AM, Douglas Doole <dougdoole@gmail.com> wrote:
> I finally figured out why docs weren't building on my machine so I was able
> to take a look at your doc patch too. I think it's fine.
>
> Reading it did suggest a couple extra negative tests to confirm that when
> 'rn' is specified, all other elements are ignored:
>
> select to_number('vii7', 'rn9');
> select to_number('7vii', '9rn');
>
>
Attached is v2 of src, tests and docs. Doc patch is unchanged from v1.
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Attachment
The following review has been posted through the commitfest application: make installcheck-world: tested, passed Implements feature: tested, passed Spec compliant: tested, passed Documentation: tested, passed Applied clean and runs fine. Previous comments were addressed. There's been some discussion in the thread about whether this is worth doing, but given that it is done it seems a bit ofa moot point. So passing this off to the committers to make a final call. The new status of this patch is: Ready for Committer -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
On Tue, Sep 19, 2017 at 12:39 AM, Oliver Ford <ojford@gmail.com> wrote: > Attached is v2 of src, tests and docs. Doc patch is unchanged from v1. Since commit ef73a816, this patch causes make check to fail: too many parallel tests (more than 20) in schedule file "./parallel_schedule" -- Thomas Munro http://www.enterprisedb.com
Oliver Ford <ojford@gmail.com> writes:
> Attached is v2 of src, tests and docs. Doc patch is unchanged from v1.
I took a quick look at this patch. The roman_to_int function itself seems
fairly reasonable, except that I don't like anything about the error
reporting. ERRCODE_INVALID_PARAMETER_VALUE doesn't really seem like the
right thing for bogus data; probably ERRCODE_INVALID_TEXT_REPRESENTATION
is the closest thing we've got. More generally, reporting a character
position in a string you're not showing to the user is neither helpful nor
per project style. It'd be much better to just report the whole input
string. I'm tempted to suggest that all the error reports could beerrmsg("incorrect Roman numeral: \"%s\"", numerals)
I'm not sure it's worth breaking it down more finely than that, and
even if it is, several of these messages aren't too intelligible as-is.
An angle that might be worth considering is whether we really want to
reject non-canonical Roman input. As is, the function rejects "IIII",
insisting that you must spell it "IV", but is that helpful or just
pedantic? Likewise I'm not that excited about expending code and a
separate translatable message to insist that people have to write "X"
rather than "VV".
However, what I really don't like is the way that you plugged roman_to_int
into the existing to_number infrastructure, which is to say that you
didn't. Putting in an "if roman then <x> else <all the existing code>"
test is not the way to make this work. As an example, that fails to
handle literal text in the format. If this works:
regression=# select to_char(123, 'foo FMRN'); to_char
------------foo CXXIII
(1 row)
then this should, but it fails:
regression=# select to_number('foo CXXIII', 'foo FMRN');
ERROR: invalid character " "
I think you need to be calling roman_to_int from the NUM_RN/NUM_rn
switch cases in NUM_processor, not trying to bypass NUM_processor
entirely.
Another issue is that on the input side, we really need to reject formats
like 'RN 999', since it's very unclear how we'd combine the input values.
There already is some logic that rejects 'RN EEEE', but it needs
extension. On the other hand, I do not see any really good reason to
reject 'RN 999' for output; it would result in duplicative output, but
if the user asked for it why not do it? (I see the existing code fails to
handle that case for some reason.) So some refactoring of the existing
roman-numeral code seems needed anyway.
regards, tom lane
On Sun, Nov 19, 2017 at 3:32 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Oliver Ford <ojford@gmail.com> writes: >> Attached is v2 of src, tests and docs. Doc patch is unchanged from v1. > > I took a quick look at this patch. This got a review, and no replies after 10 days to I am marking it as returned with feedback for now. Oliver, if you can update your patch, please feel free to send it to this thread, and also please add a new entry in the commit fest app. -- Michael