Re: Tsvector editing functions

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