proposal: variadic argument support for least, greatest function

classic Classic list List threaded Threaded
36 messages Options
12
Reply | Threaded
Open this post in threaded view
|

proposal: variadic argument support for least, greatest function

Pavel Stehule
Hi

We can pass variadic arguments as a array to any variadic function. But some our old variadic functions doesn't supports this feature.

We cannot to write

SELECT least(VARIADIC ARRAY[1,2,3]);

Attached patch add this possibility to least, greatest functions.

Regards

Pavel

minmax_variadic.patch (15K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: proposal: variadic argument support for least, greatest function

Vik Fearing-4
On 08/11/2018 15:59, Pavel Stehule wrote:

> Hi
>
> We can pass variadic arguments as a array to any variadic function. But
> some our old variadic functions doesn't supports this feature.
>
> We cannot to write
>
> SELECT least(VARIADIC ARRAY[1,2,3]);
>
> Attached patch add this possibility to least, greatest functions.

Is there any particular reason you didn't just make least and greatest
actual functions?
--
Vik Fearing                                          +33 6 46 75 15 36
http://2ndQuadrant.fr     PostgreSQL : Expertise, Formation et Support

Reply | Threaded
Open this post in threaded view
|

Re: proposal: variadic argument support for least, greatest function

Andrew Gierth
>>>>> "Vik" == Vik Fearing <[hidden email]> writes:

 >> Attached patch add this possibility to least, greatest functions.

 Vik> Is there any particular reason you didn't just make least and
 Vik> greatest actual functions?

least() and greatest() have some type unification logic that I don't
think works for actual functions.

create function s(variadic anyarray) returns anyelement
  language sql immutable
  as $$ select min(v) from unnest($1) u(v); $$;

select s(1,2,3); -- works
select s(1,2,3.0);  -- ERROR:  function s(integer, integer, numeric) does not exist
select least(1,2,3.0);  -- works

--
Andrew (irc:RhodiumToad)

Reply | Threaded
Open this post in threaded view
|

Re: proposal: variadic argument support for least, greatest function

Pavel Stehule
In reply to this post by Vik Fearing-4


so 10. 11. 2018 v 19:12 odesílatel Vik Fearing <[hidden email]> napsal:
On 08/11/2018 15:59, Pavel Stehule wrote:
> Hi
>
> We can pass variadic arguments as a array to any variadic function. But
> some our old variadic functions doesn't supports this feature.
>
> We cannot to write
>
> SELECT least(VARIADIC ARRAY[1,2,3]);
>
> Attached patch add this possibility to least, greatest functions.

Is there any particular reason you didn't just make least and greatest
actual functions?

These functions has are able to use most common type and enforce casting. Generic variadic function cannot do it.

Regards

Pavel


--
Vik Fearing                                          +33 6 46 75 15 36
http://2ndQuadrant.fr     PostgreSQL : Expertise, Formation et Support
Reply | Threaded
Open this post in threaded view
|

Re: proposal: variadic argument support for least, greatest function

Tom Lane-2
In reply to this post by Pavel Stehule
Pavel Stehule <[hidden email]> writes:
> We cannot to write
> SELECT least(VARIADIC ARRAY[1,2,3]);
> Attached patch add this possibility to least, greatest functions.

TBH, I don't find that natural at all.  If I were looking for the
functionality "smallest element of an array", I think I'd expect to find
that exposed as "array_smallest(anyarray) returns anyelement", not as
some weird syntax option for LEAST.

It also seems rather inconsistent that this behaves so differently
from, eg,

=# select least(array[1,2], array[3,4]);
 least
-------
 {1,2}
(1 row)

Normally, if you have a variadic function, it doesn't also take arrays,
so that there's less possibility for confusion.

The implementation seems mighty ugly too, in that it has to treat this
as entirely disjoint from MinMaxExpr's normal argument interpretation.
But that seems like a symptom of the fact that the definition is
disjointed itself.

In short, I'd rather see this done with a couple of array functions,
independently of MinMaxExpr.

                        regards, tom lane

Reply | Threaded
Open this post in threaded view
|

Re: proposal: variadic argument support for least, greatest function

Pavel Stehule


st 9. 1. 2019 v 1:07 odesílatel Tom Lane <[hidden email]> napsal:
Pavel Stehule <[hidden email]> writes:
> We cannot to write
> SELECT least(VARIADIC ARRAY[1,2,3]);
> Attached patch add this possibility to least, greatest functions.

TBH, I don't find that natural at all.  If I were looking for the
functionality "smallest element of an array", I think I'd expect to find
that exposed as "array_smallest(anyarray) returns anyelement", not as
some weird syntax option for LEAST.

The target of this patch is a consistency LEAST, GREATEST variadic functions (implementet) with generic variadic functions.

Sure it is possible to implement array_smallest(anyarray), but it different. This patch try to eliminate unpleasing surprising about different behave LEAST, GREATEST from other variadic functions.


It also seems rather inconsistent that this behaves so differently
from, eg,

=# select least(array[1,2], array[3,4]);
 least
-------
 {1,2}
(1 row)

Normally, if you have a variadic function, it doesn't also take arrays,
so that there's less possibility for confusion.

This is different case - the keyword VARIADIC was not used here.


The implementation seems mighty ugly too, in that it has to treat this
as entirely disjoint from MinMaxExpr's normal argument interpretation.
But that seems like a symptom of the fact that the definition is
disjointed itself.

I don't think so there is any other possibility - I have not a possibility to unpack a array to elements inside analyze stage.


In short, I'd rather see this done with a couple of array functions,
independently of MinMaxExpr.

It doesn't help to user, when they try to use VARIADIC keyword on LEAST, GREATEST functions.

Regards

Pavel
 

                        regards, tom lane
Reply | Threaded
Open this post in threaded view
|

Re: proposal: variadic argument support for least, greatest function

Chapman Flack
The following review has been posted through the commitfest application:
make installcheck-world:  tested, failed
Implements feature:       not tested
Spec compliant:           not tested
Documentation:            not tested

The argument for consistency with other functions that are variadic makes sense to me: although they are different from ordinary variadic functions in the type unification they do, their description in the manual simply calls them functions without calling attention to any way that they are different beasts. So, a reader might reasonably be surprised that VARIADIC doesn't work in the usual way.

This patch applies (with some offsets) but the build produces several incompatible pointer type assignment warnings, and fails on errors where fcinfo->arg is no longer a thing (so should be rebased over the variable-length function call args patch).

It does not yet add regression tests, or update the documentation in func.sgml.
Reply | Threaded
Open this post in threaded view
|

Re: proposal: variadic argument support for least, greatest function

Pavel Stehule
Hi

út 12. 2. 2019 v 2:00 odesílatel Chapman Flack <[hidden email]> napsal:
The following review has been posted through the commitfest application:
make installcheck-world:  tested, failed
Implements feature:       not tested
Spec compliant:           not tested
Documentation:            not tested

The argument for consistency with other functions that are variadic makes sense to me: although they are different from ordinary variadic functions in the type unification they do, their description in the manual simply calls them functions without calling attention to any way that they are different beasts. So, a reader might reasonably be surprised that VARIADIC doesn't work in the usual way.

This patch applies (with some offsets) but the build produces several incompatible pointer type assignment warnings, and fails on errors where fcinfo->arg is no longer a thing (so should be rebased over the variable-length function call args patch).

It does not yet add regression tests, or update the documentation in func.sgml.

here is fixed patch

Regards

Pavel

minmax_variadic-20190212.patch (15K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: proposal: variadic argument support for least, greatest function

Chapman Flack
The following review has been posted through the commitfest application:
make installcheck-world:  tested, passed
Implements feature:       tested, passed
Spec compliant:           not tested
Documentation:            not tested

Latest patch passes installcheck-world as of d26a810 and does what it sets out to do.

I am not sure I have an answer to the objections being raised on grounds of taste. To me, it's persuasive that GREATEST and LEAST are described in the docco as functions, they are used much like variadic functions, and this patch allows them to be used in the ways you would expect variadic functions to be usable.

But as to technical readiness, this builds and passes installcheck-world. The functions behave as expected (and return null if passed an empty array, or an array containing only nulls, or variadic null::foo[]).

Still no corresponding regression tests or doc.

The new status of this patch is: Waiting on Author
Reply | Threaded
Open this post in threaded view
|

Re: proposal: variadic argument support for least, greatest function

Pavel Stehule
Hi

čt 21. 2. 2019 v 3:20 odesílatel Chapman Flack <[hidden email]> napsal:
The following review has been posted through the commitfest application:
make installcheck-world:  tested, passed
Implements feature:       tested, passed
Spec compliant:           not tested
Documentation:            not tested

Latest patch passes installcheck-world as of d26a810 and does what it sets out to do.

I am not sure I have an answer to the objections being raised on grounds of taste. To me, it's persuasive that GREATEST and LEAST are described in the docco as functions, they are used much like variadic functions, and this patch allows them to be used in the ways you would expect variadic functions to be usable.

But as to technical readiness, this builds and passes installcheck-world. The functions behave as expected (and return null if passed an empty array, or an array containing only nulls, or variadic null::foo[]).

Still no corresponding regression tests or doc.

The new status of this patch is: Waiting on Author

I wrote doc (just one sentence) and minimal test. Both can be enhanced.

Regards

Pavel

minmax_variadic-20190221.patch (18K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: proposal: variadic argument support for least, greatest function

Tom Lane-2
Pavel Stehule <[hidden email]> writes:
> čt 21. 2. 2019 v 3:20 odesílatel Chapman Flack <[hidden email]>
> napsal:
>> I am not sure I have an answer to the objections being raised on grounds
>> of taste. To me, it's persuasive that GREATEST and LEAST are described in
>> the docco as functions, they are used much like variadic functions, and
>> this patch allows them to be used in the ways you would expect variadic
>> functions to be usable.

> I wrote doc (just one sentence) and minimal test. Both can be enhanced.

I remain of the opinion that this patch is a mess.

I don't share Pavel's opinion that this is a clean user API, though
I'll grant that others might have different opinions on that.
I could hold my nose and overlook that if it led to a clean internal
implementation.  But it doesn't: this patch just bolts a huge,
undocumented wart onto the side of MinMaxExpr.  (The arguments are
in the args field, except when they aren't?  And everyplace that
deals with MinMaxExpr needs to know that, as well as the fact that
the semantics are totally different?  Ick.)

An example of the lack of care here is that the change in struct
ExprEvalStep breaks that struct's size constraint:

     * Inline data for the operation.  Inline data is faster to access, but
     * also bloats the size of all instructions.  The union should be kept to
     * no more than 40 bytes on 64-bit systems (so that the entire struct is
     * no more than 64 bytes, a single cacheline on common systems).

Andres is going to be quite displeased if that gets committed ;-).

I still say we should reject this and invent array_greatest/array_least
functions instead.

                        regards, tom lane

Reply | Threaded
Open this post in threaded view
|

Re: proposal: variadic argument support for least, greatest function

David Fetter
On Thu, Feb 21, 2019 at 04:04:41PM -0500, Tom Lane wrote:
> I still say we should reject this and invent array_greatest/array_least
> functions instead.

Might other array_* functions of this type be in scope for this patch?

Best,
David.
--
David Fetter <david(at)fetter(dot)org> http://fetter.org/
Phone: +1 415 235 3778

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate

Reply | Threaded
Open this post in threaded view
|

Re: proposal: variadic argument support for least, greatest function

Tom Lane-2
David Fetter <[hidden email]> writes:
> On Thu, Feb 21, 2019 at 04:04:41PM -0500, Tom Lane wrote:
>> I still say we should reject this and invent array_greatest/array_least
>> functions instead.

> Might other array_* functions of this type be in scope for this patch?

Uh ... no, I wouldn't expect that.  Why would we insist on more
functionality than is there now?  (I'm only arguing about how we
present the functionality, not what it does.)

                        regards, tom lane

Reply | Threaded
Open this post in threaded view
|

Re: proposal: variadic argument support for least, greatest function

Chapman Flack
In reply to this post by Pavel Stehule
On 02/21/19 11:31, Pavel Stehule wrote:
> I wrote doc (just one sentence) and minimal test. Both can be enhanced.

Attaching minmax_variadic-20190221b.patch, identical but for
s/supports/support/ and s/a/an/ in the doc.

Regards,
-Chap

minmax_variadic-20190221b.patch (14K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: proposal: variadic argument support for least, greatest function

Chapman Flack
The following review has been posted through the commitfest application:
make installcheck-world:  tested, passed
Implements feature:       tested, passed
Spec compliant:           not tested
Documentation:            tested, passed

On 02/21/19 16:04, Tom Lane wrote:
> Pavel Stehule <[hidden email]> writes:
>> I wrote doc (just one sentence) and minimal test. Both can be enhanced.

* dutifully submits review saying latest patch passes installcheck-world and has tests and docs, could be RfC

> I still say we should reject this and invent array_greatest/array_least
> functions instead.

* reflects on own pay grade, wanders off in search of other patch to review

The new status of this patch is: Ready for Committer
Reply | Threaded
Open this post in threaded view
|

Re: proposal: variadic argument support for least, greatest function

Pavel Stehule
In reply to this post by Tom Lane-2
Hi

čt 21. 2. 2019 v 22:05 odesílatel Tom Lane <[hidden email]> napsal:
Pavel Stehule <[hidden email]> writes:
> čt 21. 2. 2019 v 3:20 odesílatel Chapman Flack <[hidden email]>
> napsal:
>> I am not sure I have an answer to the objections being raised on grounds
>> of taste. To me, it's persuasive that GREATEST and LEAST are described in
>> the docco as functions, they are used much like variadic functions, and
>> this patch allows them to be used in the ways you would expect variadic
>> functions to be usable.

> I wrote doc (just one sentence) and minimal test. Both can be enhanced.

I remain of the opinion that this patch is a mess.

I don't share Pavel's opinion that this is a clean user API, though
I'll grant that others might have different opinions on that.
I could hold my nose and overlook that if it led to a clean internal
implementation.  But it doesn't: this patch just bolts a huge,
undocumented wart onto the side of MinMaxExpr.  (The arguments are
in the args field, except when they aren't?  And everyplace that
deals with MinMaxExpr needs to know that, as well as the fact that
the semantics are totally different?  Ick.)

fixed


An example of the lack of care here is that the change in struct
ExprEvalStep breaks that struct's size constraint:

     * Inline data for the operation.  Inline data is faster to access, but
     * also bloats the size of all instructions.  The union should be kept to
     * no more than 40 bytes on 64-bit systems (so that the entire struct is
     * no more than 64 bytes, a single cacheline on common systems).


fixed
 
Andres is going to be quite displeased if that gets committed ;-).

I hope so I solved all your objections. Now, the patch is really reduced.



I still say we should reject this and invent array_greatest/array_least
functions instead.

I am not against these functions, but these functions doesn't solve a confusing of some users, so LEAST, GREATEST looks like variadic functions, but it doesn't allow VARIADIC parameter.

Comments, notes?



                        regards, tom lane

minmax_variadic-20190222.patch (11K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: proposal: variadic argument support for least, greatest function

Pavel Stehule


pá 22. 2. 2019 v 13:42 odesílatel Pavel Stehule <[hidden email]> napsal:
Hi

čt 21. 2. 2019 v 22:05 odesílatel Tom Lane <[hidden email]> napsal:
Pavel Stehule <[hidden email]> writes:
> čt 21. 2. 2019 v 3:20 odesílatel Chapman Flack <[hidden email]>
> napsal:
>> I am not sure I have an answer to the objections being raised on grounds
>> of taste. To me, it's persuasive that GREATEST and LEAST are described in
>> the docco as functions, they are used much like variadic functions, and
>> this patch allows them to be used in the ways you would expect variadic
>> functions to be usable.

> I wrote doc (just one sentence) and minimal test. Both can be enhanced.

I remain of the opinion that this patch is a mess.

I don't share Pavel's opinion that this is a clean user API, though
I'll grant that others might have different opinions on that.
I could hold my nose and overlook that if it led to a clean internal
implementation.  But it doesn't: this patch just bolts a huge,
undocumented wart onto the side of MinMaxExpr.  (The arguments are
in the args field, except when they aren't?  And everyplace that
deals with MinMaxExpr needs to know that, as well as the fact that
the semantics are totally different?  Ick.)

fixed


An example of the lack of care here is that the change in struct
ExprEvalStep breaks that struct's size constraint:

     * Inline data for the operation.  Inline data is faster to access, but
     * also bloats the size of all instructions.  The union should be kept to
     * no more than 40 bytes on 64-bit systems (so that the entire struct is
     * no more than 64 bytes, a single cacheline on common systems).


fixed
 
Andres is going to be quite displeased if that gets committed ;-).

I hope so I solved all your objections. Now, the patch is really reduced.



I still say we should reject this and invent array_greatest/array_least
functions instead.

I am not against these functions, but these functions doesn't solve a confusing of some users, so LEAST, GREATEST looks like variadic functions, but it doesn't allow VARIADIC parameter.

Comments, notes?

I am sending second variant (little bit longer, but the main code is not repeated)

regards

Pavel




                        regards, tom lane

minmax_variadic-20190222-2.patch (13K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: proposal: variadic argument support for least, greatest function

Chapman Flack
On 02/22/19 14:57, Pavel Stehule wrote:
> I am sending second variant (little bit longer, but the main code is not
> repeated)

minmax_variadic-20190222-3.patch same as -2 but for doc grammar fix
(same fix made in minmax_variadic-20190221b.patch).

Regards,
-Chap

Reply | Threaded
Open this post in threaded view
|

Re: proposal: variadic argument support for least, greatest function

Chapman Flack
On 02/22/19 19:31, Chapman Flack wrote:
> minmax_variadic-20190222-3.patch same as -2 but for doc grammar fix
> (same fix made in minmax_variadic-20190221b.patch).

and naturally I didn't attach it.

-Chap

minmax_variadic-20190222-3.patch (10K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: proposal: variadic argument support for least, greatest function

Chapman Flack
The following review has been posted through the commitfest application:
make installcheck-world:  tested, passed
Implements feature:       tested, passed
Spec compliant:           not tested
Documentation:            tested, passed

The latest patch provides the same functionality without growing the size of struct ExprEvalStep, and without using the presence/absence of args/variadic_args to distinguish the cases. It now uses the args field consistently, and distinguishes the cases with new op constants, IS_GREATEST_VARIADIC and IS_LEAST_VARIADIC, assigned at parse time. I concede Tom's points about the comparative wartiness of the former patch.

I'll change to WoA, though, for a few loose ends:

In transformMinMaxExpr:
The assignment of funcname doesn't look right.
Two new errors are elogs. If they can be caused by user input (I'm sure the second one can), should they not be ereports?
In fact, I think the second one should copy the equivalent one from parse_func.c:

> ereport(ERROR,
>     (errcode(ERRCODE_DATATYPE_MISMATCH),
>     errmsg("VARIADIC argument must be an array"),
>     parser_errposition(pstate,
>         exprLocation((Node *) llast(fargs)))));

... both for consistency of the message, and so (I assume) it can use the existing translations for that message string.

I am not sure if there is a way for user input to trigger the first one. Perhaps it can stay an elog if not. In any case, s/to determinate/determine/.

In EvalExecMinMax:

+ if (cmpresult > 0 &&
+ (operator == IS_LEAST || operator == IS_LEAST_VARIADIC))
+ *op->resvalue = value;
+ else if (cmpresult < 0 &&
+ (operator == IS_GREATEST || operator == IS_GREATEST_VARIADIC))

would it make sense to just compute a boolean isleast before entering the loop, to get simply (cmpresult > 0 && isleast) or (cmpresult < 0 && !isleast) inside the loop? I'm unsure whether to assume the compiler will see that opportunity.

Regards,
-Chap

The new status of this patch is: Waiting on Author
12