> On Nov 18, 2025, at 13:03, Chao Li <[email protected]> wrote:
>
> Hi Tatsuo-san,
>
> I just reviewed 0006 docs changes and 0001. I plan to slowly review the
> patch, maybe one commit a day.
>
>> On Nov 18, 2025, at 10:33, Tatsuo Ishii <[email protected]> wrote:
>>
>> Attached are the v35 patches for Row pattern recognition. In v34-0001
>> gram.y patch, %type for RPR were misplaced. v35-0001 fixes this. Other
>> patches are not changed.
>>
>> Best regards,
>> --
>> Tatsuo Ishii
>> SRA OSS K.K.
>> English: http://www.sraoss.co.jp/index_en/
>> Japanese:http://www.sraoss.co.jp
>> <v35-0001-Row-pattern-recognition-patch-for-raw-parser.patch><v35-0002-Row-pattern-recognition-patch-parse-analysis.patch><v35-0003-Row-pattern-recognition-patch-rewriter.patch><v35-0004-Row-pattern-recognition-patch-planner.patch><v35-0005-Row-pattern-recognition-patch-executor.patch><v35-0006-Row-pattern-recognition-patch-docs.patch><v35-0007-Row-pattern-recognition-patch-tests.patch><v35-0008-Row-pattern-recognition-patch-typedefs.list.patch>
>
> I got a few comments, maybe just questions:
>
> 1 - 0001 - kwlist.h
> ```
> +PG_KEYWORD("define", DEFINE, RESERVED_KEYWORD, BARE_LABEL)
> ```
>
> Why do we add “define” as a reserved keyword? From the SQL example you put in
> 0006:
> ```
> <programlisting>
> SELECT company, tdate, price,
> first_value(price) OVER w,
> max(price) OVER w,
> count(price) OVER w
> FROM stock
> WINDOW w AS (
> PARTITION BY company
> ORDER BY tdate
> ROWS BETWEEN CURRENT ROW AND UNBOUNDED FOLLOWING
> AFTER MATCH SKIP PAST LAST ROW
> INITIAL
> PATTERN (LOWPRICE UP+ DOWN+)
> DEFINE
> LOWPRICE AS price <= 100,
> UP AS price > PREV(price),
> DOWN AS price < PREV(price)
> );
> </programlisting>
> ```
>
> PARTITION is at the same level as DEFINE, but it’s not defined as a reserved
> keyword:
> ```
> PG_KEYWORD("partition", PARTITION, UNRESERVED_KEYWORD, BARE_LABEL)
> ```
>
> Even in this patch,”initial”,”past”, “pattern” and “seek” are defined as
> unreserved, why?
>
> So I just want to clarify.
>
> 2 - 0001 - gram.y
> ```
> opt_row_pattern_initial_or_seek:
> INITIAL { $$ = true; }
> | SEEK
> {
> ereport(ERROR,
> (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
> errmsg("SEEK is not supported"),
> errhint("Use INITIAL instead."),
> parser_errposition(@1)));
> }
> | /*EMPTY*/ { $$ = true; }
> ;
> ```
>
> As SEEK is specially listed here, I guess it might be supported in future. If
> that is true, would it be better to defer the semantic check to later parse
> phase, which would future work easier.
>
> 3 - 0001 - parsenodes.h
> ```
> +/*
> + * RowPatternCommonSyntax - raw representation of row pattern common syntax
> + *
> + */
> +typedef struct RPCommonSyntax
> +{
> + NodeTag type;
> + RPSkipTo rpSkipTo; /* Row Pattern AFTER MATCH SKIP type */
> + bool initial; /* true if <row pattern initial or seek> is
> + * initial */
> + List *rpPatterns; /* PATTERN variables (list of A_Expr) */
> + List *rpDefs; /* row pattern definitions clause (list of
> + * ResTarget) */
> +} RPCommonSyntax;
> +
> /*
> * WindowDef - raw representation of WINDOW and OVER clauses
> *
> @@ -593,6 +618,7 @@ typedef struct WindowDef
> char *refname; /* referenced window name, if any */
> List *partitionClause; /* PARTITION BY expression list */
> List *orderClause; /* ORDER BY (list of SortBy) */
> + RPCommonSyntax *rpCommonSyntax; /* row pattern common syntax */
> ```
>
> RP fields are directly defined in WindowClause, then why do we need a wrapper
> of RPCommonSyntax? Can we directly define RP fields in WindowRef as well?
>
4 - 0001 - parsenodes.h
```
+ /* Row Pattern AFTER MACH SKIP clause */
```
Typo: MACH -> MATCH
Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/
