Re: Naming of new tsvector functions

I noticed that 6943a946c introduces some new functions named delete()
and filter(). This does not seem like a terribly bright idea to me.
They may not be formally ambiguous with the corresponding keywords,
but it's not very hard to imagine how small typos could lead to
the parser taking the unintended interpretation and then producing
totally confusing error messages. It's even less hard to imagine
this choice preventing us from introducing some new syntax in future
(for instance, DELETE ... RETURNING ... as a subquery-in-FROM) because
it *would* be formally ambiguous.

I think we'd be better off to rename these to tsvector_delete() and
tsvector_filter() while we still can.

regards, tom lane

On 05/02/2016 10:27 AM, Tom Lane wrote:
> I noticed that 6943a946c introduces some new functions named delete()
> and filter(). This does not seem like a terribly bright idea to me.
> They may not be formally ambiguous with the corresponding keywords,
> but it's not very hard to imagine how small typos could lead to
> the parser taking the unintended interpretation and then producing
> totally confusing error messages. It's even less hard to imagine
> this choice preventing us from introducing some new syntax in future
> (for instance, DELETE ... RETURNING ... as a subquery-in-FROM) because
> it *would* be formally ambiguous.
>
> I think we'd be better off to rename these to tsvector_delete() and
> tsvector_filter() while we still can.

or ts_filter/delete? but no objection

JD

>
> regards, tom lane
>
>

--
Command Prompt, Inc. http://the.postgres.company/
+1-503-667-4564
PostgreSQL Centered full stack support, consulting and development.
Everyone appreciates your honesty, until you are honest with them.

I wrote:
> I think we'd be better off to rename these to tsvector_delete() and
> tsvector_filter() while we still can.

... although I now notice that hstore already exposes a function named
delete(), so that ship may have sailed already. But I'm more troubled
by filter() anyhow, since that keyword can appear in expressions ---
it seems much more likely that that would pose a parsing conflict
after future SQL extensions.

regards, tom lane

On Mon, May 2, 2016 at 1:58 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> I wrote:
>> I think we'd be better off to rename these to tsvector_delete() and
>> tsvector_filter() while we still can.
>
> ... although I now notice that hstore already exposes a function named
> delete(), so that ship may have sailed already. But I'm more troubled
> by filter() anyhow, since that keyword can appear in expressions ---
> it seems much more likely that that would pose a parsing conflict
> after future SQL extensions.

But not everybody has hstore installed, so even if that's a problem it
won't be a problem for everybody, all the time. +1 for renaming them
both.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Mon, May 02, 2016 at 01:58:11PM -0400, Tom Lane wrote:
> I wrote:
> > I think we'd be better off to rename these to tsvector_delete()
> > and tsvector_filter() while we still can.
>
> ... although I now notice that hstore already exposes a function
> named delete(), so that ship may have sailed already. But I'm more
> troubled by filter() anyhow, since that keyword can appear in
> expressions --- it seems much more likely that that would pose a
> parsing conflict after future SQL extensions.

I suspect that steering that ship would be a good idea starting with
deprecation of the old name in 9.6, etc. hs_filter(), perhaps?

Cheers,
David.
--
David Fetter <david(at)fetter(dot)org> http://fetter.org/
Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter
Skype: davidfetter XMPP: david(dot)fetter(at)gmail(dot)com

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate

> On 03 May 2016, at 00:59, David Fetter <david(at)fetter(dot)org> wrote:
>
> On Mon, May 02, 2016 at 01:58:11PM -0400, Tom Lane wrote:
>> I wrote:
>>> I think we'd be better off to rename these to tsvector_delete()
>>> and tsvector_filter() while we still can.
>>
>> ... although I now notice that hstore already exposes a function
>> named delete(), so that ship may have sailed already. But I'm more
>> troubled by filter() anyhow, since that keyword can appear in
>> expressions --- it seems much more likely that that would pose a
>> parsing conflict after future SQL extensions.
>
> I suspect that steering that ship would be a good idea starting with
> deprecation of the old name in 9.6, etc. hs_filter(), perhaps?
>
> Cheers,
> David.

In 9.5 there already were tsvector functions length(), numnode(), strip()

Recent commit added setweight(), delete(), unnest(), tsvector_to_array(), array_to_tsvector(), filter().

Last bunch can be painlessly renamed, for example to ts_setweight, ts_delete, ts_unnest, ts_filter.

The question is what to do with old ones? Leave them as is? Rename to ts_* and create aliases with deprecation warning?

--
Stas Kelvich
Postgres Professional: http://www.postgrespro.com
Russian Postgres Company

Stas Kelvich <s(dot)kelvich(at)postgrespro(dot)ru> writes:
>> On 03 May 2016, at 00:59, David Fetter <david(at)fetter(dot)org> wrote:
>> I suspect that steering that ship would be a good idea starting with
>> deprecation of the old name in 9.6, etc. hs_filter(), perhaps?

> In 9.5 there already were tsvector functions length(), numnode(), strip()

> Recent commit added setweight(), delete(), unnest(), tsvector_to_array(), array_to_tsvector(), filter().

> Last bunch can be painlessly renamed, for example to ts_setweight, ts_delete, ts_unnest, ts_filter.

> The question is what to do with old ones? Leave them as is? Rename to ts_* and create aliases with deprecation warning?

The other ones are not so problematic because they do not conflict with
SQL keywords. It's only delete() and filter() that scare me.

regards, tom lane

> On 04 May 2016, at 16:58, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>
> Stas Kelvich <s(dot)kelvich(at)postgrespro(dot)ru> writes:
>>> On 03 May 2016, at 00:59, David Fetter <david(at)fetter(dot)org> wrote:
>>> I suspect that steering that ship would be a good idea starting with
>>> deprecation of the old name in 9.6, etc. hs_filter(), perhaps?
>
>> In 9.5 there already were tsvector functions length(), numnode(), strip()
>
>> Recent commit added setweight(), delete(), unnest(), tsvector_to_array(), array_to_tsvector(), filter().
>
>> Last bunch can be painlessly renamed, for example to ts_setweight, ts_delete, ts_unnest, ts_filter.
>
>> The question is what to do with old ones? Leave them as is? Rename to ts_* and create aliases with deprecation warning?
>
> The other ones are not so problematic because they do not conflict with
> SQL keywords. It's only delete() and filter() that scare me.
>
> regards, tom lane

Okay, so changed functions to ts_setweight, ts_delete, ts_unnest, ts_filter.

Stas Kelvich <s(dot)kelvich(at)postgrespro(dot)ru> writes:
>> On 04 May 2016, at 16:58, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> The other ones are not so problematic because they do not conflict with
>> SQL keywords. It's only delete() and filter() that scare me.

> Okay, so changed functions to ts_setweight, ts_delete, ts_unnest, ts_filter.

Somehow, I don't think you read what I wrote.

Renaming the pre-existing setweight() function to ts_setweight() is
not going to happen; it's been like that for half a dozen years now.
It would make no sense to call the new variant ts_setweight() while
keeping setweight() for the existing function, either.

I also don't see that much point in ts_unnest(), since unnest()
in our implementation is a function not a keyword. I don't have
a strong opinion about that one, though.

Also, I'd supposed that we'd rename to tsvector_something, since
the same patch also introduced tsvector_to_array() and
array_to_tsvector(). What's the motivation for using ts_ as the
prefix?

regards, tom lane

> On 04 May 2016, at 20:15, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>
> Stas Kelvich <s(dot)kelvich(at)postgrespro(dot)ru> writes:
>>> On 04 May 2016, at 16:58, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>>> The other ones are not so problematic because they do not conflict with
>>> SQL keywords. It's only delete() and filter() that scare me.
>
>> Okay, so changed functions to ts_setweight, ts_delete, ts_unnest, ts_filter.
>
> Somehow, I don't think you read what I wrote.
>
> Renaming the pre-existing setweight() function to ts_setweight() is
> not going to happen; it's been like that for half a dozen years now.
> It would make no sense to call the new variant ts_setweight() while
> keeping setweight() for the existing function, either.

Oh, I accidentally renamed one of the old functions, my mistake.

> I also don't see that much point in ts_unnest(), since unnest()
> in our implementation is a function not a keyword. I don't have
> a strong opinion about that one, though.

Just to keep some level of uniformity in function names. But also i’m
not insisting.

> Also, I'd supposed that we'd rename to tsvector_something, since
> the same patch also introduced tsvector_to_array() and
> array_to_tsvector(). What's the motivation for using ts_ as the
> prefix?

There is already several functions named ts_* (ts_rank, ts_headline, ts_rewrite)
and two named starting from tsvector_* (tsvector_update_trigger, tsvector_update_trigger_column).

Personally I’d prefer ts_ over tsvector_ since it is shorter, and still keeps semantics.

>
> regards, tom lane

--
Stas Kelvich
Postgres Professional: http://www.postgrespro.com
Russian Postgres Company

On 05/05/16 21:20, Stas Kelvich wrote:
>> On 04 May 2016, at 20:15, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>>
>> Stas Kelvich <s(dot)kelvich(at)postgrespro(dot)ru> writes:
>>>> On 04 May 2016, at 16:58, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>>>> The other ones are not so problematic because they do not conflict with
>>>> SQL keywords. It's only delete() and filter() that scare me.
>>> Okay, so changed functions to ts_setweight, ts_delete, ts_unnest, ts_filter.
>> Somehow, I don't think you read what I wrote.
>>
>> Renaming the pre-existing setweight() function to ts_setweight() is
>> not going to happen; it's been like that for half a dozen years now.
>> It would make no sense to call the new variant ts_setweight() while
>> keeping setweight() for the existing function, either.
> Oh, I accidentally renamed one of the old functions, my mistake.
>
>> I also don't see that much point in ts_unnest(), since unnest()
>> in our implementation is a function not a keyword. I don't have
>> a strong opinion about that one, though.
> Just to keep some level of uniformity in function names. But also i’m
> not insisting.
>
>> Also, I'd supposed that we'd rename to tsvector_something, since
>> the same patch also introduced tsvector_to_array() and
>> array_to_tsvector(). What's the motivation for using ts_ as the
>> prefix?
> There is already several functions named ts_* (ts_rank, ts_headline, ts_rewrite)
> and two named starting from tsvector_* (tsvector_update_trigger, tsvector_update_trigger_column).
>
> Personally I’d prefer ts_ over tsvector_ since it is shorter, and still keeps semantics.
>
>> regards, tom lane
>
I've not been involved in doing any tsvector stuff, nor likely to in the
near future - but if i was, I think I'd find simpler to get into if
tsvector specific functions followed a common pattern of naming, like
Stas is suggesting.

Cheers,
Gavin

Stas Kelvich <s(dot)kelvich(at)postgrespro(dot)ru> writes:
>> On 04 May 2016, at 20:15, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> Also, I'd supposed that we'd rename to tsvector_something, since
>> the same patch also introduced tsvector_to_array() and
>> array_to_tsvector(). What's the motivation for using ts_ as the
>> prefix?

> There is already several functions named ts_* (ts_rank, ts_headline, ts_rewrite)
> and two named starting from tsvector_* (tsvector_update_trigger, tsvector_update_trigger_column).

> Personally Id prefer ts_ over tsvector_ since it is shorter, and still keeps semantics.

Yeah, I see we're already a bit inconsistent here. The problem with using
a ts_ prefix, to my mind, is that it offers no option for distinguishing
tsvector from tsquery, should you need to do that. Maybe this isn't a
problem for functions that have tsvector as input.

regards, tom lane

On 06/05/16 07:44, Tom Lane wrote:
> Stas Kelvich <s(dot)kelvich(at)postgrespro(dot)ru> writes:
>>> On 04 May 2016, at 20:15, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>>> Also, I'd supposed that we'd rename to tsvector_something, since
>>> the same patch also introduced tsvector_to_array() and
>>> array_to_tsvector(). What's the motivation for using ts_ as the
>>> prefix?
>> There is already several functions named ts_* (ts_rank, ts_headline, ts_rewrite)
>> and two named starting from tsvector_* (tsvector_update_trigger, tsvector_update_trigger_column).
>> Personally I’d prefer ts_ over tsvector_ since it is shorter, and still keeps semantics.
> Yeah, I see we're already a bit inconsistent here. The problem with using
> a ts_ prefix, to my mind, is that it offers no option for distinguishing
> tsvector from tsquery, should you need to do that. Maybe this isn't a
> problem for functions that have tsvector as input.
>
> regards, tom lane
>
>
use tsv_ and tsq_?

Cheers,
Gavin

> On 06 May 2016, at 00:46, Gavin Flower <GavinFlower(at)archidevsys(dot)co(dot)nz> wrote:
>
> On 06/05/16 07:44, Tom Lane wrote:
>>
>> Yeah, I see we're already a bit inconsistent here. The problem with using
>> a ts_ prefix, to my mind, is that it offers no option for distinguishing
>> tsvector from tsquery, should you need to do that. Maybe this isn't a
>> problem for functions that have tsvector as input.
>>
>> regards, tom lane
>>
>>
> use tsv_ and tsq_?
>
>
> Cheers,
> Gavin
>

That would be a good convention if we were able to easily rename old functions.
But now that will just create another pattern on top of three existing (no prefix, ts_*, tsvector_*).

Stas Kelvich
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

Stas Kelvich <s(dot)kelvich(at)postgrespro(dot)ru> writes:
> On 06 May 2016, at 00:46, Gavin Flower <GavinFlower(at)archidevsys(dot)co(dot)nz> wrote:
>> On 06/05/16 07:44, Tom Lane wrote:
>>> Yeah, I see we're already a bit inconsistent here. The problem with using
>>> a ts_ prefix, to my mind, is that it offers no option for distinguishing
>>> tsvector from tsquery, should you need to do that. Maybe this isn't a
>>> problem for functions that have tsvector as input.

>> use tsv_ and tsq_?

> That would be a good convention if we were able to easily rename old functions.
> But now that will just create another pattern on top of three existing (no prefix, ts_*, tsvector_*).

Yeah :-(. Well, time grows short, so let's go with ts_ for these.
I'll go make it happen.

regards, tom lane