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

On Fri, Jun 28, 2013 at 12:29 PM, Nicholas White <n(dot)j(dot)white(at)gmail(dot)com> wrote:
>> This patch will need to be rebased over those changes
>
> See attached -

Thanks. But I see a few issues...

+ [respect nulls]|[ignore nulls]

You fixed one of these but missed the other.

<replaceable class="parameter">default</replaceable> are evaluated
with respect to the current row. If omitted,
<replaceable class="parameter">offset</replaceable> defaults to 1 and
- <replaceable class="parameter">default</replaceable> to null
+ <literal>IGNORE NULLS</> is specified then the function will
be evaluated
+ as if the rows containing nulls didn't exist.
</entry>
</row>

This looks like you dropped a line during cut-and-paste.

+ null_values = (Bitmapset *) WinGetPartitionLocalMemory(
+ winobj,
+ BITMAPSET_SIZE(words_needed));
+ Assert(null_values);

This certainly seems ugly - isn't there a way to accomplish this
without having to violate the Bitmapset abstraction?

+ * A slight edge case. Consider:
+ *
+ * A | lag(A, 1)
+ * 1 |
+ * 2 | 1
+ * | ?

pgindent will reformat this into oblivion. Use ----- markers around
the comment block as done elsewhere in the code where this is an
issue, or don't use ASCII art.

I haven't really reviewed the windowing-related code in depth; I
thought Jeff might jump back in for that part of it. Jeff, is that
something you're planning to do?

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company