Hi Tatsuo,
Thank you very much for taking the time to start the review. I will reply
in step, one patch at a time, from 0001 up to wherever your review has
reached -- so far that is 0001..0004 -- and I will keep following along as
you continue.
For convenience I have attached just the four patches you have reviewed,
keeping their original v50 names but renamed to nocfbot-* so that cfbot does
not pick them up:
nocfbot-0001 Remove blank-line changes unrelated to row pattern recognition
nocfbot-0002 Remove unnecessary includes from the row pattern recognition patch
nocfbot-0003 Recognize row pattern navigation operations by name in DEFINE
nocfbot-0004 Use a dedicated ExprContext for RPR DEFINE clause evaluation
0001, 0002 and 0004 are unchanged from what you reviewed; I include them
only so this reply stands on its own for the reviewed range. 0003 folds in
the three points you raised below.
> 0001 and 0002 look good to me.
Thank you for confirming -- I have left them as they are.
>> v50-0003 Recognize row pattern navigation operations by name in DEFINE
>
> + if (argnames != NIL)
> + ereport(ERROR,
> + (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
> + errmsg("row pattern navigation operations cannot use named arguments"),
> + parser_errposition(pstate, location)));
>
> Shouldn't the error code for the named arguments case be
> ERRCODE_SYNTAX_ERROR? As far as I know the SQL standard does not allow
> to use named arguments with row pattern navigation operations. So it's
> not a required feature.
You are right, thank you. The standard does not provide for named arguments
here at all, so this is a syntax violation rather than an unimplemented
feature. I have changed the errcode to ERRCODE_SYNTAX_ERROR, which also
makes it consistent with the VARIADIC and argument-count checks alongside
it.
> Also errmsg is different from surroundings. Probably "cannot use named
> arguments with row pattern navigation function %s" is better?
Thank you for the suggested wording; I have adopted it as is:
errmsg("cannot use named arguments with row pattern navigation function %s",
navname)
It now reads in the same "cannot use X with row pattern navigation
function %s" shape as the VARIADIC message just above it. I have also
updated the expected output in rpr.sql to match (ERROR: cannot use named
arguments with row pattern navigation function PREV).
> + * * offset_arg / compound_offset_arg must not contain column refs
> + * or nested navigation operations
>
> Needs '*' in front of "or nested....".
Thank you for catching this. The last line is a continuation, but the '*'
bullets ran together with the comment's own '*' and hid that. I switched
the bullets to '-' to match the sibling rule list in this file, so the
continuation now reads clearly:
* ... enforces, for each outer RPRNavExpr (per ISO/IEC 19075-5 5.6.4
* nesting rules):
* - arg must contain at least one column reference
* - PREV/NEXT wrapping FIRST/LAST flattens to a compound kind
* - Other nestings are rejected (FIRST(PREV()), PREV(PREV()), ...)
* - offset_arg / compound_offset_arg must not contain column refs
* or nested navigation operations
I hope this addresses what you had in mind; please let me know if you would
prefer a different form.
>> v50-0004 Use a dedicated ExprContext for RPR DEFINE clause evaluation
>
> Looks good to me.
Thank you for confirming -- left as is.
> Will continue reviewes tomorrow.
Thank you, I look forward to it. I will respond to each further patch as
your review reaches it, in the same one-at-a-time fashion.
Best regards,
Henson
