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? Best regards, -- Chao Li (Evan) HighGo Software Co., Ltd. https://www.highgo.com/
