Thread: patch implementing the multi-argument aggregates (SOC project)
Hello All,
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)
I currently fully enabled the creation and usage of PolyArgAggs, like this
CREATE OR REPLACE FUNCTION sum2_trans (anyelement, anyelement,anyelement)
RETURNS anyelement
AS 'SELECT $1+$2+$3;'
LANGUAGE SQL;
CREATE AGGREGATE sum2( anyelement,anyelement)
(SFUNC=sum2_trans,
STYPE=anyelement,
INITCOND=0);
postgres=# select * from xx;
a | b | c
---+---+-----
1 | 2 | 1.2
2 | 3 | 2.3
3 | 4 | 3.4
4 | 5 | 3.4
(4 rows)
postgres=# select sum2(a,b) from xx;
sum2
------
24
(1 row)
postgres=# select sum2(a,b) from xx group by c;
sum2
------
3
16
5
(3 rows)
Also I implemented the full set of PolyArgAggs from SQL 2003
REGR_SXX(), REGR_SYY(), REGR_SXY(), REGR_AVGX(), REGR_AVGY(), REGR_R2(),
CORR(), COVAR_POP(),COVAR_SAMP(), REGR_SLOPE(), REGR_INTERCEPT(),
REGR_COUNT()
Example:
template1=# select regr_r2(pronamespace::int::numeric,prolang::int::numeric) from pg_proc;
regr_r2
------------------------
0.14226915125082682802
(1 row)
template1=# select regr_r2(pronamespace::int,prolang::int) from pg_proc;
regr_r2
-------------------
0.142269151250827
(1 row)
template1=# select regr_count(pronamespace::int,prolang::int) from pg_proc;
regr_count
------------
1956
(1 row)
The existing regression tests are working.
What is still missing:
1) The additional regression tests testing the PolyArgAggs.
2) There is a couple of moments in the PG core related part of patch,
where I'm not completely sure that I'm doing the right thing...
(they are commented in the special way in the patch (if somebody can
clarify them for me, I would be happy))
Any comments are very welcome.
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
Attachment
"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
Some small clean-up of the patch...
+ implementing the Tom's idea of minimizing the copying of the data inside
advance_transition_function by using the temporary FunctionCallInfoData
(now the computed arguments of the aggregates are putted directly into
proper fcinfo.args fields, ready for the transition function call).
On Mon, 24 Jul 2006, Tom Lane wrote:
> 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.
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
Attachment
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
"Sergey E. Koposov" <math@sai.msu.ru> writes:
> Some small clean-up of the patch...
> + implementing the Tom's idea of minimizing the copying of the data inside
> advance_transition_function by using the temporary FunctionCallInfoData
> (now the computed arguments of the aggregates are putted directly into
> proper fcinfo.args fields, ready for the transition function call).
I've committed the core parts of this, but not yet the newly added
standard aggregates (it seemed like a good idea to break the thing
into smaller chunks for review). Will work on those next.
regards, tom lane