Re: [HACKERS] new function for tsquery creartion

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