rdunklau
commented
8 years ago A few comments about this PR:
- the patch reverts the requirements to python2.7, although work as recently been contributed to have compatibility with python2.6. Any reason for that ? Generally, it looks like some patches have been reverted. Could you please take a look at that, rebasing on the latest master ?
- some code seem to have been modified for no good reason: eg, the processfdw module.
- I know for a fact you wrote tests, why not include them in the PR ? :)
About the code itself:
- the MulticornSortKey should be renamed to SortKey, since it already lives in a multicorn module.
- the documentation in init.py refers to MulticornSortKey instances as MulticornDeparsedSortGroup instances, which is the C-struct name
- the testfdw class is using convoluted string concatenations to dump the path_keys arguments. It should be better to implement that in a repr method of MulticornSortKey
- the testfdw fails to account for when it returns list instead of dicts (see the test_type = 'sequence' code path).
- the EXPLAIN code path should better either be left as it was, or implemented properly through a callback to the python API.
- the path keys are not properly serialized / deserialized. This results in a SEGFAULT whenever the need for portals arise, eg: DECLARE t1 SCROLL CURSOR FOR SELECT * FROM testmulticorn order by test1 desc; ERROR: unrecognized node type: 1953719668
- I don't understand why no paths are generated for (sorted) parameterized pathes. Shouldn't it be beneficial, so as to allow mergejoin in certain cases ?
I'll check the reference counts later on, but this is a very promising patch, with minor issues to work on.
rjuju
jmealo
oscardssmith
Hello,
This patch implements sort pushdown for multicorn.
As I'm not really familiar with C-Python API, there's probably some Py_DECREF() missing, or maybe some other issues. Let me know if I can help to fix any problem remaning.
Many thanks to Ronan Dunklau for helping with the general design and a lot of issues :)
Regards.