Re: [PATCH] Add roman support for to_number function

Hi,

Thanks for reviewing this patch.

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.

Regards,
Hunaid Sohail