On Fri, Jun 28, 2013 at 12:29 PM, Nicholas White <n.j.wh...@gmail.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.

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

Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:

Reply via email to