Horton

Thread

Commits

  1. Forbid numeric NaN in jsonpath

  2. Improve error reporting for jsonpath .double() method

  3. Partial implementation of SQL/JSON path language

  1. jsonpath versus NaN

    Tom Lane <tgl@sss.pgh.pa.us> — 2020-06-11T12:45:42Z

    Commit 72b646033 inserted this into convertJsonbScalar:

 			break;
 
 		case jbvNumeric:
+			/* replace numeric NaN with string "NaN" */
+			if (numeric_is_nan(scalarVal->val.numeric))
+			{
+				appendToBuffer(buffer, "NaN", 3);
+				*jentry = 3;
+				break;
+			}
+
 			numlen = VARSIZE_ANY(scalarVal->val.numeric);
 			padlen = padBufferToInt(buffer);

To characterize this as hack, slash, and burn programming would be
charitable.  It is entirely clear from the code, the documentation,
and the relevant RFCs that JSONB does not allow NaNs as numeric
values.  So it should be impossible for this code to do anything.

I tried taking it out, and found that this test case from the same
commit fails:

+select jsonb_path_query('"nan"', '$.double()');
+ jsonb_path_query 
+------------------
+ "NaN"
+(1 row)

However, seeing that the JSON RFC disallows NaNs, I do not understand
why it's important to accept this.  The adjacent test case showing
that 'inf' isn't accepted:

+select jsonb_path_query('"inf"', '$.double()');
+ERROR:  non-numeric SQL/JSON item
+DETAIL:  jsonpath item method .double() can only be applied to a numeric value

seems like a saner approach.

In short, I think we should rip out the above code snippet and adjust
executeItemOptUnwrapTarget, at about line jsonpath_exec.c:1076 as of HEAD,
to reject NaNs the same way it already rejects infinities.  Can you
explain why it was done like this?

(The reason I came across this was that I'm working on extending
type numeric to allow infinities, and it was not clear what to
do here.  But allowing a jsonb to contain a numeric NaN, even
transiently, seems like a completely horrid idea.)

			regards, tom lane

  2. Re: jsonpath versus NaN

    Alexander Korotkov <a.korotkov@postgrespro.ru> — 2020-06-11T18:41:00Z

    Hi Tom,

Thank you for raising this issue.

On Thu, Jun 11, 2020 at 3:45 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Commit 72b646033 inserted this into convertJsonbScalar:
>
>                         break;
>
>                 case jbvNumeric:
> +                       /* replace numeric NaN with string "NaN" */
> +                       if (numeric_is_nan(scalarVal->val.numeric))
> +                       {
> +                               appendToBuffer(buffer, "NaN", 3);
> +                               *jentry = 3;
> +                               break;
> +                       }
> +
>                         numlen = VARSIZE_ANY(scalarVal->val.numeric);
>                         padlen = padBufferToInt(buffer);
>
> To characterize this as hack, slash, and burn programming would be
> charitable.  It is entirely clear from the code, the documentation,
> and the relevant RFCs that JSONB does not allow NaNs as numeric
> values.

The JSONB itself doesn't store number NaNs.  It stores the string "NaN".

I found the relevant part of the standard.  Unfortunately, I can't
post the full standard here due to its license, but I think I can cite
the relevant part.

1) If JM specifies double, then For all j, 1 (one) ≤ j ≤ n,
Case:
a) If Ij is not a number or character string, then let ST be data
exception — non-numeric SQL/JSON item.
b) Otherwise, let X be an SQL variable whose value is Ij. Let Vj be
the result of
CAST (X AS DOUBLE PRECISION)
If this conversion results in an exception condition, then let ST be
that exception condition.

So, when we apply the .double() method to string, then the result
should be the same as if we cast string to double in SQL.  In SQL we
convert string 'NaN' to numeric NaN.  So, standard requires us to do
the same in SQL/JSON.

I didn't find yet what the standard says about serializing NaNs back
to JSON.  I'll keep you posted.

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

  3. Re: jsonpath versus NaN

    Tom Lane <tgl@sss.pgh.pa.us> — 2020-06-11T19:00:03Z

    Alexander Korotkov <a.korotkov@postgrespro.ru> writes:
> On Thu, Jun 11, 2020 at 3:45 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> It is entirely clear from the code, the documentation,
>> and the relevant RFCs that JSONB does not allow NaNs as numeric
>> values.

> The JSONB itself doesn't store number NaNs.  It stores the string "NaN".

Yeah, but you have a numeric NaN within the JsonbValue tree between
executeItemOptUnwrapTarget and convertJsonbScalar.  Who's to say that
that illegal-per-the-data-type structure won't escape to somewhere else?
Or perhaps more likely, that we'll need additional warts in other random
places in the JSON code to keep from spitting up on the transiently
invalid structure.

> I found the relevant part of the standard.  Unfortunately, I can't
> post the full standard here due to its license, but I think I can cite
> the relevant part.

I don't think this is very relevant.  The SQL standard has not got the
concepts of Inf or NaN either (see 4.4.2 Characteristics of numbers),
therefore their definition is only envisioning that a string representing
a normal finite number should be castable to DOUBLE PRECISION.  Thus,
both of the relevant standards think that "numbers" are just finite
numbers.

So when neither JSON nor SQL consider that "NaN" is an allowed sort
of number, why are you doing violence to the code to allow it in a
jsonpath?  And if you insist on doing such violence, why didn't you
do some more and kluge it to the point where "Inf" would work too?
(It would require slightly less klugery in the wake of the infinities-
in-numeric patch that I'm going to post soon ... but that doesn't make
it a good idea.)

			regards, tom lane

  4. Re: jsonpath versus NaN

    Alexander Korotkov <a.korotkov@postgrespro.ru> — 2020-06-15T10:50:11Z

    On Thu, Jun 11, 2020 at 10:00 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> Alexander Korotkov <a.korotkov@postgrespro.ru> writes:
> > On Thu, Jun 11, 2020 at 3:45 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> >> It is entirely clear from the code, the documentation,
> >> and the relevant RFCs that JSONB does not allow NaNs as numeric
> >> values.
>
> > The JSONB itself doesn't store number NaNs.  It stores the string "NaN".
>
> Yeah, but you have a numeric NaN within the JsonbValue tree between
> executeItemOptUnwrapTarget and convertJsonbScalar.  Who's to say that
> that illegal-per-the-data-type structure won't escape to somewhere else?
> Or perhaps more likely, that we'll need additional warts in other random
> places in the JSON code to keep from spitting up on the transiently
> invalid structure.


I would propose to split two things: user-visible behavior and
internal implementation.  Internal implementation, which allows
numeric NaN within the JsonbValue, isn't perfect and we could improve
it.  But I'd like to determine desired user-visible behavior first,
then we can decide how to fix the implementation.

>
> > I found the relevant part of the standard.  Unfortunately, I can't
> > post the full standard here due to its license, but I think I can cite
> > the relevant part.
>
> I don't think this is very relevant.  The SQL standard has not got the
> concepts of Inf or NaN either (see 4.4.2 Characteristics of numbers),
> therefore their definition is only envisioning that a string representing
> a normal finite number should be castable to DOUBLE PRECISION.  Thus,
> both of the relevant standards think that "numbers" are just finite
> numbers.
>
> So when neither JSON nor SQL consider that "NaN" is an allowed sort
> of number, why are you doing violence to the code to allow it in a
> jsonpath?

Yes, I see.  No standard insists we should support NaN.  However,
standard claims .double() should behave the same as CAST to double.
So, I think if CAST supports NaN, but .double() doesn't, it's still a
violation.

> And if you insist on doing such violence, why didn't you
> do some more and kluge it to the point where "Inf" would work too?


Yep, according to standard .double() should support "Inf" as soon as
CAST to double does.  The reason why it wasn't implemented is that we
use numeric as the internal storage for all the numbers. And numeric
doesn't support Inf yet.

> (It would require slightly less klugery in the wake of the infinities-
> in-numeric patch that I'm going to post soon ... but that doesn't make
> it a good idea.)


If numerics would support infinites, we can follow standard and make
.double() method work the same way as CAST to double does.  Now, I get
that there is no much reason to keep current behaviour, which supports
Nan, but doesn't support Inf.  I think we should either support both
NaN and Inf and don't support any of them.  The latter is a violation
of the standard, but provides us with a simpler and cleaner
implementation.  What do you think?

BTW, we found what the standard says about serialization of SQL/JSON items.

9.37 Serializing an SQL/JSON item (page 695)
ii) Let JV be an implementation-dependent value of type TT and
encoding ENC such that these two conditions hold:
1) JV is a JSON text.
2) When applying the General Rules of Subclause 9.36, “Parsing JSON
text” with JV as JSON TEXT, FO as FORMAT OPTION, and WITHOUT UNIQUE
KEYS as UNIQUENESS CONSTRAINT, the returned STATUS is successful
completion and the returned SQL/JSON ITEM is an SQL/JSON item that is
equivalent to SJI.
If there is no such JV, then let ST be the exception condition: data
exception — invalid JSON text.

Basically it says that the resulting text should result in the same
SQL/JSON item when parsed.  I think this literally means that
serialization of numeric NaN is impossible as soon as it's impossible
to get numeric NaN as the result json parsing.  However, in the same
way this would mean that serialization of datetime is also impossible,
but that seems like nonsense.  So, I think this paragraph of the
standard is ill-conceived.

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

  5. Re: jsonpath versus NaN

    Tom Lane <tgl@sss.pgh.pa.us> — 2020-06-17T15:33:04Z

    Alexander Korotkov <a.korotkov@postgrespro.ru> writes:
> On Thu, Jun 11, 2020 at 10:00 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> I don't think this is very relevant.  The SQL standard has not got the
>> concepts of Inf or NaN either (see 4.4.2 Characteristics of numbers),
>> therefore their definition is only envisioning that a string representing
>> a normal finite number should be castable to DOUBLE PRECISION.  Thus,
>> both of the relevant standards think that "numbers" are just finite
>> numbers.

> Yes, I see.  No standard insists we should support NaN.  However,
> standard claims .double() should behave the same as CAST to double.
> So, I think if CAST supports NaN, but .double() doesn't, it's still a
> violation.

No, I think you are completely misunderstanding the standard.  They
are saying that strings that look like legal numbers according to SQL
should be castable into numbers.  But NaN and Inf are not legal
numbers according to SQL, so there is nothing in that text that
justifies accepting "NaN".  Nor does the JSON standard provide any
support for that position.  So I think it is fine to leave NaN/Inf
out of the world of what you can write in jsonpath.

I'd be more willing to let the code do this if it didn't require such
a horrid, dangerous kluge to do so.  But it does, and I don't see any
easy way around that, so I think we should just take out the kluge.
And do so sooner not later, before some misguided user starts to
depend on it.

			regards, tom lane

  6. Re: jsonpath versus NaN

    Oleg Bartunov <obartunov@postgrespro.ru> — 2020-06-18T15:51:04Z

    On Wed, Jun 17, 2020 at 6:33 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:

> Alexander Korotkov <a.korotkov@postgrespro.ru> writes:
> > On Thu, Jun 11, 2020 at 10:00 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> >> I don't think this is very relevant.  The SQL standard has not got the
> >> concepts of Inf or NaN either (see 4.4.2 Characteristics of numbers),
> >> therefore their definition is only envisioning that a string
> representing
> >> a normal finite number should be castable to DOUBLE PRECISION.  Thus,
> >> both of the relevant standards think that "numbers" are just finite
> >> numbers.
>
> > Yes, I see.  No standard insists we should support NaN.  However,
> > standard claims .double() should behave the same as CAST to double.
> > So, I think if CAST supports NaN, but .double() doesn't, it's still a
> > violation.
>
> No, I think you are completely misunderstanding the standard.  They
> are saying that strings that look like legal numbers according to SQL
> should be castable into numbers.  But NaN and Inf are not legal
> numbers according to SQL, so there is nothing in that text that
> justifies accepting "NaN".  Nor does the JSON standard provide any
> support for that position.  So I think it is fine to leave NaN/Inf
> out of the world of what you can write in jsonpath.
>

rfc and sql json forbid Nan and Inf ( Technical Report is freely available,
https://standards.iso.org/ittf/PubliclyAvailableStandards/c067367_ISO_IEC_TR_19075-6_2017.zip
)

Page 10 JSON terminology.
“A sequence comprising an integer part, optionally followed by a fractional
part and/or
an exponent part (non-numeric values, such as infinity and NaN are not
permitted)”


>
> I'd be more willing to let the code do this if it didn't require such
> a horrid, dangerous kluge to do so.  But it does, and I don't see any
> easy way around that, so I think we should just take out the kluge.
> And do so sooner not later, before some misguided user starts to
> depend on it.
>

The problem is that we tried to find a trade-off  between standard and
postgres
implementation, for example, in postgres CAST  allows NaN and Inf, and SQL
Standard
requires .double should works as CAST.

 SELECT 'nan'::real, 'inf'::real;
 float4 |  float4
--------+----------
    NaN | Infinity
(1 row)



>
>                         regards, tom lane
>
>
>

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

  7. Re: jsonpath versus NaN

    Tom Lane <tgl@sss.pgh.pa.us> — 2020-06-18T16:07:44Z

    Oleg Bartunov <obartunov@postgrespro.ru> writes:
> The problem is that we tried to find a trade-off between standard and
> postgres implementation, for example, in postgres CAST allows NaN and
> Inf, and SQL Standard requires .double should works as CAST.

As I said, I think this is a fundamental misreading of the standard.
The way I read it is that it requires the set of values that are legal
according to the standard to be processed the same way as CAST would.

While we certainly *could* choose to extend jsonpath, and/or jsonb
itself, to allow NaN/Inf, I do not think that it's sane to argue that
the standard requires us to do that; the wording in the opposite
direction is pretty clear.  Also, I do not find it convincing to
extend jsonpath that way when we haven't extended jsonb.  Quite aside
from the ensuing code warts, what in the world is the use-case?

			regards, tom lane

  8. Re: jsonpath versus NaN

    Robert Haas <robertmhaas@gmail.com> — 2020-06-18T16:24:31Z

    On Thu, Jun 18, 2020 at 11:51 AM Oleg Bartunov <obartunov@postgrespro.ru> wrote:
> The problem is that we tried to find a trade-off  between standard and postgres
> implementation, for example, in postgres CAST  allows NaN and Inf, and SQL Standard
> requires .double should works as CAST.

It seems like the right thing is to implement the standard, not to
implement whatever PostgreSQL happens to do in other cases. I can't
help feeling like re-using the numeric data type for other things has
led to this confusion. I think that fails in other cases, too: like
what if you have a super-long integer that can't be represented as a
numeric? I bet jsonb will fail, or maybe it will convert it to a
string, but I don't see how it can do anything else.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

  9. Re: jsonpath versus NaN

    Alexander Korotkov <a.korotkov@postgrespro.ru> — 2020-06-18T16:34:32Z

    Tom,

On Thu, Jun 18, 2020 at 7:07 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Oleg Bartunov <obartunov@postgrespro.ru> writes:
> > The problem is that we tried to find a trade-off between standard and
> > postgres implementation, for example, in postgres CAST allows NaN and
> > Inf, and SQL Standard requires .double should works as CAST.
>
> As I said, I think this is a fundamental misreading of the standard.
> The way I read it is that it requires the set of values that are legal
> according to the standard to be processed the same way as CAST would.

Thank you for your answer. I'm trying to understand your point.
Standard claims that .double() method should behave the same way as
CAST to double.  However, standard references the standard behavior of
CAST here, not behavior of your implementation of CAST.  So, if we
extend the functionality of standard CAST in our implementation, that
doesn't automatically mean we should extend the .double() jsonpath
method in the same way.  Is it correct?

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

  10. Re: jsonpath versus NaN

    Tom Lane <tgl@sss.pgh.pa.us> — 2020-06-18T16:35:54Z

    Robert Haas <robertmhaas@gmail.com> writes:
> On Thu, Jun 18, 2020 at 11:51 AM Oleg Bartunov <obartunov@postgrespro.ru> wrote:
>> The problem is that we tried to find a trade-off  between standard and postgres
>> implementation, for example, in postgres CAST  allows NaN and Inf, and SQL Standard
>> requires .double should works as CAST.

> It seems like the right thing is to implement the standard, not to
> implement whatever PostgreSQL happens to do in other cases. I can't
> help feeling like re-using the numeric data type for other things has
> led to this confusion. I think that fails in other cases, too: like
> what if you have a super-long integer that can't be represented as a
> numeric? I bet jsonb will fail, or maybe it will convert it to a
> string, but I don't see how it can do anything else.

Actually, the JSON spec explicitly says that any number that doesn't fit
in an IEEE double isn't portable [1].  So we're already very far above and
beyond the spec's requirements by using numeric.  We don't need to improve
on that.  But I concur with your point that just because PG does X in
some other cases doesn't mean that we must do X in json or jsonpath.

			regards, tom lane

[1] https://tools.ietf.org/html/rfc7159#page-6

   This specification allows implementations to set limits on the range
   and precision of numbers accepted.  Since software that implements
   IEEE 754-2008 binary64 (double precision) numbers [IEEE754] is
   generally available and widely used, good interoperability can be
   achieved by implementations that expect no more precision or range
   than these provide, in the sense that implementations will
   approximate JSON numbers within the expected precision.  A JSON
   number such as 1E400 or 3.141592653589793238462643383279 may indicate
   potential interoperability problems, since it suggests that the
   software that created it expects receiving software to have greater
   capabilities for numeric magnitude and precision than is widely
   available.

   Note that when such software is used, numbers that are integers and
   are in the range [-(2**53)+1, (2**53)-1] are interoperable in the
   sense that implementations will agree exactly on their numeric
   values.

  11. Re: jsonpath versus NaN

    Alexander Korotkov <a.korotkov@postgrespro.ru> — 2020-06-18T16:36:02Z

    On Thu, Jun 18, 2020 at 7:34 PM Alexander Korotkov
<a.korotkov@postgrespro.ru> wrote:
> Thank you for your answer. I'm trying to understand your point.
> Standard claims that .double() method should behave the same way as
> CAST to double.  However, standard references the standard behavior of
> CAST here, not behavior of your implementation of CAST.

Typo here: please read "our implementation of CAST" here.

> So, if we
> extend the functionality of standard CAST in our implementation, that
> doesn't automatically mean we should extend the .double() jsonpath
> method in the same way.  Is it correct?


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

  12. Re: jsonpath versus NaN

    Tom Lane <tgl@sss.pgh.pa.us> — 2020-06-18T16:45:46Z

    Alexander Korotkov <a.korotkov@postgrespro.ru> writes:
> Thank you for your answer. I'm trying to understand your point.
> Standard claims that .double() method should behave the same way as
> CAST to double.  However, standard references the standard behavior of
> CAST here, not behavior of your implementation of CAST.  So, if we
> extend the functionality of standard CAST in our implementation, that
> doesn't automatically mean we should extend the .double() jsonpath
> method in the same way.  Is it correct?

Right.  We could, if we chose, extend jsonpath to allow Inf/NaN, but
I don't believe there's an argument that the spec requires us to.

Also the larger point is that it doesn't make sense to extend jsonpath
that way when we haven't extended json(b) that way.  This code wart
wouldn't exist were it not for that inconsistency.  Also, I find it hard
to see why anyone would have a use for NaN in a jsonpath test when they
can't write NaN in the input json data, nor have it be correctly reflected
into output json data either.

Maybe there's a case for extending json(b) that way; it wouldn't be so
different from the work I'm doing nearby to extend type numeric for
infinities.  But we'd have to have a conversation about whether
interoperability with other JSON implementations is worth sacrificing
to improve consistency with our float and numeric datatypes.  In the
meantime, though, we aren't allowing Inf/NaN in json(b) so I don't think
jsonpath should accept them either.

			regards, tom lane

  13. Re: jsonpath versus NaN

    Alexander Korotkov <a.korotkov@postgrespro.ru> — 2020-06-18T17:04:10Z

    On Thu, Jun 18, 2020 at 7:45 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Alexander Korotkov <a.korotkov@postgrespro.ru> writes:
> > Thank you for your answer. I'm trying to understand your point.
> > Standard claims that .double() method should behave the same way as
> > CAST to double.  However, standard references the standard behavior of
> > CAST here, not behavior of your implementation of CAST.  So, if we
> > extend the functionality of standard CAST in our implementation, that
> > doesn't automatically mean we should extend the .double() jsonpath
> > method in the same way.  Is it correct?
>
> Right.  We could, if we chose, extend jsonpath to allow Inf/NaN, but
> I don't believe there's an argument that the spec requires us to.
>
> Also the larger point is that it doesn't make sense to extend jsonpath
> that way when we haven't extended json(b) that way.  This code wart
> wouldn't exist were it not for that inconsistency.  Also, I find it hard
> to see why anyone would have a use for NaN in a jsonpath test when they
> can't write NaN in the input json data, nor have it be correctly reflected
> into output json data either.

Ok, I got the point.  I have nothing against removing support of NaN
in jsonpath as far as it doesn't violates the standard.  I'm going to
write the patch for this.

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

  14. Re: jsonpath versus NaN

    Andrew Dunstan <andrew.dunstan@2ndquadrant.com> — 2020-06-18T20:53:15Z

    On 6/18/20 12:35 PM, Tom Lane wrote:
> Robert Haas <robertmhaas@gmail.com> writes:
>> On Thu, Jun 18, 2020 at 11:51 AM Oleg Bartunov <obartunov@postgrespro.ru> wrote:
>>> The problem is that we tried to find a trade-off  between standard and postgres
>>> implementation, for example, in postgres CAST  allows NaN and Inf, and SQL Standard
>>> requires .double should works as CAST.
>> It seems like the right thing is to implement the standard, not to
>> implement whatever PostgreSQL happens to do in other cases. I can't
>> help feeling like re-using the numeric data type for other things has
>> led to this confusion. I think that fails in other cases, too: like
>> what if you have a super-long integer that can't be represented as a
>> numeric? I bet jsonb will fail, or maybe it will convert it to a
>> string, but I don't see how it can do anything else.
> Actually, the JSON spec explicitly says that any number that doesn't fit
> in an IEEE double isn't portable [1].  So we're already very far above and
> beyond the spec's requirements by using numeric.  We don't need to improve
> on that.  But I concur with your point that just because PG does X in
> some other cases doesn't mean that we must do X in json or jsonpath.
>
> 			regards, tom lane
>
> [1] https://tools.ietf.org/html/rfc7159#page-6
>
>    This specification allows implementations to set limits on the range
>    and precision of numbers accepted.  Since software that implements
>    IEEE 754-2008 binary64 (double precision) numbers [IEEE754] is
>    generally available and widely used, good interoperability can be
>    achieved by implementations that expect no more precision or range
>    than these provide, in the sense that implementations will
>    approximate JSON numbers within the expected precision.  A JSON
>    number such as 1E400 or 3.141592653589793238462643383279 may indicate
>    potential interoperability problems, since it suggests that the
>    software that created it expects receiving software to have greater
>    capabilities for numeric magnitude and precision than is widely
>    available.
>
>    Note that when such software is used, numbers that are integers and
>    are in the range [-(2**53)+1, (2**53)-1] are interoperable in the
>    sense that implementations will agree exactly on their numeric
>    values.
>


Just to complete the historical record, that standard wasn't published
at the time we created the JSON type, and the then existing standard
(rfc4627) contains no such statement. We felt it was important to be
able to represent any Postgres data value in as natural a manner as
possible given the constraints of JSON. rfc7159 was published just as we
were finalizing 9.4 with JSONB, although I'm not sure it made a heavy
impact on our consciousness. If it had I would still not have wanted to
impose any additional limitation on numerics. If you want portable
numbers cast the numeric to double before producing the JSON.

ISTR having a conversation about the extended use of jsonb in jsonpath a
while back, although I don't remember if that was on or off list. I know
it troubled me some.


cheers


andrew



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

  15. Re: jsonpath versus NaN

    Alexander Korotkov <aekorotkov@gmail.com> — 2020-07-06T12:19:21Z

    On Thu, Jun 18, 2020 at 8:04 PM Alexander Korotkov
<a.korotkov@postgrespro.ru> wrote:
> On Thu, Jun 18, 2020 at 7:45 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> > Alexander Korotkov <a.korotkov@postgrespro.ru> writes:
> > > Thank you for your answer. I'm trying to understand your point.
> > > Standard claims that .double() method should behave the same way as
> > > CAST to double.  However, standard references the standard behavior of
> > > CAST here, not behavior of your implementation of CAST.  So, if we
> > > extend the functionality of standard CAST in our implementation, that
> > > doesn't automatically mean we should extend the .double() jsonpath
> > > method in the same way.  Is it correct?
> >
> > Right.  We could, if we chose, extend jsonpath to allow Inf/NaN, but
> > I don't believe there's an argument that the spec requires us to.
> >
> > Also the larger point is that it doesn't make sense to extend jsonpath
> > that way when we haven't extended json(b) that way.  This code wart
> > wouldn't exist were it not for that inconsistency.  Also, I find it hard
> > to see why anyone would have a use for NaN in a jsonpath test when they
> > can't write NaN in the input json data, nor have it be correctly reflected
> > into output json data either.
>
> Ok, I got the point.  I have nothing against removing support of NaN
> in jsonpath as far as it doesn't violates the standard.  I'm going to
> write the patch for this.

The patchset is attached, sorry for the delay.

The first patch improves error messages, which appears to be unclear
for me.  If one applies .double() method to a numeric value, we
restrict that this numeric value should fit to double precision type.
If it doesn't fit, the current error message just says the following.

ERROR: jsonpath item method .double() can only be applied to a numeric value

But that's confusing, because .double() method is naturally applied to
a numeric value.  Patch makes this message explicitly report that
numeric value is out of range for double type.  This patch also adds
test exercising this error.  When string can't be converted to double
precision, I think it's better to explicitly say that we expected
valid string representation of double precision type.

Second patch forbids to convert NaN using .double() method.  As I get,
NaN can't be result of any jsonpath computations assuming there is no
NaN input.  So, I just put an assert to convertJsonbScalar() ensuring
there is no NaN in JsonbValue.

------
Regards,
Alexander Korotkov

  16. Re: jsonpath versus NaN

    Alexander Korotkov <aekorotkov@gmail.com> — 2020-07-07T22:12:38Z

    On Mon, Jul 6, 2020 at 3:19 PM Alexander Korotkov <aekorotkov@gmail.com> wrote:
> The patchset is attached, sorry for the delay.
>
> The first patch improves error messages, which appears to be unclear
> for me.  If one applies .double() method to a numeric value, we
> restrict that this numeric value should fit to double precision type.
> If it doesn't fit, the current error message just says the following.
>
> ERROR: jsonpath item method .double() can only be applied to a numeric value
>
> But that's confusing, because .double() method is naturally applied to
> a numeric value.  Patch makes this message explicitly report that
> numeric value is out of range for double type.  This patch also adds
> test exercising this error.  When string can't be converted to double
> precision, I think it's better to explicitly say that we expected
> valid string representation of double precision type.
>
> Second patch forbids to convert NaN using .double() method.  As I get,
> NaN can't be result of any jsonpath computations assuming there is no
> NaN input.  So, I just put an assert to convertJsonbScalar() ensuring
> there is no NaN in JsonbValue.

I'm going to push 0002 if there is no objection.

Regarding 0001, I think my new error messages need review.

------
Regards,
Alexander Korotkov

  17. Re: jsonpath versus NaN

    Tom Lane <tgl@sss.pgh.pa.us> — 2020-07-07T22:16:42Z

    Alexander Korotkov <aekorotkov@gmail.com> writes:
> I'm going to push 0002 if there is no objection.
> Regarding 0001, I think my new error messages need review.

I do intend to review these, just didn't get to it yet.

			regards, tom lane

  18. Re: jsonpath versus NaN

    Alexander Korotkov <aekorotkov@gmail.com> — 2020-07-07T22:17:45Z

    On Wed, Jul 8, 2020 at 1:16 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Alexander Korotkov <aekorotkov@gmail.com> writes:
> > I'm going to push 0002 if there is no objection.
> > Regarding 0001, I think my new error messages need review.
>
> I do intend to review these, just didn't get to it yet.

OK, that you for noticing.  I wouldn't push anything before your review.

------
Regards,
Alexander Korotkov

  19. Re: jsonpath versus NaN

    Tom Lane <tgl@sss.pgh.pa.us> — 2020-07-08T22:20:57Z

    Alexander Korotkov <aekorotkov@gmail.com> writes:
> The patchset is attached, sorry for the delay.

> The first patch improves error messages, which appears to be unclear
> for me.  If one applies .double() method to a numeric value, we
> restrict that this numeric value should fit to double precision type.
> If it doesn't fit, the current error message just says the following.

> ERROR: jsonpath item method .double() can only be applied to a numeric value

> But that's confusing, because .double() method is naturally applied to
> a numeric value.  Patch makes this message explicitly report that
> numeric value is out of range for double type.  This patch also adds
> test exercising this error.  When string can't be converted to double
> precision, I think it's better to explicitly say that we expected
> valid string representation of double precision type.

I see your point here, but the English of the replacement error messages
could be improved.  I suggest respectively

numeric argument of jsonpath item method .%s() is out of range for type double precision

string argument of jsonpath item method .%s() is not a valid representation of a double precision number

As for 0002, I'd rather see the convertJsonbScalar() code changed back
to the way it was, ie just

 		case jbvNumeric:
 			numlen = VARSIZE_ANY(scalarVal->val.numeric);
 			padlen = padBufferToInt(buffer);
			...

There is no argument for having an its-not-NaN assertion here when
there aren't similar assertions throughout the jsonb code.

Also, it seems like it'd be smart to reject isinf() and isnan() results
from float8in_internal_opt_error in both executeItemOptUnwrapTarget code
paths, ie numeric source as well as string source.  Yeah, we don't expect
to see those cases in a jbvNumeric (so I wouldn't change the error message
text), but it's cheap insurance.

No other comments.

			regards, tom lane

  20. Re: jsonpath versus NaN

    Alexander Korotkov <aekorotkov@gmail.com> — 2020-07-09T01:04:19Z

    On Thu, Jul 9, 2020 at 1:20 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Alexander Korotkov <aekorotkov@gmail.com> writes:
> > The patchset is attached, sorry for the delay.
>
> > The first patch improves error messages, which appears to be unclear
> > for me.  If one applies .double() method to a numeric value, we
> > restrict that this numeric value should fit to double precision type.
> > If it doesn't fit, the current error message just says the following.
>
> > ERROR: jsonpath item method .double() can only be applied to a numeric value
>
> > But that's confusing, because .double() method is naturally applied to
> > a numeric value.  Patch makes this message explicitly report that
> > numeric value is out of range for double type.  This patch also adds
> > test exercising this error.  When string can't be converted to double
> > precision, I think it's better to explicitly say that we expected
> > valid string representation of double precision type.
>
> I see your point here, but the English of the replacement error messages
> could be improved.  I suggest respectively
>
> numeric argument of jsonpath item method .%s() is out of range for type double precision
>
> string argument of jsonpath item method .%s() is not a valid representation of a double precision number

Good, thank you for corrections!

> As for 0002, I'd rather see the convertJsonbScalar() code changed back
> to the way it was, ie just
>
>                 case jbvNumeric:
>                         numlen = VARSIZE_ANY(scalarVal->val.numeric);
>                         padlen = padBufferToInt(buffer);
>                         ...
>
> There is no argument for having an its-not-NaN assertion here when
> there aren't similar assertions throughout the jsonb code.
>
> Also, it seems like it'd be smart to reject isinf() and isnan() results
> from float8in_internal_opt_error in both executeItemOptUnwrapTarget code
> paths, ie numeric source as well as string source.  Yeah, we don't expect
> to see those cases in a jbvNumeric (so I wouldn't change the error message
> text), but it's cheap insurance.

OK, corrected as you proposed.

> No other comments.

Revised patches are attached.

I understand both patches as fixes and propose to backpatch them to 12
if no objections.

------
Regards,
Alexander Korotkov

  21. Re: jsonpath versus NaN

    Alexander Korotkov <aekorotkov@gmail.com> — 2020-07-11T00:26:23Z

    On Thu, Jul 9, 2020 at 4:04 AM Alexander Korotkov <aekorotkov@gmail.com> wrote:
> I understand both patches as fixes and propose to backpatch them to 12
> if no objections.

Both patches are pushed.

------
Regards,
Alexander Korotkov

  22. Re: jsonpath versus NaN

    Tom Lane <tgl@sss.pgh.pa.us> — 2020-07-11T17:46:37Z

    Alexander Korotkov <aekorotkov@gmail.com> writes:
> On Thu, Jul 9, 2020 at 4:04 AM Alexander Korotkov <aekorotkov@gmail.com> wrote:
>> I understand both patches as fixes and propose to backpatch them to 12
>> if no objections.

> Both patches are pushed.

Thanks for taking care of that!

			regards, tom lane