[PATCH] Support negative indexes in split_part
I posted the idea of allowing negative indexes in split_part to pgsql-general
last week, and it seemed there was some interest:
/messages/by-id/CAPWqQZR+-5pAZNSSrnmYczRaX-huemc=oO8URvDZvUA-M=MOBA@mail.gmail.com
Attached is a patch, based on master, that implements the approach as described
in that discussion.
The motivation is that the existing idioms for splitting a string and selecting
the nth-to-last element are rather complicated and/or inefficient:
1. (string_to_array('foo bar baz', ' '))[cardinality(string_to_array('foo bar baz', ' ')) - 1]
2. reverse(split_part(reverse('foo bar baz'), ' ', 1))
3. (regexp_match('foo baz bar', '\S*$'))[1]
With the patch, split_part(haystack, needle, -1) selects the last field of the
string, split_part(haystack, needle, -2) selects the second-to-last field, and
so on. Per Tom Lane, there is precedent for this design, where negative indices
meaning "count from the end", namely the left and right string functions.
The patch includes updates to the docs and regression tests. If the feature is
deemed desirable, I believe the patch is "commit quality" (though, fair warning,
this is my first contribution to Postgres, so I might have the wrong notion
of what a committable patch looks like).
Note that the implementation is deliberately a bit inefficient to keep things
simple. When presented with a negative index, the implementation does an extra
pass over the string to count the total number of fields, in order to convert
the negative index to a positive index. Then it proceeds as it normally would.
One can imagine adding support for backwards B-M-H, but I'm not sure that could
be made to work with all multibyte encodings. We could at least avoid the extra
pass over the string by allocating a circular buffer of size |n| when n is
negative, but that wasn't clearly worthwhile. (I did implement the optimization
for the special case of -1, since its implementation was trivial.)
Cheers,
Nikhil
Attachments:
0001-Support-negative-indexes-in-split_part.patchtext/plain; charset=UTF-8; name=0001-Support-negative-indexes-in-split_part.patch; x-mac-creator=0; x-mac-type=0Download+139-10
Nikhil Benesch <nikhil.benesch@gmail.com> writes:
I posted the idea of allowing negative indexes in split_part to pgsql-general
last week, and it seemed there was some interest:
/messages/by-id/CAPWqQZR+-5pAZNSSrnmYczRaX-huemc=oO8URvDZvUA-M=MOBA@mail.gmail.com
Attached is a patch, based on master, that implements the approach as described
in that discussion.
Please add an entry to the upcoming commitfest, to make sure we don't
lose track of this:
https://commitfest.postgresql.org/30/
regards, tom lane
On 11/1/20 9:56 PM, Tom Lane wrote:
Please add an entry to the upcoming commitfest, to make sure we don't
lose track of this:
The following review has been posted through the commitfest application:
make installcheck-world: tested, passed
Implements feature: tested, passed
Spec compliant: not tested
Documentation: not tested
Patch looks good to me. Seems like a useful feature, and I agree that the two-pass implementation makes the change very easy to review.
Quick note on test coverage: gcov marks the "needle not found" branch (the one marked `/* special case if fldsep not found at all */`) as being completely uncovered. I don't think that needs to gate this patch; it looks like it was uncovered before this feature was added.
Doc builds are currently failing due to what appears to be an xmllint failure:
/usr/bin/xmllint --path . --noout --valid postgres.sgml
error : Unknown IO error
postgres.sgml:21: warning: failed to load external entity "http://www.oasis-open.org/docbook/xml/4.5/docbookx.dtd"
but that doesn't have anything to do with this patch. Marking Ready for Committer. (I'm a little new to this myself, so someone please let me know if I'm jumping the gun.)
Thanks!
--Jacob
The new status of this patch is: Ready for Committer
Jacob Champion <pchampion@pivotal.io> writes:
Patch looks good to me. Seems like a useful feature, and I agree that the two-pass implementation makes the change very easy to review.
LGTM too. I made a couple of cosmetic improvements and pushed it.
Quick note on test coverage: gcov marks the "needle not found" branch (the one marked `/* special case if fldsep not found at all */`) as being completely uncovered. I don't think that needs to gate this patch; it looks like it was uncovered before this feature was added.
We seem to be trying for full test coverage of this function now,
so I added a test case for that branch too.
Doc builds are currently failing due to what appears to be an xmllint failure:
Unrelated, but see
/messages/by-id/E2EE6B76-2D96-408A-B961-CAE47D1A86F0@yesql.se
regards, tom lane