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.
* 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.
* 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
