Re: [PATCH] Negative Transition Aggregate Functions (WIP)

On Jan14, 2014, at 11:06 , David Rowley <dgrowleyml@gmail.com> wrote:
> Here's a patch which removes sum(numeric) and changes the documents a little to remove a reference to using
sum(numeric)to workaround the fact that there's no inverse transitions for sum(float). I also made a small change in
theaggregates.sql tests to check that the aggfnoid <= 9999. 

I've looked over the patch, here a few comments.

For STRICT pairs of transfer and inverse transfer functions we should complain if any of them ever return NULL. That
cannever be correct anyway, since a STRICT function cannot undo a non-NULL -> NULL transition. 

The same goes for non-STRICT, at least if we ever want to allow an inverse transfer function to indicate "Sorry, cannot
undoin this case, please rescan". If we allowed NULL as just another state value now, we couldn't easily undo that
later,so we'd rob ourselves of the obvious way for the inverse transfer function to indicate this condition to its
caller.

The notnullcount machinery seems to apply to both STRICT and non-STRICT transfer function pairs. Shouldn't that be
constrainedto STRICT transfer function pairs? For non-STRICT pairs, it's up to the transfer functions to deal with NULL
inputshowever they please, no? 

The logic around movedaggbase in eval_windowaggregates() seems a bit convoluted. Couldn't the if be moved before the
codethat pulls aggregatedbase up to frameheadpos using the inverse aggregation function?  

Also, could we easily check whether the frames corresponding to the individual rows are all either identical or
disjoint,and don't use the inverse transfer function then? Currently, for a frame which contains either just the
currentrow, or all the current row's peers, I think we'd use the inverse transfer function to fully un-add the old
frame,and then add back the new frame. 

best regards,
Florian Pflug