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

On Jan16, 2014, at 02:32 , Florian Pflug <fgp@phlo.org> wrote:
> On Jan14, 2014, at 17:39 , Florian Pflug <fgp@phlo.org> wrote:
>> 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,
cannotundo in this case, please rescan". If we allowed NULL as just another state value now, we couldn't easily undo
thatlater, 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? 
>
> I modified the patch to do this, and ran into a problem. Currently, aggregates with state type "internal" cannot have
astrict transfer function, even if they behave like they did, i.e. ignore non-NULL inputs. This is because the only
possibleinitial value for state type "internal" is NULL, and it's up to the transfer function to create the state -
usuallyupon seeing the first non-NULL input. Now, currently that isn't a huge problem - the transfer function simply
hasto check for NULL input values itself, and if it's indeed conceptually strict, it just returns in this case. 
>
> With inverse transfer functions, however, each such pair of forward and inverse transfer functions would also need to
duplicatethe NULL-counting logic from nodeWindowAgg.c if it want's to be conceptually strict, i.e. ignore NULL inputs,
butreturn NULL if there are *only* NULL inputs (or no inputs at all). That seems like a lot of duplicated code in the
longrun. 
>
> In essence, what we'd want for strict pairs of forward and inverse transfer functions is for the forward transfer
functionto be strict in all arguments except the state, and the inverse to be strict according to the usual definition.
Wecan't express that property of the forward transfer function within pg_proc, but we don't really have to - a local
hackin nodeWindowAgg.c suffices. So what I'm proposing is: 
>
> We allow the forward transfer function to be non-strict even if the inverse is strict, but only if the initial value
isNULL. In that case we behave as if the forward transfer function was strict, except that upon seeing the first
non-NULLinput we call it with a NULL state. The return value must still be non-NULL in all cases. 

Ok, I tried this and it worked out quite OK.

Updated patch is attached. It passes regression tests, but those currently don't seem to include any aggregates which
*don't*ignore NULL values, so that case is probably untested. Also, it allows non-strict forward transfer functions
togetherwith strict inverse transfer functions even for non-NULL initial states now. It seemed rather pedantic to
forbidthis. 

BTW, as it stands, the patch currently uses the inverse transition function only when *all* the aggregates belonging to
onewindow clause provide one. After working with the code a bit, I think that a bit of reshuffling would allow that
distinctionto be made on a per-aggregate basis. The question is, is it worth it? 

best regards,
Florian Pflug