Re: jsonpath

On 01/17/2018 04:01 PM, Andrew Dunstan wrote:
>
> On 01/15/2018 07:24 PM, Andrew Dunstan wrote:
>> On 01/10/2018 05:42 PM, Nikita Glukhov wrote:
>>> Attached new 8th version of jsonpath related patches. Complete
>>> documentation is still missing.
>>>
>>> The first 4 small patches are necessary datetime handling in jsonpath:
>>> 1. simple refactoring, extracted function that will be used later in
>>> jsonpath
>>> 2. throw an error when the input or format string contains trailing
>>> elements
>>> 3. avoid unnecessary cstring to text conversions
>>> 4. add function for automatic datetime type recognition by the
>>> presence of formatting components
>>>
>>> Should they be posted in a separate thread?
>>>
>> The first of these refactors the json/jsonb timestamp formatting into a
>> single function, removing a lot of code duplication. The involves
>> exposing time2tm() and timetz2tm(). I don't think that's a tragedy, so
>> unless there is any objection I propose to commit it shortly.
>>
>> The next three expose a bit more of the date/time API. I'm still
>> reviewing those.
>>
>
> I have committed the first of these patches.
>
> I have reviewed the next three, and I think they are generally good.
> There is no real point in committing them ahead of the jsonpath patch
> since there would be no point in having them at all but for that patch.
>
> Note that these do export the following hitherto internal bits of the
> datetime functionality:
>
> tm2time
> tm2timetz
> AdjustTimeForTypmod
> AdjustTimestampForTypmod
>
> Moving on to review the main jsonpath patch.
>

OK, I understand a good deal more than I did about how the jsopnpath
code works, but the commenting is abysmal.

I got quite nervous about adding a new datetime variant to JsonbValue.
However, my understanding from the code is that this will only ever be
used in an intermediate jsonpath processing result, and it won't be used
in a stored or build jsonb object. Is that right? If it is we need to
say so, and moreover we need to warn coders in the header file about the
restricted use of this variant.  I'm not sure we can enforce it with an
Assert, but If we can we should. I'm not 100% sure that doing it this
way, i.e. by extending and resuing jsonbValue, is desirable, I'd like to
know some of the thinking behind the design.

The encoding of a jsonpath value into a binary string is quite nifty,
but it needs to be documented. Once I understood it better some of my
fears about parser overhead were alleviated.

The use of a function pointer inside JsonPathVariable seems unnecessary
and baroque. It would be much more straightforward to set an isnull flag
in the struct and process the value in an "if" statement accordingly,
avoiding the function pointer altogether.

I am going to be travelling for a few days, then back online for a day
or two, then gone for a week. I hope we can make progress on these
features, but this needs lots more eyeballs, reviewing code as well as
testing, and lots more responsiveness. The whole suite is enormous.

cheers

andrew

--
Andrew Dunstan https://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services