2014-10-25 15:43 GMT+07:00 Pavel Stehule <pavel.steh...@gmail.com>: > > > 2014-10-25 10:16 GMT+02:00 Ali Akbar <the.ap...@gmail.com>: > >> makeArrayResult1 - I have no better name now >>>> >>>> I 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_transfn >>>> >>> >> Fixed. >> >> >>> 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