On Tue, Dec 15, 2015 at 12:07 PM, Tomas Vondra <tomas.von...@2ndquadrant.com> wrote: > Hi, > > On 12/07/2015 03:05 PM, Stas Kelvich wrote: >> >> Hello. >> >> Done with the list of suggestions. Also heavily edit delete function. >> > > I did a quick review of the updated patch. I'm not a tsvector-expert so > hopefully my comments won't be entirely bogus. > > 1) It's a bit difficult to judge the usefulness of the API, as I've > always been a mere user of full-text search, and I never had a need > (or courage) to mess with the tsvectors. OTOH I don't see a good > reason no to have such API, when there's a need for it. > > The API seems to be reasonably complete, with one exception - when > looking at editing function we have for 'hstore', we do have these > variants for delete() > > delete(hstore,text) > delete(hstore,text[]) > delete(hstore,hstore) > > while this patch only adds delete(tsvector,text). Would it make > sense to add variants similar to hstore? It probably does not make > much sense to add delete(tsvector,tsvector), right? But being able > to delete a bunch of lexemes in one go seems like a good thing. > > What do you think? > > > 2) tsvector_op.c needs a bit of love, to eliminate the two warnings it > currently triggers: > > tsvector_op.c:211:2: warning: ISO C90 forbids mixed ... > tsvector_op.c:635:9: warning: variable ‘lexeme2copy’ set but ... > > 3) the patch also touches tsvector_setweight(), only to do change: > > elog(ERROR, "unrecognized weight: %d", cw); > > to > > elog(ERROR, "unrecognized weight: %c", cw); > > That should probably go get committed separately, as a bugfix. > > > 4) I find it rather annoying that there are pretty much no comments in > the code. Granted, there are pretty much no comments in the > surrounding code, but I doubt that's a good reason for not having > any comments in new code. It makes reviews unnecessarily difficult. > > > 5) tsvector_concat() is not mentioned in docs at all > > > 6) Docs don't mention names of the new parameters in function > signatures, just data types. The functions with a single parameter > probably don't need to do that, but multi-parameter ones should. > > 7) Some of the functions use intexterm that does not match the function > name. I see two such cases - to_tsvector and setweight. Is there a > reason for that?
I have marked this patch as returned with feedback based on the presence of a review and a lack of replies from the author. Stas, if you are still working on the patch, please feel free to move it to the next commit fest. -- Michael -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
