Re: count_nulls(VARIADIC "any")

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?

> + /*
> + * Non-null argument had better be an array. We assume that any call
> + * context that could let get_fn_expr_variadic return true will have
> + * checked that a VARIADIC-labeled parameter actually is an array. So
> + * it should be okay to just Assert that it's an array rather than
> + * doing a full-fledged error check.
> + */
> + 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...

> + /* OK, safe to fetch the array value */
> + arr = PG_GETARG_ARRAYTYPE_P(0);
> +
> + ndims = ARR_NDIM(arr);
> + dims = ARR_DIMS(arr);
> + nitems = ArrayGetNItems(ndims, dims);
> +
> + bitmap = ARR_NULLBITMAP(arr);
> + if (bitmap)
> + {
> + bitmask = 1;
> +
> + for (i = 0; i < nitems; i++)
> + {
> + if ((*bitmap & bitmask) == 0)
> + count++;
> +
> + bitmask <<= 1;
> + if (bitmask == 0x100)
> + {
> + bitmap++;
> + bitmask = 1;

For brevity and example sake it'd probably be better to just use the
normal iterator, unless there's a serious speed difference?

In the unit test, I'd personally prefer just building a table with the
test cases and the expected NULL/NOT NULL results, at least for all the
calls that would fit that paradigm. That should significantly reduce the
size of the test. Not a huge deal though...

Also, I don't think anything is testing multiples of whatever value...
how 'bout change the generate_series CASE statement to >40 instead of <>40?
--
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