Hi Chao,
>> On Nov 18, 2025, at 19:19, Vik Fearing <[email protected]> wrote:
>>
>>
>> Because of position. Without making DEFINE a reserved keyword, how do you
>> know that it isn't another variable in the PATTERN clause?
>>
>
> Ah, thanks for the clarification. Now I got it.
>
> I’m continue to review 0002.
Thanks for the review!
> 5 - 0002 - parse_clause.c
> ```
> + ereport(ERROR,
> + (errcode(ERRCODE_SYNTAX_ERROR),
> + errmsg("FRAME must start at current row when
> row patttern recognition is used")));
> ```
>
> Nit typo: patttern -> pattern
Will fix (this involves regression test change because this changes
the ERROR messages in the expected file).
> 6 - 0002 - parse_clause.c
> ```
> + /* DEFINE variable name initials */
> + static char *defineVariableInitials = "abcdefghijklmnopqrstuvwxyz”;
> ```
>
> This string can also be const, so “static const char *”
Agreed. Will fix.
> 7 - 0002 - parse_clause.c
> ```
> + /*
> + * Create list of row pattern DEFINE variable name's initial. We assign
> + * [a-z] to them (up to 26 variable names are allowed).
> + */
> + restargets = NIL;
> + i = 0;
> + initialLen = strlen(defineVariableInitials);
> +
> + foreach(lc, windef->rpCommonSyntax->rpDefs)
> + {
> + char initial[2];
> +
> + restarget = (ResTarget *) lfirst(lc);
> + name = restarget->name;
> ```
>
> Looks like “name” is not used inside “foreach”.
Oops. Will fix.
> 8 - 0002 - parse_clause.c
> ```
> + /*
> + * Create list of row pattern DEFINE variable name's initial. We assign
> + * [a-z] to them (up to 26 variable names are allowed).
> + */
> + restargets = NIL;
> + i = 0;
> + initialLen = strlen(defineVariableInitials);
> +
> + foreach(lc, windef->rpCommonSyntax->rpDefs)
> + {
> + char initial[2];
> ```
>
> I guess this “foreach” for build initial list can be combined into the
> previous foreach loop of checking duplication. If an def has no dup, then
> assign an initial to it.
You are right. Will change.
> 9 - 0002 - parse_clause.c
> ```
> +static void
> +transformPatternClause(ParseState *pstate, WindowClause *wc,
> + WindowDef *windef)
> +{
> + ListCell *lc;
> +
> + /*
> + * Row Pattern Common Syntax clause exists?
> + */
> + if (windef->rpCommonSyntax == NULL)
> + return;
> ```
>
> Checking (windef->rpCommonSyntax == NULL) seems a duplicate, because
> transformPatternClause() is only called by transformRPR(), and transformRPR()
> has done the check.
Yeah. transformDefineClause() already does the similar check using
Assert. What about using Assert in transPatternClause() as well?
Assert(windef->rpCommonSyntax != NULL);
Best regards,
--
Tatsuo Ishii
SRA OSS K.K.
English: http://www.sraoss.co.jp/index_en/
Japanese:http://www.sraoss.co.jp
