proposal: variadic argument support for least, greatest function

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

"Vik" == Vik Fearing <vik.fearing@2ndquadrant.com> 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)

so 10. 11. 2018 v 19:12 odesílatel Vik Fearing <vik.fearing@2ndquadrant.com>
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

--

Pavel Stehule <pavel.stehule@gmail.com> 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

st 9. 1. 2019 v 1:07 odesílatel Tom Lane <tgl@sss.pgh.pa.us> napsal:

Pavel Stehule <pavel.stehule@gmail.com> 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

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.

Hi

út 12. 2. 2019 v 2:00 odesílatel Chapman Flack <chap@anastigmatix.net>
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

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

Hi

čt 21. 2. 2019 v 3:20 odesílatel Chapman Flack <chap@anastigmatix.net>
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

Pavel Stehule <pavel.stehule@gmail.com> writes:

čt 21. 2. 2019 v 3:20 odesílatel Chapman Flack <chap@anastigmatix.net>
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

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

David Fetter <david@fetter.org> 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

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

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 <pavel.stehule@gmail.com> 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

Hi

čt 21. 2. 2019 v 22:05 odesílatel Tom Lane <tgl@sss.pgh.pa.us> napsal:

Pavel Stehule <pavel.stehule@gmail.com> writes:

čt 21. 2. 2019 v 3:20 odesílatel Chapman Flack <chap@anastigmatix.net>
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?

pá 22. 2. 2019 v 13:42 odesílatel Pavel Stehule <pavel.stehule@gmail.com>
napsal:

Hi

čt 21. 2. 2019 v 22:05 odesílatel Tom Lane <tgl@sss.pgh.pa.us> napsal:

Pavel Stehule <pavel.stehule@gmail.com> writes:

čt 21. 2. 2019 v 3:20 odesílatel Chapman Flack <chap@anastigmatix.net>
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

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

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

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