Обсуждение: Re: [PATCHES] patch implementing the multi-argument aggregates (SOC project)
"Sergey E. Koposov" <math@sai.msu.ru> writes: > Since the feature freeze is in a few days, I'm sending the first iteration > of my patch implementing the multi-argument aggregates (PolyArgAgg) (SOC > project) This patch is nowhere near ready for submission :-(. Most of the comments seem to be "I don't know what to do here" ... A general hint on the polymorphic stuff is that you should be able to exactly duplicate what's done for polymorphic functions --- or even better, get rid of the separate code for aggregates and just invoke the existing logic for functions. (You might need to refactor code a little bit to separate out the common functionality.) Instead of copying data inside advance_transition_function, it might be better for the caller to store the values into the right fields of a temporary FunctionCallInfoData struct, and just pass that to advance_transition_function. The names for the new aggregates seem a bit, how to say, terse and unfriendly. SQL generally tends to a more verbose style of naming. regards, tom lane
On Mon, 24 Jul 2006, Tom Lane wrote: > "Sergey E. Koposov" <math@sai.msu.ru> writes: >> Since the feature freeze is in a few days, I'm sending the first iteration >> of my patch implementing the multi-argument aggregates (PolyArgAgg) (SOC >> project) > > This patch is nowhere near ready for submission :-(. Most of the :-( But now at least I know that... > comments seem to be "I don't know what to do here" ... > No that's not quite true... I have only ~ 2-3 such comments, all others just express that I marked the places where I've had any little doubts and which I'll check additionally... > A general hint on the polymorphic stuff is that you should be able to > exactly duplicate what's done for polymorphic functions --- or even > better, get rid of the separate code for aggregates and just invoke > the existing logic for functions. (You might need to refactor code > a little bit to separate out the common functionality.) > > Instead of copying data inside advance_transition_function, it might > be better for the caller to store the values into the right fields > of a temporary FunctionCallInfoData struct, and just pass that to > advance_transition_function. Thank you for the hints, I'll think about them... > The names for the new aggregates seem a bit, how to say, terse and > unfriendly. SQL generally tends to a more verbose style of naming. > The names for the functions came from SQL 2003 standart... Regards, Sergey ******************************************************************* Sergey E. Koposov Max Planck Institute for Astronomy/Sternberg Astronomical Institute Tel: +49-6221-528-349 Web: http://lnfm1.sai.msu.ru/~math E-mail: math@sai.msu.ru
Tom Lane <tgl@sss.pgh.pa.us> writes: > This patch is nowhere near ready for submission :-(. Most of the > comments seem to be "I don't know what to do here" ... Just to be clear, I think what Tom's saying it's not ready to be *applied*. Sending patches to this list early and often during development is generally encouraged. -- Gregory Stark EnterpriseDB http://www.enterprisedb.com
Gregory Stark <gsstark@mit.edu> writes: > Tom Lane <tgl@sss.pgh.pa.us> writes: >> This patch is nowhere near ready for submission :-(. Most of the >> comments seem to be "I don't know what to do here" ... > Just to be clear, I think what Tom's saying it's not ready to be *applied*. > Sending patches to this list early and often during development is generally > encouraged. Indeed, but if that was the point we should have been seeing drafts of this patch long ago. I interpreted Sergey's submission as "getting this in before feature freeze", and in that context the bar is higher. If a patch isn't pretty nearly ready to be applied when the freeze deadline arrives, we won't wait around for it. regards, tom lane