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

On Jan26, 2014, at 00:24 , David Rowley <dgrowleyml(at)gmail(dot)com> wrote:
>> On Sat, Jan 25, 2014 at 3:21 PM, Florian Pflug <fgp(at)phlo(dot)org> wrote:
>> On Jan24, 2014, at 08:47 , Dean Rasheed <dean(dot)a(dot)rasheed(at)gmail(dot)com> wrote:
>> The invtrans_minmax patch doesn't contain any patches yet - David, could
>> you provide some for these functions, and also for bool_and and bool_or?
>> We don't need to test every datatype, but a few would be nice.
>
> I've added a few regression tests for min, min and bool_or and bool_and.
> I've pushed these up to here:
>
> https://github.com/david-rowley/postgres/commits/invtrans_minmax

OK, I've pushed this to github. I haven't produced new patches yet - I
think it's probably better to wait for a few things to pile up, to make
this less of a moving target. But ultimately, this is up to Dean - Dean,
I'll do whatever makes your life as a reviewer easier.

> I did wonder if you'd want to see uses of FILTER in there as I'm thinking
> that should really be covered by the core patch and the tests here really
> should just be checking the inverse transition functions for min and max.

I don't mind the FILTER - when this gets committed, the distinction between
what was in which patch goes away anyway. What I did realize when merging
this is that we currently don't tests the case of a window which lies
fully after the current row (i.e. BETWEEN N FOLLOWING AND m FOLLOWING). We
do test BETWEEN n PRECEDING AND m PRECEDING, because the array_agg and
string_agg tests do that. So maybe some of the MIN/MAX or arithmetic tests
could exercise that case?

> As for the data types tested, I ended just adding tests for int and text
> for min and max. Let me know if you think that more should be tested.

That's sufficient for the regression tests, I think.

> As for bool_or and bool_and. I didn't think there was much extra that would
> need tested after I added 1 test. It's pretty simple code and adding anything
> extra seems like it would be testing something else.

Sounds fine to me.

best regards,
Florian Pflug