Thread: Implement for window functions

Hi, it looks like Vik Fearing's patch does not apply anymore, because there are many conflicts with recent changes, fixed patch attached.

I am interested in reviewing and testing it for the next commitfest, if it's design and implementation is found to be acceptable.

Additionally, if it is also acceptable, I can add support for handling negative indexes for nth_value(), to be able to reverse order from first/from last for the window frame.

Fixed patch attached, after new introduced conflicts.

Vik, can you add it to the next commitfest, to be able to test it.

Also, all tests from Oliver Ford's old patch also passed successfully.

Hi, after latest committed patches about multirange datatypes, I get a compilation error,

when I try to apply a patch about respect/ignore null for window functions.

Without it applied, it complies clean and all checks are passed.

[krasiyan@localhost build]$ /home/krasiyan/pgsql/postgresql/configure --with-openssl --with-libxml --with-libxslt --with-systemd --with-selinux --with-perl --with-python --enable-cassert --prefix=/var/lib/pgsql/14

...

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 typecmds.o /home/krasiyan/pgsql/postgresql/src/backend/commands/typecmds.c
/home/krasiyan/pgsql/postgresql/src/backend/commands/typecmds.c: In function ‘makeMultirangeConstructors’:
/home/krasiyan/pgsql/postgresql/src/backend/commands/typecmds.c:1849:9: warning: passing argument 17 of ‘ProcedureCreate’ makes integer from pointer without a cast [-Wint-conversion]
 1849 |         argtypes, /* parameterTypes */
      |         ^~~~~~~~
      |         |
      |         oidvector *
In file included from /home/krasiyan/pgsql/postgresql/src/backend/commands/typecmds.c:52:
/home/krasiyan/pgsql/postgresql/src/include/catalog/pg_proc.h:206:16: note: expected ‘char’ but argument is of type ‘oidvector *’
  206 |           char parallel,
      |           ~~~~~^~~~~~~~
In file included from /home/krasiyan/pgsql/postgresql/src/backend/commands/typecmds.c:32:
/home/krasiyan/pgsql/postgresql/src/include/postgres.h:556:29: warning: passing argument 18 of ‘ProcedureCreate’ makes pointer from integer without a cast [-Wint-conversion]
  556 | #define PointerGetDatum(X) ((Datum) (X))
      |                            ~^~~~~~~~~~~~
      |                             |
      |                             long unsigned int
/home/krasiyan/pgsql/postgresql/src/backend/commands/typecmds.c:1850:9: note: in expansion of macro ‘PointerGetDatum’
 1850 |         PointerGetDatum(NULL), /* allParameterTypes */
      |         ^~~~~~~~~~~~~~~
In file included from /home/krasiyan/pgsql/postgresql/src/backend/commands/typecmds.c:52:
/home/krasiyan/pgsql/postgresql/src/include/catalog/pg_proc.h:207:22: note: expected ‘oidvector *’ but argument is of type ‘long unsigned int’
  207 |           oidvector *parameterTypes,
      |           ~~~~~~~~~~~^~~~~~~~~~~~~~
In file included from /home/krasiyan/pgsql/postgresql/src/include/access/tupdesc.h:19,
                 from /home/krasiyan/pgsql/postgresql/src/include/utils/relcache.h:18,
                 from /home/krasiyan/pgsql/postgresql/src/include/access/genam.h:21,
                 from /home/krasiyan/pgsql/postgresql/src/backend/commands/typecmds.c:34:
/home/krasiyan/pgsql/postgresql/src/include/nodes/pg_list.h:65:19: warning: passing argument 21 of ‘ProcedureCreate’ makes integer from pointer without a cast [-Wint-conversion]
   65 | #define NIL      ((List *) NULL)
      |                  ~^~~~~~~~~~~~~~
      |                   |
      |                   List *
/home/krasiyan/pgsql/postgresql/src/backend/commands/typecmds.c:1853:9: note: in expansion of macro ‘NIL’
 1853 |         NIL, /* parameterDefaults */
      |         ^~~
In file included from /home/krasiyan/pgsql/postgresql/src/backend/commands/typecmds.c:52:
/home/krasiyan/pgsql/postgresql/src/include/catalog/pg_proc.h:210:17: note: expected ‘Datum’ {aka ‘long unsigned int’} but argument is of type ‘List *’
  210 |           Datum parameterNames,
      |           ~~~~~~^~~~~~~~~~~~~~
In file included from /home/krasiyan/pgsql/postgresql/src/backend/commands/typecmds.c:32:
/home/krasiyan/pgsql/postgresql/src/include/postgres.h:556:29: warning: passing argument 22 of ‘ProcedureCreate’ makes pointer from integer without a cast [-Wint-conversion]
  556 | #define PointerGetDatum(X) ((Datum) (X))
      |                            ~^~~~~~~~~~~~
      |                             |
      |                             long unsigned int
/home/krasiyan/pgsql/postgresql/src/backend/commands/typecmds.c:1854:9: note: in expansion of macro ‘PointerGetDatum’
 1854 |         PointerGetDatum(NULL), /* trftypes */
      |         ^~~~~~~~~~~~~~~
In file included from /home/krasiyan/pgsql/postgresql/src/backend/commands/typecmds.c:52:
/home/krasiyan/pgsql/postgresql/src/include/catalog/pg_proc.h:211:17: note: expected ‘List *’ but argument is of type ‘long unsigned int’
  211 |           List *parameterDefaults,
      |           ~~~~~~^~~~~~~~~~~~~~~~~
/home/krasiyan/pgsql/postgresql/src/backend/commands/typecmds.c:1833:11: error: too few arguments to function ‘ProcedureCreate’
 1833 |  myself = ProcedureCreate(name, /* name: same as multirange type */
      |           ^~~~~~~~~~~~~~~
In file included from /home/krasiyan/pgsql/postgresql/src/backend/commands/typecmds.c:52:
/home/krasiyan/pgsql/postgresql/src/include/catalog/pg_proc.h:190:22: note: declared here
  190 | extern ObjectAddress ProcedureCreate(const char *procedureName,
      |                      ^~~~~~~~~~~~~~~
/home/krasiyan/pgsql/postgresql/src/backend/commands/typecmds.c:1892:9: warning: passing argument 17 of ‘ProcedureCreate’ makes integer from pointer without a cast [-Wint-conversion]
 1892 |         argtypes, /* parameterTypes */
      |         ^~~~~~~~
      |         |
      |         oidvector *
In file included from /home/krasiyan/pgsql/postgresql/src/backend/commands/typecmds.c:52:
/home/krasiyan/pgsql/postgresql/src/include/catalog/pg_proc.h:206:16: note: expected ‘char’ but argument is of type ‘oidvector *’
  206 |           char parallel,
      |           ~~~~~^~~~~~~~
In file included from /home/krasiyan/pgsql/postgresql/src/backend/commands/typecmds.c:32:
/home/krasiyan/pgsql/postgresql/src/include/postgres.h:556:29: warning: passing argument 18 of ‘ProcedureCreate’ makes pointer from integer without a cast [-Wint-conversion]
  556 | #define PointerGetDatum(X) ((Datum) (X))
      |                            ~^~~~~~~~~~~~
      |                             |
      |                             long unsigned int
/home/krasiyan/pgsql/postgresql/src/backend/commands/typecmds.c:1893:9: note: in expansion of macro ‘PointerGetDatum’
 1893 |         PointerGetDatum(NULL), /* allParameterTypes */
      |         ^~~~~~~~~~~~~~~
In file included from /home/krasiyan/pgsql/postgresql/src/backend/commands/typecmds.c:52:
/home/krasiyan/pgsql/postgresql/src/include/catalog/pg_proc.h:207:22: note: expected ‘oidvector *’ but argument is of type ‘long unsigned int’
  207 |           oidvector *parameterTypes,
      |           ~~~~~~~~~~~^~~~~~~~~~~~~~
In file included from /home/krasiyan/pgsql/postgresql/src/include/access/tupdesc.h:19,
                 from /home/krasiyan/pgsql/postgresql/src/include/utils/relcache.h:18,
                 from /home/krasiyan/pgsql/postgresql/src/include/access/genam.h:21,
                 from /home/krasiyan/pgsql/postgresql/src/backend/commands/typecmds.c:34:
/home/krasiyan/pgsql/postgresql/src/include/nodes/pg_list.h:65:19: warning: passing argument 21 of ‘ProcedureCreate’ makes integer from pointer without a cast [-Wint-conversion]
   65 | #define NIL      ((List *) NULL)
      |                  ~^~~~~~~~~~~~~~
      |                   |
      |                   List *
/home/krasiyan/pgsql/postgresql/src/backend/commands/typecmds.c:1896:9: note: in expansion of macro ‘NIL’
 1896 |         NIL, /* parameterDefaults */
      |         ^~~
In file included from /home/krasiyan/pgsql/postgresql/src/backend/commands/typecmds.c:52:
/home/krasiyan/pgsql/postgresql/src/include/catalog/pg_proc.h:210:17: note: expected ‘Datum’ {aka ‘long unsigned int’} but argument is of type ‘List *’
  210 |           Datum parameterNames,
      |           ~~~~~~^~~~~~~~~~~~~~
In file included from /home/krasiyan/pgsql/postgresql/src/backend/commands/typecmds.c:32:
/home/krasiyan/pgsql/postgresql/src/include/postgres.h:556:29: warning: passing argument 22 of ‘ProcedureCreate’ makes pointer from integer without a cast [-Wint-conversion]
  556 | #define PointerGetDatum(X) ((Datum) (X))
      |                            ~^~~~~~~~~~~~
      |                             |
      |                             long unsigned int
/home/krasiyan/pgsql/postgresql/src/backend/commands/typecmds.c:1897:9: note: in expansion of macro ‘PointerGetDatum’
 1897 |         PointerGetDatum(NULL), /* trftypes */
      |         ^~~~~~~~~~~~~~~
In file included from /home/krasiyan/pgsql/postgresql/src/backend/commands/typecmds.c:52:
/home/krasiyan/pgsql/postgresql/src/include/catalog/pg_proc.h:211:17: note: expected ‘List *’ but argument is of type ‘long unsigned int’
  211 |           List *parameterDefaults,
      |           ~~~~~~^~~~~~~~~~~~~~~~~
/home/krasiyan/pgsql/postgresql/src/backend/commands/typecmds.c:1876:11: error: too few arguments to function ‘ProcedureCreate’
 1876 |  myself = ProcedureCreate(name, /* name: same as multirange type */
      |           ^~~~~~~~~~~~~~~
In file included from /home/krasiyan/pgsql/postgresql/src/backend/commands/typecmds.c:52:
/home/krasiyan/pgsql/postgresql/src/include/catalog/pg_proc.h:190:22: note: declared here
  190 | extern ObjectAddress ProcedureCreate(const char *procedureName,
      |                      ^~~~~~~~~~~~~~~
/home/krasiyan/pgsql/postgresql/src/backend/commands/typecmds.c:1932:9: warning: passing argument 17 of ‘ProcedureCreate’ makes integer from pointer without a cast [-Wint-conversion]
 1932 |         argtypes, /* parameterTypes */
      |         ^~~~~~~~
      |         |
      |         oidvector *
In file included from /home/krasiyan/pgsql/postgresql/src/backend/commands/typecmds.c:52:
/home/krasiyan/pgsql/postgresql/src/include/catalog/pg_proc.h:206:16: note: expected ‘char’ but argument is of type ‘oidvector *’
  206 |           char parallel,
      |           ~~~~~^~~~~~~~
In file included from /home/krasiyan/pgsql/postgresql/src/backend/commands/typecmds.c:32:
/home/krasiyan/pgsql/postgresql/src/include/postgres.h:556:29: warning: passing argument 18 of ‘ProcedureCreate’ makes pointer from integer without a cast [-Wint-conversion]
  556 | #define PointerGetDatum(X) ((Datum) (X))
      |                            ~^~~~~~~~~~~~
      |                             |
      |                             long unsigned int
/home/krasiyan/pgsql/postgresql/src/backend/commands/typecmds.c:1933:9: note: in expansion of macro ‘PointerGetDatum’
 1933 |         PointerGetDatum(allParameterTypes), /* allParameterTypes */
      |         ^~~~~~~~~~~~~~~
In file included from /home/krasiyan/pgsql/postgresql/src/backend/commands/typecmds.c:52:
/home/krasiyan/pgsql/postgresql/src/include/catalog/pg_proc.h:207:22: note: expected ‘oidvector *’ but argument is of type ‘long unsigned int’
  207 |           oidvector *parameterTypes,
      |           ~~~~~~~~~~~^~~~~~~~~~~~~~
In file included from /home/krasiyan/pgsql/postgresql/src/include/access/tupdesc.h:19,
                 from /home/krasiyan/pgsql/postgresql/src/include/utils/relcache.h:18,
                 from /home/krasiyan/pgsql/postgresql/src/include/access/genam.h:21,
                 from /home/krasiyan/pgsql/postgresql/src/backend/commands/typecmds.c:34:
/home/krasiyan/pgsql/postgresql/src/include/nodes/pg_list.h:65:19: warning: passing argument 21 of ‘ProcedureCreate’ makes integer from pointer without a cast [-Wint-conversion]
   65 | #define NIL      ((List *) NULL)
      |                  ~^~~~~~~~~~~~~~
      |                   |
      |                   List *
/home/krasiyan/pgsql/postgresql/src/backend/commands/typecmds.c:1936:9: note: in expansion of macro ‘NIL’
 1936 |         NIL, /* parameterDefaults */
      |         ^~~
In file included from /home/krasiyan/pgsql/postgresql/src/backend/commands/typecmds.c:52:
/home/krasiyan/pgsql/postgresql/src/include/catalog/pg_proc.h:210:17: note: expected ‘Datum’ {aka ‘long unsigned int’} but argument is of type ‘List *’
  210 |           Datum parameterNames,
      |           ~~~~~~^~~~~~~~~~~~~~
In file included from /home/krasiyan/pgsql/postgresql/src/backend/commands/typecmds.c:32:
/home/krasiyan/pgsql/postgresql/src/include/postgres.h:556:29: warning: passing argument 22 of ‘ProcedureCreate’ makes pointer from integer without a cast [-Wint-conversion]
  556 | #define PointerGetDatum(X) ((Datum) (X))
      |                            ~^~~~~~~~~~~~
      |                             |
      |                             long unsigned int
/home/krasiyan/pgsql/postgresql/src/backend/commands/typecmds.c:1937:9: note: in expansion of macro ‘PointerGetDatum’
 1937 |         PointerGetDatum(NULL), /* trftypes */
      |         ^~~~~~~~~~~~~~~
In file included from /home/krasiyan/pgsql/postgresql/src/backend/commands/typecmds.c:52:
/home/krasiyan/pgsql/postgresql/src/include/catalog/pg_proc.h:211:17: note: expected ‘List *’ but argument is of type ‘long unsigned int’
  211 |           List *parameterDefaults,
      |           ~~~~~~^~~~~~~~~~~~~~~~~
/home/krasiyan/pgsql/postgresql/src/backend/commands/typecmds.c:1916:11: error: too few arguments to function ‘ProcedureCreate’
 1916 |  myself = ProcedureCreate(name, /* name: same as multirange type */
      |           ^~~~~~~~~~~~~~~
In file included from /home/krasiyan/pgsql/postgresql/src/backend/commands/typecmds.c:52:
/home/krasiyan/pgsql/postgresql/src/include/catalog/pg_proc.h:190:22: note: declared here
  190 | extern ObjectAddress ProcedureCreate(const char *procedureName,
      |                      ^~~~~~~~~~~~~~~
make[3]: *** [<builtin>: typecmds.o] Error 1
make[3]: Leaving directory '/home/krasiyan/pgsql/build/src/backend/commands'
make[2]: *** [/home/krasiyan/pgsql/postgresql/src/backend/common.mk:39: commands-recursive] Error 2
make[2]: Leaving directory '/home/krasiyan/pgsql/build/src/backend'
make[1]: *** [Makefile:42: all-backend-recurse] Error 2
make[1]: Leaving directory '/home/krasiyan/pgsql/build/src'
make: *** [GNUmakefile:11: all-src-recurse] Error 2
[krasiyan@localhost build]$

Krasiyan:

Happy New Year.

+           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.

Similar question for the last else statement below in WinGetFuncArgInFrame():

+       else if (seektype == WINDOW_SEEK_TAIL)
+           step = -1;
+       else
+           step = 0;

Thanks

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;
      |   ~~~~~~~^~~~~~~

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