Re: jsonpath - Mailing list pgsql-hackers

On Mon, Oct 29, 2018 at 2:20 AM Tomas Vondra
<tomas.vondra@2ndquadrant.com> wrote:
>
> Hi,
>
> On 10/02/2018 04:33 AM, Michael Paquier wrote:
> > On Sat, Sep 08, 2018 at 02:21:27AM +0300, Nikita Glukhov wrote:
> >> Attached 18th version of the patches rebased onto the current master.
> >
> > Nikita, this version fails to apply, as 0004 has conflicts with some
> > regression tests.  Could you rebase?  I am moving the patch to CF
> > 2018-11, waiting for your input.
> > --
> > Michael
> >
>
> As Michael mentioned, the patch does not apply anymore, so it would be
> good to provide a rebased version for CF 2018-11. I've managed to do
> that, as the issues are due to minor bitrot, so that I can do some quick
> review of the current version.
>
> I haven't done much testing, pretty much just compiling, running the
> usual regression tests and reading through the patches. I plan to do
> more testing once the rebased patch is submitted.
>
> Now, a couple of comments based on eye-balling the diffs.
>
>
> 1) There are no docs, which makes it pretty much non-committable for
> now. I know there is [1] and it was a good intro for the review, but we
> need something as a part of our sgml docs.

SQL/JSON documentation discussed in separate topic
https://www.postgresql.org/message-id/flat/732208d3-56c3-25a4-8f08-3be1d54ad51b%40postgrespro.ru


>
>
> 2) 0001 says this:
>
>     *typmod = -1; /* TODO implement FF1, ..., FF9 */
>
> but I have no idea what FF1 or FF9 are. I guess it needs to be
> implemented, or explained better.
>
>
> 3) The makefile rule for scan.o does this:
>
> +# Latest flex causes warnings in this file.
> +ifeq ($(GCC),yes)
> +scan.o: CFLAGS += -Wno-error
> +endif
>
> That seems a bit ugly, and we should probably try to make it work with
> the latest flex, instead of hiding the warnings. I don't think we have
> any such ad-hoc rules in other Makefiles. If we really need it, can't we
> make it part of configure, and/or restrict it depending on flex version?
>
>
> 4) There probably should be .gitignore rule for jsonpath_gram.h, just
> like for other generated header files.
>
>
> 5) jbvType says jbvDatetime is "virtual type" but does not explain what
> it is. IMHO that deserves a comment or something.
>
>
> 6) I see the JsonPath definition says this about header:
>
>     /* just version, other bits are reservedfor future use */
>
> but the very next thing it does is defining two pieces stored in the
> header - version AND "lax" mode flag. Which makes the comment invalid
> (also, note the missing space after "reserved").
>
> FWIW, I'd use JSONPATH_STRICT instead of JSONPATH_LAX. The rest of the
> codebase works with "strict" flags passed around, and it's easy to
> forget to negate the flag somewhere (at least that's my experience).
>
>
> 7) I see src/include/utils/jsonpath_json.h adds support for plain json
> by undefining various jsonb macros and redirecting them to the json
> variants. I find that rather suspicious - imagine you're investigating
> something in code using those jsonb macros, and wondering why it ends up
> calling the json stuff. I'd be pretty confused ...
>
> Also, some of the redefinitions are not really needed for example
> JsonbWrapItemInArray and JsonbWrapItemsInArray are not used anywhere
> (and neither are the json variants).
>
>
> 8) I see 0002 defines IsAJsonbScalar like this:
>
>   #define IsAJsonbScalar(jsonbval)  (((jsonbval)->type >= jbvNull && \
>                                      (jsonbval)->type <= jbvBool) || \
>                                      (jsonbval)->type == jbvDatetime)
>
> while 0004 does this
>
>   #define jspIsScalar(type) ((type) >= jpiNull && (type) <= jpiBool)
>
> I know those are for different data types (jsonb vs. jsonpath), but I
> suppose jspIsScalar should include the datetime too.
>
> FWIW JsonPathItemType would deserve better documentation what the
> various items mean (a comment for each line would be enough). I've been
> wondering if "jpiDouble" means scalar double value until I realized
> there's a ".double()" function for paths.
>
>
> 9) It's generally a good idea to make the individual pieces committable
> separately, but that means e.g. the regression tests have to pass after
> each patch. At the moment that does not seem to be the case for 0002,
> see the attached file. I'm running with -DRANDOMIZE_ALLOCATED_MEMORY,
> not sure if that's related.
>
> regards
>
>
> [1] https://github.com/obartunov/sqljsondoc/blob/master/README.jsonpath.md
>
> --
> Tomas Vondra                  http://www.2ndQuadrant.com
> PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
>


-- 
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company