Re: Request for Patch Feedback: Lag & Lead Window Functions Can Ignore Nulls

On Mon, 2014-07-07 at 01:21 -0700, Jeff Davis wrote:
> On Sun, 2014-07-06 at 21:11 -0700, Jeff Davis wrote:
> > On Wed, 2014-04-16 at 12:50 +0100, Nicholas White wrote:
> > > Thanks for the detailed feedback, I'm sorry it took so long to
> > > incorporate it. I've attached the latest version of the patch, fixing
> > > in particular:

As innocent as this patch seemed at first, it actually opens up a lot of
questions.

Attached is the (incomplete) edit of the patch so far.

Changes from your patch:

* changed test to be locale-insensitive
* lots of refactoring in the execution itself
* fix offset 0 case
* many test improvements
* remove bitmapset and just use an array bitmap
* fix error message typo

Open Issues:

I don't think exposing the frame options is a good idea. That's an
internal concept now, but putting it in windowapi.h will mean that it
needs to live forever.

The struct is private, so there's no easy hack to access the frame
options directly. That means that we need to work with the existing API
functions, which is OK because I think that everything we want to do can
go into WinGetFuncArgInPartition(). If we do the same thing for
WinGetFuncArgInFrame(), then first/last/nth also work.

That leaves the questions:
* Do we want IGNORE NULLS to work for every window function, or only a
specified subset?
* If it only works for some window functions, is that hard-coded or
driven by the catalog?
* If it works for all window functions, could it cause some
pre-existing functions to behave strangely?

Also, I'm re-thinking Dean's comments here:

http://www.postgresql.org/message-id/CAEZATCWT3=P88nv2ThTjvRDLpOsVtAPxaVPe=MaWe-x=GuhSmg@mail.gmail.com

He brings up a few good points. I will look into the frame vs. window
option, though it looks like you've already at least fixed the crash.
His other point about actually eliminating the NULLs from the window
itself is interesting, but I don't think it works. IGNORE NULLS ignores
*other* rows with NULL, but (per spec) does not ignore the current row.
That sounds awkward if you've already removed the NULL rows from the
window, but maybe there's something that could work.

And there are a few other things I'm still looking into, but hopefully
they don't raise new issues.

Regards,
Jeff Davis