On 12.02.25 05:31, Euler Taveira wrote:
> On Mon, Dec 2, 2024, at 6:09 AM, Peter Eisentraut wrote:
>> On 26.08.24 08:09, Peter Eisentraut wrote:
>> > This patch allows using text position search functions with
>> > nondeterministic collations. These functions are
>> >
>> > - position, strpos
>> > - replace
>> > - split_part
>> > - string_to_array
>> > - string_to_table
>> >
>> > which all use common internal infrastructure.
>>
>> > Some exploratory testing could be useful here. The present test
>> > coverage was already quite helpful during development, but there is
>> > always the possibility that something was overlooked.
>
> I took a look at this patch.
>
> * Most callers will require "greedy" semantics, meaning that
> we need
> * to find the longest such substring, not the shortest. For
> callers
> * don't don't need greedy semantics, we can finish on the first
>
> s/don't don't/that don't/ ?
fixed
> - Assert(len1 > 0);
> Assert(len2 > 0);
>
> Is there a reason to remove this assert?
len1 is the length of the "haystack". The previous code did not call
text_position_setup() with an empty haystack, due to early exits at
/* Empty needle always matches at position 1 */
and
/* Otherwise, can't match if haystack is shorter than needle */
but the second one does not apply for nondeterministic collations, so we
have to deal with with zero-length haystacks.
> * (With nondeterministic collations, the search was already
> * multibyte-aware, so we don't need this.)
>
> s/was/is/
fixed
> The commit title could be changed to reflect that you are adding support for
> multiple functions. The POSITION gives the impression that it is only
> for the
> position() function. Something like
>
> Support position search functions with nondeterministic collations
fixed
> I did a couple of tests (some are shown below) and I didn't find issues.
Thanks for the extra tests. I have committed this with the adjustments
mentioned above.
