Hello Robert,

[...] But we can't have things that are logically part of patch #2 just tossed in with patch #1.

So you want integer functions without type in patch #1.

I was in the middle of ripping that out of the patch when I realized
that evalFunc() is pretty badly designed.

Probably, we are just at v30:-)

What you've done is made it the job of each particular function to evaluate its arguments.


I did that for the multiple issues you raise below, and some you did not yet foresee: handling a variable number of arguments (0, 1, 2, 3, n), avoiding dynamic structures or arbitrary limitations, checking for a valid number of arguments, and also the fact that the evaluation call was not too horrible (2 lines per evaluation, factored out by groups of functions [operators, min/max, randoms, ...], it is not fully repeated).

There are 5 sub-expression evaluation in the function, totalling 10 LOCs.


I don't think that's a good plan.

The one you suggest does not strike me as intrinsically better: it is a trade between some code ugliness (5 repeated evals = 10 lines, a little more with the double functions, probably 20 lines) to other uglinesses (number of args limit or dynamic allocation, array length to manage and test somehow, list to array conversion code, things that will mean far more than the few lines of repeated code under discussion).

So I think that it is just a choice between two plans, really, the better of which is debatable.

I experimented with trying to do this and ran into a problem:

Yep. There are other problems, all of which solvable obviously, but which means that a lot more that 10 lines will be added to avoid the 5*2 lines repetition.

My opinion is that the submitted version is simple and fine for the purpose, and the plan you suggest replaces 5*2 repetitions by a lot of code and complexity, which is not desirable and should be avoided.

However, for obvious reasons the committer opinion prevails:-)

After considering the various points I raised above, could you confirm that you do still require the implementation of this array approach before I spend time doing such a thing?

I also went over your documentation changes.

Thanks, this looks better.


Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:

Reply via email to