Обсуждение: count_nulls(VARIADIC "any")

Hi,

I'd like to suggest $SUBJECT for inclusion in Postgres 9.6.  I'm sure
everyone would've found it useful at some point in their lives, and the
fact that it can't be properly implemented in any language other than C
I think speaks for the fact that we as a project should provide it.

A quick and dirty proof of concept (patch attached):

=# select count_nulls(null::int, null::text, 17, 'bar');
  count_nulls
-------------
            2
(1 row)

Its natural habitat would be CHECK constraints, e.g:

   CHECK (count_nulls(a,b,c) IN (0, 3))

Will finish this up for the next CF, unless someone wants to tell me how
stupid this idea is before that.


.m

On Wed, Aug 12, 2015 at 6:18 PM, Marko Tiikkaja <marko@joh.to> wrote:
> Will finish this up for the next CF, unless someone wants to tell me how
> stupid this idea is before that.

I'm kind of puzzled what kind of schema would need this.

-- 
greg

Greg Stark wrote:
> On Wed, Aug 12, 2015 at 6:18 PM, Marko Tiikkaja <marko@joh.to> wrote:
> > Will finish this up for the next CF, unless someone wants to tell me how
> > stupid this idea is before that.
> 
> I'm kind of puzzled what kind of schema would need this.

I've seen cases where you want some entity to be of either of some
number of types.  In those cases you have nullable FKs in separate
columns, and only one of them can be non null.

The name count_nulls() suggest an aggregate function to me, though.

-- 
Álvaro Herrera                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Wed, Aug 12, 2015 at 10:30 AM, Alvaro Herrera
<alvherre@2ndquadrant.com> wrote:
> The name count_nulls() suggest an aggregate function to me, though.

I thought the same.


-- 
Peter Geoghegan

On 2015-08-12 7:23 PM, Greg Stark wrote:
> On Wed, Aug 12, 2015 at 6:18 PM, Marko Tiikkaja <marko@joh.to> wrote:
>> Will finish this up for the next CF, unless someone wants to tell me how
>> stupid this idea is before that.
>
> I'm kind of puzzled what kind of schema would need this.

The first example I could find from our schema was specifying the URL 
for a Remote Procedure Call.  You can either specify a single request 
URI, or you can specify the pieces: protocol, host, port, path.  So the 
constraints look roughly like this:
  CHECK ((fulluri IS NULL) <> (protocol IS NULL)),  CHECK ((protocol IS NULL) = ALL(ARRAY[host IS NULL, port IS NULL, 
path IS NULL]))

Obviously the second one would be much prettier with count_nulls().

The other example is an OOP inheritance-like schema where an object 
could be one of any X number of types.  You could write that:
  CHECK ((a IS NULL)::int + (b IS NULL)::int + (c IS NULL)::int) = 1)

or just:
  CHECK (count_nulls(a,b,c) = 1)

The first example could be redesigned with three tables, but that seems 
like a cure worse than the disease.


.m

On 2015-08-12 7:35 PM, Pavel Stehule wrote:
> maybe nulls_count ?
>
> we have regr_count already

But that's an aggregate as well..


.m

Peter Geoghegan <pg@heroku.com> writes:
> On Wed, Aug 12, 2015 at 10:30 AM, Alvaro Herrera
> <alvherre@2ndquadrant.com> wrote:
>> The name count_nulls() suggest an aggregate function to me, though.

> I thought the same.

Ditto.  I'd be fine with this if we can come up with a name that
doesn't sound like an aggregate.  The best I can do offhand is
"number_of_nulls()", which doesn't seem very pretty.
        regards, tom lane

On 8/13/15 9:18 AM, Shulgin, Oleksandr wrote:
> nnulls()

I think I'd prefer num_nulls() over that.


.m

Hello,

Here's a patch implementing this under the name num_nulls().  For
January's CF, of course.



.m

Marko Tiikkaja <marko@joh.to> writes:
> Here's a patch implementing this under the name num_nulls().  For 
> January's CF, of course.

What's this do that "count(*) - count(x)" doesn't?
        regards, tom lane

On 2015-11-21 06:06, Tom Lane wrote:
> Marko Tiikkaja <marko@joh.to> writes:
>> Here's a patch implementing this under the name num_nulls().  For
>> January's CF, of course.
>
> What's this do that "count(*) - count(x)" doesn't?

This is sort of a lateral version of count(x); the input is a list of 
expressions rather than an expression executed over a bunch of input rows.


.m

On 2015-11-21 06:02, I wrote:
> Here's a patch implementing this under the name num_nulls().  For
> January's CF, of course.

I forgot to update the some references in the documentation.  Fixed in
v3, attached.


.m

On 11/20/15 11:12 PM, Marko Tiikkaja wrote:
> On 2015-11-21 06:02, I wrote:
>> Here's a patch implementing this under the name num_nulls().  For
>> January's CF, of course.
>
> I forgot to update the some references in the documentation.  Fixed in
> v3, attached.

I thought there was going to be a not-null equivalent as well? I've 
definitely wanted both variations in the past.
-- 
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

On 2015-11-21 06:52, Jim Nasby wrote:
> On 11/20/15 11:12 PM, Marko Tiikkaja wrote:
>> On 2015-11-21 06:02, I wrote:
>>> Here's a patch implementing this under the name num_nulls().  For
>>> January's CF, of course.
>>
>> I forgot to update the some references in the documentation.  Fixed in
>> v3, attached.
>
> I thought there was going to be a not-null equivalent as well? I've
> definitely wanted both variations in the past.

I'm not sure that's necessary.  It's quite simple to implement yourself 
using the  int - int  operator.


.m

On 11/20/15 11:55 PM, Marko Tiikkaja wrote:
> On 2015-11-21 06:52, Jim Nasby wrote:
>> On 11/20/15 11:12 PM, Marko Tiikkaja wrote:
>>> On 2015-11-21 06:02, I wrote:
>>>> Here's a patch implementing this under the name num_nulls().  For
>>>> January's CF, of course.
>>>
>>> I forgot to update the some references in the documentation.  Fixed in
>>> v3, attached.
>>
>> I thought there was going to be a not-null equivalent as well? I've
>> definitely wanted both variations in the past.
>
> I'm not sure that's necessary.  It's quite simple to implement yourself
> using the  int - int  operator.

Only if you know how many columns there already are.

Or does this not work if you hand it a row?
-- 
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

On 2015-11-22 18:29, Jim Nasby wrote:
> Only if you know how many columns there already are.
>
> Or does this not work if you hand it a row?

It "works" in the sense that it tells you whether the row is NULL or 
not.  I.e. the answer will always be 0 or 1.


.m

On 11/22/15 11:34 AM, Marko Tiikkaja wrote:
> On 2015-11-22 18:29, Jim Nasby wrote:
>> Only if you know how many columns there already are.
>>
>> Or does this not work if you hand it a row?
>
> It "works" in the sense that it tells you whether the row is NULL or
> not.  I.e. the answer will always be 0 or 1.

Hrm, I was hoping something like count_nulls(complex_type.*) would work.

I guess one could always create a wrapper function that does 
count_not_nulls() anyway.
-- 
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

On 2015-11-22 21:17, Jim Nasby wrote:
> On 11/22/15 11:34 AM, Marko Tiikkaja wrote:
>> On 2015-11-22 18:29, Jim Nasby wrote:
>>> Only if you know how many columns there already are.
>>>
>>> Or does this not work if you hand it a row?
>>
>> It "works" in the sense that it tells you whether the row is NULL or
>> not.  I.e. the answer will always be 0 or 1.
>
> Hrm, I was hoping something like count_nulls(complex_type.*) would work.

Nope:

=# select num_nulls((f).*) from (select '(1,2,3)'::foo) ss(f);
ERROR:  row expansion via "*" is not supported here


.m

<div dir="ltr">Hi<br /><div class="gmail_extra"><br /><div class="gmail_quote">2015-08-12 19:18 GMT+02:00 Marko
Tiikkaja<span dir="ltr"><<a href="mailto:marko@joh.to" target="_blank">marko@joh.to</a>></span>:<br /><blockquote
class="gmail_quote"style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Hi,<br /><br /> I'd like to
suggest$SUBJECT for inclusion in Postgres 9.6.  I'm sure everyone would've found it useful at some point in their
lives,and the fact that it can't be properly implemented in any language other than C I think speaks for the fact that
weas a project should provide it.<br /><br /> A quick and dirty proof of concept (patch attached):<br /><br /> =#
selectcount_nulls(null::int, null::text, 17, 'bar');<br />  count_nulls<br /> -------------<br />            2<br /> (1
row)<br/><br /> Its natural habitat would be CHECK constraints, e.g:<br /><br />   CHECK (count_nulls(a,b,c) IN (0,
3))<br/><br /> Will finish this up for the next CF, unless someone wants to tell me how stupid this idea is before
that.<spanclass="HOEnZb"><font color="#888888"><br /><br /><br /> .m<br /></font></span></blockquote></div><br
/></div><divclass="gmail_extra">I am sending updated version - support num_nulls and num_notnulls<br /><br /></div><div
class="gmail_extra">Regards<br/><br /></div><div class="gmail_extra">Pavel<br /></div></div> 

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

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

On 03/01/16 22:49, Jim Nasby wrote:
> 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...

I don't really see the point.  "The size of the test" doesn't seem like 
a worthwhile optimization target, unless the test scripts are somehow 
really unnecessarily large.

Further, if you were developing code related to this, previously you 
could just copy-paste the defective test case in order to easily 
reproduce a problem.  But now suddenly you need a ton of different setup.

I don't expect to really have a say in this, but I think the tests are 
now worse than they were before.


.m

On 1/21/16 1:48 PM, Pavel Stehule wrote:
>     the form of regress tests is not pretty significant issue. Jim's
>     design is little bit transparent, Marko's is maybe little bit
>     practical. Both has sense from my opinion, and any hasn't
>     significant advantage against other.
>
>
> any possible agreement, how these tests should be designed?
>
> simple patch, simple regress tests, so there are no reason for long waiting.

I don't really see how individual tests are more practical (you can 
still cut and paste a table...), but since there's no strong consensus 
either way I'd say it's up to you as author.
-- 
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

On 25/01/16 19:57, Pavel Stehule wrote:
> Marco is a author of this patch, so - Marco, please, send final version of
> this patch

I don't really care about the tests.  Can we not use the v5 patch 
already in the thread?  As far as I could tell there were no reviewer's 
comments on it anymore.


.m

Pavel Stehule <pavel.stehule@gmail.com> writes:
> [ num_nulls_v6.patch ]

I started looking through this.  It seems generally okay, but I'm not
very pleased with the function name "num_notnulls".  I think it would
be better as "num_nonnulls", as I see Oleksandr suggested already.
        regards, tom lane

I wrote:
> Pavel Stehule <pavel.stehule@gmail.com> writes:
>> [ num_nulls_v6.patch ]

> I started looking through this.  It seems generally okay, but I'm not
> very pleased with the function name "num_notnulls".  I think it would
> be better as "num_nonnulls", as I see Oleksandr suggested already.

Not hearing any complaints, I pushed it with that change and some other
cosmetic adjustments.
        regards, tom lane

<p dir="ltr"><br /> Dne 5. 2. 2016 1:33 napsal uživatel "Tom Lane" <<a
href="mailto:tgl@sss.pgh.pa.us">tgl@sss.pgh.pa.us</a>>:<br/> ><br /> > Pavel Stehule <<a
href="mailto:pavel.stehule@gmail.com">pavel.stehule@gmail.com</a>>writes:<br /> > > [ num_nulls_v6.patch ]<br
/>><br /> > I started looking through this.  It seems generally okay, but I'm not<br /> > very pleased with
thefunction name "num_notnulls".  I think it would<br /> > be better as "num_nonnulls", as I see Oleksandr suggested
already.<pdir="ltr">I have no problem with it.<p dir="ltr">Regards<p dir="ltr">Pavel<br /> ><br /> >            
           regards, tom lane<br /> 

On 2016-02-05 05:06, Tom Lane wrote:
> I wrote:
>> Pavel Stehule <pavel.stehule@gmail.com> writes:
>>> [ num_nulls_v6.patch ]
>
>> I started looking through this.  It seems generally okay, but I'm not
>> very pleased with the function name "num_notnulls".  I think it would
>> be better as "num_nonnulls", as I see Oleksandr suggested already.
>
> Not hearing any complaints, I pushed it with that change and some other
> cosmetic adjustments.

Thanks Tom and Pavel and everyone who provided feedback.


.m

On Fri, Feb 5, 2016 at 5:06 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> I wrote:
>> Pavel Stehule <pavel.stehule@gmail.com> writes:
>>> [ num_nulls_v6.patch ]
>
>> I started looking through this.  It seems generally okay, but I'm not
>> very pleased with the function name "num_notnulls".  I think it would
>> be better as "num_nonnulls", as I see Oleksandr suggested already.
>
> Not hearing any complaints, I pushed it with that change and some other
> cosmetic adjustments.

Would num_values be a better name than num_nonnulls?

-- 
Thomas Munro
http://www.enterprisedb.com

Thomas Munro <thomas.munro@enterprisedb.com> writes:
> Would num_values be a better name than num_nonnulls?

If "value" is a term that excludes null values, it's news to me.
        regards, tom lane

On Tue, Feb 9, 2016 at 9:26 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Thomas Munro <thomas.munro@enterprisedb.com> writes:
>> Would num_values be a better name than num_nonnulls?
>
> If "value" is a term that excludes null values, it's news to me.

Ah, right, I was thinking of null as the absence of a value.  But in
fact it is a special value that indicates the absence of a "data
value".  And num_data_values doesn't sound great.

-- 
Thomas Munro
http://www.enterprisedb.com