Re: Function array_agg(array)

2014-10-25 8:33 GMT+02:00 Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com>:

>
>
> 2014-10-25 8:19 GMT+02:00 Ali Akbar <the(dot)apaan(at)gmail(dot)com>:
>
>> 2014-10-25 10:29 GMT+07:00 Ali Akbar <the(dot)apaan(at)gmail(dot)com>:
>>
>>> I fixed small issue in regress tests and I enhanced tests for varlena
>>>> types and null values.
>>>
>>> Thanks.
>>>
>>> it is about 15% faster than original implementation.
>>>
>>> 15% faster than array_agg(scalar)? I haven't verify the performance, but
>>> because the internal array data and null bitmap is copied as-is, that will
>>> be faster.
>>>
>>> 2014-10-25 1:51 GMT+07:00 Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com>:
>>>
>>>> Hi Ali
>>>>
>>>
>>>> I checked a code. I am thinking so code organization is not good.
>>>> accumArrayResult is too long now. makeMdArrayResult will not work, when
>>>> arrays was joined (it is not consistent now). I don't like a usage of
>>>> state->is_array_accum in array_userfunc.c -- it is signal of wrong
>>>> wrapping.
>>>>
>>> Yes, i was thinking the same. Attached WIP patch to reorganizate the
>>> code. makeMdArrayResult works now, with supplied arguments act as override
>>> from default values calculated from ArrayBuildStateArray.
>>>
>>> In array_userfunc.c, because we don't want to override ndims, dims and
>>> lbs, i copied array_agg_finalfn and only change the call to
>>> makeMdArrayResult (we don't uses makeArrayResult because we want to set
>>> release to false). Another alternative is to create new
>>> makeArrayResult-like function that has parameter bool release.
>>>
>>
>> adding makeArrayResult1 (do you have better name for this?), that accepts
>> bool release parameter. array_agg_finalfn becomes more clean, and no
>> duplicate code in array_agg_anyarray_finalfn.
>>
>
> 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
>
> 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"
>

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.

Regards

Pavel

>
>
>>
>>> next question: there is function array_append(anyarray, anyelement).
>>>> Isn't time to define array_append(anyarray, anyarray) now?
>>>>
>>>
>>> There is array_cat(anyarray, anyarray):
>>>
>>> /*-----------------------------------------------------------------------------
>>> * array_cat :
>>> * concatenate two nD arrays to form an nD array, or
>>> * push an (n-1)D array onto the end of an nD array
>>>
>>> *----------------------------------------------------------------------------
>>> */
>>>
>>
>> Regards,
>> --
>> Ali Akbar
>>
>
>