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

From: Jeff Davis <pgsql(at)j-davis(dot)com>
To: Nicholas White <n(dot)j(dot)white(at)gmail(dot)com>
Cc: Bruce Momjian <bruce(at)momjian(dot)us>, Robert Haas <robertmhaas(at)gmail(dot)com>, Alvaro Herrera <alvherre(at)2ndquadrant(dot)com>, Troels Nielsen <bn(dot)troels(at)gmail(dot)com>, Hitoshi Harada <umi(dot)tanuki(at)gmail(dot)com>, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>, "pgsql-hackers(at)postgresql(dot)org" <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: Request for Patch Feedback: Lag & Lead Window Functions Can Ignore Nulls
Date: 2014-07-07 04:11:36
Message-ID: 1404706296.9081.163.camel@jeff-desktop
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

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.

Jeff Davis

Attachment Content-Type Size
lead_lag_jeff_20140706.patch text/x-patch 44.0 KB

In response to


Browse pgsql-hackers by date

  From Date Subject
Next Message Rajeev rastogi 2014-07-07 04:20:12 Re: [RFC: bug fix?] Connection attempt block forever when the synchronous standby is not running
Previous Message Fujii Masao 2014-07-07 03:50:17 Re: 9.4 documentation: duplicate paragraph in logical decoding example