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

On Jan17, 2014, at 20:34 , David Rowley <dgrowleyml@gmail.com> wrote:
> On Fri, Jan 17, 2014 at 3:05 PM, Florian Pflug <fgp@phlo.org> wrote:
>
>> I've now shuffled things around so that we can use inverse transition functions
>> even if only some aggregates provide them, and to allow inverse transition
>> functions to force a restart by returning NULL. The rules regarding NULL handling
>> are now the following
>
> Maybe this is me thinking out loud, but I'm just thinking about the numeric case again.
> Since the patch can now handle inverse transition functions returning NULL when they
> fail to perform inverse transitions, I'm wondering if we could add an "expectedscale"
> to NumericAggState, set it to -1 initially, when we get the first value set the
> expectedscale to the dscale of that numeric, then if we get anything but that value
> we'll set the expectedscale back to -1 again, if we are asked to perform an inverse
> transition with a expectedscale as -1 we'll return null, otherwise we can perform
> the inverse transition...

You could do better than that - the numeric problem amounts to tracking the maximum
scale AFAICS, so it'd be sufficient to restart if we remove a value whose scale equals
the current maximum. But if we do SUM(numeric) at all, I think we should do so
without requiring restarts - I still think the overhead of tracking the maximum scale
within the aggregated values isn't that bad. If we zero the dscale counters lazily,
the numbers of entries we have to zero is bound by the maximum dscale we encounter.
Since actually summing N digits is probably more expensive than zeroing them, and since
we pay the zeroing overhead only once per aggregation but save potentially many
summations, I'm pretty sure we come out ahead by quite some margin.

It'd be interesting to do float() similar to the way you describe, though. We might
not be able to guarantee that we yield exactly the same result as without inverse
aggregation, but we might be able to bound the error. That might make it acceptable
to do this - as Kevin pointed out, float is always an approximation anyway. I haven't
really thought that through, though...

Anyway, with time running out fast if we want to get this into 9.4, I think we should
focus on getting this into a committable state right now.

I've started to look over what you've pushed to github, and it looks mostly fine.
I have a few comments - mostly cosmetic stuff - that I'll post once I finished reading
through it. I also plan to do some basic performance testing to verify that my
reshuffling of eval_windowaggregates() doesn't hurt aggregates without an inverse
transition function.

best regards,
Florian Pflug