Re: Windowing Function Patch Review -> Standard Conformance

Hitoshi Harada wrote:
> 2008/11/20 David Rowley <dgrowley(at)gmail(dot)com>:
> > -- The following query gives incorrect results on the
> > -- maxhighbid column
> >
> > SELECT auctionid,
> > category,
> > description,
> > highestbid,
> > reserve,
> > MAX(highestbid) OVER (ORDER BY auctionid) AS maxhighbid,
> > MAX(reserve) OVER() AS highest_reserve
> > FROM auction_items;
> >
> > If you remove the highest_reserve column you get the correct results.
> >
> > The bug also affects MIN, AVG, COUNT, STDDEV but not SUM.
> Good report! I fixed this bug, which was by a trival missuse of
> list_concat() in the planner. This was occurred when the first
> aggregate trans func is strict and the second aggregate argument may
> be null. Yep, the argument of the second was implicitly passed to the
> first wrongly. That's why it didn't occur if you delete the second
> MAX().
>
> I added a test case with the existing data emulating yours (named
> "strict aggs") but if it is wrong, let me know.

It's not quite right yet. I'm also getting regression tests failing on
window. Let me know if you want the diffs.

I've created a query that uses the table in your regression test.
max_salary1 gives incorrect results. If you remove the max_salary2 column it
gives the correct results.

Please excuse the lack of sanity with the query. I had to do it this way to
get 2 columns with NULLs.

SELECT depname,
empno,
salary,
salary1,
salary2,
MAX(salary1) OVER (ORDER BY empno) AS max_salary1,
MAX(salary2) OVER() AS max_salary2
FROM (SELECT depname,
empno,
salary,
(CASE WHEN salary < 5000 THEN NULL ELSE salary END) AS salary1,
(CASE WHEN salary >= 5000 THEN NULL ELSE salary END) AS salary2
FROM empsalary
) empsalary;

Actual results:

depname | empno | salary | salary1 | salary2 | max_salary1 | max_salary2
-----------+-------+--------+---------+---------+-------------+-------------
sales | 1 | 5000 | 5000 | | | 4800
personnel | 2 | 3900 | | 3900 | | 4800
sales | 3 | 4800 | | 4800 | | 4800
sales | 4 | 4800 | | 4800 | | 4800
personnel | 5 | 3500 | | 3500 | | 4800
develop | 7 | 4200 | | 4200 | | 4800
develop | 8 | 6000 | 6000 | | | 4800
develop | 9 | 4500 | | 4500 | | 4800
develop | 10 | 5200 | 5200 | | | 4800
develop | 11 | 5200 | 5200 | | | 4800

Correct results:

depname | empno | salary | salary1 | salary2 | max_salary1 | max_salary2
-----------+-------+--------+---------+---------+-------------+-------------
sales | 1 | 5000 | 5000 | | 5000 | 4800
personnel | 2 | 3900 | | 3900 | 5000 | 4800
sales | 3 | 4800 | | 4800 | 5000 | 4800
sales | 4 | 4800 | | 4800 | 5000 | 4800
personnel | 5 | 3500 | | 3500 | 5000 | 4800
develop | 7 | 4200 | | 4200 | 5000 | 4800
develop | 8 | 6000 | 6000 | | 6000 | 4800
develop | 9 | 4500 | | 4500 | 6000 | 4800
develop | 10 | 5200 | 5200 | | 6000 | 4800
develop | 11 | 5200 | 5200 | | 6000 | 4800

This might be a good regression test once it's fixed.

I'm at a bit of a loss to what to do now. Should I wait for your work
Heikki? Or continue validating this patch?

The best thing I can think to do right now is continue and any problems I
find you can add regression tests for, then if we keep your regression tests
for Heikki's changes then we can validate those changes more quickly.

Any thoughts? Better ideas?

David.