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

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:

I took a good look at this today.

* It fails for offset of 0 with IGNORE NULLS. Fixed (trivial).

* The tests are locale-sensitive. Fixed (trivial).

* The leadlag_common function is just way too long. I refactored the
IGNORE NULLS code into it's own function (win_get_arg_ignore_nulls())
with the same API as WinGetFuncArgInPartition. This cleans things up
substantially, and makes it easier to add useful comments.

* "We're implementing the former semantics, so we'll need to correct
slightly" sounds arbitrary, but it's mandated by the standard. That
should be clarified.

* I did a lot of other refactoring within win_get_arg_ignore_nulls for
the constant case. I'm not done yet, and I'm not 100% sure it's a net
gain, because the code ended up a little longer. But the previous
version was quite hard to follow because of so many special cases around
positive versus negative offsets. For instance, having the negative
'next' value in your code actually means something quite different than
when it's positive, but it took me a while to figure that out, so I made
it into two variables. I hope my code is moving it in a direction that's
easier for others to understand.

Please let me know if you think I am making things worse with my
refactorings. Otherwise I'll keep working on this and hopefully get it
committable soon.

The attached patch is still a WIP; just posting it here in case you see
any red flags.

Regards,
	Jeff Davis