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

Поиск
Список
Период
Сортировка
От Pavel Stehule
Тема Re: proposal: variadic argument support for least, greatest function
Дата
Msg-id CAFj8pRDV0HG2YKJ2Y3aE4ZbAAtZOnFCccK1Ny9u0z87m6oLb9w@mail.gmail.com
обсуждение исходный текст
Ответ на Re: proposal: variadic argument support for least, greatest function  (Chapman Flack <chap@anastigmatix.net>)
Ответы Re: proposal: variadic argument support for least, greatest function  (Chapman Flack <chap@anastigmatix.net>)
Список pgsql-hackers


so 23. 2. 2019 v 4:50 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:            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.

good idea, done


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
 

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.

I am have not strong opinion about it. Personally I dislike a two variables - but any discussion is partially about premature optimization. What about using macros?



Regards,
-Chap

The new status of this patch is: Waiting on Author
Вложения

В списке pgsql-hackers по дате отправления:

Предыдущее
От: Chapman Flack
Дата:
Сообщение: Re: proposal: variadic argument support for least, greatest function
Следующее
От: Surafel Temesgen
Дата:
Сообщение: Re: FETCH FIRST clause PERCENT option