Re: Implement <null treatment> for window functions

From: Zhihong Yu <zyu(at)yugabyte(dot)com>
To: Krasiyan Andreev <krasiyan(at)gmail(dot)com>
Cc: Vik Fearing <vik(at)postgresfriends(dot)org>, David Fetter <david(at)fetter(dot)org>, PostgreSQL Hackers <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: Implement <null treatment> for window functions
Date: 2021-01-09 15:29:11
Message-ID: CALNJ-vQCZR29saSz5WOCkkktFP35NX+xQyusBAOdb3N_vVuDxA@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

Hi,
For WinGetFuncArgInFrame():

+ if (winobj->null_treatment == NULL_TREATMENT_IGNORE)
{
...
+ if (target > 0)
+ step = 1;
+ else if (target < 0)
+ step = -1;
+ else if (seektype == WINDOW_SEEK_HEAD)
+ step = 1;
+ else if (seektype == WINDOW_SEEK_TAIL)
+ step = -1;
+ else
+ step = 0;
...
+ relpos = 0;
+ }

Why is relpos always set to 0 ?
In similar code in WinGetFuncArgInPartition(), I saw the following:

+ if (target > 0)
+ step = 1;
+ else if (target < 0)
+ step = -1;
+ else
+ step = 0;
+ relpos = step;

Maybe add a comment above the relpos assignment.

Thanks

On Sat, Jan 9, 2021 at 3:31 AM Krasiyan Andreev <krasiyan(at)gmail(dot)com> wrote:

> Hi, the building warning below is fixed now, no other changes. Also, I can
> confirm that the corner case with offset=0 in lead and lag works correctly.
>
> gcc -Wall -Wmissing-prototypes -Wpointer-arith
> -Wdeclaration-after-statement -Werror=vla -Wendif-labels
> -Wmissing-format-attribute -Wimplicit-fallthrough=3 -Wcast-function-type
> -Wformat-security -fno-strict-aliasing -fwrapv -fexcess-precision=standard
> -Wno-format-truncation -Wno-stringop-truncation -O2 -I../../../src/include
> -I/home/krasiyan/pgsql/postgresql/src/include -D_GNU_SOURCE
> -I/usr/include/libxml2 -c -o nodeWindowAgg.o
> /home/krasiyan/pgsql/postgresql/src/backend/executor/nodeWindowAgg.c
> /home/krasiyan/pgsql/postgresql/src/backend/executor/nodeWindowAgg.c: In
> function ‘WinGetFuncArgInPartition’:
> /home/krasiyan/pgsql/postgresql/src/backend/executor/nodeWindowAgg.c:3274:10:
> warning: ‘step’ may be used uninitialized in this function
> [-Wmaybe-uninitialized]
> 3274 | relpos += step;
> | ~~~~~~~^~~~~~~
> /home/krasiyan/pgsql/postgresql/src/backend/executor/nodeWindowAgg.c: In
> function ‘WinGetFuncArgInFrame’:
> /home/krasiyan/pgsql/postgresql/src/backend/executor/nodeWindowAgg.c:3531:10:
> warning: ‘step’ may be used uninitialized in this function
> [-Wmaybe-uninitialized]
> 3531 | relpos += step;
> | ~~~~~~~^~~~~~~
>
>
>
> На пт, 8.01.2021 г. в 2:02 ч. Vik Fearing <vik(at)postgresfriends(dot)org>
> написа:
>
>> On 1/1/21 10:21 PM, Zhihong Yu wrote:
>> > Krasiyan:
>> > Happy New Year.
>> >
>> > For WinGetFuncArgInPartition():
>> >
>> > + if (target > 0)
>> > + step = 1;
>> > + else if (target < 0)
>> > + step = -1;
>> > + else
>> > + step = 0;
>> >
>> > When would the last else statement execute ? Since the above code is
>> > for WINDOW_SEEK_CURRENT, I wonder why step should be 0.
>>
>> Hi.
>>
>> "lag(expr, 0) over w" is useless but valid.
>> --
>> Vik Fearing
>>
>

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Michael Banck 2021-01-09 15:40:59 Fix \watch if expanded output is on and there are no results
Previous Message Andrey Borodin 2021-01-09 15:25:39 Re: Spurious "apparent wraparound" via SimpleLruTruncate() rounding