Re: [PATCH]Feature improvement for MERGE tab completion

On Tue, 3 Jan 2023 at 12:30, vignesh C <vignesh21@gmail.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