On Wed, 23 Mar 2022 at 12:50, Zhihong Yu <zyu@yugabyte.com> wrote:
> The following code seems to be common between if / else blocks (w.r.t. wfunc_left) of find_window_run_conditions().
> It would be nice if this code can be shared.
I remember thinking about that and thinking that I didn't want to
overcomplicate the if conditions for the strategy tests. I'd thought
these would have become:
if ((wfunc_left && (strategy == BTLessStrategyNumber ||
strategy == BTLessEqualStrategyNumber)) ||
(!wfunc_left && (strategy == BTGreaterStrategyNumber ||
strategy == BTGreaterEqualStrategyNumber)))
which I didn't think was very readable. That caused me to keep it separate.
On reflection, we can just leave the strategy checks as they are, then
add the additional code for checking wfunc_left when checking the
res->monotonic, i.e:
if ((wfunc_left && (res->monotonic & MONOTONICFUNC_INCREASING)) ||
(!wfunc_left && (res->monotonic & MONOTONICFUNC_DECREASING)))
I think that's more readable than doubling up the strategy checks, so
I've done it that way in the attached.
>
> + WindowClause *wclause = (WindowClause *)
> + list_nth(subquery->windowClause,
> + wfunc->winref - 1);
>
> The code would be more readable if list_nth() is indented.
That's just the way pgindent put it.
> + /* Check the left side of the OpExpr */
>
> It seems the code for checking left / right is the same. It would be better to extract and reuse the code.
I've moved some of that code into find_window_run_conditions() which
removes about 10 lines of code.
Updated patch attached. Thanks for looking.
David