so 23. 2. 2019 v 18:28 odesÃlatel Chapman Flack <c...@anastigmatix.net>
napsal:
> On 02/23/19 01:22, Pavel Stehule wrote:
> > so 23. 2. 2019 v 4:50 odesÃlatel Chapman Flack <c...@anastigmatix.net>
> > napsal:
>
> >> In transformMinMaxExpr:
> >> The assignment of funcname doesn't look right.
>
> ... it still doesn't.
>
fixed
> >> Two new errors are elogs. ...
> >> 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/.
> >>
> >
> > It is +/- internal error and usually should not be touched - so there I
> > prefer elog. I fix message
>
> ... still needs s/to //.
>
fixed
> Can the sentence added to the doc be changed to "These functions support
> passing parameters as an array after <literal>VARIADIC</literal> keyword."?
> That is, s/supports/support/ and s/a/an/. I've done that after a couple of
> recent patches, but it seems to be on springs.
>
> > What about using macros?
>
> Meh ... the macros look nicer, but still rely just as much on the compiler
> to hoist the tests out of the loop. I suppose it probably can.
>
reverted, I use a variables
>
> I wouldn't have thought it necessary to change the switch statements in
> FigureColnameInternal or get_rule_expr ... cases with multiple labels are
> seen often enough, and it's probably hard to do better.
>
>
> Taking a step back ...
>
>
> All of this still begs Tom's question about whether array_greatest/
> array_least would be preferable.
>
> I understand Pavel's point:
>
> >> 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.
>
> but, to advocate the other side for a moment, perhaps that could be viewed
> as more of a documentation problem.
>
> At bottom, the confusion potential that concerns Pavel exists because
> these things look like variadic functions, and the manual calls them
> "the GREATEST and LEAST functions", but they won't accept a VARIADIC array
> parameter as a genuine variadic function would.
>
> But that's hardly the only way these differ from normal functions.
> You can't find them in pg_proc or call them through fmgr. In fact, they
> share these non-function properties with all of their siblings in the
> functions-conditional section of the manual. (Arguably, if GREATEST and
> LEAST end up taking a VARIADIC array parameter, COALESCE will be wanting
> one next. And indeed, there doesn't seem to be an existing
> array_firstnonnull function for that job, either.) And these "functions"
> have special, type-unifying argument rules (already documented in
> typeconv-union-case), late argument evaluation in the case of COALESCE,
> and so on.
>
> In other words, any effort to make these animals act more like functions
> will be necessarily incomplete, and differences will remain.
>
> Maybe the confusion-potential could be fixed by having one more sentence
> at the top of the whole functions-conditional section, saying "Some of
> these resemble functions, but are better viewed as function-like
> expressions, with special rules for their argument lists." Then the section
> for GREATEST/LEAST could have a "see also" for array_greatest/array_least,
> the COALESCE section a "see also" for array_firstnonnull, and those simple
> functions could be written.
>
> The special rules for these constructs don't really buy anything for the
> array-argument flavors: you can't pass in an array with heterogeneous
> types or late-evaluated values anyway, so ordinary functions would suffice.
>
> From the outside, it would look tidy and parsimonious to just have
> GREATEST(VARIADIC...)/LEAST(VARIADIC...)/COALESCE(VARIADIC...) and have
> just one set of "function" names to remember, rather than separate
> array_* variants. But the cost of that tidiness from the outside is an
> implementation that has to frob half a dozen source files on the inside.
>
This is goal of this small patch - do life little bit more easier and
little bit more consistent.
It is very small patch without risks or slowdowns. So I expect the cost of
this patch is small. Just it is small step forward to users.
I wrote "greatest", "least" support before I wrote variadic functions
support. If I wrote it in different order, probably, greatest, least
functions was pg_proc based. On second hand, the limitation of pg_proc
functions was motivation for variadic function support.
With my experience, I am not sure if better documentation does things
better. For some kind of users some smart magic is important. They don't
want to see implementation details.
In my car, i lost hope to understand completely to engine. I am not sure if
users should to know so we have three kind of functions - a) pg_proc based
functions, b) parser based functions (and looks like functions), c) parser
based functions (and looks like constant).
I know so is important to understand to things, but nobody can understand
to all. And then it is nice, so the things just works
>
> The approach with more parsimony indoors would be to just create a few
> new ordinary functions, and add to the doc explaining why they are
> different, and that would be a patch only needing to touch a couple files.
>
> I have a feeling that the final decision on whether the indoor or outdoor
> parsimony matters more will be made by Tom, or another committer; I find
> myself seeing both sides, and not feeling insider-y enough myself to
> pick one.
>
sure, every time it is commiter's decision.
Thank you for precious review :)
please, see, attached patch
> -Chap
>
diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index 86ff4e5c9e..aa4e80cca7 100644
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -12679,6 +12679,12 @@ SELECT NULLIF(value, '(none)') ...
</synopsis>
<synopsis>
<function>LEAST</function>(<replaceable>value</replaceable> <optional>, ...</optional>)
+</synopsis>
+<synopsis>
+<function>GREATEST</function>(VARIADIC <replaceable>anyarray</replaceable>)
+</synopsis>
+<synopsis>
+<function>LEAST</function>(VARIADIC <replaceable>anyarray</replaceable>)
</synopsis>
<para>
@@ -12697,6 +12703,11 @@ SELECT NULLIF(value, '(none)') ...
make them return NULL if any argument is NULL, rather than only when
all are NULL.
</para>
+
+ <para>
+ These functions supports passing parameters as a array after
+ <literal>VARIADIC</literal> keyword.
+ </para>
</sect2>
</sect1>
diff --git a/src/backend/executor/execExprInterp.c b/src/backend/executor/execExprInterp.c
index a018925d4e..b1f3bc86de 100644
--- a/src/backend/executor/execExprInterp.c
+++ b/src/backend/executor/execExprInterp.c
@@ -2811,6 +2811,10 @@ ExecEvalMinMax(ExprState *state, ExprEvalStep *op)
FunctionCallInfo fcinfo = op->d.minmax.fcinfo_data;
MinMaxOp operator = op->d.minmax.op;
int off;
+ ArrayIterator array_iterator = NULL;
+ int nelems;
+ bool is_greatest;
+ bool is_least;
/* set at initialization */
Assert(fcinfo->args[0].isnull == false);
@@ -2819,16 +2823,52 @@ ExecEvalMinMax(ExprState *state, ExprEvalStep *op)
/* default to null result */
*op->resnull = true;
- for (off = 0; off < op->d.minmax.nelems; off++)
+ is_least = operator == IS_LEAST || operator == IS_LEAST_VARIADIC;
+ is_greatest = operator == IS_GREATEST || operator == IS_GREATEST_VARIADIC;
+
+ if (IsVariadicMinMax(operator))
+ {
+ ArrayType *arr;
+
+ /* result is null, when variadic argument is NULL */
+ if (nulls[0])
+ return;
+
+ /* prepare iterarator */
+ arr = DatumGetArrayTypeP(values[0]);
+ array_iterator = array_create_iterator(arr, 0, NULL);
+
+ nelems = ArrayGetNItems(ARR_NDIM(arr), ARR_DIMS(arr));
+ }
+ else
+ nelems = op->d.minmax.nelems;
+
+ for (off = 0; off < nelems; off++)
{
+ Datum value;
+ bool isnull;
+
+ if (array_iterator)
+ {
+ bool has_data PG_USED_FOR_ASSERTS_ONLY;
+
+ has_data = array_iterate(array_iterator, &value, &isnull);
+ Assert(has_data);
+ }
+ else
+ {
+ value = values[off];
+ isnull = nulls[off];
+ }
+
/* ignore NULL inputs */
- if (nulls[off])
+ if (isnull)
continue;
if (*op->resnull)
{
/* first nonnull input, adopt value */
- *op->resvalue = values[off];
+ *op->resvalue = value;
*op->resnull = false;
}
else
@@ -2837,19 +2877,23 @@ ExecEvalMinMax(ExprState *state, ExprEvalStep *op)
/* apply comparison function */
fcinfo->args[0].value = *op->resvalue;
- fcinfo->args[1].value = values[off];
+ fcinfo->args[1].value = value;
fcinfo->isnull = false;
cmpresult = DatumGetInt32(FunctionCallInvoke(fcinfo));
if (fcinfo->isnull) /* probably should not happen */
continue;
- if (cmpresult > 0 && operator == IS_LEAST)
- *op->resvalue = values[off];
- else if (cmpresult < 0 && operator == IS_GREATEST)
- *op->resvalue = values[off];
+ if (cmpresult > 0 && is_least)
+ *op->resvalue = value;
+ else if (cmpresult < 0 && is_greatest)
+ *op->resvalue = value;
}
}
+
+ /* release iterator */
+ if (array_iterator)
+ array_free_iterator(array_iterator);
}
/*
diff --git a/src/backend/nodes/nodeFuncs.c b/src/backend/nodes/nodeFuncs.c
index 8ed30c011a..efabaa0046 100644
--- a/src/backend/nodes/nodeFuncs.c
+++ b/src/backend/nodes/nodeFuncs.c
@@ -465,7 +465,8 @@ exprTypmod(const Node *expr)
int32 typmod;
ListCell *arg;
- if (exprType((Node *) linitial(mexpr->args)) != minmaxtype)
+ if (!IsVariadicMinMax(mexpr->op) &&
+ exprType((Node *) linitial(mexpr->args)) != minmaxtype)
return -1;
typmod = exprTypmod((Node *) linitial(mexpr->args));
if (typmod < 0)
diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y
index a68f78e0e0..65f44a8df6 100644
--- a/src/backend/parser/gram.y
+++ b/src/backend/parser/gram.y
@@ -13803,6 +13803,22 @@ func_expr_common_subexpr:
v->location = @1;
$$ = (Node *)v;
}
+ | GREATEST '(' VARIADIC a_expr ')'
+ {
+ MinMaxExpr *v = makeNode(MinMaxExpr);
+ v->args = list_make1($4);
+ v->op = IS_GREATEST_VARIADIC;
+ v->location = @1;
+ $$ = (Node *)v;
+ }
+ | LEAST '(' VARIADIC a_expr ')'
+ {
+ MinMaxExpr *v = makeNode(MinMaxExpr);
+ v->args = list_make1($4);
+ v->op = IS_LEAST_VARIADIC;
+ v->location = @1;
+ $$ = (Node *)v;
+ }
| XMLCONCAT '(' expr_list ')'
{
$$ = makeXmlExpr(IS_XMLCONCAT, NULL, NIL, $3, @1);
diff --git a/src/backend/parser/parse_expr.c b/src/backend/parser/parse_expr.c
index e559353529..f274b3ba5c 100644
--- a/src/backend/parser/parse_expr.c
+++ b/src/backend/parser/parse_expr.c
@@ -2263,9 +2263,12 @@ transformMinMaxExpr(ParseState *pstate, MinMaxExpr *m)
MinMaxExpr *newm = makeNode(MinMaxExpr);
List *newargs = NIL;
List *newcoercedargs = NIL;
- const char *funcname = (m->op == IS_GREATEST) ? "GREATEST" : "LEAST";
+ const char *funcname;
ListCell *args;
+ funcname = (m->op == IS_GREATEST || m->op == IS_GREATEST_VARIADIC)
+ ? "GREATEST" : "LEAST";
+
newm->op = m->op;
foreach(args, m->args)
{
@@ -2276,22 +2279,47 @@ transformMinMaxExpr(ParseState *pstate, MinMaxExpr *m)
newargs = lappend(newargs, newe);
}
- newm->minmaxtype = select_common_type(pstate, newargs, funcname, NULL);
- /* minmaxcollid and inputcollid will be set by parse_collate.c */
+ if (IsVariadicMinMax(newm->op))
+ {
+ Oid array_typid;
+ Oid element_typid;
- /* Convert arguments if necessary */
- foreach(args, newargs)
+ array_typid = exprType(linitial(newargs));
+
+ if (array_typid == InvalidOid)
+ elog(ERROR, "cannot determine result type");
+
+ element_typid = get_base_element_type(array_typid);
+ if (!OidIsValid(element_typid))
+ ereport(ERROR,
+ (errcode(ERRCODE_DATATYPE_MISMATCH),
+ errmsg("VARIADIC argument must be an array"),
+ parser_errposition(pstate,
+ exprLocation((Node *) linitial(newargs)))));
+
+ newm->minmaxtype = element_typid;
+ newm->args = newargs;
+ }
+ else
{
- Node *e = (Node *) lfirst(args);
- Node *newe;
+ newm->minmaxtype = select_common_type(pstate, newargs, funcname, NULL);
+ /* minmaxcollid and inputcollid will be set by parse_collate.c */
- newe = coerce_to_common_type(pstate, e,
- newm->minmaxtype,
- funcname);
- newcoercedargs = lappend(newcoercedargs, newe);
+ /* Convert arguments if necessary */
+ foreach(args, newargs)
+ {
+ Node *e = (Node *) lfirst(args);
+ Node *newe;
+
+ newe = coerce_to_common_type(pstate, e,
+ newm->minmaxtype,
+ funcname);
+ newcoercedargs = lappend(newcoercedargs, newe);
+ }
+
+ newm->args = newcoercedargs;
}
- newm->args = newcoercedargs;
newm->location = m->location;
return (Node *) newm;
}
diff --git a/src/backend/parser/parse_target.c b/src/backend/parser/parse_target.c
index 0e9598ebfe..013596756d 100644
--- a/src/backend/parser/parse_target.c
+++ b/src/backend/parser/parse_target.c
@@ -1843,9 +1843,11 @@ FigureColnameInternal(Node *node, char **name)
switch (((MinMaxExpr *) node)->op)
{
case IS_GREATEST:
+ case IS_GREATEST_VARIADIC:
*name = "greatest";
return 2;
case IS_LEAST:
+ case IS_LEAST_VARIADIC:
*name = "least";
return 2;
}
diff --git a/src/backend/utils/adt/ruleutils.c b/src/backend/utils/adt/ruleutils.c
index 1258092dc8..6e7ebb1861 100644
--- a/src/backend/utils/adt/ruleutils.c
+++ b/src/backend/utils/adt/ruleutils.c
@@ -8468,13 +8468,23 @@ get_rule_expr(Node *node, deparse_context *context,
switch (minmaxexpr->op)
{
case IS_GREATEST:
+ case IS_GREATEST_VARIADIC:
appendStringInfoString(buf, "GREATEST(");
break;
case IS_LEAST:
+ case IS_LEAST_VARIADIC:
appendStringInfoString(buf, "LEAST(");
break;
}
- get_rule_expr((Node *) minmaxexpr->args, context, true);
+
+ if (IsVariadicMinMax(minmaxexpr->op))
+ {
+ appendStringInfoString(buf, "VARIADIC ");
+ get_rule_expr(linitial(minmaxexpr->args), context, true);
+ }
+ else
+ get_rule_expr((Node *) minmaxexpr->args, context, true);
+
appendStringInfoChar(buf, ')');
}
break;
diff --git a/src/include/nodes/primnodes.h b/src/include/nodes/primnodes.h
index a7efae7038..827fcf6d42 100644
--- a/src/include/nodes/primnodes.h
+++ b/src/include/nodes/primnodes.h
@@ -1080,9 +1080,14 @@ typedef struct CoalesceExpr
typedef enum MinMaxOp
{
IS_GREATEST,
- IS_LEAST
+ IS_LEAST,
+ IS_GREATEST_VARIADIC,
+ IS_LEAST_VARIADIC
} MinMaxOp;
+#define IsVariadicMinMax(op) (op == IS_GREATEST_VARIADIC || \
+ op == IS_LEAST_VARIADIC)
+
typedef struct MinMaxExpr
{
Expr xpr;
diff --git a/src/test/regress/expected/case.out b/src/test/regress/expected/case.out
index c0c8acf035..eca127ed2d 100644
--- a/src/test/regress/expected/case.out
+++ b/src/test/regress/expected/case.out
@@ -392,3 +392,54 @@ ROLLBACK;
--
DROP TABLE CASE_TBL;
DROP TABLE CASE2_TBL;
+--
+-- greatest and least tests
+--
+SELECT least(10,20,30,40);
+ least
+-------
+ 10
+(1 row)
+
+SELECT greatest(10,20,30,40);
+ greatest
+----------
+ 40
+(1 row)
+
+SELECT least(VARIADIC ARRAY[10,20,30,40]);
+ least
+-------
+ 10
+(1 row)
+
+SELECT greatest(VARIADIC ARRAY[10,20,30,40]);
+ greatest
+----------
+ 40
+(1 row)
+
+SELECT least(10,20,30,40, NULL);
+ least
+-------
+ 10
+(1 row)
+
+SELECT greatest(10,20,30,40, NULL);
+ greatest
+----------
+ 40
+(1 row)
+
+SELECT least(VARIADIC ARRAY[10,20,30,40, NULL]);
+ least
+-------
+ 10
+(1 row)
+
+SELECT greatest(VARIADIC ARRAY[10,20,30,40, NULL]);
+ greatest
+----------
+ 40
+(1 row)
+
diff --git a/src/test/regress/sql/case.sql b/src/test/regress/sql/case.sql
index 17436c524a..c482676e17 100644
--- a/src/test/regress/sql/case.sql
+++ b/src/test/regress/sql/case.sql
@@ -252,3 +252,16 @@ ROLLBACK;
DROP TABLE CASE_TBL;
DROP TABLE CASE2_TBL;
+
+--
+-- greatest and least tests
+--
+SELECT least(10,20,30,40);
+SELECT greatest(10,20,30,40);
+SELECT least(VARIADIC ARRAY[10,20,30,40]);
+SELECT greatest(VARIADIC ARRAY[10,20,30,40]);
+
+SELECT least(10,20,30,40, NULL);
+SELECT greatest(10,20,30,40, NULL);
+SELECT least(VARIADIC ARRAY[10,20,30,40, NULL]);
+SELECT greatest(VARIADIC ARRAY[10,20,30,40, NULL]);
