Hi jian, > I don't think keeping the above EXPLAIN from failing justifies > adding a new field (validate) to EvalDefineOffsetsContext.
The complexity cost is a fair point, and I do not have a settled answer to it. Where validate lives, and when the error should be raised, are both things I would rather spend attention on later, in a pass over the finished series. I think we are weighting attention differently more than we are disagreeing -- I have been spending mine on the correctness defects, and beside those this reads to me as a question of where an error is best raised rather than whether a wrong answer can come out. Adjusting what already behaves correctly is what I have been putting behind them, and not this proposal in particular. One note for that later pass, so it is not lost: dropping validate moves the check earlier only for a literal. An offset written as a bind parameter, or the PARAM_EXEC a correlated SRF leaves behind, has no value at executor init, so it still plans and still fails at execution. > Maybe we can just let such cases error out. Parser guarantees that > PREV() and friends contain at least one column reference ... > Hitting this requires deliberately building a one row table that > also inlines to a constant, which is a corner case. I would not concede that one. The pull-up there did not fold a subexpression of the argument -- it took the argument's only column reference away. pull_up_simple_values() fires only for a single-row VALUES, so what it leaves behind is a scan PREV has no row to navigate to: DEFINE A AS PREV(v / 0) > 0 -> DEFINE A AS NULL That is the fold the query is owed, and null is what it returns today; rpr_base.sql has the row to show for it. Raising instead is not accepting a corner case, it is producing the wrong answer where the right one is a constant. And the cost of skipping the argument is not that corner case anyway: DEFINE A AS PREV(v + 2 * 3) > 0 -> DEFINE A AS PREV(v + 6) > 0 That one does not happen today either. The case copies arg through untouched, so the multiply is evaluated again for every row the navigation is asked about, and the XXX already in clauses.c says as much -- "every row of the match pays for what was not." Folding all the way to that constant null is the right end state, but it takes knowing the navigation can never reach a row, which is not in front of eval_const_expressions(). On the complexity ground you raised above, I would rather not build that here, and nothing observable is lost by not building it: the query returns the same row either way, the DEFINE just stays an expression instead of collapsing. So the line I would draw is the column reference, not the corner case. Recurse into arg and fold it as usual while it still has one -- PREV(v + 2 * 3) then folds, and PREV(v + 1 / 0) raises exactly where v + 1 / 0 raises outside a navigation. Once it has none left the argument no longer depends on the row the navigation lands on, so folding it can only manufacture an error, and that is the case your example builds. The rewrites are owed either way, since they are what makes the argument executable at all, and that is the half the three XXX cases in rpr_base.sql are waiting on. Best regards, Henson
