On 1/3/16 10:23 PM, Pavel Stehule wrote:
> Hi
>
> 2016-01-03 22:49 GMT+01:00 Jim Nasby <Jim.Nasby@bluetreble.com
> <mailto:Jim.Nasby@bluetreble.com>>:
>
> On 1/3/16 2:37 PM, Pavel Stehule wrote:
>
> + /* num_nulls(VARIADIC NULL) is defined as NULL */
> + if (PG_ARGISNULL(0))
> + return false;
>
>
> Could you add to the comment explaining why that's the desired behavior?
>
>
> This case should be different than num_nulls(VARIADIC ARRAY[..]) - this
> situation is really equivalent of missing data and NULL is correct
> answer. It should not be too clean in num_nulls, but when it is cleaner
> for num_notnulls. And more, it is consistent with other variadic
> functions in Postgres: see concat_internal and text_format.
Makes sense, now that you explain it. Which is why I'm thinking it'd be
good to add that explanation to the comment... ;)
> Assert(OidIsValid(get_base_element_type(get_fn_expr_argtype(fcinfo->flinfo, 0))));
>
>
> Erm... is that really the way to verify that what you have is an
> array? ISTM there should be a macro for that somewhere...
>
>
> really, it is. It is used more time. Although I am not against some
> macro, I don't think so it is necessary. The macro should not be too
> shorter than this text.
Well, if there's other stuff doing that... would be nice to refactor
that though.
> For brevity and example sake it'd probably be better to just use the
> normal iterator, unless there's a serious speed difference?
>
>
> The iterator does some memory allocations and some access to type cache.
> Almost all work of iterator is useless for this case. This code is
> developed by Marko, but I agree with this design. Using the iterator is
> big gun for this case. I didn't any performance checks, but it should be
> measurable for any varlena arrays.
Makes sense then.
--
Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX
Experts in Analytics, Data Architecture and PostgreSQL
Data in Trouble? Get it in Treble! http://BlueTreble.com
