Re: Windowing Function Patch Review -> Standard Conformance
I wrote:
All,
This is my first patch review for PostgreSQL. I did submit a patch last
commit fest (Boyer-Moore) so I feel I should review one this commit fest.
I'm quite new to PostgreSQL so please don't rely on me totally. I'll do my
best. Heikki is also reviewing this patch which makes me feel better.My aim is to get the author has much feed back as quickly as possible.
For this reason I'm going to be breaking down my reviews into the
following topics.1. Does patch apply cleanly?
2. Any compiler warnings?
3. Do the results follow the SQL standard?
4. Performance Comparison, does it perform better than alternate ways of
doing things. Self joins, sub queries etc.5. Performance, no comparison. How does it perform with larger tables?
I regret starting the individual thread for each function now. I was
expecting them to get longer. So I'll use this one for the remainder of the
standard conformance tests.
I covered all of the non-aggregate functions in previous tests. I will do
more final tests on them soon. Tonight I started testing the aggregate
functions with NULLable columns and I've found a small problem. I'll just
post my test script to make it easy for you to see what I mean.
BEGIN WORK;
CREATE TABLE auction_items (
auctionid INT NOT NULL,
category VARCHAR(32) NOT NULL,
description VARCHAR(128) NOT NULL,
highestbid INT NULL, -- NULL when no bids
reserve INT NULL, -- NULL when no reserve
started TIMESTAMP NOT NULL, -- When the action opened
days INT NOT NULL, -- how many days the auction runs.
CHECK(days > 0),
PRIMARY KEY (auctionid)
);
INSERT INTO auction_items
VALUES(1,'BIKE','Broken Bicycle',NULL,100,NOW(),10);
INSERT INTO auction_items
VALUES(2,'BIKE','Mountain Bike',120,NULL,NOW(),10);
INSERT INTO auction_items
VALUES(3,'BIKE','Racer',230,NULL,NOW(),7);
INSERT INTO auction_items
VALUES(4,'CAR','Bmw M3',5000,6000,NOW(),7);
INSERT INTO auction_items
VALUES(5,'CAR','Audi S3',NULL,6500,NOW(),7);
INSERT INTO auction_items
VALUES(6,'CAR','Holden Kingswood',NULL,2000,NOW(),7);
COMMIT;
-- 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.
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.
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.
Keep up the good work.
David.
Import Notes
Reply to msg id not found:
2008/11/20 David Rowley <dgrowley@gmail.com>:
-- The following query gives incorrect results on the
-- maxhighbid columnSELECT 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
Attachments:
window_functions.patch.20081124-1.gzapplication/x-gzip; name=window_functions.patch.20081124-1.gzDownload
�wX*I window_functions.patch.20081124-1 ��kw�F�0�Y�+��iC��(�E�3z����<�����$�)�!@��$�o��W4@P�������@w�����U����(Y���b
�};Z���+�����������������������[�>����^DoG��u�c7z}v�]�Q��g�etz���,����}������Ger�O�P�n�nE�{�OQ�}�Z����>���+��/��4�,�w�x��=y�
�����/��g�k���U~m��$�&�#��*�e�NoU�+����&��C6L��d���2K�(��!x
��3J�A2�F G�x0�
����� N��:)�5���&�.g��\�����D�L�h�%���e���<�O�`,�`�x�����
�:�`�f�7���c��j�(����)J DQf�Bw$��2��Q�G��ZV ppa�3�zU��l2�9k�r�&����8�&�����$/�h�@��i>�D�|T�x�����7i2T�������}�����K5�����0��������$�kt���������L�I�S_�Z���I�U\����U���� ��#��4�M��4��m�_V��P����Q�N��U����f�u���K�����:��l�1w����2���@$�����Q/���t
�xyJ'�d�"j�,�i�O?N�iQ0��o�@�@���SD���&�N&E�6=�N�q�������]�g���1���B�>��8���o
p�i�$�g2�K�?�M�� �h�_|�������q�D����4E��lL������K�q��l�s���i`��!��1�����J��wN�(}�nqW{��`� ���x�B�'/������]�B�k���K�n�|,X��4�>$�a�5�@��eN;�(�Q�Q��,���az
vp6�3�)��Ok�����w_c��������"-��?:ma�^%��7�O]{���e��>V��c����b\3���()�����g�)�*�X����|l��H^y� ��9���S���w���k��`6��I�o!�i�Jt|U�e-Q�_L�\e�J����q����@|����[z��k���Wjw��i���L`�?����� �t�-����}���I����������S��9x|��
m�����r\�g��7��|
����B�?�p7�ql\�<�lIB�������A4����;g���X�n���q^|>��~�;K<����\���5������g��� ��^�sy��&��=�z����w�tX�z��A/�Vo-��l��O���s�����}:~y����w��`~�SC���m�5x��eX_:���-�<%?���x�w�����������n������~�^������3�E������������������=d/a�����Sn���Cc��o��������������?d��������������x��[�$�=;:<zu|��F�����vs�(����f���[[x?�N�=�c�������b���y������������������������x��Fo'��X�Xx'��|s������?n;�����s���dko-��,�U||����?�<�I/�m&�y����������>��{[k�����<������g�8���A���N1��+�qV��Y���L�� ���� /�.E'C�����������et�_��l�8JF#2��tE����b�t\���h���n�"��f�kc������0V���wt8JfEc������qZ�
c�R#�f.������y6�n7������40���l�~{v��m����l� -�X����6��)q��@�8�*f�a5�xE��
J�����"+��`�k�+J H�Y�P��/8A�U_�E���8B=
�_Y����L
}y\gx��G��) Ym
��zU?�4R{4��_�bl��M�Y�i~h��Z�@���t�F������C
��j2hh=!�|R;��/�\���F�)M�c���O�`�������1)�� 9���!i�!`H���rV��,)��S�"�7�D��\����@Q ��B�wyf�Q���N'ePH�^��j�h��#�mr�k��V�"dW.�����b�Gt���u���������CY`�@�u;�
��d 7u,��*� �4�o'��#L��?���[��!z����Z ��'���R��i����C�;A1�jG>��|��B�E��dP��G����DW�)?��>��1��ng J�6��}�c`�R������c�<��i�Q��8&��K��O
�)R����t��o�L��(����#f@j :j�����; E/ ������J�;*���+�GQ��}4���x��z(\E%KT�|'E�y�(@�����k[��'d�}��c�Lo'�Q�������k+��;��f{S|b��w��h�OVrD%�{J��(L��`T�%R]��T&<�����H��|t��n�Q2E)8�P���%P�A?�t)r���at� AP'��&p�a��ftW�!�F�4�O�+�f
��sW�c���6�/r�`` 0H�x��
B���r�$��4�D���>M�,�F@�Q"B����)Th�4�N��k �����=�GK������7�@��[�\w��.�nG4�n�� }��>����H�L�T<��=�~����<