On Apr13, 2014, at 20:23 , Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Dean Rasheed <dean.a.rasheed@gmail.com> writes:
>> OK, I'm marking this ready for committer attention, on the
>> understanding that that doesn't include the invtrans_minmax patch.
>
> I've finished reviewing/revising/committing this submission.
Cool! Thanks!
> Some notes:
>
> * I left out the EXPLAIN ANALYZE statistics, as I still feel that they're
> of little if any use to regular users, and neither very well defined nor
> well documented. We can revisit that later of course.
>
> * I also left out the table documenting which aggregates have this
> optimization. That's not the kind of thing we ordinarily document,
> and it seems inevitable to me that such a table would be noteworthy
> mostly for wrong/incomplete/obsolete information in the future.
I can live with each leaving each of these out individually, but leaving
them both out means there's now no way of determining how a particular
sliding window aggregation will behave. That leaves our users a bit
out in the cold IMHO.
For example, with this patch you'd usually want to write SUM(numeric_value::numeric(n,m))
instead of SUM(numeric_value)::numeric(n,m)
in a sliding window aggregation, and there should be *some* way for
users to figure this out.
We have exhaustive tables of all the functions, operators and aggregates
we support, and maintenance of those seems to work well for the most
part. Why would a table of invertible aggregates be any different? If
it's the non-locality that bothers you - I guess the same information
could be added to the descriptions of the individual aggregates, instead
of having one big table.
> * I rejected the invtrans_collecting sub-patch altogether. I didn't
> like anything about the idea of adding a poorly-documented field to
> ArrayBuildState and then teaching some random subset of the functions
> using that struct what to do with it. I think it would've been simpler,
> more reliable, and not that much less efficient to just do memmove's in
> shiftArrayResult. The string-aggregate changes were at least more
> localized, but they still seemed awfully messy. And there's a bigger
> issue: these aggregates have to do O(N) work per row for a frame of length
> N anyway, so it's not clear to me that there's any big-O gain to be had
> from making them into moving aggregates.
Yeah, those probably aren't super-usefull. I initially added array_agg
because with the nonstrict-forward/strict-reverse rule still in place,
there wouldn't otherwise have been an in-core aggregate which exercises
the non-strict case, which seemed like a bad idea. For the string case -
I didn't expect that to turn out to be *quite* this messy when I started
implementing it.
> Anyway, this is nice forward progress for 9.4, even if we get no further.
Yup! Thanks to everybody who made this happens!
best regards,
Florian Pflug