Thread: Tab completion for AT TIME ZONE

Hi hackers,

A while back we added support for completing time zone names after SET
TIMEZONE, but we failed to do the same for the AT TIME ZONE operator.
Here's a trivial patch for that.

- ilmari

From 57138e851552a99174d00a0e48ce55ca3170df75 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Dagfinn=20Ilmari=20Manns=C3=A5ker?= <ilmari@ilmari.org>
Date: Wed, 29 Mar 2023 11:16:01 +0100
Subject: [PATCH] psql: tab-complete time zones after AT TIME ZONE

Commit 7fa3db367986160dee2b2b0bbfb61e1a51d486fd added support for
completing time zone names, use that after the AT TIME ZONE operator.
---
 src/bin/psql/tab-complete.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/src/bin/psql/tab-complete.c b/src/bin/psql/tab-complete.c
index e38a49e8bd..9bb8ae0dc3 100644
--- a/src/bin/psql/tab-complete.c
+++ b/src/bin/psql/tab-complete.c
@@ -4655,6 +4655,10 @@ psql_completion(const char *text, int start, int end)
     else if (TailMatches("JOIN"))
         COMPLETE_WITH_SCHEMA_QUERY(Query_for_list_of_selectables);
 
+/* ... AT TIME ZONE ... */
+    else if (TailMatches("AT", "TIME", "ZONE"))
+        COMPLETE_WITH_TIMEZONE_NAME();
+
 /* Backslash commands */
 /* TODO:  \dc \dd \dl */
     else if (TailMatchesCS("\\?"))
-- 
2.39.2

Dagfinn Ilmari Mannsåker <ilmari@ilmari.org> writes:

> Hi hackers,
>
> A while back we added support for completing time zone names after SET
> TIMEZONE, but we failed to do the same for the AT TIME ZONE operator.
> Here's a trivial patch for that.

Added to the 2023-07 commitfest:

https://commitfest.postgresql.org/43/4274/

- ilmari

Hi,

Is this supposed to provide tab completion for the AT TIME ZONE operator 
like in this query?

SELECT '2023-04-14 08:00:00' AT TIME ZONE 'Europe/Lisbon';

The patch applied cleanly but I'm afraid I cannot reproduce the intended 
behaviour:

postgres=# SELECT '2023-04-14 08:00:00' AT<tab>

postgres=# SELECT '2023-04-14 08:00:00' AT T<tab>

postgres=# SELECT '2023-04-14 08:00:00' AT TIME Z<tab>

Perhaps I'm testing it in the wrong place?

Best, Jim

On 12.04.23 19:53, Dagfinn Ilmari Mannsåker wrote:
> Dagfinn Ilmari Mannsåker <ilmari@ilmari.org> writes:
>
>> Hi hackers,
>>
>> A while back we added support for completing time zone names after SET
>> TIMEZONE, but we failed to do the same for the AT TIME ZONE operator.
>> Here's a trivial patch for that.
> Added to the 2023-07 commitfest:
>
> https://commitfest.postgresql.org/43/4274/
>
> - ilmari
>
>

Hi Jim,

Thanks for having a look at my patch, but please don't top post on
PostgreSQL lists.

Jim Jones <jim.jones@uni-muenster.de> writes:

> Hi,
>
> On 12.04.23 19:53, Dagfinn Ilmari Mannsåker wrote:
>> Dagfinn Ilmari Mannsåker <ilmari@ilmari.org> writes:
>>
>>> Hi hackers,
>>>
>>> A while back we added support for completing time zone names after SET
>>> TIMEZONE, but we failed to do the same for the AT TIME ZONE operator.
>>> Here's a trivial patch for that.
>>
>
> Is this supposed to provide tab completion for the AT TIME ZONE operator
> like in this query?
>
> SELECT '2023-04-14 08:00:00' AT TIME ZONE 'Europe/Lisbon';
>
> The patch applied cleanly but I'm afraid I cannot reproduce the intended
> behaviour:
>
> postgres=# SELECT '2023-04-14 08:00:00' AT<tab>
>
> postgres=# SELECT '2023-04-14 08:00:00' AT T<tab>
>
> postgres=# SELECT '2023-04-14 08:00:00' AT TIME Z<tab>
>
> Perhaps I'm testing it in the wrong place?

It doesn't tab complete the AT TIME ZONE operator itself, just the
timezone name after it, so this sholud work:

    # SELECT now() AT TIME ZONE <tab><tab>

or

    # SELECT now() AT TIME ZONE am<tab>


However, looking more closely at the grammar, the word AT only occurs in
AT TIME ZONE, so we could complete the operator itself as well.  Updated
patch attatched.

> Best, Jim

- ilmari

From 365844db04d27c5bcd1edf8a9d0d44353bc34631 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Dagfinn=20Ilmari=20Manns=C3=A5ker?= <ilmari@ilmari.org>
Date: Wed, 29 Mar 2023 11:16:01 +0100
Subject: [PATCH v2] psql: tab completion for AT TIME ZONE

Commit 7fa3db367986160dee2b2b0bbfb61e1a51d486fd added support for
completing time zone names, use that after the AT TIME ZONE operator.

Also complete the operator itself, since it's the only thing in the
grammar starting with AT.
---
 src/bin/psql/tab-complete.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/src/bin/psql/tab-complete.c b/src/bin/psql/tab-complete.c
index 5825b2a195..e3870c68e9 100644
--- a/src/bin/psql/tab-complete.c
+++ b/src/bin/psql/tab-complete.c
@@ -4657,6 +4657,14 @@ psql_completion(const char *text, int start, int end)
     else if (TailMatches("JOIN"))
         COMPLETE_WITH_SCHEMA_QUERY(Query_for_list_of_selectables);
 
+/* ... AT TIME ZONE ... */
+    else if (TailMatches("AT"))
+        COMPLETE_WITH("TIME ZONE");
+    else if (TailMatches("AT", "TIME"))
+        COMPLETE_WITH("ZONE");
+    else if (TailMatches("AT", "TIME", "ZONE"))
+        COMPLETE_WITH_TIMEZONE_NAME();
+
 /* Backslash commands */
 /* TODO:  \dc \dd \dl */
     else if (TailMatchesCS("\\?"))
-- 
2.39.2

On 14.04.23 11:29, Dagfinn Ilmari Mannsåker wrote:
> It doesn't tab complete the AT TIME ZONE operator itself, just the
> timezone name after it, so this sholud work:
>
>      # SELECT now() AT TIME ZONE <tab><tab>
>
> or
>
>      # SELECT now() AT TIME ZONE am<tab>
>
>
> However, looking more closely at the grammar, the word AT only occurs in
> AT TIME ZONE, so we could complete the operator itself as well.  Updated
> patch attatched.
>
>> Best, Jim
> - ilmari

Got it.

In that case, everything seems to work just fine:

postgres=# SELECT now() AT <tab>

.. autocompletes TIME ZONE :

postgres=# SELECT now() AT TIME ZONE


postgres=# SELECT now() AT TIME ZONE <tab><tab>
Display all 598 possibilities? (y or n)


postgres=# SELECT now() AT TIME ZONE 'Europe/Is<tab><tab>
Europe/Isle_of_Man  Europe/Istanbul


also neglecting the opening single quotes ...

postgres=# SELECT now() AT TIME ZONE Europe/Is<tab>

... autocompletes it after <tab>:

postgres=# SELECT now() AT TIME ZONE 'Europe/Is


The patch applies cleanly and it does what it is proposing. - and it's 
IMHO a very nice addition.

I've marked the CF entry as "Ready for Committer".

Jim

On Fri, Apr 14, 2023 at 12:05:25PM +0200, Jim Jones wrote:
> The patch applies cleanly and it does what it is proposing. - and it's IMHO
> a very nice addition.
>
> I've marked the CF entry as "Ready for Committer".

+/* ... AT TIME ZONE ... */
+    else if (TailMatches("AT"))
+        COMPLETE_WITH("TIME ZONE");
+    else if (TailMatches("AT", "TIME"))
+        COMPLETE_WITH("ZONE");
+    else if (TailMatches("AT", "TIME", "ZONE"))
+        COMPLETE_WITH_TIMEZONE_NAME();

This style will for the completion of timezone values even if "AT" is
the first word of a query.  Shouldn't this be more selective by making
sure that we are at least in the context of a SELECT query?
--
Michael

Michael Paquier <michael@paquier.xyz> writes:

> On Fri, Apr 14, 2023 at 12:05:25PM +0200, Jim Jones wrote:
>> The patch applies cleanly and it does what it is proposing. - and it's IMHO
>> a very nice addition.
>> 
>> I've marked the CF entry as "Ready for Committer".
>
> +/* ... AT TIME ZONE ... */
> +    else if (TailMatches("AT"))
> +        COMPLETE_WITH("TIME ZONE");
> +    else if (TailMatches("AT", "TIME"))
> +        COMPLETE_WITH("ZONE");
> +    else if (TailMatches("AT", "TIME", "ZONE"))
> +        COMPLETE_WITH_TIMEZONE_NAME();
>
> This style will for the completion of timezone values even if "AT" is
> the first word of a query.  Shouldn't this be more selective by making
> sure that we are at least in the context of a SELECT query?

It's valid anywhere an expression is, which is a lot more places than
just SELECT queries.  Off the top of my head I can think of WITH,
INSERT, UPDATE, VALUES, CALL, CREATE TABLE, CREATE INDEX.

As I mentioned upthread, the only place in the grammar where the word AT
occurs is in AT TIME ZONE, so there's no ambiguity.  Also, it doesn't
complete time zone names after AT, it completes the literal words TIME
ZONE, and you have to then hit tab again to get a list of time zones.
If we (or the SQL committee) were to invent more operators that start
with the word AT, we can add those to the first if clause above and
complete with the appropriate values after each one separately.

- ilmari

On 10/12/23 10:27, Dagfinn Ilmari Mannsåker wrote:
> Michael Paquier <michael@paquier.xyz> writes:
> 
>> On Fri, Apr 14, 2023 at 12:05:25PM +0200, Jim Jones wrote:
>>> The patch applies cleanly and it does what it is proposing. - and it's IMHO
>>> a very nice addition.
>>>
>>> I've marked the CF entry as "Ready for Committer".
>>
>> +/* ... AT TIME ZONE ... */
>> +    else if (TailMatches("AT"))
>> +        COMPLETE_WITH("TIME ZONE");
>> +    else if (TailMatches("AT", "TIME"))
>> +        COMPLETE_WITH("ZONE");
>> +    else if (TailMatches("AT", "TIME", "ZONE"))
>> +        COMPLETE_WITH_TIMEZONE_NAME();
>>
>> This style will for the completion of timezone values even if "AT" is
>> the first word of a query.  Shouldn't this be more selective by making
>> sure that we are at least in the context of a SELECT query?
> 
> It's valid anywhere an expression is, which is a lot more places than
> just SELECT queries.  Off the top of my head I can think of WITH,
> INSERT, UPDATE, VALUES, CALL, CREATE TABLE, CREATE INDEX.
> 
> As I mentioned upthread, the only place in the grammar where the word AT
> occurs is in AT TIME ZONE, so there's no ambiguity.  Also, it doesn't
> complete time zone names after AT, it completes the literal words TIME
> ZONE, and you have to then hit tab again to get a list of time zones.
> If we (or the SQL committee) were to invent more operators that start
> with the word AT, we can add those to the first if clause above and
> complete with the appropriate values after each one separately.

Speaking of this...

The SQL committee already has another operator starting with AT which is 
AT LOCAL.  I am implementing it in 
https://commitfest.postgresql.org/45/4343/ where I humbly admit that I 
did not think of psql tab completion at all.

These two patches are co-dependent and whichever goes in first the other 
will need to be adjusted accordingly.
-- 
Vik Fearing

On Fri, Oct 13, 2023 at 03:07:25AM +0200, Vik Fearing wrote:
> The SQL committee already has another operator starting with AT which is AT
> LOCAL.

The other patch was the reason why I looked at this one.  At the end,
I've made peace with Dagfinn's argument two messages ago, and applied
the patch after adding LOCAL to the keywords, but after also removing
the completion for "ZONE" after typing "AT TIME" because AT would be
completed by "TIME ZONE".

> I am implementing it in https://commitfest.postgresql.org/45/4343/
> where I humbly admit that I did not think of psql tab completion at all.

psql completion is always nice to have but not really mandatory IMO,
so leaving it out if one does not want to implement it is fine by me
to not complicate a patch.  Completion could always be tackled on top
of any feature related to it that got committed.
--
Michael

On 10/13/23 06:31, Michael Paquier wrote:
> On Fri, Oct 13, 2023 at 03:07:25AM +0200, Vik Fearing wrote:
>> The SQL committee already has another operator starting with AT which is AT
>> LOCAL.
> 
> The other patch was the reason why I looked at this one. 


Thank you for updating and committing this patch!


> but after also removing
> the completion for "ZONE" after typing "AT TIME" because AT would be
> completed by "TIME ZONE".


Why?  The user can tab at any point.
-- 
Vik Fearing

On Fri, Oct 13, 2023 at 08:01:08AM +0200, Vik Fearing wrote:
> On 10/13/23 06:31, Michael Paquier wrote:
>> but after also removing
>> the completion for "ZONE" after typing "AT TIME" because AT would be
>> completed by "TIME ZONE".
>
> Why?  The user can tab at any point.

IMO this leads to unnecessary bloat in tab-complete.c because we
finish with the full completion as long as "TIME" is not fully typed.
--
Michael