Обсуждение: Implement for window functions

Поиск
Список
Период
Сортировка

Implement for window functions

От
Vik Fearing
Дата:
This feature adds RESPECT NULLS and IGNORE NULLS syntax to several
window functions, according to the SQL Standard.

Unlike the last time this was attempted[1], my version does not hardcode
the spec's list of functions that this applies to.  Instead, it accepts
it for all true window functions (that is, it does not apply to
aggregates acting as window functions).

This patch also does not attempt to solve the FROM LAST problem.  That
remains unimplemented.

For the CREATE FUNCTION syntax, I used TREAT NULLS so as to avoid
creating new keywords.

The second patch adds some new window functions in order to test that
the null treatment works correctly for cases that aren't covered by the
standard functions but that custom functions might want to use.  It is
*not* intended to be committed; I am only submitting the first patch for
inclusion in core.

This is based off of 324435eb14.

[1]
https://www.postgresql.org/message-id/CAGMVOdsbtRwE_4%2Bv8zjH1d9xfovDeQAGLkP_B6k69_VoFEgX-A%40mail.gmail.com
-- 
Vik Fearing
Вложения

Re: Implement for window functions

От
ilmari@ilmari.org (Dagfinn Ilmari Mannsåker)
Дата:
Vik Fearing <vik@postgresfriends.org> writes:

> The second patch adds some new window functions in order to test that
> the null treatment works correctly for cases that aren't covered by the
> standard functions but that custom functions might want to use.  It is
> *not* intended to be committed; I am only submitting the first patch for
> inclusion in core.

Would it make stense to add them as a test extension under
src/test/modules/?

- ilmari
-- 
"A disappointingly low fraction of the human race is,
 at any given time, on fire." - Stig Sandbeck Mathisen

Re: Implement for window functions

От
Daniel Gustafsson
Дата:
> On 30 Jun 2020, at 15:54, Vik Fearing <vik@postgresfriends.org> wrote:

> This feature adds RESPECT NULLS and IGNORE NULLS syntax to several
> window functions, according to the SQL Standard.

This fails compilation due to a compiler warning in WinGetFuncArgInPartition
and WinGetFuncArgInFrame (same warning in both):

nodeWindowAgg.c: In function ‘WinGetFuncArgInPartition’:
nodeWindowAgg.c:3274:10: error: ‘step’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
   relpos += step;
          ^
This was with GCC in the Travis build, the Windows build passed and so does
clang locally for me.

cheers ./daniel

Re: Implement for window functions

От
Michael Paquier
Дата:
On Wed, Jul 01, 2020 at 02:27:45PM +0200, Daniel Gustafsson wrote:
> This was with GCC in the Travis build, the Windows build passed and so does
> clang locally for me.

This was two months ago, so this patch has been marked as returned
with feedback.  Please feel free to resubmit once you have a new
version.
--
Michael
Вложения

Re: Implement for window functions

От
Krasiyan Andreev
Дата:
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.
Вложения

Re: Implement for window functions

От
Vik Fearing
Дата:
On 11/12/20 11:35 PM, Krasiyan Andreev wrote:
> Hi, it looks like Vik Fearing's patch does not apply anymore, because there
> are many conflicts with recent changes, fixed patch attached.

Thanks!  I've been away from this list for a while and I have some
catching up to do.
-- 
Vik Fearing

Re: Implement for window functions

От
Krasiyan Andreev
Дата:
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.

На пт, 13.11.2020 г. в 0:44 ч. Vik Fearing <vik@postgresfriends.org> написа:
On 11/12/20 11:35 PM, Krasiyan Andreev wrote:
> Hi, it looks like Vik Fearing's patch does not apply anymore, because there
> are many conflicts with recent changes, fixed patch attached.

Thanks!  I've been away from this list for a while and I have some
catching up to do.
--
Vik Fearing
Вложения

Re: Implement for window functions

От
Vik Fearing
Дата:
On 11/21/20 10:07 AM, Krasiyan Andreev wrote:
> Fixed patch attached, after new introduced conflicts.
> Vik, can you add it to the next commitfest, to be able to test it.


I have done this now.  Thanks!
-- 
Vik Fearing

Re: Implement for window functions

От
Krasiyan Andreev
Дата:
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]$

На вт, 8.12.2020 г. в 16:27 ч. Vik Fearing <vik@postgresfriends.org> написа:
On 11/21/20 10:07 AM, Krasiyan Andreev wrote:
> Fixed patch attached, after new introduced conflicts.
> Vik, can you add it to the next commitfest, to be able to test it.


I have done this now.  Thanks!
--
Vik Fearing
Вложения

Re: Implement for window functions

От
David Fetter
Дата:
On Wed, Dec 30, 2020 at 09:32:26PM +0200, Krasiyan Andreev wrote:
> Hi, after latest committed patches about multirange datatypes, I get a
> compilation error,

Oh, right. I'd been meaning to send a patch to fix that. Here it is.

Best,
David.
-- 
David Fetter <david(at)fetter(dot)org> http://fetter.org/
Phone: +1 415 235 3778

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate
Вложения

Re: Implement for window functions

От
Krasiyan Andreev
Дата:
It works - now it compiles clean and all checks are passed, thank you. I will continue with more complex tests.

На ср, 30.12.2020 г. в 21:50 ч. David Fetter <david@fetter.org> написа:
On Wed, Dec 30, 2020 at 09:32:26PM +0200, Krasiyan Andreev wrote:
> Hi, after latest committed patches about multirange datatypes, I get a
> compilation error,

Oh, right. I'd been meaning to send a patch to fix that. Here it is.

Best,
David.
--
David Fetter <david(at)fetter(dot)org> http://fetter.org/
Phone: +1 415 235 3778

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate

Re: Implement for window functions

От
Krasiyan Andreev
Дата:
Hi, it looks like cfbot.cputube.org didn't recognize and can't apply a patch, so I resend it now with a different format, no other changes.

На ср, 30.12.2020 г. в 22:16 ч. Krasiyan Andreev <krasiyan@gmail.com> написа:
It works - now it compiles clean and all checks are passed, thank you. I will continue with more complex tests.

На ср, 30.12.2020 г. в 21:50 ч. David Fetter <david@fetter.org> написа:
On Wed, Dec 30, 2020 at 09:32:26PM +0200, Krasiyan Andreev wrote:
> Hi, after latest committed patches about multirange datatypes, I get a
> compilation error,

Oh, right. I'd been meaning to send a patch to fix that. Here it is.

Best,
David.
--
David Fetter <david(at)fetter(dot)org> http://fetter.org/
Phone: +1 415 235 3778

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate
Вложения

Re: Implement for window functions

От
Zhihong Yu
Дата:
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.

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

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

Thanks

On Fri, Jan 1, 2021 at 12:59 PM Krasiyan Andreev <krasiyan@gmail.com> wrote:
Hi, it looks like cfbot.cputube.org didn't recognize and can't apply a patch, so I resend it now with a different format, no other changes.

На ср, 30.12.2020 г. в 22:16 ч. Krasiyan Andreev <krasiyan@gmail.com> написа:
It works - now it compiles clean and all checks are passed, thank you. I will continue with more complex tests.

На ср, 30.12.2020 г. в 21:50 ч. David Fetter <david@fetter.org> написа:
On Wed, Dec 30, 2020 at 09:32:26PM +0200, Krasiyan Andreev wrote:
> Hi, after latest committed patches about multirange datatypes, I get a
> compilation error,

Oh, right. I'd been meaning to send a patch to fix that. Here it is.

Best,
David.
--
David Fetter <david(at)fetter(dot)org> http://fetter.org/
Phone: +1 415 235 3778

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate

Re: Implement for window functions

От
David Fetter
Дата:
On Fri, Jan 01, 2021 at 01:21:10PM -0800, 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.

If it does actually need step to be one of those three choices, it
might be shorter (well, less branchy) to write as

    step = (target > 0) - (target < 0);

Best,
David.
-- 
David Fetter <david(at)fetter(dot)org> http://fetter.org/
Phone: +1 415 235 3778

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate

Re: Implement for window functions

От
Vik Fearing
Дата:
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

Re: Implement for window functions

От
Krasiyan Andreev
Дата:
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@postgresfriends.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
Вложения

Re: Implement for window functions

От
Krasiyan Andreev
Дата:
Hi, patch applies and compiles, all included and external tests and building of the docs pass.
After the last run of the cfbot, there are no any building warnings.
I am using last version in our testing environment with real data and I didn't find any bugs,
so I'm marking this patch as ready for the committer in the commitfest app.

На сб, 9.01.2021 г. в 13:30 ч. Krasiyan Andreev <krasiyan@gmail.com> написа:
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@postgresfriends.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

Re: Implement for window functions

От
Zhihong Yu
Дата:
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@gmail.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@postgresfriends.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

Re: Implement for window functions

От
Tom Lane
Дата:
I started to look through this patch, and the first thing I'm wondering
about is why bother with a new pg_proc column, ie why not just apply the
behavior to any window function?  The fact that the SQL committee
restricts this syntax to a few window functions is just their usual
design tic of creating one-off syntax that could be much more general.
We've not hesitated to generalize in similar situations in the past.

The main thing I can see against that position is that it's not very
clear what to do if the window function has more than one window-ized
argument --- or at least, the only plausible interpretation would be
to ignore rows in which any of those arguments is null, which this
implementation is incapable of doing (since we don't know exactly
which arguments the function will try to treat as window-ized).
However, having a pronulltreatment column isn't helping that
situation at all: somebody could mark a multi-input window function
as ignoring nulls, and we'd silently do the wrong thing in any case
where those inputs weren't nulls at exactly the same rows.

My thought, therefore, is to drop pg_proc.pronulltreatment and instead
enforce an implementation restriction that when IGNORE NULLS is specified, 
WinGetFuncArgInPartition and WinGetFuncArgInFrame throw an error if
asked about any argument position other than the first one.  As long
as only the first argument is window-ized, the implementation you have
here will act correctly.  If anybody ever finds that annoying, they can
figure out how to relax the restriction at that time.

The need for a TREAT NULLS option to CREATE FUNCTION would thereby
also go away, which is good because I don't think this patch has
fully implemented that (notably, I don't see any pg_dump changes).

As far as the actual implementation goes:

* The undocumented relationship between "relpos" (which used to be
constant and now isn't) and "target" and "step" makes my head hurt.
I'm sure this could be redesigned to be simpler, or if not, at
least it should be commented a lot more thoroughly.

* I'm quite concerned about performance; it looks like this will
degrade to O(N^2) in practical situations, which isn't going to
satisfy anyone.  I think we need to track how many nulls we've
already seen so that we aren't re-visiting earlier rows over and
over.  That should make it possible to un-disable the set_mark
optimization, which is something that's independently catastrophic
for performance.  While I've not stopped to design this fully, maybe
we could keep state along the lines of "there are j rows with null
values of the window-ized argument before row k of the partition."
Updating that by dead reckoning as we navigate would be enough to
fix the O(N^2) problem for typical scenarios.  I think.

            regards, tom lane

Re: Implement for window functions

От
David Steele
Дата:
Hi Vik,

On 1/11/21 5:00 PM, Tom Lane wrote:
> I started to look through this patch...

I see you moved this patch to PG15. If you won't be working on the patch 
in this CF perhaps it would be better to close it as Returned with 
Feedback for now and reopen it when you have a new patch?

I'll do that on March 23 unless I hear arguments to the contrary.

Regards,
-- 
-David
david@pgmasters.net

Re: Implement for window functions

От
Vik Fearing
Дата:
On 3/18/21 4:12 PM, David Steele wrote:
> Hi Vik,
> 
> On 1/11/21 5:00 PM, Tom Lane wrote:
>> I started to look through this patch...
> 
> I see you moved this patch to PG15. If you won't be working on the patch
> in this CF

Correct.  I won't be working on this again until I finish my review of
the system versioning patch.

> perhaps it would be better to close it as Returned with
> Feedback for now and reopen it when you have a new patch?

If that is preferred over moving it to PG15, then no objection.  As long
as people don't think I've abandoned it.
-- 
Vik Fearing

Re: Implement for window functions

От
David Steele
Дата:
On 3/18/21 12:03 PM, Vik Fearing wrote:
> On 3/18/21 4:12 PM, David Steele wrote:
>> Hi Vik,
>>
>> On 1/11/21 5:00 PM, Tom Lane wrote:
>>> I started to look through this patch...
>>
>> I see you moved this patch to PG15. If you won't be working on the patch
>> in this CF
> 
> Correct.  I won't be working on this again until I finish my review of
> the system versioning patch.
> 
>> perhaps it would be better to close it as Returned with
>> Feedback for now and reopen it when you have a new patch?
> 
> If that is preferred over moving it to PG15, then no objection.  

It is, because it means it doesn't need to be looked at again until you 
have had time to work on it.

> As long
> as people don't think I've abandoned it.

This declaration should prevent that.

Regards,
-- 
-David
david@pgmasters.net