Re: Windowing Function Patch Review -> Standard Conformance

From: "Hitoshi Harada" <umi(dot)tanuki(at)gmail(dot)com>
To: "David Rowley" <dgrowley(at)gmail(dot)com>
Cc: pgsql-hackers(at)postgresql(dot)org, heikki(dot)linnakangas(at)enterprisedb(dot)com
Subject: Re: Windowing Function Patch Review -> Standard Conformance
Date: 2008-11-24 07:48:55
Message-ID: e08cc0400811232348qa31d939x9b1c0c5aa34e66b3@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

patch-2

2008/11/24 Hitoshi Harada <umi(dot)tanuki(at)gmail(dot)com>:
> 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.
>
>>
>> I've also had a little look at the documentation included in the patch.
>> At first scan the only thing I can see that is wrong is
>>
>> +
>> + <para>
>> + Window functions are put in the <command>SELECT</command> list.
>> + It is forbidden anywhere else such as <literal>GROUP BY</literal>,
>>
>> I think this needs to be rewritten as they are allowed in the ORDER BY
>> clause, works in patch and spec says the same.
>>
>> Maybe it should read something more like:
>>
>> "Window functions are only permitted in the <command>SELECT</command> and
>> the <command>ORDER BY</command> clause of the query. They are forbidden
>> anywhere else..." etc.
> You're right. I modified this part, with <literal> tag instead of
> <command>. I am not sure about the clear difference of <command> and
> <literal> but followed what the surroundings tell.
>
>> I won't be around until Monday evening (Tuesday morning JST). I'll pickup
>> the review again there. It's really time for me to push on with this review.
>> The patch is getting closer to getting the thumbs up from me. I'm really
>> hoping that can happen on Monday. Then it's over to Heikki for more code
>> feedback.
>
> This time I only fixed some bugs and rebased against the HEAD, but did
> not refactored. I can figure out some part of what Heikki claimed but
> not all, so I'd like to see what he will send and post another patch
> if needed.
>
> Regards,
>
>
> --
> Hitoshi Harada
>

--
Hitoshi Harada

Attachment Content-Type Size
window_functions.patch.20081124-2.gz application/x-gzip 92.8 KB

In response to

Browse pgsql-hackers by date

  From Date Subject
Next Message Tao Ma 2008-11-24 07:57:20 huge query tree cost too much time to copyObject()
Previous Message Hitoshi Harada 2008-11-24 07:48:24 Re: Windowing Function Patch Review -> Standard Conformance