>> I just finished reviewing 0007 and 0008. This patch set really demonstrates the full lifecycle of adding a SQL
feature,from changing the syntax in gram.y all the way down to the executor, including tests and docs. I learned a lot
fromit. Thanks!
>
> You are welcome!
>
>> 23 - 0007
>>
>> As you mentioned earlier, pattern regular expression support *, + and ?, but I don’t see ? is tested.
>
> Good catch. I will add the test case. In the mean time I found a bug
> with gram.y part which handles '?' quantifier. gram.y can be
> successfully compiled but there's no way to put '?' quantifier in a
> SQL statement. We cannot write directly "ColId '?'" like other
> quantifier '*' and '+' in the grammer because '?' is not a "self"
> token. So we let '?' matches Op first then check if it's '?' or
> not.
>
> | ColId Op
> {
> if (strcmp("?", $2))
> ereport(ERROR,
> errcode(ERRCODE_SYNTAX_ERROR),
> errmsg("unsupported quantifier"),
> parser_errposition(@2));
>
> $$ = (Node *) makeSimpleA_Expr(AEXPR_OP, "?", (Node *)makeString($1), NULL, @1);
> }
>
> Another bug was with executor (nodeWindowAgg.c). The code to check
> greedy quantifiers missed the case '?'.
>
>> 24 - 0007
>>
>> I don’t see negative tests for unsupported quantifiers, like PATTERN (A+?).
>
> I will add such test cases.
>
>> 25 - 0007
>> ```
>> +-- basic test with none greedy pattern
>> ```
>>
>> Typo: “none greedy” -> non-greedy
>
> Will fix.
>
>> No comment for 0008.
>
> Thanks again for the review. I will post v35 patch set soon. Attached
> is the diff from v34.
Attached are the v35 patches for Row pattern recognition. Chao Li
reviewed v34 thoroughly. Thank you! v35 reflects the review
comments. Major differences from v34 include:
- Make "DEFINE" an unreserved keyword. Previously it was a reserved keyword.
- Refactor transformDefineClause() to make two foreach loops into single foreach loop.
- Make '?' quantifier in PATTERN work as advertised. Test for '?' quantifier is added too.
- Unsupported quantifier test added.
- Fix get_rule_define().
- Fix memory leak related to regcomp.
- Move regcomp compiled result cache from static data to WindowAggState.
- Fix several typos.
Best regards,
--
Tatsuo Ishii
SRA OSS K.K.
English: http://www.sraoss.co.jp/index_en/
Japanese:http://www.sraoss.co.jp
