On Mon, Oct 6, 2025 at 5:03 PM Masahiko Sawada <[email protected]> wrote: > > On Fri, Oct 3, 2025 at 2:06 PM Masahiko Sawada <[email protected]> wrote: > > > > On Thu, Sep 25, 2025 at 6:16 PM Yugo Nagata <[email protected]> wrote: > > > > > > On Thu, 25 Sep 2025 14:31:18 -0700 > > > Masahiko Sawada <[email protected]> wrote: > > > > > > > > > I fixed it to use 'generator'. > > > > > > > > LGTM. I've pushed the 0001 patch. > > > > > > Thank you! > > > > > > > > > > --- > > > > > > > - /* Complete COPY <sth> FROM <sth> */ > > > > > > > - else if (Matches("COPY|\\copy", MatchAny, "FROM", MatchAny)) > > > > > > > + /* Complete COPY <sth> FROM [PROGRAM] <sth> */ > > > > > > > + else if (Matches("COPY|\\copy", MatchAny, "FROM", > > > > > > > MatchAnyExcept("PROGRAM")) || > > > > > > > + Matches("COPY|\\copy", MatchAny, "FROM", "PROGRAM", > > > > > > > MatchAny)) > > > > > > > > > > > > > > I see this kind of conversion many places in the patch; convert > > > > > > > one > > > > > > > condition with MatchAny into two conditions with > > > > > > > MatchAnyExcept("PROGRAM") and '"PROGRAM", MatchAny'. How about > > > > > > > simplifying it using MatchAnyN. For example, > > > > > > > > > > > > > > else if (Matches("COPY|\\copy", MatchAny, "FROM", MatchAny, > > > > > > > MatchAnyN)) > > > > > > > > > > > > We could simplify this by using MatchAnyN, but doing so would cause > > > > > > "WITH (" > > > > > > or "WHERE" to be suggested after "WITH (...)", even though that is > > > > > > not allowed > > > > > > by the syntax. This could be misleading for users, so I wonder > > > > > > whether it is > > > > > > worth adding a bit of complexity to prevent possible confusion. > > > > > > > > > > There was a mistake in the previous statement: "WHERE" appearing > > > > > after "WITH (...)" > > > > > is actually correct. However, this also results in "WITH" being > > > > > suggested after > > > > > "WHERE", which is not permitted by the syntax. > > > > > > > > True. How about other places? That is, where we check the completion > > > > after "WITH (". For example: > > > > > > > > - /* Complete COPY <sth> TO filename WITH ( */ > > > > - else if (Matches("COPY|\\copy", MatchAny, "TO", MatchAny, "WITH", > > > > "(")) > > > > + /* Complete COPY <sth> TO [PROGRAM] filename WITH ( */ > > > > + else if (Matches("COPY|\\copy", MatchAny, "TO", > > > > MatchAnyExcept("PROGRAM"), "WITH", "(") || > > > > + Matches("COPY|\\copy", MatchAny, "TO", "PROGRAM", > > > > MatchAny, "WITH", "(")) > > > > > > > > Does it make sense to replace these two lines with the following one > > > > line? > > > > > > > > else if (Matches("COPY|\\copy", MatchAny, "TO", MatchAny, MatchAnyN, > > > > "WITH", "(")) > > > > > > That works for other places where options are suggested after "WITH (" and > > > "WHERE" is suggested after "WITH (*)". > > > > > > I've attached updated patches using MatchAnyN following your suggestion. > > > > Thank you for updating the patch! > > > > After reviewing both the v6 and v7 patches, I realize that your > > original approach in v6 is actually better than what I suggested. > > While it requires more lines of code, it provides more precise > > completions. Additionally, since most of these extra lines are removed > > by the next patch (v6-0003), the code size isn't really an issue. > > Would it be possible to withdraw my previous comments and proceed with > > the v6 approach? I apologize for the back-and-forth on this. > > > > I have two review comments about the complete_from_files() function: > > > > + * This function wraps _complete_from_files() so that both literal keywords > > + * and filenames can be included in the completion results. > > + * > > + * If completion_charpp is set to a null-terminated array of literal > > keywords, > > + * those keywords are added to the completion results alongside filenames, > > + * as long as they case-insensitively match the current input. > > > > How about rephrasing the comments to the following? > > > > /* > > * This function returns in order one of a fixed, NULL pointer terminated > > list > > * of string that matches file names or optionally specified list of > > keywords. > > * > > * If completion_charpp is set to a null-terminated array of literal > > keywords, > > * those keywords are added to the completion results alongside filenames if > > * they case-insensitively match the current input. > > */ > > > > --- > > + /* Return a filename that matches */ > > + if (!files_done && (result = _complete_from_files(text, state))) > > + return result; > > + else if (!completion_charpp) > > + return NULL; > > + else > > + files_done = true; > > > > It works but it's odd a bit that we don't set files_done to true if > > completion_charpp is not NULL. I think it becomes more readable if we > > could set files_done to true if _complete_from_files() doesn't return > > a string and proceed with the hard-wired keywords. > > > > The attached patch that can be applied on top of v6-0002 patch, > > implements my suggestions and includes pgindent fixes. Please review > > these changes. > > I believe we can split the first patch into two patches: one adds > support for STDIN/STDOUT with COMPLETE_WIHT_FILES_PLUS() and another > one adds support for COPY syntaxes using the PROGRAM clause. I've > attached the reorganized patch set and made cosmetic changes to the > 0003 patch (i.e., improving COPY option list). What do you think?
Pushed the first two patches. As for the remaining patch that adds tab completion for COPY option lists, I think it would be a good idea to add tab completion for other options too such as HEADER and FREEZE. Regards, -- Masahiko Sawada Amazon Web Services: https://aws.amazon.com
