Re: jsonpath - Mailing list pgsql-hackers

On 23.01.2018 01:41, Andrew Dunstan wrote:

> 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.
Thank you for reviewing.

> 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?
Yes, you are right. Datetime JsonbValues are used only for for in-memory 
representation of SQL/JSON datetime items, they converted into ordinary 
JSON strings in ISO format when json/jsonb encoded into a datum.

> 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.
Datetime support was added to our jsonpath implementation when there was 
already a lot of code using plain JsonbValue. So, the simplest are most 
effective solution I found was JsonbValue extension. We could also 
introduce extended struct SqlJsonItem, but it seems that there will be a 
lot of unnecessary conversions between SqlJsonItem and JsonbValue.

> 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.
Callback in JsonPathVariable is used for on-demand evaluation of 
SQL/JSON PASSING parameters (see EvalJsonPathVar() from patch 
0010-sqljson-v08.patch). For jsonpath itself it is really unnecessary.

> 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

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