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
Inever 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'sa 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
probablydoes not make much sense to add delete(tsvector,tsvector), right? But being able to delete a bunch of
lexemesin 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
commentsin the surrounding code, but I doubt that's a good reason for not having any comments in new code. It makes
reviewsunnecessarily 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
singleparameter 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?
regards
--
Tomas Vondra http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
