Re: [HACKERS] new function for tsquery creartion

From: Victor Drobny <v(dot)drobny(at)postgrespro(dot)ru>
To: Tomas Vondra <tomas(dot)vondra(at)2ndquadrant(dot)com>
Cc: Thomas Munro <thomas(dot)munro(at)enterprisedb(dot)com>, Robert Haas <robertmhaas(at)gmail(dot)com>, pgsql-hackers(at)postgresql(dot)org
Subject: Re: [HACKERS] new function for tsquery creartion
Date: 2017-11-28 06:56:10
Message-ID: 15517a04afd2be779e2e6d69611c348b@postgrespro.ru
Views: Raw Message | Whole Thread | Download mbox
Thread:
Lists: pgsql-hackers

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

Attachment Content-Type Size
queryto_tsquery3_01_around.patch text/x-diff 12.0 KB
queryto_tsquery3_02_queryto_tsquery.patch text/x-diff 18.5 KB
queryto_tsquery3_03_documentation.patch text/x-diff 4.9 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Rajkumar Raghuwanshi 2017-11-28 07:07:48 Re: [HACKERS] Partition-wise aggregation/grouping
Previous Message Nikolay Shaplov 2017-11-28 06:12:01 Re: [HACKERS] [PATCH] Move all am-related reloption code into src/backend/access/[am-name] and get rid of relopt_kind for custom AM