On 25 January 2014 02:21, Florian Pflug <f...@phlo.org> wrote: > I've now split this up into > > invtrans_base: Basic machinery plus tests with SQL-language aggregates > invtrans_arith: COUNT(),SUM(),AVG(),STDDEV() and the like > invtrans_minmax: MIN(),MAX(),BOOL_AND(),BOOL_OR() > invtrans_collecting: ARRAY_AGG(), STRING_AGG() > invtrans_docs: Documentation >
Thanks. That makes it a bit easier to review, and hopefully easier to commit. The thing that I'm currently trying to get my head round is the null/not null, strict/not strict handling in this patch, which I find a bit odd, particularly when transfn and inv_transfn have different strictness settings: 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. 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. 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. 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. 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. Thoughts? Regards, Dean -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
