From: | Dean Rasheed <dean(dot)a(dot)rasheed(at)gmail(dot)com> |
---|---|
To: | vignesh C <vignesh21(at)gmail(dot)com> |
Cc: | Fujii Masao <masao(dot)fujii(at)oss(dot)nttdata(dot)com>, Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org>, bt22kawamotok <bt22kawamotok(at)oss(dot)nttdata(dot)com>, Shinya Kato <Shinya11(dot)Kato(at)oss(dot)nttdata(dot)com>, pgsql-hackers(at)postgresql(dot)org |
Subject: | Re: [PATCH]Feature improvement for MERGE tab completion |
Date: | 2023-01-10 12:24:18 |
Message-ID: | CAEZATCUdBsHsbpdu2XasSVTM0bLf5Ag+zg-4raBndPkwaf7esQ@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
On Tue, 3 Jan 2023 at 12:30, vignesh C <vignesh21(at)gmail(dot)com> wrote:
>
> The patch does not apply on top of HEAD as in [1], please post a rebased patch:
>
This is because 0001 has been committed.
Re-uploading 0002, to keep the CF-bot happy.
Reviewing 0002...
I'm not entirely convinced that the PartialMatches() changes are
really necessary. As far as I can see USING followed by ON doesn't
appear anywhere else in the grammar, and the later completions
involving WHEN [NOT] MATCHED are definitely unique to MERGE.
Nonetheless, I guess it's not a bad thing to check that it really is a
MERGE. Also the new matching function might prove useful for other
cases.
Some more detailed code comments:
I find the name PartialMatches() a little off, since "partial" doesn't
really accurately describe what it's doing. HeadMatches() and
TailMatches() are also partial matches (matching the head and tail
parts). So perhaps MidMatches() would be a better name.
I also found the comment description of PartialMatchesImpl() misleading:
/*
* Implementation of PartialMatches and PartialMatchesCS macros: do parts of
* the words in previous_words match the variadic arguments?
*/
I think a more accurate description would be:
/*
* Implementation of MidMatches and MidMatchesCS macros: do any N consecutive
* words in previous_words match the variadic arguments?
*/
Similarly, instead of:
/* Match N words on the line partially, case-insensitively. */
how about:
/* Match N consecutive words anywhere on the line, case-insensitively. */
In PartialMatchesImpl()'s main loop:
if (previous_words_count - startpos < narg)
{
va_end(args);
return false;
}
couldn't that just be built into the loop's termination clause (i.e.,
loop while startpos <= previous_words_count - narg)?
For the first block of changes using the new function:
else if (PartialMatches("MERGE", "INTO", MatchAny, "USING") ||
PartialMatches("MERGE", "INTO", MatchAny, "AS", MatchAny,
"USING") ||
PartialMatches("MERGE", "INTO", MatchAny, MatchAny, "USING"))
{
/* Complete MERGE INTO ... ON with target table attributes */
if (TailMatches("INTO", MatchAny, "USING", MatchAny, "ON"))
COMPLETE_WITH_ATTR(prev4_wd);
else if (TailMatches("INTO", MatchAny, "AS", MatchAny,
"USING", MatchAny, "AS", MatchAny, "ON"))
COMPLETE_WITH_ATTR(prev8_wd);
else if (TailMatches("INTO", MatchAny, MatchAny, "USING",
MatchAny, MatchAny, "ON"))
COMPLETE_WITH_ATTR(prev6_wd);
wouldn't it be simpler to just include "MERGE" in the TailMatches()
arguments, and leave these 3 cases outside the new code block. I.e.:
/* Complete MERGE INTO ... ON with target table attributes */
else if (TailMatches("MERGE", "INTO", MatchAny, "USING", MatchAny, "ON"))
COMPLETE_WITH_ATTR(prev4_wd);
else if (TailMatches("MERGE", "INTO", MatchAny, "AS", MatchAny,
"USING", MatchAny, "AS", MatchAny, "ON"))
COMPLETE_WITH_ATTR(prev8_wd);
else if (TailMatches("MERGE", "INTO", MatchAny, MatchAny, "USING",
MatchAny, MatchAny, "ON"))
COMPLETE_WITH_ATTR(prev6_wd);
Regards,
Dean
Attachment | Content-Type | Size |
---|---|---|
v9-0002-psql-Add-PartialMatches-macro-for-better-tab-complet.patch | text/x-patch | 7.5 KB |
From | Date | Subject | |
---|---|---|---|
Next Message | Nazir Bilal Yavuz | 2023-01-10 12:37:17 | Re: Use windows VMs instead of windows containers on the CI |
Previous Message | Jakub Wartak | 2023-01-10 12:23:19 | Re: psql's FETCH_COUNT (cursor) is not being respected for CTEs |