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

On 28 January 2014 20:16, Florian Pflug <fgp(at)phlo(dot)org> wrote:
> On Jan27, 2014, at 23:28 , Dean Rasheed <dean(dot)a(dot)rasheed(at)gmail(dot)com> wrote:
>> strict transfn vs non-strict inv_transfn
>> ----------------------------------------
>>
>> This case is explicitly forbidden, both in CREATE AGGREGATE and in the
>> executor. To me, that seems overly restrictive --- if transfn is
>> strict, then you know for sure that no NULL values are added to the
>> aggregate state, so surely it doesn't matter whether or not
>> inv_transfn is strict. It will never be asked to remove NULL values.
>>
>> I think there are definite use-cases where a user might want to use a
>> pre-existing function as the inverse transition function, so it seems
>> harsh to force them to wrap it in a strict function in this case.
>>
>> AFAICS, the code in advance/retreat_windowaggregate should just work
>> if those checks are removed.
>
> True. It didn't use to in earlier version of the patch because the advance
> and retreat functions looked at the strict settings directly, but now that
> there's an ignore_nulls flag in the executor node, the only requirement
> should be that ignore_nulls is set if either of the transition functions
> is strict.
>
> I'm not sure that the likelihood of someone wanting to combine a strict
> forward with a non-strict inverse function is very hight, but neither
>

Me neither, but the checks to forbid it aren't adding anything, and I
think it's best to make it as flexible as possible.

>> non-strict transfn vs strict inv_transfn
>> ----------------------------------------
>>
>> At first this seems as though it must be an error --- the forwards
>> transition function allows NULL values into the aggregate state, and
>> the inverse transition function is strict, so it cannot remove them.
>>
>> But actually what the patch is doing in this case is treating the
>> forwards transition function as partially strict --- it won't be
>> passed NULL values (at least in a window context), but it may be
>> passed a NULL state in order to build the initial state when the first
>> non-NULL value arrives.
>
> Exactly.
>
>> It looks like this behaviour is intended to support aggregates that
>> ignore NULL values, but cannot actually be made to have a strict
>> transition function. I think typically this is because the aggregate's
>> initial state is NULL and it's state type differs from the type of the
>> values being aggregated, and so can't be automatically created from
>> the first non-NULL value.
>
> Yes. I added this because the alternative would haven been to count
> non-NULL values in most of the existing SUM() aggregates.
>
>> That all seems quite ugly though, because now you have a transition
>> function that is not strict, which is passed NULL values when used in
>> a normal aggregate context, and not passed NULL values when used in a
>> window context (whether sliding or not), except for the NULL state for
>> the first non-NULL value.
>
> Ugh, true. Clearly, nodeAgg.c needs to follow what nodeWindowAgg.c does
> and skip NULL inputs if the aggregate has a strict inverse transition
> function. That fact that we never actually *call* the inverse doesn't
> matter. Will fix that once we decided on the various strictness issues.
>

Yuk!

>> I'm not sure if there is a better way to do it though. If we disallow
>> this case, these aggregates would have to use non-strict functions for
>> both the forward and inverse transition functions, which means they
>> would have to implement their own non-NULL value counting.
>> Alternatively, allowing strict transition functions for these
>> aggregates would require that we provide some other way to initialise
>> the state from the first non-NULL input, such as a new initfn.
>
> I'm not sure an initfn would really help. It would allow us to return
> to the initial requirement that the strict settings match - but you
> already deem the check that the forward transition function can only
> be strict if the inverse is also overly harsh, so would that really be
> an improvement? It's also cost us an additional pg_proc entry per aggregate.
>
> Another idea would be to have an explicit nulls=ignore|process option
> for CREATE AGGREGATE. If nulls=process and either of the transition
> functions are strict, we could either error out, or simply do what
> normal functions calls do and pretend they return NULL for NULL inputs.
> Not sure how the rule that forward transition functions may not return
> NULL if there's an inverse transition function would fit in if we do
> the latter, though.
>
> The question is - is it worth it the effort to add that flag?
>

Yeah, I thought about a flag too. I think that could work quite nicely.

Basically where I'm coming from is trying to make this as flexible as
possible, without pre-judging the kinds of aggregates that users may
want to add.

One use-case I had in mind upthread was suppose you wanted to write a
custom version of array_agg that only collected non-NULL values. With
such a flag, that would be trivial, but with the current patch you'd
have to (count-intuitively) wrap the inverse transition function in a
strict function.

Another use-case I can imagine is suppose you wanted a custom version
of sum() that returned NULL if any of the input values were NULL. If
you knew that most of your data was non-NULL, you might still wish to
benefit from an inverse transition function. Right now the patch won't
allow that because of the error in advance_windowaggregate(), but
possibly that could be relaxed by forcing a restart in that case. If
I've understood correctly that should give similar to current
performance if NULLs are present, and enhanced performance as the
window moved over non-NULL data.

In that second case, it would also be nice if you could simply re-use
the existing sum forward and inverse transition functions, with a
different null-handling flag.

Also, in cases where the forwards transition function cannot be made
strict (e.g., it's state type is internal) and there is no inverse
transition function, there might be a small performance gain to be had
from not calling the transition function with NULL values, rather than
have it ignore them.

So I think an ignore-nulls flag would have real benefits, as well as
being a cleaner design than relying on a strict inverse transition
function.

What do others think?

Regards,
Dean