On Tue, Jan 28, 2025 at 9:02 AM Tatsuo Ishii <ishii@postgresql.org> wrote:
>
> >> +/*
> >> + * ignorenulls_getfuncarginframe
> >> + * For IGNORE NULLS, get the next nonnull value in the frame, moving forward or backward
> >> + * until we find a value or reach the frame's end.
> >> + */
> >> +static Datum
> >> +ignorenulls_getfuncarginframe(WindowObject winobj, int argno,
> >>
> >> Do you assume that win_nonnulls is sorted by pos? I think it's
> >> necessarily true that pos in win_nonnulls array is sorted. Is that ok?
> >
> > Yes it must be sorted on my understanding of the code.
>
> Then the patch has a problem. I ran a query below and examined
> win_nonnulls. It seems it was not sorted out.
>
> SELECT
> x,y,
> nth_value(y,1) IGNORE NULLS OVER w
> FROM (VALUES (1,1), (2,2), (3,NULL), (4,4), (5,NULL), (6,6), (7,7)) AS t(x,y)
> WINDOW w AS (ORDER BY x ROWS BETWEEN 1 PRECEDING AND 1 FOLLOWING EXCLUDE CURRENT ROW);
>
>
> (gdb) p *winobj->win_nonnulls @ winobj->nonnulls_len
> $8 = {1, 0, 3, 6, 5}
>
I've looked at it again and I think the code is correct, but I
miswrote that the array needs to be sorted. The above query returns:
x | y | nth_value
---+---+-----------
1 | 1 | 2
2 | 2 | 1
3 | | 2
4 | 4 |
5 | | 4
6 | 6 | 7
7 | 7 | 6
(7 rows)
This is correct, for values of x:
1: The first non-null value of y is at position 0, however we have
EXCLUDE CURRENT ROW so it picks the next non-null value at position 1
and stores it in the array, returning 2.
2: We can now take the first non-null value of y at position 0 and
store it in the array, returning 1.
3. We take 1 preceding, using the position stored in the array, returning 2.
4. 1 preceding and 1 following are both null, and we exclude the
current row, so returning null.
5. 1 preceding is at position 3, store it in the array, returning 4.
6. 1 preceding is null and we exclude the current row, so store
position 6 in the array, returning 7.
7. 1 preceding is at position 5, store it in the array and return 6.
It will be unordered when the EXCLUDE clause is used but the code
should handle this correctly.
