On Jan14, 2014, at 11:06 , David Rowley <dgrowle...@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 
> the aggregates.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 can never 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 undo in 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 constrained to STRICT transfer 
function pairs? For non-STRICT pairs, it's up to the transfer functions to deal 
with NULL inputs however they please, no?

The logic around movedaggbase in eval_windowaggregates() seems a bit 
convoluted. Couldn't the if be moved before the code that 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 current 
row, 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

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

Reply via email to