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
