Re: Row pattern recognition

[Henson Choi]

Hi Tatsuo,

Thanks for the review. I agree with the direction -- the (void) is
confusing. I did try your exact suggestion and the regression tests pass
unchanged, but I would keep one guard, for the following reason.

> Is there any reason that we call row_is_in_reduced_frame() here,
> instead of something like:
>
>    update_frameheadpos(winstate);
>    update_reduced_frame(winobj, pos);

row_is_in_reduced_frame() guards the update:

    state = get_reduced_frame_status(winstate, pos);
    if (state == RF_NOT_DETERMINED)
    {
        update_frameheadpos(winstate);
        update_reduced_frame(winobj, pos);
    }

Calling update_reduced_frame() unconditionally drops that guard. When
currentpos is already determined -- e.g. an interior row of a previous
match under AFTER MATCH SKIP PAST LAST ROW (RF_SKIPPED) -- it takes an
O(1) early return and overwrites the shared rpr_match_* record, silently
reclassifying a SKIPPED row as UNMATCHED.

This is not observable today -- I confirmed with a guarded-vs-unguarded
differential (patterns, both skip modes, various aggregates: identical
output), because both paths end up with an empty reduced frame. But it
does change the state semantics and leans on that convergence, so I would
rather keep the guard. Two ways, both of which remove the (void):

  A) Keep the guard inline at the call site.

  B) Factor it into a small helper shared by the nav-driving call site
     and row_is_in_reduced_frame():

        static void
        ensure_reduced_frame(WindowObject winobj, int64 pos)
        {
            WindowAggState *winstate = winobj->winstate;

            if (get_reduced_frame_status(winstate, pos) ==
RF_NOT_DETERMINED)
            {
                update_frameheadpos(winstate);
                update_reduced_frame(winobj, pos);
            }
        }

I lean towards B (no duplicated guard, and the name states the intent),
but A is the smaller diff. Which do you prefer?

Best regards,
Henson