Re: [PATCH] Add roman support for to_number function

Hi,

> On Mon, Dec 9, 2024 at 3:07 AM Tomas Vondra <tomas(at)vondra(dot)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