Re: jsonb and nested hstore

On 2014-02-26 16:23:12 -0500, Andrew Dunstan wrote:
> On 02/10/2014 09:11 PM, Andres Freund wrote:

> >Is it just me or is jsonapi.h not very well documented?
>
>
> What about it do you think is missing? In any case, it's hardly relevant to
> this patch, so I'll take that as obiter dicta.

It's relevant insofer because I tried to understand it, to understand
whether this patch's usage is sensible.

O n a quick reread of the header, what I am missing is:
* what's semstate in JsonSemAction? Private data?
* what's object_start and object_field_start? Presumably object vs
keypair? Why not use element as ifor the array?
* scalar_action is called for which types of tokens?
* what's exactly the meaning of the isnull parameter for ofield_action
and aelem_action?
* How is one supposed to actually access data in the callbacks, not
obvious for all the callbacks.
* are scalar callbacks triggered for object keys, object/array values?
...

> >>+static void
> >>+putEscapedValue(StringInfo out, JsonbValue *v)
> >>+{
> >>+ switch (v->type)
> >>+ {
> >>+ case jbvNull:
> >>+ appendBinaryStringInfo(out, "null", 4);
> >>+ break;
> >>+ case jbvString:
> >>+ escape_json(out, pnstrdup(v->string.val, v->string.len));
> >>+ break;
> >>+ case jbvBool:
> >>+ if (v->boolean)
> >>+ appendBinaryStringInfo(out, "true", 4);
> >>+ else
> >>+ appendBinaryStringInfo(out, "false", 5);
> >>+ break;
> >>+ case jbvNumeric:
> >>+ appendStringInfoString(out, DatumGetCString(DirectFunctionCall1(numeric_out, PointerGetDatum(v->numeric))));
> >>+ break;
> >>+ default:
> >>+ elog(ERROR, "unknown jsonb scalar type");
> >>+ }
> >>+}
> >Hm, will the jbvNumeric always result in correct correct quoting?
> >datum_to_json() does extra hangups for that case, any reason we don't
> >need that here?

> Yes, there is a reason we don't need it here. datum_to_json is converting
> SQL numerics to json, and these might be strings such as 'Nan'. But we never
> store something in a jsonb numeric field unless it came in as a json numeric
> format, which never needs quoting. The json parser will never parse 'NaN' as
> a numeric value.

Ah, yuck. Makes sense. Not your fault at all, but I do dislike json's
definition of numeric values.

> >>+char *
> >>+JsonbToCString(StringInfo out, char *in, int estimated_len)
> >>+{
> >...
> >>+ while (redo_switch || ((type = JsonbIteratorGet(&it, &v, false)) != 0))
> >>+ {
> >>+ redo_switch = false;
> >Not sure if I see the advantage over the goto here. A comment explaining
> >what the reason for the goto is wouldhave sufficed.
>
> I think you're being pretty damn picky here. You whined about the goto, I
> removed it, now you don't like that either. Personally I think this is
> cleaner.

Sorry, should perhaps have been a bit more precise in my disagreement
abou the goto version. I didn't dislike the goto itself, but that it was
a undocumented and unobvious change in control flow.

It's the reviewers job to be picky, I pretty damn sure don't expect you
to agree with all my points and I am perfectly fine if you disregard
several of them. I've just read through the patch quickly, so it's not
surprising if I misidentify some.

> >
> >>+ case WJB_KEY:
> >>+ if (first == false)
> >>+ appendBinaryStringInfo(out, ", ", 2);
> >>+ first = true;
> >>+
> >>+ putEscapedValue(out, &v);
> >>+ appendBinaryStringInfo(out, ": ", 2);
> >putEscapedValue doesn't gurantee only strings are output, but
> >datum_to_json does extra hangups for that case.
>
> But the key here will always be a string. It's enforced by the JSON rules. I
> suppose we could call escape_json directly here and save a function call,
> but I don't agree that there is any problem here.

Ah, yes, it will already have been converted to a string during the
initial conversion, right. /* json rules guarantee this is a string */
or something?

> >
> >>+ type = JsonbIteratorGet(&it, &v, false);
> >>+ if (type == WJB_VALUE)
> >>+ {
> >>+ first = false;
> >>+ putEscapedValue(out, &v);
> >>+ }
> >>+ else
> >>+ {
> >>+ Assert(type == WJB_BEGIN_OBJECT || type == WJB_BEGIN_ARRAY);
> >>+ /*
> >>+ * We need to rerun current switch() due to put
> >>+ * in current place object which we just got
> >>+ * from iterator.
> >>+ */
> >"due to put"?
>
>
> I think that's due to the author not being a native English speaker. I've
> tried to improve it a bit.

Oh, I perfectly understand that problem, believe me... I make many of
those myself, and I often don't see them in my own patches without them
being pointed out...
> >>+ if (va->string.len == vb->string.len)
> >>+ {
> >>+ res = memcmp(va->string.val, vb->string.val, va->string.len);
> >>+ if (res == 0 && arg)
> >>+ *(bool *) arg = true;
> >Should be NULL, not 0.
>
> No, the compiler doesn't like that for int values.

Yes, please disregard, I misread. I think I wanted actually to say that
the test for arg should be arg != NULL, because we don't usually do
pointer truth tests (which I personally find odd, but well).

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services