Andrew Gierth <and...@tao11.riddles.org.uk> writes:
> So I'm looking to commit this, and here's my comments so far:
I took a quick look over this. I agree with your nitpicks, and have
a couple more:
* Please run it through pgindent. That will, at a minimum, remove some
gratuitous whitespace changes in this patch. I think it'll also expose
some places where you need to change the code layout to prevent pgindent
from making it look ugly. Notably, this:
actives[nActive].uniqueOrder = list_concat_unique(
list_copy(wc->partitionClause), wc->orderClause);
is not per project style for function call layout. Given the other
comment about using list_union, I'd probably lay it out like this:
actives[nActive].uniqueOrder = list_union(wc->partitionClause,
wc->orderClause);
* The initial comment in select_active_windows,
/* First, make a list of the active windows */
is now seriously inadequate as a description of what the subsequent
loop does; it needs to be expanded. I'd also say that it's not
building a list anymore, but an array. Further, there needs to be
an explanation of why what it's doing is correct at all ---
list_union doesn't make many promises about the order of the resulting
list (nor did the phraseology with list_concat_unique), but what we're
doing below certainly requires that order to have something to do with
the window semantics.
* I'm almost thinking that changing to list_union is a bad idea, because
that obscures the fact that we're relying on the relative order of
elements in partitionClause and orderClause to not change; any future
reimplementation of list_union would utterly break this code. I'm also a
bit suspicious as to whether the code is even correct; does it *really*
match what will happen later when we create sort plan nodes? (Maybe it's
fine; I haven't looked.)
* The original comments also made explicit that we were not considering
framing options, and I'm not too happy that that disappeared.
* It'd be better if common_prefix_cmp didn't cast away const.
regards, tom lane
