Hunaid Sohail 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 un
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')
Hunaid Sohail 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.
Y
On Tue, Jan 21, 2025 at 1:35 PM Hunaid Sohail 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 wi
Hi,
On Tue, Jan 21, 2025 at 5:19 AM Maciek Sakrejda
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_n
Hi,
On Mon, Jan 20, 2025 at 9:25 PM Tom Lane 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
Maciek Sakrejda 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-formatte
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_n
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', '99');
> to_number
> ---
> 12345
> (1 row)
> I've not tracked down the exact cause of that, but I think it
> has to do with the fac
Hunaid Sohail writes:
> On Sat, Jan 18, 2025 at 5:27 AM Tom Lane 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 inpu
Hi Tom,
On Sat, Jan 18, 2025 at 5:27 AM Tom Lane wrote:
> Hunaid Sohail 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
>
Hunaid Sohail 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
Hi,
> On Mon, Dec 9, 2024 at 3:07 AM Tomas Vondra 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
Hi,
Thanks for reviewing this patch.
On Mon, Dec 9, 2024 at 3:07 AM Tomas Vondra 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
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 co
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
>
From 951f3e9620f0d50fcdaff095652a07d6304b490c Mon Sep 17 00:00:00 2001
From: Hunaid Sohail
Date: Wed, 30 Oct 2024 12:00:47 +0500
Subject: [PATCH v5] Add roman su
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
>
v4-0001-Add-roman-support-for-to_number-function.patch
Description: Binary data
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 wrot
Hi,
On Sun, Sep 8, 2024 at 4:09 AM Tom Lane 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
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 with
Maciek Sakrejda 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 "". I don't feel strongly ab
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
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 le
Hi,
On Thu, Sep 5, 2024 at 2:41 PM Aleksander Alekseev
> 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 gen
On Thu, Sep 5, 2024 at 2:41 PM Aleksander Alekseev
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, 3
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 f
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.
Regard
On Tue, Sep 3, 2024 at 6:29 AM Hunaid Sohail 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 t
On Mon, Sep 2, 2024 at 11:41 PM Maciek Sakrejda
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 foun
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
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
31 matches
Mail list logo
