Re: Add RESPECT/IGNORE NULLS and FROM FIRST/LAST options - Mailing list pgsql-hackers

> Probably I misunderstood what you said. Now I realize what you are
> suggesting was, throwing an error *only* when a RESPECT/IGNORE NULLS
> option is given and the function did not call
> WinAllowNullTreatmentOption. If the option is not given, no error is
> thrown even if WinAllowNullTreatmentOption is not called. I am okay
> with this direction. I will post a patch for this.

While I was implementing this, I realized that in order to check if
WinAllowNullTreatmentOption had been called or not, it's necessary to
call the window function at least once in mainline execution of
nodeWindowAgg.c (I supposed in eval_windowfunction). I don't like this
because I expected to throw an error *before* calling the window
function.

So I studied your idea:
> Alternatively, you could just drop the entire concept of throwing an
> error for that.

Attached is a patch to implement this. Previously window functions
should call WinCheckAndInitializeNullTreatment with
allowNullTreatment==true if they accept a null treatment
clause. Otherwise, they are called as if null treatment clause is not
specified. With the patch, window functions accept a null treatment
clause as specified without calling
WinCheckAndInitializeNullTreatment.

There's one thing which might be different from what you suggested
is, I want to give window functions a method to stat that they do not
want accept a null treatment clause. For this purpose
WinCheckAndInitializeNullTreatment (with allowNullTreatment==false)
can be called.(Alternatively we could eliminate allowNullTreatment
argument and rename it something like WinDisallowNullTreatmentOption).

Some of built-in window functions that do not accept a null treatment
clause call this in the patch. This way, we do not need to test the
case when the functions are given a null treatment option except just
they throw an error. User defined functions would call the function
for the same purpose as built-in window functions.

> The implementation is entirely
> within nodeWindowAgg.c and does not depend in any way on the
> cooperation of the window function.

I am not sure. For example built-in lead function's behavior (with
IGNORE NULLS option) is defined by the standard. Unlike RESPECT NULLS
case, the expected behavior may not be obvious. According the
standard:

1. If lead's "offset" option is 0, the argument evaluated on current
   row is returned regardless the value is NULL or NOT.

2. Otherwise, returns the value evaluated on a row which is nth NOT NULL.

For me, 2 is obvious but 1 was not so obvious because I thought that
lead() returns only non NULL value (except there's no non null values
or specified offset is out of partition).

Thus lead() calls WinGetFuncArgInPartition with
seektype==WINDOW_SEEK_CURRENT. Thus WinGetFuncArgInPartition with
WINDOW_SEEK_CURRENT is implemented in a way to satisfy the lead()
semantics above. This means if someone tries to implement a new window
function calling WinGetFuncArgInPartition with WINDOW_SEEK_CURRENT,
the function must has the same semantics as lead(). I think there's a
cooperation between nodeWindowAgg.c and window functions.

> +   This option is only allowed for the following functions: <function>lag</function>,
> +   <function>lead</function>, <function>first_value</function>, <function>last_value</function>,
> +   <function>nth_value</function>.
> 
> as this fails to account for the possibility of user-defined window
> functions.  IMO we could drop the error check altogether and rewrite
> the docs along the lines of "Not all window functions pay attention
> to this option.  Of the built-in window functions, only blah blah
> and blah do."

Fixing docs are not included in the patch (yet).

Best regards,
--
Tatsuo Ishii
SRA OSS K.K.
English: http://www.sraoss.co.jp/index_en/
Japanese:http://www.sraoss.co.jp