On Fri, Nov 6, 2015 at 5:00 AM, Fabien COELHO <coelho@cri.ensmp.fr> wrote:
>> Those can be avoided in other ways. For example:
>
> Ok, ok, I surrender:-)
>
> Here is a v15 which hides conversions and assignment details in macros and
> factors out type testing of overloaded operators so that the code expansion
> is minimal (basically the operator evaluation is duplicated for int &
> double, but the rest is written once). The evaluation cost is probably
> slightly higher than the previous version because of the many hidden type
> tests.
>
> Note that variables are only int stored as text. Another patch may try to
> propagate the value structure for variables, but then it changes the query
> expansion code, it is more or less orthogonal to add functions. Moreover
> double variables would not be really useful anyway.
OK, comments on this version:
1. It's really not appropriate to fold the documentation changes
raised on the other thread into this patch. I'm not going to commit
something where the commit message is a laundry list of unrelated
changes. Please separate out the documentation changes as a separate
patch. Let's do that first, and then make this patch apply on top of
those changes. The related changes in getGaussianRand etc. should
also be part of that patch, not this one.
2. Please reduce the churn in the pgbench output example. Most of the
lines that you've changed don't actually need to be changed.
3. I think we should not have ENODE_INTEGER_CONSTANT and
ENODE_DOUBLE_CONSTANT. We should just have ENODE_CONSTANT, and it
should store the same datatype we use to represent values in the
evaluator.
4. The way you've defined the value type is not great. int64_t isn't
used anywhere in our source base. Don't start here. I think the
is_none, is_int, is_double naming is significantly inferior to what I
suggested, and unlike what we do through the rest of our code.
Similarly, the coercion functions are not very descriptive named, and
I don't see why we'd want those to be macros rather than static
functions.
5. The declaration of PgBenchExprList has a cuddled opening brace,
which is not PostgreSQL style.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
