Re: BUG #5469: regexp_matches() has poor behaviour and more poor documentation

On Wed, May 26, 2010 at 7:58 AM, Daniele Varrazzo
<daniele(dot)varrazzo(at)gmail(dot)com> wrote:
>> I'm not sure that it's very productive to refer to the behavior of our
>> code as insane.  We do document this in section 9.7.3, pretty
>> explicitly:
>>
>> "The regexp_matches function returns all of the captured substrings
>> resulting from matching a POSIX regular expression pattern. It has the
>> syntax regexp_matches(string, pattern  [, flags  ]). If there is no
>> match to the pattern, the function returns no rows. If there is a
>> match, the function returns a text array whose n'th element is the
>> substring matching the n'th parenthesized subexpression of the pattern
>> (not counting "non-capturing" parentheses; see below for details)."
>>
>> I think that's pretty clear.  Your mileage may vary, of course.
>
> "If there is no match to the pattern, the function returns no rows" is
> easily overlooked as "it returns null", or some other behaviour that
> don't change the returned set. The point is, because the function is
> listed in the string function, you would expect the function to
> manipulate text, not the dataset. The function as it is is not safe to
> be used in a construct
>
>    SELECT foo, bar, regexp_matches(bar, pattern) FROM table;
>
> unless you really wanted:
>
>    SELECT foo, bar, regexp_matches(bar, pattern) FROM table WHERE bar
> ~ pattern;

Well, even that's not really the same thing... if you're surprised by
getting no rows for a row in the source table, you could easily also
be surprised by getting more than one.

> otherwise you have to take measures to be able to deal with records in
> which the pattern is not matched, for example:
>
>    SELECT foo, bar, regexp_matches(bar, pattern || '|') FROM table;
>
> the latter still doesn't work when bar is NULL: in this case the
> record is dropped anyway, so I don't think it can be proposed as
> general solution.
>
> The characteristics of returning a set of text[] is useful when the
> user wants all the matches, not only the first one: the behaviour is
> selected specifying the flag 'g' as third argument.
>
> From this point of view, I hope it can be stated that in its current
> form the regexp_matches() has not the most optimal interface. Please
> accept my apology for the tone being too rude in my previous message.

No sweat. I don't agree that with the statement that regexp_matches()
is not the optimal interface, but I would agree with an alternative
statement that some people might prefer a different interface.

>> I'm less confident than what we have in table 9-6 (other string
>> functions, in section 9.4, string functions and operators) is clear on
>> first reading, but neither do I immediately know how to improve it.
>> Perhaps instead of critiquing our insanity you could provide some
>> specific suggestions for improvement.
>>
>> Similarly, if you think we should have another function besides
>> regexp_matches(), rather than just complaining that we don't, it would
>> be more useful to suggest a name and a specific behavior and ideally
>> maybe even provide a patch (or just the docs portion of a patch) -
>> especially if you can point to a specific use-case that is hard to do
>> with the SRF but would be easier with a function with a different
>> interface.
>
> Below I assume an alternative function is provided. I have problems in
> finding a name for the function, as regexp_matches() is already used.
> I would call it regexp_match() in reference to the fact that it
> returns a single value (being an array) and not a list of matches as
> potentially regexp_matches() could. The quite similar names could be a
> problem though.

Actually, I kind of like that. I think it would be reasonable to
provide regexp_match() returning text[] and regexp_matches() returning
setof text[].

> Because table 9-6 is the index people look for when they have a task
> related to strings, I would say wording should be:
>
> [regexp_matches:] Return all groups of captured substrings resulting
> from matching a POSIX regular expression against the string. Warning:
> in case of no match, tested record is dropped. See Section 9.7.3 for
> more information.
> [regexp_match:] Return the first group of captured substrings
> resulting from matching a POSIX regular expression against the string.
> In case of no match, return NULL. See Section 9.7.3 for more
> information.
>
> In section 9.7.3, after "If there is no match to the pattern, the
> function returns no rows." I would add "This means that if the
> function is used in a SELECT, records where the string don't match the
> pattern are discarded from the dataset. If such records are required,
> use regexp_match() instead".

I think that talking about rows being dropped is confusing and not
really accurate. What I would say is that regexp_matches() is a
set-returning function and can return multiple rows, or none.
Therefore, calling it in the target list may increase or decrease the
number of output rows. If this behavior is not desired, use
regexp_match() instead.

> If the problem is acknowledged, I'd be happy to provide relevant patches.

Assuming a lack of violent disagreement, I'd go for it.

http://wiki.postgresql.org/wiki/Submitting_a_Patch

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise Postgres Company