On 01/02/2018 05:23 PM, Andrew Dunstan wrote: > > On 01/02/2018 05:04 PM, Nikita Glukhov wrote: >> I have removed all extra features from the patch set, they can be >> found in our >> github repository: >> https://github.com/postgrespro/sqljson/tree/sqljson_ext. >> >> Now there are 10 patches which have the following dependencies: >> >> 1: >> 2: 1 >> 3: 2 >> 4: 2 >> 5: >> 6: >> 7: 2, 5, 6 >> 8: 7, 4 >> 9: 7 >> 10: 8, 9 >> > > OK. We need to get this into the commitfest. Preferably not all in one > piece. I would do: > > 1, 5, and 6 independently > 2, 3 and 4 together > 7 and 8 together > 9 and 10 together > > Those seem like digestible pieces. > > Also, there is a spurious BOM at the start of > src/test/regress/sql/sqljson.sql in patch 7. Please fix that. >
OK, an extended version of patch 1 has been committed. I'm working on patches 2, 3, and 4 (the jsonpath patches) as time permits (and right now time is very tight). Here are some very preliminary comments:
* The documentation needs improvement. A para with contents of "TODO" is not acceptable.
There should be introduction to JSONPath expressions
* I'm not generally a fan of using flex/bison for small languages like this. Something similar to what we're using for json itself (a simple lexer and a recursive descent parser) would be more suitable. Others might have different views.
I am think so flex/bison in this case is correct - JSONPath expression is more complex language than JSON self
Regards
Pavel
* The timestamp handling refactoring is a good thing but would probably be better done as a separate patch. * the code is pretty sparsely commented. Quite apart from other considerations that makes it harder to review.
I also note that the later patches have no documentation whatsoever. That needs serious work, and if you want to get these patches in then please supply some documentation ASAP. If you need help with English we can work on that, but just throwing patches of this size and complexity over the wall into the commitfest without any documentation is not the way to proceed.
cheers
andrew
-- Andrew Dunstan https://www.2ndQuadrant.com PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services