Re: [PATCH]Feature improvement for MERGE tab completion

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