On 2017-11-19 04:30, Tomas Vondra wrote:
Hello,
> Hi,
>
> On 09/13/2017 10:57 AM, Victor Drobny wrote:
>> On 2017-09-09 06:03, Thomas Munro wrote:
>>> Please send a rebased version of the patch for people to review and
>>> test as that one has bit-rotted.
>> Hello,
>> Thank you for interest. In the attachment you can find rebased
>> version(based on 69835bc8988812c960f4ed5aeee86b62ac73602a commit)
>>
>
> I did a quick review of the patch today. The patch unfortunately no
> longer applies, so I had to use an older commit from September. Please
> rebase to current master.
Thank you for your time. In the attachment you can find rebased version.
(based on e842791b0 commit)
> I've only looked on the diff at this point, will do more testing once
> the rebase happens.
>
> Some comments:
>
> 1) This seems to mix multiple improvements into one patch. There's the
> support for alternative query syntax, and then there are the new
> operators (AROUND and <m,n>). I propose to split the patch into two or
> more parts, each addressing one of those bits.
I agree. I have split it in 3 parts: support for around operator,
queryto_tsquery function and documentation.
> I guess there will be two or three parts - first adding the syntax,
> second adding <m,n> and third adding the AROUND(n). Seems reasonable?
>
> 2) I don't think we should mention Google in the docs explicitly. Not
> that I'm somehow anti-google, but this syntax was certainly not
> invented
> by Google - I vividly remember using something like that on Altavista
> (yeah, I'm old). And it's used by pretty much every other web search
> engine out there ...
Yes, those syntax is not introduced by google, but, as for me, it was
the
easiest way to give a brief description of it. Of cause it can be
changed,
I just don't know how. Any suggestions are welcomed! ;)
> 3) In the SGML docs, please use <literal></literal> instead of just
> quoting the values. So it should be <literal>|</literal> instead of '|'
> etc. Just like in the parts describing plainto_tsquery, for example.
Fixed. I hope that i didn't miss anything.
> 4) Also, I recommend adding a brief explanation what the examples do.
> Right now there's just a bunch of queryto_tsquery, and the reader is
> expected to understand the output. I suggest adding a sentence or two,
> explaining what's happening (just like for plainto_tsquery examples).
>
> 5) I'm not sure about negative values in the <n,m> operator. I don't
> find it particularly confusing - once you understand that (a <n,m> b)
> means "there are 'k' words between 'a' and 'b' (n <= k <= m)", then
> negative values seem like a fairly straightforward extension.
>
> But I guess the main question is "Is there really a demand for the new
> <n,m> operator, or have we just invented if because we can?"
The operator <n,m> is not introduced yet. It's just a concept. It were
our
thoughts about implementation AROUND operator through <n,m> in future.
> 6) There seem to be some new constants defined, with names not
> following
> the naming convention. I mean this
>
> #define WAITOPERAND 1
> #define WAITOPERATOR 2
> #define WAITFIRSTOPERAND 3
> #define WAITSINGLEOPERAND 4
> #define INSIDEQUOTES 5 <-- the new one
>
> and
>
> #define TSPO_L_ONLY 0x01
> #define TSPO_R_ONLY 0x02
> #define TSPO_BOTH 0x04
> #define TS_NOT_EXAC 0x08 <-- the new one
>
> Perhaps that's OK, but it seems a bit inconsistent.
I agree. I have fixed it.
>
> regards
--
Victor Drobny
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company
