Subject: Re: Patch bug: Fix jsonpath .* on Arrays

From: "David E. Wheeler" <[email-masked]>
Date: Tue, 25 Jun 2024 13:48:45 -0400

On Jun 25, 2024, at 12:46 AM, Степан Неретин <[email-masked]> wrote:

> Hi! Looks good to me, but I have several comments.

Thanks for your review!

> Your patch improves tests, but why did you change formatting in jsonpath_exec.c? What's the motivation?

It’s not just formatting. From the commit message:

> While at it, teach `executeAnyItem()` to return `jperOk` when `found`
> exist, not because it will be used (the result and `found` are inspected
> by different functions), but because it seems like the proper thing to
> return from `executeAnyItem()` when considered in isolation.

I have since realized it’s not a complete fix for the issue, and hacked around it in my Go version. Would be fine to remove that bit, but IIRC this was the only execution function that would return `jperNotFound` when it in fact adds items to the `found` list. The current implementation only looks at one or the other, so it’s not super important, but I found the inconsistency annoying and sometimes confusing.

>  [1] select jsonb_path_query('[1,2,3,{"b": [3,4,5]}]', 'strict $.*');
> I propose adding a similar test with explicitly specified lax mode: select jsonb_path_query('[1,2,3,{"b": [3,4,5]}]', 'lax $.*'); to show what lax mode is set by default.

Very few of the other tests do so; I can add it if it’s important for this case, though.

> Odd empty result for the test: select jsonb '[1,2,3,{"b": [3,4,5]}]' @? 'strict $.*';
> I expected an error like in test [1]. This behavior is not obvious to me.

@? suppresses a number of errors. Perhaps I should add a variant of the error-raising query that passes the silent arg, which would also suppress the error.

> Everything else is cool. Thanks to the patch and the discussion above, I began to understand better how wildcards in JSON work.

Yeah, it’s kind of wild TBH.

Best,

David

---

From: "David E. Wheeler" <[email-masked]>
Date: Wed, 26 Jun 2024 14:16:05 -0400

> I have since realized it’s not a complete fix for the issue, and hacked around it in my Go version. Would be fine to remove that bit, but IIRC this was the only execution function that would return…

I’ve removed this change.

> Very few of the other tests do so; I can add it if it’s important for this case, though.

Went ahead and added lax.

> @? suppresses a number of errors. Perhaps I should add a variant of the error-raising query that passes the silent arg, which would also suppress the error.

Added a variant where the silent param suppresses the error, too.

V2 attached and the PR updated:

  https://github.com/theory/postgres/pull/4/files

Best,

David

---

From: Stepan Neretin <[email-masked]>
Date: Thu, 27 Jun 2024 11:53:14 +0700

On Thu, Jun 27, 2024 at 1:16 AM David E. Wheeler <[email-masked]>
wrote:

> > I have since realized it’s not a complete fix for the issue, and hacked
> around it in my Go version. Would be fine to remove that bit, but IIRC this
> was the only execution function that would return `jperNotFound` when it in
> fact adds items to the `found` list. The current implementation only looks
> at one or the other, so it’s not super important, but I found the
> inconsistency annoying and sometimes confusing.
>
> >> I propose adding a similar test with explicitly specified lax mode:
> select jsonb_path_query('[1,2,3,{"b": [3,4,5]}]', 'lax $.*'); to show what
> lax mode is set by default.
> >
> > @? suppresses a number of errors. Perhaps I should add a variant of the
> error-raising query that passes the silent arg, which would also suppress
> the error.
>
HI! Now it looks good for me.
Best regards, Stepan Neretin.

---

From: Michael Paquier <[email-masked]>
Date: Thu, 27 Jun 2024 17:17:03 +0900

> HI! Now it looks good for me.

The tests of jsonb_jsonpath.sql include a lot of patterns for @? and
jsonb_path_query with the lax and strict modes, so shouldn't these
additions be grouped closer to the existing tests rather than added at 
the end of the file?
--
Michael

---

From: "David E. Wheeler" <[email-masked]>
Date: Thu, 27 Jun 2024 11:05:31 -0400

> additions be grouped closer to the existing tests rather than added at 

I think you could argue that they should go with other tests for array unwrapping, though it’s kind of mixed throughout. But that’s more the bit I was testing; almost all the tests are lax, and it’s less the strict behavior to test here than the explicit behavior of unwrapping in lax mode.

But ultimately I don’t care where they go, just that we have them.

D

---

From: "David E. Wheeler" <[email-masked]>
Date: Mon, 8 Jul 2024 12:09:15 -0400

> additions be grouped closer to the existing tests rather than added at 

I’ve moved them closer to other tests for unwrapping behavior in the attached updated and rebased v3 patch.

Best,

David

---

From: Stepan Neretin <[email-masked]>
Date: Mon, 15 Jul 2024 18:07:48 +0700

> attached updated and rebased v3 patch.

Hi! Looks good to me now! Please, register a patch in CF.
Best regards, Stepan Neretin.

---

From: "David E. Wheeler" <[email-masked]>
Date: Mon, 15 Jul 2024 10:29:32 -0400

> Best regards, Stepan Neretin.

It’s here:

  https://commitfest.postgresql.org/48/5017/

Best,

David

---

From: Michael Paquier <[email-masked]>
Date: Fri, 19 Jul 2024 14:42:35 +0900

>   https://commitfest.postgresql.org/48/5017/

Sorry for the delay.  Finally came back to it, and applied the tests.
Thanks!

It was fun to see that HEAD was silenced with the first patch of this
thread that tweaked the behavior with arrays.

Regarding the comments, I have left them out for now.  That may be a
good start, but it also feels like we should do a much better job
overall with the area of jsonpath_exec.c.  One thing that may help as
a start is to reorganize the routines of the file and divide them into
sub-categories, or even go through a deeper refactoring to help
readers go through the existing 4.5k lines of code that are in this
single file..
--
Michael

---

From: "David E. Wheeler" <[email-masked]>
Date: Fri, 19 Jul 2024 09:49:50 -0400

> Sorry for the delay.  Finally came back to it, and applied the tests.

Awesome, thank you!

> thread that tweaked the behavior with arrays.

Uh, what? Sorry I don’t follow.

> overall with the area of jsonpath_exec.c.

I put them in because it took me a bit to track down that they were among the implementors of JsonPathGetVarCallback as I was porting the code.

> One thing that may help as

After I got all the tests passing in the port to Go, I split it up into 14 implementation and 15 test files (with all of jsonb_jsonpath.(sql.out) in one file). Was much easier to reason about that way.

https://github.com/theory/sqljson/tree/main/path/exec

Best,

David

---

From: Michael Paquier <[email-masked]>
Date: Mon, 22 Jul 2024 09:54:18 +0900

> Uh, what? Sorry I don’t follow.

What I mean is that the main regression test suite did not complain on
your original patch posted here:
https://www.postgresql.org/message-id/A95346F9-6147-46E0-809E-532A485D71D6%40justatheory.com

But the new tests showed a difference, and that's enough for me to see
value in the new tests.
--
Michael

---

From: "David E. Wheeler" <[email-masked]>
Date: Sun, 21 Jul 2024 22:18:40 -0400

> But the new tests showed a difference, and that's enough for me to see

Oh, got it, nice!

Thanks,

David