Re: [PATCH] Add roman support for to_number function

Hi,

On Tue, Jan 21, 2025 at 5:19 AM Maciek Sakrejda <m(dot)sakrejda(at)gmail(dot)com>
wrote:

> V7 passes check-world here. But, just for kicks, I generated all
> possible 7-character sequences of Roman digits [1] to confirm whether
> everything either parsed cleanly or errored cleanly. Reviewing the
> output, I noticed that to_number accepts some dubiously-formatted
> values:
>
> postgres=# select to_number('mmmdcm', 'RN');
> to_number
> -----------
> 4400
> (1 row)
>
> I'm assuming this was not intended, since the function comment notes
> the result will be in the 1-3999 range. (And to_char fails to parse
> this, failing the round-trip test.)
>
> Thanks,
> Maciek
>
> [1]: with rd(d) as (VALUES('i'),('v'),('x'),('l'),('c'),('d'),('m')),
> rn as (select d1.d || d2.d || d3.d || d4.d || d5.d || d6.d || d7.d as
> n from rd d1, rd d2, rd d3, rd d4, rd d5, rd d6, rd d7) select 'select
> to_number(' || rn.n || ', ''RN'');' from rn;
>

Thanks for checking more cases. I feel that I found the source of
the issue. It turns out I missed the rule that V, L, and D cannot
appear before a larger numeral.

As a result, cases like the following are also invalid:

SELECT to_number('VIX', 'RN');
SELECT to_number('LXC', 'RN');
SELECT to_number('DCM', 'RN');
select to_number('MMDCM', 'RN');
select to_number('CLXC', 'RN');

I just need to add an if statement to handle these cases correctly.
I will include the fix in the next patch, and also add these cases
to the regression tests.

Regards,
Hunaid Sohail