Re: Function array_agg(array) - Mailing list pgsql-hackers
| From | Ali Akbar |
|---|---|
| Subject | Re: Function array_agg(array) |
| Date | |
| Msg-id | CACQjQLqCCVWcxzCPFpAcm3b=oSkxbQ7KhG+fHGBMaaUaT-08=A@mail.gmail.com Whole thread Raw |
| In response to | Re: Function array_agg(array) (Pavel Stehule <pavel.stehule@gmail.com>) |
| Responses |
Re: Function array_agg(array)
|
| List | pgsql-hackers |
2014-10-25 15:43 GMT+07:00 Pavel Stehule <pavel.stehule@gmail.com>:
2014-10-25 10:16 GMT+02:00 Ali Akbar <the.apaan@gmail.com>:makeArrayResult1 - I have no better name nowI found one next minor detail.you reuse a array_agg_transfn function. Inside is a message "array_agg_transfn called in non-aggregate context". It is not correct for array_agg_anyarray_transfnFixed.probably specification dim and lbs in array_agg_finalfn is useless, when you expect a natural result. A automatic similar to array_agg_anyarray_finalfn should work there too.
makeMdArrayResultArray and appendArrayDatum should be marked as "static"ok, marked all of that as static.I am thinking so change of makeArrayResult is wrong. It is developed for 1D array. When we expect somewhere ND result now, then we should to use makeMdArrayResult there. So makeArrayResult should to return 1D array in all cases. Else a difference between makeArrayResult and makeMdArrayResult hasn't big change and we have a problem with naming.I'm thinking it like this:- if we want to accumulate array normally, use accumArrayResult and makeArrayResult. If we accumulate scalar the result will be 1D array. if we accumulate array, the resulting dimension is incremented by 1.- if we want, somehow to affect the normal behavior, and change the dimensions, use makeMdArrayResult.Searching through the postgres' code, other than internal use in arrayfuncs.c, makeMdArrayResult is used only in src/pl/plperl/plperl.c:plperl_array_to_datum, while converting perl array to postgres array.So if somehow we will accumulate array other than in array_agg, i think the most natural way is using accumArrayResult and then makeArrayResult.ok, there is more variants and I can't to decide. But I am not satisfied with this API. We do some wrong in structure. makeMdArrayResult is now ugly.
One approach that i can think is we cleanly separate the structures and API. We don't touch existing ArrayBuildState, accumArrayResult, makeArrayResult and makeMdArrayResult, and we create new functions: ArrayBuildStateArray, accumArrayResultArray, makeArrayResultArray and makeArrayResultArrayCustom.
But if we do that, the array(subselect) implementation will not be as simple as current patch. We must also change all the ARRAY_SUBLINK-related accumArrayResult call.
Regarding current patch implementation, the specific typedefs are declared as:
+typedef struct ArrayBuildStateScalar
+{
+ ArrayBuildState astate;
...
+} ArrayBuildStateArray;
so it necessities rather ugly code like this:
+ result->elemtype = astate->astate.element_type;
Can we use C11 feature of unnamed struct like this? :
+typedef struct ArrayBuildStateScalar
+{
+ ArrayBuildState;
...
+} ArrayBuildStateArray;
so the code can be a little less ugly by doing it like this:
+ result->elemtype = astate->element_type;
I don't know whether all currently supported compilers implements this feature..
Can you suggest a better structure for this?
If we can't settle on a better structure, i think i'll reimplement array_agg without the performance improvement, using deconstruct_array and unchanged accumArrayResult & makeMdArrayResult.
Thanks,
-- Ali Akbar
pgsql-hackers by date: