On Tue, Dec 15, 2015 at 12:07 PM, Tomas Vondra
<tomas.vondra@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