Hi Henson,
> 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):
Thanks for the explanation. That makese sense.
> 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?
I like B too.
Regards,
--
Tatsuo Ishii
SRA OSS K.K.
English: http://www.sraoss.co.jp/index_en/
Japanese:http://www.sraoss.co.jp
