Hi jian,

Thanks for the follow-up review patches. Once v48 is posted I'll base the
work on it and submit the accepted ones in v49. Reasoning per patch below,
with the disposition summarized at the end.

> v47-0001 Replace some "AST" to "clause".

Agreed on dropping "AST" -- it isn't our house term; "parse tree" is the
idiom across the tree, and "AST" barely shows up outside these RPR files
(just a jsonpath comment). One tweak: where the text names the data
structure (the README diagram and III-2 header, the parsenodes.h comments
on rpPattern), I'd use "PATTERN parse tree" rather than "clause", since
"clause" loses the tree-vs-flat-NFA contrast. Plain prose that just means
the SQL clause -> "clause", as you have it.

> v47-0002 group Op and trailing ALT into one IF condition, this might
> improve readability.

Here I'd lean toward keeping the current form. The complexity here comes
from the lexer (regex operators tokenized by the SQL operator lexer), and
grouping the branches doesn't remove it -- it just layers more cleverness
on top. I'd rather not add code complexity onto the lexer complexity; the
flat one-token-one-branch form stays easy to scan. A real simplification
would be lexer-side, out of scope here, so I'd leave 0002 as-is unless you
feel strongly.

> v47-0003 validate_rpr_define_volatility ... is removed.
> contain_volatile_functions() already covers both volatile FuncExpr
> callees and NextValueExpr ... The trade-off is that we lose the error
> cursor position, but that seems better than maintaining extra code.

You're right the detection is equivalent. But this restructures the
parse-side define_walker, which the upcoming PREV/NEXT name-binding fix
also reworks -- it even removes the spot where this check incidentally
backstops a nav mis-binding. I'd rather make the simplify-vs-keep call
against that final walker shape than churn it twice, so I'll keep the
current planner-side check for now and revisit once that fix lands.

> v47-0004 validateRPRPatternVarCount() ... the `rpDefs != NULL`
> sentinel ... is awkward. That cross-check only needs to run once;
> better expressed in the caller, transformDefineClause().

Agreed. The cross-check is a one-shot, list-level test -- a different kind
of thing from the recursive per-node count, and not really what
validateRPRPatternVarCount is for (validating the pattern var count).
Moving it to the caller runs it once by construction and leaves the
function true to its name, a pure count. Rejection behavior is unchanged.

> v47-0005 ... The more intuitive order should be: transformExpr ->
> coerce_to_boolean -> pull_var_clause, so pull_var_clause always sees
> the final expression form.

Agreed, that order is more intuitive. I'll make the change and watch for
any change in error output or behavior -- coerce_to_boolean now runs
per-define before pull_var_clause rather than in a second pass -- with
guard tests for domain-over-bool, nav Var preserved, and non-coercible
error, and submit it in v49 if those and the regression pass clean.

> v47-0006 ... has_column_ref is not necessary. Column reference checks
> can use contain_var_clause, and it's cheap. Also the message change:
> -row pattern navigation offset must be a run-time constant
> +row pattern navigation offset expression must not contain column
>  references

Agreed contain_var_clause is simpler and the new message reads better.
But this restructures define_walker too, which the nav name-binding fix
also reworks -- so as with 0003 I'd rather settle it against the post-fix
walker shape than churn it twice, and revisit once that fix lands.

Summary:

  0001  -> v49   (use "parse tree" where it names the structure, not "clause")
  0002  -> keep the current flat form
  0003  -> revisit after the nav name-binding fix
  0004  -> v49
  0005  -> v49   (after guard tests + regression pass)
  0006  -> revisit after the nav name-binding fix

For 0003 and 0006 -- both swap a hand-rolled walker for a standard helper
(contain_volatile_functions, contain_var_clause) -- I'll think this
through properly when I rework the walker: the convention of reaching for
those helpers against what the current checks buy us (the error cursor,
the specific messages), rather than deciding it by reflex.

More broadly, the substantive correctness defects take priority: the nav
name-binding fix and the rest of that series come ahead of these cleanup
refactors. Once v48 is posted I'll write up the current list of known
issues and send it to the thread.

Thanks again for the careful pass.

Best,
Henson