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
