I will prepare a new patch and make it ready soon.
Regards,
Haozhou
On Thu, Jul 12, 2018 at 2:03 AM Heikki Linnakangas <hlinnaka@iki.fi> wrote:
On 26/03/18 19:07, Nikita Glukhov wrote: > Attached fixed 3th version of the patch:
Thanks, I'm reviewing this now. Nice speedup!
There is no test coverage for some of the added code. You can get a code coverage report with:
./configure --enable-coverage ... make make -C src/pl/plpython check make coverage-html
That produces a code coverage report in coverage/index.html. Please look at the coverage of the new functions, and add tests to src/pl/plpython/sql/plpython_types.sql so that all the new code is covered.
In some places, where you've already checked the object type e.g. with PyFloat_Check(), I think you could use the less safe variants, like PyFloat_AS_DOUBLE() instead of PyFloat_AsDouble(). Since this patch is all about performance, seems worth doing.
Some of the conversions seem a bit questionable. For example:
> /* > * Convert a Python object to a PostgreSQL float8 datum directly. > * If can not convert it directly, fallback to PLyObject_ToScalar > * to convert it. > */ > static Datum > PLyObject_ToFloat8(PLyObToDatum *arg, PyObject *plrv, > bool *isnull, bool inarray) > { > if (plrv == Py_None) > { > *isnull = true; > return (Datum) 0; > } > > if (PyFloat_Check(plrv) || PyInt_Check(plrv) || PyLong_Check(plrv)) > { > *isnull = false; > return Float8GetDatum((double)PyFloat_AsDouble(plrv)); > } > > return PLyObject_ToScalar(arg, plrv, isnull, inarray); > }
The conversion from Python int to C double is performed by PyFloat_AsDouble(). But there's no error checking. And wouldn't PyLong_AsDouble() be more appropriate in that case, since we already checked the python type?