Horton

Thread

Commits

  1. Support RN (roman-numeral format) in to_number().

  1. [PATCH] Add roman support for to_number function

    Hunaid Sohail <hunaidpgml@gmail.com> — 2024-08-30T07:21:52Z

    Hi,

While looking at formatting.c file, I noticed a TODO about "add support for
roman number to standard number conversion"  (
https://github.com/postgres/postgres/blob/master/src/backend/utils/adt/formatting.c#L52
)

I have attached the patch that adds support for this and converts roman
numerals to standard numbers (1 to 3999) while also checking if roman
numerals are valid or not.
I have also added a new error code: ERRCODE_INVALID_ROMAN_NUMERAL in case
of invalid numerals.

A few examples:

postgres=# SELECT to_number('MC', 'RN');
 to_number
-----------
      1100
(1 row)

postgres=# SELECT to_number('XIV', 'RN');
 to_number
-----------
        14
(1 row)

postgres=# SELECT to_number('MMMCMXCIX', 'RN');
 to_number
-----------
      3999
(1 row)

postgres=# SELECT to_number('MCCD', 'RN');
ERROR:  invalid roman numeral

I would appreciate your feedback on the following cases:
- Is it okay for the function to handle Roman numerals in a
case-insensitive way? (e.g., 'XIV', 'xiv', and 'Xiv' are all seen as 14).
- How should we handle Roman numerals with leading or trailing spaces, like
' XIV' or 'MC '? Should we trim the spaces, or would it be better to throw
an error in such cases?

I have tested the changes and would appreciate any suggestions for
improvement. I will update the docs and regressions in the next version of
patch.

Regards,
Hunaid Sohail

  2. Re: [PATCH] Add roman support for to_number function

    Maciek Sakrejda <m.sakrejda@gmail.com> — 2024-09-02T18:40:43Z

    Thanks for the contribution.

I took a look at the patch, and it works as advertised. It's too late
for the September commitfest, but I took the liberty of registering
your patch for the November CF [1]. In the course of that, I found an
older thread proposing this feature seven years ago [2]. That patch
was returned with feedback and (as far as I can tell), was not
followed-up on by the author. You may want to review that thread for
feedback; I won't repeat it here.

On Fri, Aug 30, 2024 at 12:22 AM Hunaid Sohail <hunaidpgml@gmail.com> wrote:
>While looking at formatting.c file, I noticed a TODO about "add support for roman number to standard number conversion"  (https://github.com/postgres/postgres/blob/master/src/backend/utils/adt/formatting.c#L52)

Your patch should also remove the TODO =)

> - Is it okay for the function to handle Roman numerals in a case-insensitive way? (e.g., 'XIV', 'xiv', and 'Xiv' are all seen as 14).

The patch in the thread I linked also took a case-insensitive
approach. I did not see any objections to that, and it seems
reasonable to me as well.

> - How should we handle Roman numerals with leading or trailing spaces, like ' XIV' or 'MC '? Should we trim the spaces, or would it be better to throw an error in such cases?

I thought we could reference existing to_number behavior here, but
after playing with it a bit, I'm not really sure what that is:

-- single leading space
maciek=# select to_number(' 1', '9');
 to_number
-----------
         1
(1 row)

-- two leading spaces
maciek=# select to_number('  1', '9');
ERROR:  invalid input syntax for type numeric: " "
-- two leading spaces and template pattern with a decimal
maciek=# select to_number('  1', '9D9');
 to_number
-----------
         1
(1 row)

Separately, I also noticed some unusual Roman representations work
with your patch:

postgres=# select to_number('viv', 'RN');
 to_number
-----------
         9
(1 row)

Is this expected? In contrast, some somewhat common alternative
representations don't work:

postgres=# select to_number('iiii', 'RN');
ERROR:  invalid roman numeral

I know this is expected, but is this the behavior we want? If so, we
probably want to reject the former case, too. If not, maybe that one
is okay, too.

I know I've probably offered more questions than answers, but I hope
finding the old thread here is useful.

Thanks,
Maciek

[1]: https://commitfest.postgresql.org/50/5233/
[2]: https://www.postgresql.org/message-id/flat/CAGMVOduAJ9wKqJXBYnmFmEetKxapJxrG3afUwpbOZ6n_dWaUnA%40mail.gmail.com

  3. Re: [PATCH] Add roman support for to_number function

    Hunaid Sohail <hunaidpgml@gmail.com> — 2024-09-03T13:28:59Z

    On Mon, Sep 2, 2024 at 11:41 PM Maciek Sakrejda <m.sakrejda@gmail.com>
wrote:

> Thanks for the contribution.
>
> I took a look at the patch, and it works as advertised. It's too late
> for the September commitfest, but I took the liberty of registering
> your patch for the November CF [1]. In the course of that, I found an
> older thread proposing this feature seven years ago [2]. That patch
> was returned with feedback and (as far as I can tell), was not
> followed-up on by the author. You may want to review that thread for
> feedback; I won't repeat it here.
>

I submitted the patch on Aug 30 because I read that new patches should be
submitted in CF with "Open" status.


> On Fri, Aug 30, 2024 at 12:22 AM Hunaid Sohail <hunaidpgml@gmail.com>
> wrote:
> >While looking at formatting.c file, I noticed a TODO about "add support
> for roman number to standard number conversion"  (
> https://github.com/postgres/postgres/blob/master/src/backend/utils/adt/formatting.c#L52
> )
>
> Your patch should also remove the TODO =)
>

Noted.

> - How should we handle Roman numerals with leading or trailing spaces,
> like ' XIV' or 'MC '? Should we trim the spaces, or would it be better to
> throw an error in such cases?
>
> I thought we could reference existing to_number behavior here, but
> after playing with it a bit, I'm not really sure what that is:
>
> -- single leading space
> maciek=# select to_number(' 1', '9');
>  to_number
> -----------
>          1
> (1 row)
>
> -- two leading spaces
> maciek=# select to_number('  1', '9');
> ERROR:  invalid input syntax for type numeric: " "
> -- two leading spaces and template pattern with a decimal
> maciek=# select to_number('  1', '9D9');
>  to_number
> -----------
>          1
> (1 row)
>

Yes, you are right. I can't understand the behaviour of trailing spaces
too. Trailing spaces are ignored (doesn't matter how many spaces) but
leading spaces are ignored if there is 1 leading space. For more leading
spaces, error is returned.
A few cases of trailing spaces.
postgres=# select to_number('  1', '9');
ERROR:  invalid input syntax for type numeric: " "
postgres=# select to_number('1 ', '9');
 to_number
-----------
         1
(1 row)

postgres=# select to_number('1  ', '9');
 to_number
-----------
         1
(1 row)

postgres=# select to_number('1       ', '9');
 to_number
-----------
         1
(1 row)


Separately, I also noticed some unusual Roman representations work
> with your patch:
>
> postgres=# select to_number('viv', 'RN');
>  to_number
> -----------
>          9
> (1 row)
>
> Is this expected? In contrast, some somewhat common alternative
> representations don't work:
>
> postgres=# select to_number('iiii', 'RN');
> ERROR:  invalid roman numeral
>
> I know this is expected, but is this the behavior we want? If so, we
> probably want to reject the former case, too. If not, maybe that one
> is okay, too.
>

Yes, 'viv' is invalid. Thanks for pointing this out. Also, found a few
other invalid cases like 'lxl' or 'dcd'. I will fix them in the next patch.
Thank you for your feedback.

Regards,
Hunaid Sohail

  4. Re: [PATCH] Add roman support for to_number function

    Maciek Sakrejda <maciek@pganalyze.com> — 2024-09-03T15:47:43Z

    On Tue, Sep 3, 2024 at 6:29 AM Hunaid Sohail <hunaidpgml@gmail.com> wrote:
> I submitted the patch on Aug 30 because I read that new patches should be submitted in CF with "Open" status.

Oh my bad! I missed that you had submitted it to the September CF:
https://commitfest.postgresql.org/49/5221/

I don't see a way to just delete CF entries, so I've marked the
November one that I created as Withdrawn.

I'll add myself as reviewer to the September CF entry.

Thanks,
Maciek

  5. Re: [PATCH] Add roman support for to_number function

    Hunaid Sohail <hunaidpgml@gmail.com> — 2024-09-05T08:07:03Z

    Hi,

I have attached a new patch v2 with following changes:

- Handled invalid cases like 'viv', 'lxl', and 'dcd'.
- Changed errcode to 22P07 because 22P06 was already taken.
- Removed TODO.
- Added a few positive & negative tests.
- Updated documentation.

Looking forward to your feedback.

Regards,
Hunaid Sohail


On Tue, Sep 3, 2024 at 8:47 PM Maciek Sakrejda <maciek@pganalyze.com> wrote:

> On Tue, Sep 3, 2024 at 6:29 AM Hunaid Sohail <hunaidpgml@gmail.com> wrote:
> > I submitted the patch on Aug 30 because I read that new patches should
> be submitted in CF with "Open" status.
>
> Oh my bad! I missed that you had submitted it to the September CF:
> https://commitfest.postgresql.org/49/5221/
>
> I don't see a way to just delete CF entries, so I've marked the
> November one that I created as Withdrawn.
>
> I'll add myself as reviewer to the September CF entry.
>
> Thanks,
> Maciek
>

  6. Re: [PATCH] Add roman support for to_number function

    Aleksander Alekseev <aleksander@timescale.com> — 2024-09-05T09:41:42Z

    Hi,

> I have attached a new patch v2 with following changes:
>
> - Handled invalid cases like 'viv', 'lxl', and 'dcd'.
> - Changed errcode to 22P07 because 22P06 was already taken.
> - Removed TODO.
> - Added a few positive & negative tests.
> - Updated documentation.
>
> Looking forward to your feedback.

While playing with the patch I noticed that to_char(..., 'RN') doesn't
seem to be test-covered. I suggest adding the following test:

```
WITH rows AS (
    SELECT i, to_char(i, 'FMRN') AS roman
    FROM generate_series(1, 3999) AS i
) SELECT bool_and(to_number(roman, 'RN') = i) FROM rows;

 bool_and
----------
 t
```

... in order to fix this while on it. The query takes ~12 ms on my laptop.

-- 
Best regards,
Aleksander Alekseev

  7. Re: [PATCH] Add roman support for to_number function

    Hunaid Sohail <hunaidpgml@gmail.com> — 2024-09-05T13:07:34Z

    On Thu, Sep 5, 2024 at 2:41 PM Aleksander Alekseev <aleksander@timescale.com>
wrote:

>
> While playing with the patch I noticed that to_char(..., 'RN') doesn't
> seem to be test-covered. I suggest adding the following test:
>
> ```
> WITH rows AS (
>     SELECT i, to_char(i, 'FMRN') AS roman
>     FROM generate_series(1, 3999) AS i
> ) SELECT bool_and(to_number(roman, 'RN') = i) FROM rows;
>
>  bool_and
> ----------
>  t
> ```
>

I also noticed there are no tests for to_char roman format. The test you
provided covers roman format in both to_char and to_number. I will add it.
Thank you.

Regards,
Hunaid Sohail

  8. Re: [PATCH] Add roman support for to_number function

    Hunaid Sohail <hunaidpgml@gmail.com> — 2024-09-07T09:24:23Z

    Hi,


On Thu, Sep 5, 2024 at 2:41 PM Aleksander Alekseev <aleksander@timescale.com>
> wrote:
>
>>
>> While playing with the patch I noticed that to_char(..., 'RN') doesn't
>> seem to be test-covered. I suggest adding the following test:
>>
>> ```
>> WITH rows AS (
>>     SELECT i, to_char(i, 'FMRN') AS roman
>>     FROM generate_series(1, 3999) AS i
>> ) SELECT bool_and(to_number(roman, 'RN') = i) FROM rows;
>>
>>  bool_and
>> ----------
>>  t
>> ```
>>
>
>
I have added this test in the attached patch (v3). Thank you once again,
Aleksander, for the suggestion.

Regards,
Hunaid Sohail

  9. Re: [PATCH] Add roman support for to_number function

    Maciek Sakrejda <m.sakrejda@gmail.com> — 2024-09-07T18:51:29Z

    The following review has been posted through the commitfest application:
make installcheck-world:  tested, passed
Implements feature:       tested, failed
Spec compliant:           tested, failed
Documentation:            tested, passed

Tested again, and the patch looks good. It does not accept leading or trailing whitespace, which seems reasonable, given the unclear behavior of to_number with other format strings. It also rejects less common Roman spellings like "IIII". I don't feel strongly about that one way or the other, but perhaps a test codifying that behavior would be useful to make it clear it's intentional.

I'm marking it RfC.

The new status of this patch is: Ready for Committer

  10. Re: [PATCH] Add roman support for to_number function

    Maciek Sakrejda <m.sakrejda@gmail.com> — 2024-09-07T18:57:00Z

    Sorry, it looks like I failed to accurately log my review in the
review app due to the current broken layout issues [1]. The summary
should be:

make installcheck-world:  tested, passed
Implements feature:       tested, passed
Spec compliant:           not tested (not sure what the spec has to
say about this)
Documentation:            tested, passed

[1]: https://www.postgresql.org/message-id/flat/CAD68Dp1GgTeBiA0YVWXpfPCMC7%3DnqBCLn6%2BhuOkWURcKRnFw5g%40mail.gmail.com

  11. Re: [PATCH] Add roman support for to_number function

    Tom Lane <tgl@sss.pgh.pa.us> — 2024-09-07T23:09:40Z

    Maciek Sakrejda <m.sakrejda@gmail.com> writes:
> Tested again, and the patch looks good. It does not accept leading or trailing whitespace, which seems reasonable, given the unclear behavior of to_number with other format strings. It also rejects less common Roman spellings like "IIII". I don't feel strongly about that one way or the other, but perhaps a test codifying that behavior would be useful to make it clear it's intentional.

Yeah, I don't have a strong feeling about that either, but probably
being strict is better.  to_number has a big problem with "garbage in
garbage out" already, and being lax will make that worse.

A few notes from a quick read of the patch:

* roman_to_int() should have a header comment, notably explaining its
result convention.  I find it fairly surprising that "0" means an
error --- I realize that Roman notation can't represent zero, but
wouldn't it be better to use -1?

* Do *not* rely on toupper().  There are multiple reasons why not,
but in this context the worst one is that in Turkish locales it's
likely to translate "i" to "İ", on which you will fail.  I'd use
pg_ascii_toupper().

* I think roman_to_int() is under-commented internally too.
To start with, where did the magic "15" come from?  And why
should we have such a test anyway --- what if the format allows
for trailing stuff after the roman numeral?  (That is, I think
you need to fix this so that roman_to_int reports how much data
it ate, instead of assuming that it must read to the end of the
input.)  The logic for detecting invalid numeral combinations
feels a little opaque too.  Do you have a source for the rules
you're implementing, and if so could you cite it?

* This code will get run through pgindent before commit, so you
might want to revisit whether your comments will still look nice
afterwards.  There's not a lot of consistency in them about initial
cap versus lower case or trailing period versus not, too.

* roman_result could be declared where used, no?

* I'm not really convinced that we need a new errcode
ERRCODE_INVALID_ROMAN_NUMERAL rather than using a generic one like
ERRCODE_INVALID_TEXT_REPRESENTATION.  However, if we do do it
like that, why did you pick the out-of-sequence value 22P07?

* Might be good to have a few test cases demonstrating what happens
when there's other stuff combined with the RN format spec.  Notably,
even if RN itself won't eat whitespace, there had better be a way
to write the format string to allow that.

* Further to Aleksander's point about lack of test coverage for
the to_char direction, I see from
https://coverage.postgresql.org/src/backend/utils/adt/formatting.c.gcov.html
that almost none of the existing roman-number code paths are covered
today.  While it's not strictly within the charter of this patch
to improve that, maybe we should try to do so --- at the very
least it'd raise formatting.c's coverage score a few notches.

			regards, tom lane

  12. Re: [PATCH] Add roman support for to_number function

    Tom Lane <tgl@sss.pgh.pa.us> — 2024-09-08T21:44:20Z

    I wrote:
> * Further to Aleksander's point about lack of test coverage for
> the to_char direction, I see from
> https://coverage.postgresql.org/src/backend/utils/adt/formatting.c.gcov.html
> that almost none of the existing roman-number code paths are covered
> today.  While it's not strictly within the charter of this patch
> to improve that, maybe we should try to do so --- at the very
> least it'd raise formatting.c's coverage score a few notches.

For the archives' sake: I created a patch and a separate discussion
thread [1] for that.

			regards, tom lane

[1] https://www.postgresql.org/message-id/flat/2956175.1725831136@sss.pgh.pa.us

  13. Re: [PATCH] Add roman support for to_number function

    Hunaid Sohail <hunaidpgml@gmail.com> — 2024-09-09T12:45:17Z

    Hi,

On Sun, Sep 8, 2024 at 4:09 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:

> A few notes from a quick read of the patch:
>
> * roman_to_int() should have a header comment, notably explaining its
> result convention.  I find it fairly surprising that "0" means an
> error --- I realize that Roman notation can't represent zero, but
> wouldn't it be better to use -1?
>

Noted.


>
> * Do *not* rely on toupper().  There are multiple reasons why not,
> but in this context the worst one is that in Turkish locales it's
> likely to translate "i" to "İ", on which you will fail.  I'd use
> pg_ascii_toupper().
>

Noted.


>
> * I think roman_to_int() is under-commented internally too.
> To start with, where did the magic "15" come from?  And why
> should we have such a test anyway --- what if the format allows
> for trailing stuff after the roman numeral?  (That is, I think
> you need to fix this so that roman_to_int reports how much data
> it ate, instead of assuming that it must read to the end of the
> input.)


MMMDCCCLXXXVIII is the longest valid standard roman numeral (15
characters). I will add appropriate comment on length check.

I am not sure I am able to understand the latter part. Could you please
elaborate it?
Are you referring to cases like these?
SELECT to_number('CXXIII foo', 'RN foo');
SELECT to_number('CXXIII foo', 'RN');
Please check my comments on Oracle's behaviour below.



> The logic for detecting invalid numeral combinations
> feels a little opaque too.  Do you have a source for the rules
> you're implementing, and if so could you cite it?
>

There are many sources on the internet.
A few sources:
1. https://www.toppr.com/guides/maths/knowing-our-numbers/roman-numerals/
2. https://www.cuemath.com/numbers/roman-numerals/

Note that we are following the standard form:
https://en.wikipedia.org/wiki/Roman_numerals#Standard_form


>
> * This code will get run through pgindent before commit, so you
> might want to revisit whether your comments will still look nice
> afterwards.  There's not a lot of consistency in them about initial
> cap versus lower case or trailing period versus not, too.
>

Noted.


>
> * roman_result could be declared where used, no?
>

Noted.


>
> * I'm not really convinced that we need a new errcode
> ERRCODE_INVALID_ROMAN_NUMERAL rather than using a generic one like
> ERRCODE_INVALID_TEXT_REPRESENTATION.  However, if we do do it
> like that, why did you pick the out-of-sequence value 22P07?
>

22P07 is not out-of-sequence. 22P01 to 22P06 are already used.
However, I do agree with you that we can use
ERRCODE_INVALID_TEXT_REPRESENTATION. I will change it.


>
> * Might be good to have a few test cases demonstrating what happens
> when there's other stuff combined with the RN format spec.  Notably,
> even if RN itself won't eat whitespace, there had better be a way
> to write the format string to allow that.
>

The current patch (v3) simply ignores other formats with RN except when RN
is used EEEE which returns error.
```
postgres=# SELECT to_number('CXXIII', 'foo RN');
 to_number
-----------
       123
(1 row)

postgres=# SELECT to_number('CXXIII', 'MIrn');
 to_number
-----------
       123
(1 row)

postgres=# SELECT to_number('CXXIII', 'EEEErn');
ERROR:  "EEEE" must be the last pattern used
```

However, I think that other formats except FM should be rejected when used
with RN in NUMDesc_prepare function. Any opinions?

If we look into Oracle, they are strict in this regard and don't allow any
other format with RN.
1. SELECT to_char(12, 'MIrn') from dual;
2. SELECT to_char(12, 'foo RN') from dual;
results in error:
ORA-01481: invalid number format model

I also found that there is no check when RN is used twice (both in to_char
and to_number) and that results in unexpected output.

```
postgres=# SELECT to_number('CXXIII', 'RNrn');
 to_number
-----------
       123
(1 row)

postgres=# SELECT to_char(12, 'RNrn');
            to_char
--------------------------------
             XII            xii
(1 row)

postgres=# SELECT to_char(12, 'rnrn');
            to_char
--------------------------------
             xii            xii
(1 row)

postgres=# SELECT to_char(12, 'FMrnrn');
 to_char
---------
 xiixii
(1 row)
```


> * Further to Aleksander's point about lack of test coverage for
> the to_char direction, I see from
>
> https://coverage.postgresql.org/src/backend/utils/adt/formatting.c.gcov.html
> that almost none of the existing roman-number code paths are covered
> today.  While it's not strictly within the charter of this patch
> to improve that, maybe we should try to do so --- at the very
> least it'd raise formatting.c's coverage score a few notches.
>
>
I see that you have provided a patch to increase test coverage of to_char
roman format including some other formats. I will try to add more tests for
the to_number roman format.


I will provide the next patch soon.

Regards,
Hunaid Sohail

  14. Re: [PATCH] Add roman support for to_number function

    Hunaid Sohail <hunaidpgml@gmail.com> — 2024-09-12T07:44:23Z

    Hi,

I have started working on the next version of the patch and have addressed
the smaller issues identified by Tom. Before proceeding further, I would
like to have opinions on some comments/questions in my previous email.

Regards,
Hunaid Sohail

On Mon, Sep 9, 2024 at 5:45 PM Hunaid Sohail <hunaidpgml@gmail.com> wrote:

> Hi,
>
> On Sun, Sep 8, 2024 at 4:09 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
>> A few notes from a quick read of the patch:
>>
>> * roman_to_int() should have a header comment, notably explaining its
>> result convention.  I find it fairly surprising that "0" means an
>> error --- I realize that Roman notation can't represent zero, but
>> wouldn't it be better to use -1?
>>
>
> Noted.
>
>
>>
>> * Do *not* rely on toupper().  There are multiple reasons why not,
>> but in this context the worst one is that in Turkish locales it's
>> likely to translate "i" to "İ", on which you will fail.  I'd use
>> pg_ascii_toupper().
>>
>
> Noted.
>
>
>>
>> * I think roman_to_int() is under-commented internally too.
>> To start with, where did the magic "15" come from?  And why
>> should we have such a test anyway --- what if the format allows
>> for trailing stuff after the roman numeral?  (That is, I think
>> you need to fix this so that roman_to_int reports how much data
>> it ate, instead of assuming that it must read to the end of the
>> input.)
>
>
> MMMDCCCLXXXVIII is the longest valid standard roman numeral (15
> characters). I will add appropriate comment on length check.
>
> I am not sure I am able to understand the latter part. Could you please
> elaborate it?
> Are you referring to cases like these?
> SELECT to_number('CXXIII foo', 'RN foo');
> SELECT to_number('CXXIII foo', 'RN');
> Please check my comments on Oracle's behaviour below.
>
>
>
>> The logic for detecting invalid numeral combinations
>> feels a little opaque too.  Do you have a source for the rules
>> you're implementing, and if so could you cite it?
>>
>
> There are many sources on the internet.
> A few sources:
> 1. https://www.toppr.com/guides/maths/knowing-our-numbers/roman-numerals/
> 2. https://www.cuemath.com/numbers/roman-numerals/
>
> Note that we are following the standard form:
> https://en.wikipedia.org/wiki/Roman_numerals#Standard_form
>
>
>>
>> * This code will get run through pgindent before commit, so you
>> might want to revisit whether your comments will still look nice
>> afterwards.  There's not a lot of consistency in them about initial
>> cap versus lower case or trailing period versus not, too.
>>
>
> Noted.
>
>
>>
>> * roman_result could be declared where used, no?
>>
>
> Noted.
>
>
>>
>> * I'm not really convinced that we need a new errcode
>> ERRCODE_INVALID_ROMAN_NUMERAL rather than using a generic one like
>> ERRCODE_INVALID_TEXT_REPRESENTATION.  However, if we do do it
>> like that, why did you pick the out-of-sequence value 22P07?
>>
>
> 22P07 is not out-of-sequence. 22P01 to 22P06 are already used.
> However, I do agree with you that we can use
> ERRCODE_INVALID_TEXT_REPRESENTATION. I will change it.
>
>
>>
>> * Might be good to have a few test cases demonstrating what happens
>> when there's other stuff combined with the RN format spec.  Notably,
>> even if RN itself won't eat whitespace, there had better be a way
>> to write the format string to allow that.
>>
>
> The current patch (v3) simply ignores other formats with RN except when RN
> is used EEEE which returns error.
> ```
> postgres=# SELECT to_number('CXXIII', 'foo RN');
>  to_number
> -----------
>        123
> (1 row)
>
> postgres=# SELECT to_number('CXXIII', 'MIrn');
>  to_number
> -----------
>        123
> (1 row)
>
> postgres=# SELECT to_number('CXXIII', 'EEEErn');
> ERROR:  "EEEE" must be the last pattern used
> ```
>
> However, I think that other formats except FM should be rejected when used
> with RN in NUMDesc_prepare function. Any opinions?
>
> If we look into Oracle, they are strict in this regard and don't allow any
> other format with RN.
> 1. SELECT to_char(12, 'MIrn') from dual;
> 2. SELECT to_char(12, 'foo RN') from dual;
> results in error:
> ORA-01481: invalid number format model
>
> I also found that there is no check when RN is used twice (both in to_char
> and to_number) and that results in unexpected output.
>
> ```
> postgres=# SELECT to_number('CXXIII', 'RNrn');
>  to_number
> -----------
>        123
> (1 row)
>
> postgres=# SELECT to_char(12, 'RNrn');
>             to_char
> --------------------------------
>              XII            xii
> (1 row)
>
> postgres=# SELECT to_char(12, 'rnrn');
>             to_char
> --------------------------------
>              xii            xii
> (1 row)
>
> postgres=# SELECT to_char(12, 'FMrnrn');
>  to_char
> ---------
>  xiixii
> (1 row)
> ```
>
>
>> * Further to Aleksander's point about lack of test coverage for
>> the to_char direction, I see from
>>
>> https://coverage.postgresql.org/src/backend/utils/adt/formatting.c.gcov.html
>> that almost none of the existing roman-number code paths are covered
>> today.  While it's not strictly within the charter of this patch
>> to improve that, maybe we should try to do so --- at the very
>> least it'd raise formatting.c's coverage score a few notches.
>>
>>
> I see that you have provided a patch to increase test coverage of to_char
> roman format including some other formats. I will try to add more tests for
> the to_number roman format.
>
>
> I will provide the next patch soon.
>
> Regards,
> Hunaid Sohail
>

  15. Re: [PATCH] Add roman support for to_number function

    Hunaid Sohail <hunaidpgml@gmail.com> — 2024-09-19T07:51:51Z

    Hi,

I have attached a new patch with following changes:
- Addressed minor issues identified by Tom.
- Rejected other formats with RN and updated the docs.
- Added a few more tests.


Regards,
Hunaid Sohail

>

  16. Re: [PATCH] Add roman support for to_number function

    Hunaid Sohail <hunaidpgml@gmail.com> — 2024-10-30T07:07:51Z

    Hi,

I have attached a rebased patch if someone wants to review in the next CF.
No changes as compared to v4.

Regards,
Hunaid Sohail

>

  17. Re: [PATCH] Add roman support for to_number function

    Tomas Vondra <tomas@vondra.me> — 2024-12-08T00:36:38Z

    Hi,

On 10/30/24 08:07, Hunaid Sohail wrote:
> Hi,
> 
> I have attached a rebased patch if someone wants to review in the next
> CF. No changes as compared to v4.
> 
> Regards,
> Hunaid Sohail
> 

Thanks for a nice patch. I did a quick review today, seems almost there,
I only have a couple minor comments:

1) Template Patterns for Numeric Formatting

Why the wording change? "input between 1 and 3999" sounds fine to me.


2) The docs say "standard numbers" but I'm not sure what that is? We
don't use that term anywhere else, I think. Same for "standard roman
numeral".


3) A couple comments need a bit of formatting. Multi-line comments
should start with an empty line, some lines are a bit too long.


4) I was wondering if the regression tests check all interesting cases,
and it seems none of the tests hit these two cases:

    if (vCount > 1 || lCount > 1 || dCount > 1)
        return -1;

and

    if (!IS_VALID_SUB_COMB(currChar, nextChar))
        return -1;

I haven't tried constructing tests to hit those cases, though.

Seems ready to go otherwise.


regards

-- 
Tomas Vondra

  18. Re: [PATCH] Add roman support for to_number function

    Hunaid Sohail <hunaidpgml@gmail.com> — 2024-12-09T06:09:37Z

    Hi,

Thanks for reviewing this patch.

On Mon, Dec 9, 2024 at 3:07 AM Tomas Vondra <tomas@vondra.me> wrote:

> Thanks for a nice patch. I did a quick review today, seems almost there,
> I only have a couple minor comments:
>
> 1) Template Patterns for Numeric Formatting
>
> Why the wording change? "input between 1 and 3999" sounds fine to me.
>
input... seemed to refer to numeric input for to_char roman format. But,
after roman support in to_number function shouldn't we modify it? It is the
reason I changed it to "valid for numbers 1 to 3999".

2) The docs say "standard numbers" but I'm not sure what that is? We
> don't use that term anywhere else, I think. Same for "standard roman
> numeral".
>
I will change "standard numbers" to "numbers".
Note that we are following the standard form. There are other forms too.
For example, 4 can be represented as IV (standard) or IIII (non standard).
https://en.wikipedia.org/wiki/Roman_numerals#Standard_form


> 3) A couple comments need a bit of formatting. Multi-line comments
> should start with an empty line, some lines are a bit too long.
>
I will fix the multi-line formatting.


> 4) I was wondering if the regression tests check all interesting cases,
> and it seems none of the tests hit these two cases:
>
>     if (vCount > 1 || lCount > 1 || dCount > 1)
>         return -1;
>
SELECT to_number('viv', 'RN') test covers this if statement for the
subtraction part. But, I noticed that no test covers simple counts of V, L,
D.
I will add SELECT to_number('VV', 'RN') for that.


> and
>
>     if (!IS_VALID_SUB_COMB(currChar, nextChar))
>         return -1;
>
Noted. SELECT to_number('IL', 'RN') test can be added.

Regards,
Hunaid Sohail

  19. Re: [PATCH] Add roman support for to_number function

    Hunaid Sohail <hunaidpgml@gmail.com> — 2024-12-13T05:29:28Z

    Hi,


> On Mon, Dec 9, 2024 at 3:07 AM Tomas Vondra <tomas@vondra.me> wrote:
>
>> Thanks for a nice patch. I did a quick review today, seems almost there,
>> I only have a couple minor comments:
>>
>> 1) Template Patterns for Numeric Formatting
>>
>> Why the wording change? "input between 1 and 3999" sounds fine to me.
>>
> input... seemed to refer to numeric input for to_char roman format. But,
> after roman support in to_number function shouldn't we modify it? It is the
> reason I changed it to "valid for numbers 1 to 3999".
>
> 2) The docs say "standard numbers" but I'm not sure what that is? We
>> don't use that term anywhere else, I think. Same for "standard roman
>> numeral".
>>
> I will change "standard numbers" to "numbers".
> Note that we are following the standard form. There are other forms too.
> For example, 4 can be represented as IV (standard) or IIII (non standard).
> https://en.wikipedia.org/wiki/Roman_numerals#Standard_form
>
>
>> 3) A couple comments need a bit of formatting. Multi-line comments
>> should start with an empty line, some lines are a bit too long.
>>
> I will fix the multi-line formatting.
>
>
>> 4) I was wondering if the regression tests check all interesting cases,
>> and it seems none of the tests hit these two cases:
>>
>>     if (vCount > 1 || lCount > 1 || dCount > 1)
>>         return -1;
>>
> SELECT to_number('viv', 'RN') test covers this if statement for the
> subtraction part. But, I noticed that no test covers simple counts of V, L,
> D.
> I will add SELECT to_number('VV', 'RN') for that.
>
>
>> and
>>
>>     if (!IS_VALID_SUB_COMB(currChar, nextChar))
>>         return -1;
>>
> Noted. SELECT to_number('IL', 'RN') test can be added.
>

I’ve attached a new patch that addresses comments 2, 3, and 4 from Tomas.

Looking forward to your feedback.

Regards,
Hunaid Sohail

  20. Re: [PATCH] Add roman support for to_number function

    Tom Lane <tgl@sss.pgh.pa.us> — 2025-01-18T00:27:43Z

    Hunaid Sohail <hunaidpgml@gmail.com> writes:
> I’ve attached a new patch that addresses comments 2, 3, and 4 from Tomas.

I'm still quite unhappy that this doesn't tolerate trailing
whitespace.  That's not how other format patterns work, and it makes
it impossible to round-trip the output of to_char(..., 'RN').
I think the core of the problem is that you tried to cram the logic
in where the existing "not implemented" error is thrown.  But on
inspection there is nothing sane about that entire "Roman correction"
stanza -- what it does is either useless (zeroing already-zeroed
fields) or in the wrong place (if we don't want to allow other flags
with IS_ROMAN, that should be done via an error in NUMDesc_prepare,
like other similar cases).  In the attached I moved the roman_to_int
call into NUM_processor's main loop so it's more like other format
patterns, and fixed it to not eat any more characters than it should.
This might allow putting back some format-combination capabilities,
but other than the whitespace angle I figure we can leave that for
people to request it.

			regards, tom lane

  21. Re: [PATCH] Add roman support for to_number function

    Hunaid Sohail <hunaidpgml@gmail.com> — 2025-01-20T07:32:51Z

    Hi Tom,

On Sat, Jan 18, 2025 at 5:27 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:

> Hunaid Sohail <hunaidpgml@gmail.com> writes:
> > I’ve attached a new patch that addresses comments 2, 3, and 4 from Tomas.
>
> I'm still quite unhappy that this doesn't tolerate trailing
> whitespace.  That's not how other format patterns work, and it makes
> it impossible to round-trip the output of to_char(..., 'RN').
> I think the core of the problem is that you tried to cram the logic
> in where the existing "not implemented" error is thrown.  But on
> inspection there is nothing sane about that entire "Roman correction"
> stanza -- what it does is either useless (zeroing already-zeroed
> fields) or in the wrong place (if we don't want to allow other flags
> with IS_ROMAN, that should be done via an error in NUMDesc_prepare,
> like other similar cases).  In the attached I moved the roman_to_int
> call into NUM_processor's main loop so it's more like other format
> patterns, and fixed it to not eat any more characters than it should.
> This might allow putting back some format-combination capabilities,
> but other than the whitespace angle I figure we can leave that for
> people to request it.
>

Thank you for the tweaks to the patch. Overall, it looks great.

Initially, when I looked at the test case:
SELECT to_number('M CC', 'RN');

I was confused about whether it should be 'MCC'. After further inspection,
I realized that the output is 1000 for 'M'. The format of the input can be a
bit unclear. Perhaps we could add a note in the documentation
or issue a warning in roman_to_int function when input is truncated,
to clarify this behavior.

+       bool            truncated = false;
+
+       /*
+        * Collect and decode valid roman numerals, consuming at most
+        * MAX_ROMAN_LEN characters.  We do this in a separate loop to avoid
+        * repeated decoding and because the main loop needs to know when
it's at
+        * the last numeral.
+        */
+       for (len = 0; len < MAX_ROMAN_LEN && !OVERLOAD_TEST; len++)
+       {
+               char            currChar = pg_ascii_toupper(*Np->inout_p);
+               int                     currValue = ROMAN_VAL(currChar);
+
+               if (currValue == 0)
+               {
+                       truncated = true;
+                       break;                          /* Not a valid
roman numeral. */
+               }
+               romanChars[len] = currChar;
+               romanValues[len] = currValue;
+               Np->inout_p++;
+       }
+
+       if (len == 0)
+               return -1;                              /* No valid roman
numerals. */
+
+       /* Check for truncation. */
+       if (truncated)
+       {
+               ereport(WARNING,
+
(errcode(ERRCODE_INVALID_TEXT_REPRESENTATION),
+                                errmsg("Input truncated to \"%.*s\"",
+                                               len, romanChars)));
+       }

Output after change:

postgres=# SELECT to_number('MMXX  ', 'RN');
WARNING:  Input truncated to "MMXX"
 to_number
-----------
      2020
(1 row)

postgres=# SELECT to_number('  XIV  ', '  RN');
WARNING:  Input truncated to "XIV"
 to_number
-----------
        14
(1 row)

postgres=# SELECT to_number('M CC', 'RN');
WARNING:  Input truncated to "M"
 to_number
-----------
      1000
(1 row)

Also, some modifications to the test cases will be required
if we go with these changes.

Regards,
Hunaid Sohail

  22. Re: [PATCH] Add roman support for to_number function

    Tom Lane <tgl@sss.pgh.pa.us> — 2025-01-20T16:25:05Z

    Hunaid Sohail <hunaidpgml@gmail.com> writes:
> On Sat, Jan 18, 2025 at 5:27 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Initially, when I looked at the test case:
> SELECT to_number('M CC', 'RN');

> I was confused about whether it should be 'MCC'. After further inspection,
> I realized that the output is 1000 for 'M'. The format of the input can be a
> bit unclear. Perhaps we could add a note in the documentation
> or issue a warning in roman_to_int function when input is truncated,
> to clarify this behavior.

I don't think a warning is a great idea at all.  There is no other
place in formatting.c that issues warnings, and this doesn't seem
like the place to start.  As an example,

regression=# select to_number('123garbage', '999999');
 to_number 
-----------
       123
(1 row)

There's certainly a case that could be made that to_number should
be stricter about trailing garbage, but it's probably a couple
decades too late to change now.  In any case it seems completely
inappropriate to me for RN to adopt that policy all by itself.

A different way that we could potentially look at this example is
that RN should be willing to skip embedded spaces, leading to 'M CC'
producing 1200 not 1000.  There is a little bit of support for that
idea in the existing behavior

regression=# select to_number('123 456', '999999');
 to_number 
-----------
    123456
(1 row)

However, when you poke at that a bit closer, it's not a license
for unlimited whitespace:

regression=# select to_number('123  456', '999999');
 to_number 
-----------
     12345
(1 row)

I've not tracked down the exact cause of that, but I think it
has to do with the fact that NUM_numpart_from_char() is willing
to eat a single leading space per call, even if it's not the
first call.  The original author's reasoning for that is lost
to history thanks to the lack of comments, but I believe that
the idea was not "eat random whitespace" but "consume a space
that was emitted as a substitute for a plus sign".  The fact
that it can happen once per '9' code feels more like a bug than
something intentional.  I'm not proposing that we do something
about that, at least not today, but it doesn't seem like
something we want to model RN's behavior on either.  So I'm
not in favor of allowing embedded spaces.

However ... in poking at this, I noticed that I had been
mis-understanding what to_char(..., 'RN') does: it fills
to 15 characters with *leading* spaces not trailing spaces
as I'd been thinking:

regression=# select to_char(433, '|RN|');
      to_char      
-------------------
 |       CDXXXIII|
(1 row)

So on the argument that to_number's RN should be willing to
consume what to_char's RN produces, we're both wrong and
roman_to_int should be willing to eat leading spaces.
What do you think?

			regards, tom lane

  23. Re: [PATCH] Add roman support for to_number function

    Tom Lane <tgl@sss.pgh.pa.us> — 2025-01-20T21:16:35Z

    I wrote:
> However, when you poke at that a bit closer, it's not a license
> for unlimited whitespace:

> regression=# select to_number('123  456', '999999');
>  to_number 
> -----------
>      12345
> (1 row)

> I've not tracked down the exact cause of that, but I think it
> has to do with the fact that NUM_numpart_from_char() is willing
> to eat a single leading space per call, even if it's not the
> first call.

I tried simply removing that bit:

diff --git a/src/backend/utils/adt/formatting.c b/src/backend/utils/adt/formatting.c
index 3960235e14..366430863f 100644
--- a/src/backend/utils/adt/formatting.c
+++ b/src/backend/utils/adt/formatting.c
@@ -5134,12 +5134,6 @@ NUM_numpart_from_char(NUMProc *Np, int id, int input_len)
 		 (id == NUM_0 || id == NUM_9) ? "NUM_0/9" : id == NUM_DEC ? "NUM_DEC" : "???");
 #endif
 
-	if (OVERLOAD_TEST)
-		return;
-
-	if (*Np->inout_p == ' ')
-		Np->inout_p++;
-
 	if (OVERLOAD_TEST)
 		return;
 

All our regression tests still pass, and this example seems to behave
more sanely:

regression=# select to_number('123 456', '999999');
 to_number 
-----------
     12345
(1 row)

regression=# select to_number('123  456', '999999');
 to_number 
-----------
      1234
(1 row)

That might or might not be a behavior you like, but it's at least
possible to see where it's coming from: it's eating the digits that
appear within a six-character window, and not any more.

Not sure if anyone would thank us for making this change though.
I can't avoid the suspicion that somebody somewhere is depending
on the current behavior.

Anyway, this is getting off-topic for the current thread.

			regards, tom lane

  24. Re: [PATCH] Add roman support for to_number function

    Maciek Sakrejda <m.sakrejda@gmail.com> — 2025-01-21T00:18:44Z

    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;

  25. Re: [PATCH] Add roman support for to_number function

    Tom Lane <tgl@sss.pgh.pa.us> — 2025-01-21T01:41:25Z

    Maciek Sakrejda <m.sakrejda@gmail.com> writes:
> 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)

Ugh.  This makes more urgent my question about where roman_to_int's
algorithm came from, because there's evidently something not right
about it.

			regards, tom lane

  26. Re: [PATCH] Add roman support for to_number function

    Hunaid Sohail <hunaidpgml@gmail.com> — 2025-01-21T08:35:47Z

    Hi,

On Mon, Jan 20, 2025 at 9:25 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:

> I've not tracked down the exact cause of that, but I think it
> has to do with the fact that NUM_numpart_from_char() is willing
> to eat a single leading space per call, even if it's not the
> first call.  The original author's reasoning for that is lost
> to history thanks to the lack of comments, but I believe that
> the idea was not "eat random whitespace" but "consume a space
> that was emitted as a substitute for a plus sign".  The fact
> that it can happen once per '9' code feels more like a bug than
> something intentional.  I'm not proposing that we do something
> about that, at least not today, but it doesn't seem like
> something we want to model RN's behavior on either.  So I'm
> not in favor of allowing embedded spaces.
>

Agreed. We should not allow trailing and embedded spaces.
Since, trailing spaces and become embedded spaces like
'MCC  ' or 'M CC'.
It means cases like these should be invalid:
SELECT to_number('MMXX  ', 'RN');
SELECT to_number('  XIV  ', '  RN');
SELECT to_number('M CC', 'RN');


> So on the argument that to_number's RN should be willing to
> consume what to_char's RN produces, we're both wrong and
> roman_to_int should be willing to eat leading spaces.
> What do you think?
>

Agreed. Your changes in v7 consumes leading spaces if leading space
is present in format ('  RN'). But we need it to work on cases like:
SELECT to_number('  XIV', 'RN');
So, I can add this logic in my next patch.

Regards,
Hunaid Sohail

  27. Re: [PATCH] Add roman support for to_number function

    Hunaid Sohail <hunaidpgml@gmail.com> — 2025-01-21T08:45:12Z

    Hi,

On Tue, Jan 21, 2025 at 5:19 AM Maciek Sakrejda <m.sakrejda@gmail.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

  28. Re: [PATCH] Add roman support for to_number function

    Hunaid Sohail <hunaidpgml@gmail.com> — 2025-01-21T09:26:12Z

    On Tue, Jan 21, 2025 at 1:35 PM Hunaid Sohail <hunaidpgml@gmail.com> wrote:

> Agreed. Your changes in v7 consumes leading spaces if leading space
> is present in format ('  RN'). But we need it to work on cases like:
> SELECT to_number('  XIV', 'RN');
> So, I can add this logic in my next patch.
>

So, I was playing with leading spaces in v7 and found something
interesting.

postgres=# --both input and format both 2 spaces
postgres=# SELECT to_number('  XIV', '  RN');
 to_number
-----------
        14
(1 row)

postgres=# --input 1 space, format 2 spaces
postgres=# --Input is read from "IV" ignoring "X"
postgres=# SELECT to_number(' XIV', '  RN');
 to_number
-----------
         4
(1 row)

postgres=# --should work after leading spaces are handled
postgres=# SELECT to_number('  XIV', ' RN');
ERROR:  invalid Roman numeral
postgres=# select to_number('123456', ' 999999');
 to_number
-----------
     23456
(1 row)

postgres=# select to_number('123456', '  999999');
 to_number
-----------
      3456
(1 row)

The leading spaces are consumed in the RN (from the main loop
in Num_processor), and this behavior seems consistent with how
simple numbers are handled. The Roman numeral parsing
appears to start from where "RN" is in the format after
leading spaces are consumed.

Please review the first two examples of Roman numerals.
I'd appreciate your opinions on whether this behavior should be considered
valid.

Regards,
Hunaid Sohail

  29. Re: [PATCH] Add roman support for to_number function

    Tom Lane <tgl@sss.pgh.pa.us> — 2025-01-21T15:48:56Z

    Hunaid Sohail <hunaidpgml@gmail.com> writes:
> The leading spaces are consumed in the RN (from the main loop
> in Num_processor), and this behavior seems consistent with how
> simple numbers are handled. The Roman numeral parsing
> appears to start from where "RN" is in the format after
> leading spaces are consumed.

Yes, a space in the format string should consume a character
from the input (whether it's a space or not).

regression=# select to_number('x123', '999');
 to_number 
-----------
        12
(1 row)

regression=# select to_number('x123', ' 999');
 to_number 
-----------
       123
(1 row)

I used to think that a space in the format would consume any number of
spaces from the input, but that doesn't seem to be true --- I think
I was misled by that perhaps-bug in NUM_numpart_from_char.  For
example:

regression=# select to_number('   123', ' 999');
 to_number 
-----------
        12
(1 row)

One of the three input spaces is eaten by the format space, and
one is taken as the sign, and then the 9's match ' 12'.  (I don't
think we should be expecting a sign in RN though.)  However,
as we add more input spaces:

regression=# select to_number('    123', ' 999');
 to_number 
-----------
        12
(1 row)

regression=# select to_number('     123', ' 999');
 to_number 
-----------
         1
(1 row)

This is bizarrely inconsistent, and I think what's causing it
is the extra space-consumption in NUM_numpart_from_char.

			regards, tom lane

  30. Re: [PATCH] Add roman support for to_number function

    Hunaid Sohail <hunaidpgml@gmail.com> — 2025-01-22T13:33:48Z

    Hi,

I have attached a new patch v8 with the following changes:
1. Fixed cases reported by Maciek.
2. Handles leading spaces in input string.

I have also added a few more negative tests after Maciek reported
a few cases.

I have also updated the format in the round trip test:
...to_char(i, 'FMRN') ... to
...to_char(i, 'RN') ...
which verifies that leading spaces are now handled in input
string with simple 'RN' when used with to_number.

Looking forward to your feedback.

Regards,
Hunaid Sohail

  31. Re: [PATCH] Add roman support for to_number function

    Tom Lane <tgl@sss.pgh.pa.us> — 2025-01-22T20:29:04Z

    Hunaid Sohail <hunaidpgml@gmail.com> writes:
> I have attached a new patch v8 with the following changes:
> 1. Fixed cases reported by Maciek.
> 2. Handles leading spaces in input string.

I pushed this after fooling with it a bit more:

* The way you did the leading-space skip would actually skip
anything whatsoever until it found a valid roman character.
I don't think that's what's wanted here.  We just want to
skip whitespace --- the idea more or less is to consume
leading whitespace that to_char('RN') might have emitted.

* You'd also re-introduced failure on trailing whitespace
(or indeed trailing anything), which I still don't agree with.
It's not appropriate for RN to dictate that nothing can follow it.
There's a separate discussion to be had about whether to_number
is too lax about trailing garbage, but the policy should not be
different for RN than it is for other format codes.

* I made some other cosmetic changes such as rearranging the
error checks into an order that made more sense to me.

Thanks for submitting this patch!

			regards, tom lane