ietf-wg-jsonpath / draft-ietf-jsonpath-base

Development of a JSONPath internet draft
https://ietf-wg-jsonpath.github.io/draft-ietf-jsonpath-base/
Other
59 stars 20 forks source link

Update to syntax breakdown #258

Closed gregsdennis closed 1 year ago

gregsdennis commented 2 years ago

Resolves #201 Resolves #255

Reorganizes and simplifies the syntax declarations.

Initial discussions yielded this document. This served as a target. I've added the target to the repo as a reference for myself as I reworked it with nicer commits. It will be removed just before merge as a last step of this PR.

I'm sure that there are some build issues. I probably have links that go nowhere. I think I got all of the ABNF reworked correctly.

Also, I've used //PICKER// and //PICKERS// as placeholders for us to find a name for these things that appear inside the []. Let's do that now.

gregsdennis commented 2 years ago

I'm not sure I can use this build error to find the issue.

/github/workspace/draft-ietf-jsonpath-base.xml(765): Error: Did not expect a numbered section after an unnumbered section (seen on line 751)

I haven't been able to get the build to run locally. How can I translate where this is in the MD?

cabo commented 2 years ago

As the error message says, you can't have numbered sections following unnumbered ones. Since currently the Syntax and Semantics subsubsections are unnumbered, you can't follow them up with numbered ones. Of course, we could turn Syntax and Semantics subsubsections into numbered, but maybe there is a better structure hiding somewhere.

I do recommend investing in a local install.

gregsdennis commented 2 years ago

As the error message says, you can't have numbered sections following unnumbered ones.

Yes. I read that. But the line numbers are on the XML. Since I'm working in the MD, these line numbers don't mean much.

I do recommend investing in a local install.

As I mentioned, I haven't had luck getting it to work. I've tried. The tooling isn't well-suited for windows, and WSL isn't sorting the problem.

gregsdennis commented 2 years ago

Okay... I got the build running locally, but...

/mnt/c/projects/draft-ietf-jsonpath-base/draft-ietf-jsonpath-base.xml(765): Error: Did not expect a numbered section after an unnumbered section (seen on line 751)
/mnt/c/projects/draft-ietf-jsonpath-base/draft-ietf-jsonpath-base.xml(995): Error: Did not expect a numbered section after an unnumbered section (seen on line 751)
/mnt/c/projects/draft-ietf-jsonpath-base/draft-ietf-jsonpath-base.xml(1069): Error: Did not expect a numbered section after an unnumbered section (seen on line 751)
/mnt/c/projects/draft-ietf-jsonpath-base/draft-ietf-jsonpath-base.xml(1277): Error: Did not expect a numbered section after an unnumbered section (seen on line 751)
draft-ietf-jsonpath-base.xml(0): Error: IDREF attribute target references an unknown ID "filter-__PICKER__", at None
draft-ietf-jsonpath-base.xml(0): Error: IDREF attribute target references an unknown ID "filter-__PICKER__", at None
draft-ietf-jsonpath-base.xml(0): Error: IDREF attribute target references an unknown ID "index-__PICKER__", at None
draft-ietf-jsonpath-base.xml(0): Error: IDREF attribute target references an unknown ID "index-selector", at None
draft-ietf-jsonpath-base.xml(0): Error: IDREF attribute target references an unknown ID "index-wildcard-selector", at None
draft-ietf-jsonpath-base.xml(0): Error: IDREF attribute target references an unknown ID "dot-selector", at None
draft-ietf-jsonpath-base.xml(0): Error: IDREF attribute target references an unknown ID "filter-selector", at None
draft-ietf-jsonpath-base.xml(0): Error: IDREF attribute target references an unknown ID "filter-selector", at None
draft-ietf-jsonpath-base.xml(0): Error: IDREF attribute target references an unknown ID "name-__PICKER__", at None
/mnt/c/projects/draft-ietf-jsonpath-base/draft-ietf-jsonpath-base.xml(11): Error: Invalid document before running preptool.
Unable to complete processing draft-ietf-jsonpath-base.xml
make: *** [lib/main.mk:122: draft-ietf-jsonpath-base.txt] Error 1
rm draft-ietf-jsonpath-base.xml

That last line... it reports all of the errors based on the XML file, then deletes the XML file?! HOW AM I SUPPOSED TO KNOW WHERE THE ERROR IS?!

The //PICKER// stuff is causing problems as expected. I'll see if I can resolve those other target ref issues.

gregsdennis commented 2 years ago

I think all that's left is to decide on a word for //PICKER//.

cabo commented 2 years ago
rm draft-ietf-jsonpath-base.xml

Welcome to the wonderful world of make(1).

You can give draft-ietf-jsonpath-base.xml as your make target:

$ make draft-ietf-jsonpath-base.xml

Once you have one lying around, make(1) will no longer delete this intermediate product.

gregsdennis commented 2 years ago

@glyn, you wanted a rendered version. See attached.

For this version, I've replaced all of the //PICKER// instances with the word "picker" (likewise for the plural form). This word is still open for suggestion, but it should suffice for your needs.

draft-ietf-jsonpath-base.txt

glyn commented 2 years ago

Thank you very much for the rendered version. I'm AFK until next week, but am looking forward to doing a thorough review and seeing how this version reads.

On Tue, 6 Sep 2022, 08:46 Greg Dennis, @.***> wrote:

@glyn https://github.com/glyn, you wanted a rendered version. See attached.

For this version, I've replaced all of the //PICKER// instance with the word "picker" (likewise for the plural form). This word is still open for suggestion, but it should suffice for your needs.

draft-ietf-jsonpath-base.txt https://github.com/ietf-wg-jsonpath/draft-ietf-jsonpath-base/files/9494424/draft-ietf-jsonpath-base.txt

— Reply to this email directly, view it on GitHub https://github.com/ietf-wg-jsonpath/draft-ietf-jsonpath-base/pull/258#issuecomment-1237779990, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAAXF2P2JKKBOYKILKMHHODV43ZDPANCNFSM6AAAAAAQEUPARI . You are receiving this because you were mentioned.Message ID: @.*** com>

gregsdennis commented 2 years ago

One thing that I'm not quite pleased with is how the Syntax and Semantics subsections of Child Selector are numbered. They weren't previously, but I couldn't get around the "numbered section after un-numbered" thing, which was caused by adding numbered subsections for each of the pickers.

glyn commented 2 years ago

One way of dealing with the overly deep nesting/section numbering difficulty might be to create a Pickers section as a sibling of the current Selectors section?

On Tue, 6 Sep 2022, 08:51 Greg Dennis, @.***> wrote:

One thing that I'm not quite pleased with is how the Syntax and Semantics subsections of Child Selector are numbered. They weren't previously, but I couldn't get around the "numbered section after un-numbered" thing, which was caused by adding numbered subsections for each of the pickers.

— Reply to this email directly, view it on GitHub https://github.com/ietf-wg-jsonpath/draft-ietf-jsonpath-base/pull/258#issuecomment-1237786168, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAAXF2LJOVO3MEVBSO42Y7LV43ZZNANCNFSM6AAAAAAQEUPARI . You are receiving this because you were mentioned.Message ID: @.*** com>

cabo commented 2 years ago

The pickers are not specific to the two appenders they can occur in, so they don't belong there.

gregsdennis commented 2 years ago

I thought about doing that, but it doesn't make sense to have them before the "Child Selector" section because you need that as a context for what a *picker is. I could put them after "Descendant Selector," but then you end up reading that "Child Selector" needs a *picker without finding out what it is before moving on to the next selector. We can link to the *picker section, but I don't know if that fixes things.

I'll post a render with the *picker section after descendants for comparison when I can.

goessner commented 2 years ago

Thanks for the new version and your work.

"It's hard to describe arrays without having a clue about the data types going into it" ... we talked about that. I would see the pickers before the appenders, which together form the selectors, so ...

Selector syntax is sufficiently shown in the Examples section (maybe an Overview section would be of help here). Having a clue how to use *pickers, I would like to read about them first. Users and implementors usually don't read the spec linearly like a book.

I agree to Carsten, to consequently distinguish between pickers and appenders.

We already used index for member names and array index. So I would like to expand it's use to

which would be the least difference to the current spec (and surprise to readers). What do you think about that? Thus ...

is my proposal. Thanks

Stefan

gregsdennis commented 2 years ago
  • *appender = accessor
  • selector

@goessner, what's the difference between these that you can see? When @cabo suggested these placeholders, they were *starter, *appender, and *picker.

Having a clue how to use *pickers, I would like to read about them first. Users and implementors usually don't read the spec linearly like a book.

If you don't think that reading about *pickers before the context in which they exist would be a problem, sure, the sections can be arranged this way.

We already used index for member names and array index. So I would like to expand it's use...

In the next rendered examples (picker sections pulled out), I'll use "index" instead of "picker" and we can see how that reads.

goessner commented 2 years ago

In the next rendered examples (picker sections pulled out), I'll use "index" instead of "picker" and we can see how that reads.

No ... pickers should stay where they are. I merely want to start brainstorming in parallel, how to finally coin the xxx terms. We need to agree about them, before you change them.

goessner commented 2 years ago

When @cabo suggested these placeholders, they were *starter, *appender, and *picker

Yes, *starters are merely special selectors. We did not eliminate the term selector. We just need to coin that three terms.

*starter
$ root node
@ current node during iteration
*appender allows ...
. accessing members of objects
.. accessing descendants of structured values
[] accessing children of structured values
..[] compound *appender, accessing descendants of structured values

Even if the first two are merely used for shortcuts, they are valid *appenders.

*picker
* wildcard index
'name' object member index
<int> array element index
<start>:<end>:<step> array slice index
?... filter index

In the end we have five pickers, two appenders, two *starters and some shortcuts.

gregsdennis commented 2 years ago

We need to agree about them, before you change them. - @goessner

Yes, but I can't render the doc to provide previews without something there. //PICKER// breaks the build.

gregsdennis commented 2 years ago

*pickers should stay where they are.

Having a clue how to use *pickers, I would like to read about them first.

These two statements are in conflict. If you want to read about them first, they need to be pulled out of "Child Selectors" and moved forward. That needs to occur anyway to reduce those sections' nesting level.

goessner commented 2 years ago

*pickers should stay where they are.

I wanted to say: the term '*pickers' should stay as it is, for now ...

gregsdennis commented 2 years ago

Okay, so for these two previews, I have extracted the *pickers out into their own H2 section. One has the section before the selectors, and the other has them after. I have no preference.

draft-ietf-jsonpath-base (pickers before selectors).txt draft-ietf-jsonpath-base (pickers after selectors).txt

glyn commented 2 years ago

@gregsdennis I've started reviewing this PR. I prefer to read rendered HTML rather than text, but the current document fails to render. Any chance you could fix the following errors, please?

./scripts/gen.sh
draft-ietf-jsonpath-base.xml(0): Error: IDREF attribute target references an unknown ID "index-__PICKER__", at None
draft-ietf-jsonpath-base.xml(0): Error: IDREF attribute target references an unknown ID "filter-__PICKER__", at None
draft-ietf-jsonpath-base.xml(0): Error: IDREF attribute target references an unknown ID "filter-__PICKER__", at None
draft-ietf-jsonpath-base.xml(0): Error: IDREF attribute target references an unknown ID "filter-__PICKER__", at None
draft-ietf-jsonpath-base.xml(0): Error: IDREF attribute target references an unknown ID "filter-__PICKER__", at None
draft-ietf-jsonpath-base.xml(0): Error: IDREF attribute target references an unknown ID "name-__PICKER__", at None
draft-ietf-jsonpath-base.xml(0): Error: IDREF attribute target references an unknown ID "name-__PICKER__", at None
/home/glyn/dev/ietf/draft-ietf-jsonpath-base/draft-ietf-jsonpath-base.xml(16): Error: Invalid document before running preptool.
Unable to complete processing draft-ietf-jsonpath-base.xml
gregsdennis commented 2 years ago

@glyn , no. I will not replace those until we have decided on a term for them. You're welcome to change it locally and render it yourself.

glyn commented 2 years ago

@glyn , no. I will not replace those until we have decided on a term for them. You're welcome to change it locally and render it yourself.

I wanted it rendered with a wildcard such as *picker of //PICKER// in place. I agree we shouldn't decide on the term prematurely. I can attempt this locally, but WIBNI we could all review the same rendered version (by pushing to a branch in the main repo)?

gregsdennis commented 2 years ago

I wanted it rendered with a wildcard such as *picker of //PICKER// in place.

That would still result in the build failure.

I think the text version is sufficient. I've been gracious enough to provide that. If you want some thing further, please handle it yourself.

glyn commented 2 years ago

I wanted it rendered with a wildcard such as *picker of //PICKER// in place.

That would still result in the build failure.

I think the text version is sufficient. I've been gracious enough to provide that. If you want some thing further, please handle it yourself.

I replaced //PICKER// with picker and //PICKERS// with pickers (in commit 832286a5d824a) and got:

./scripts/gen.sh
draft-ietf-jsonpath-base.xml(493): Error: Invalid attribute slugifiedName for element name, at /rfc/middle/section[3]/section[3]/name
/home/glyn/dev/ietf/draft-ietf-jsonpath-base/draft-ietf-jsonpath-base.xml(16): Warning: Invalid document after running preptool.

... line too long warnings omitted ...

 Created file draft-ietf-jsonpath-base.txt

draft-ietf-jsonpath-base.xml(493): Error: Invalid attribute slugifiedName for element name, at /rfc/middle/section[3]/section[3]/name
/home/glyn/dev/ietf/draft-ietf-jsonpath-base/draft-ietf-jsonpath-base.xml(16): Warning: Invalid document after running preptool.
(No source line available): Warning: Duplicate id="name-semantics" found in generated HTML.
 Created file draft-ietf-jsonpath-base.html

stdin(152:63): error: Illegal character '\' - skipping to end of line
stdin(153:0): error: Missing ')'?  Extra '('?
stdin(157:49): error: Illegal character '\' - skipping to end of line
stdin(158:0): error: Missing ')'?  Extra '('?
parsing failed: 4 errors encountered

What do I need to do other than replace //PICKER// and //PICKERS// globally?

gregsdennis commented 2 years ago

I replaced //PICKER// with picker and //PICKERS// with pickers

This is what I did to generate the previews. I received no errors.

gregsdennis commented 2 years ago

To summarize my stance:

Pickers are the actors within the Child Selector, which merely functions as an aggregator. If you want to think of the pickers as simply identifying (or matching) which children to select, then sure. It makes no difference really. This is the crux of the relationship between them.

@glyn has failed to explain why he feels that *pickers or their semantics are not sufficiently defined. I've not changed much of the text; I've merely moved it around a bit.

There is also a debate around iteration. Either the selector or the picker must do iteration. For me, it doesn't make sense for the selector to do the iteration because the only picker that needs iteration is the filter picker. The other pickers (index, name, slice) don't need to iterate through the values to identify which ones to pick. Only the filter needs to do this. Because of this asymmetry, it only makes sense to have the filter picker iterate.

Alternatively, if language can be devised where iteration is not used at all, I could be happy with that.

the resultant document is less readable for users than the current draft because they have to deal with an extra //PICKER// term which doesn't add much value and because the document structure is more deeply nested.

This is purely subjective. The *pickers term (whatever it ends up being) is necessary. We need some term for the inside bits of the [] selector, even in the document's current state (i.e. what is the "list selector" a list of?).

Additionally, I have been gracious enough to supply alternate renderings where the *pickers are brought outside of the "Child Selectors" section in order to address nesting. I have received no feedback other than Glyn compaining that he prefers HTML.

gregsdennis commented 2 years ago

What I am proposing here is more correct than the current document, semantically and syntactically.

I'm open to massaging the text and fixing errata, but the concept is objectively more sound for reasons I've explained repeatedly in #201.

glyn commented 2 years ago

While waiting for others to review this PR, let me respond to some of these points:

To summarize my stance:

Pickers are the actors within the Child Selector, which merely functions as an aggregator. If you want to think of the pickers as simply identifying (or matching) which children to select, then sure. It makes no difference really. This is the crux of the relationship between them.

@glyn has failed to explain why he feels that *pickers or their semantics are not sufficiently defined. I've not changed much of the text; I've merely moved it around a bit.

I was hoping for a clearer distinction between the semantics of pickers and selectors. If we go in this aggregation direction, then this is pretty close to the semantics of the current draft IMO except that we'll have two very similar terms, which could be confusing (unless "picker" turns into "selector").

There is also a debate around iteration. Either the selector or the picker must do iteration. For me, it doesn't make sense for the selector to do the iteration because the only picker that needs iteration is the filter *picker.

Note that I anticipate adding a wildcard picker in due course.

The other pickers (index, name, slice) don't need to iterate through the values to identify which ones to pick. Only the filter needs to do this. Because of this asymmetry, it only makes sense to have the filter *picker iterate.

I don't follow the logic there. The other *pickers could simply fail to match items which aren't relevant (based on their names or indices if we adopted option 2).

Alternatively, if language can be devised where iteration is not used at all, I could be happy with that.

the resultant document is less readable for users than the current draft because they have to deal with an extra //PICKER// term which doesn't add much value and because the document structure is more deeply nested.

This is purely subjective.The *pickers term (whatever it ends up being) is necessary. We need some term for the inside bits of the [] selector, even in the document's current state (i.e. what is the "list selector" a list of?).

I agree it would be good to name the items inside [] and to unify the list selector and the [] selector with a single item inside. Perhaps this could be done in the syntax but with the semantics defined consistently in terms of the [] selector with one or more items inside.

Additionally, I have been gracious enough to supply alternate renderings where the *pickers are brought outside of the "Child Selectors" section in order to address nesting. I have received no feedback other than Glyn complaining that he prefers HTML.

The point-in-time text renderings were useful, thanks, but I'd like to keep rendering the latest version of the PR so I can see how the document flows with the latest text. If we can't automate this, I'll render it locally, but I haven't yet managed to get this to work. (Next, I'll try bisecting the commits to see when the errors I am seeing crept in.) I prefer HTML so I can easily see how the document flows when navigating certain hyperlinks. I suspect I'll prefer pickers being lifted out to a separate section before selectors, to minimise forward references, but the success of that somewhat depends on how well picker semantics can be isolated from selector semantics.

cabo commented 2 years ago

If we can't automate this,

We can. Apologies for this. I didn't see a need to fully port this repo to the I-D-template; I'll get started with that now.

cabo commented 2 years ago

If we can't automate this,

We can. Apologies for this. I didn't see a need to fully port this repo to the I-D-template; I'll get started with that now.

Oh, it is converted, it's just the draft that is broken. See https://github.com/ietf-wg-jsonpath/draft-ietf-jsonpath-base/runs/8289624534?check_suite_focus=true

I will fix that.

cabo commented 2 years ago

This now builds, but doesn't update the gh-pages. Will look later why.

glyn commented 2 years ago

@cabo wrote:

This now builds, but doesn't update the gh-pages. Will look later why.

I updated to ba6d370cc8, but the build still fails locally:

./scripts/gen.sh
draft-ietf-jsonpath-base.xml(493): Error: Invalid attribute slugifiedName for element name, at /rfc/middle/section[3]/section[3]/name
/home/glyn/dev/ietf/draft-ietf-jsonpath-base/draft-ietf-jsonpath-base.xml(16): Warning: Invalid document after running preptool.

... multiple line too long warnings ...

 Created file draft-ietf-jsonpath-base.txt

draft-ietf-jsonpath-base.xml(493): Error: Invalid attribute slugifiedName for element name, at /rfc/middle/section[3]/section[3]/name
/home/glyn/dev/ietf/draft-ietf-jsonpath-base/draft-ietf-jsonpath-base.xml(16): Warning: Invalid document after running preptool.
(No source line available): Warning: Duplicate id="name-semantics" found in generated HTML.
 Created file draft-ietf-jsonpath-base.html

stdin(152:63): error: Illegal character '\' - skipping to end of line
stdin(153:0): error: Missing ')'?  Extra '('?
stdin(157:49): error: Illegal character '\' - skipping to end of line
stdin(158:0): error: Missing ')'?  Extra '('?
parsing failed: 4 errors encountered

Any ideas?

cabo commented 2 years ago

On 2022-09-12, at 17:13, Glyn Normington @.***> wrote:

@cabo wrote:

This now builds, but doesn't update the gh-pages. Will look later why.

I updated to ba6d370, but the build still fails locally:

./scripts/gen.sh

Don’t do that — just make…

draft-ietf-jsonpath-base.xml(493): Error: Invalid attribute slugifiedName for element name, at /rfc/middle/section[3]/section[3]/name /home/glyn/dev/ietf/draft-ietf-jsonpath-base/draft-ietf-jsonpath-base.xml(16): Warning: Invalid document after running preptool.

This is weird output (error in xml2rfc), because a TXT file is still being created:

Created file draft-ietf-jsonpath-base.txt

draft-ietf-jsonpath-base.xml(493): Error: Invalid attribute slugifiedName for element name, at /rfc/middle/section[3]/section[3]/name /home/glyn/dev/ietf/draft-ietf-jsonpath-base/draft-ietf-jsonpath-base.xml(16): Warning: Invalid document after running preptool. (No source line available): Warning: Duplicate id="name-semantics" found in generated HTML. Created file draft-ietf-jsonpath-base.html

… and an HTML file.

So that all works.

stdin(152:63): error: Illegal character '\' - skipping to end of line stdin(153:0): error: Missing ')'? Extra '('? stdin(157:49): error: Illegal character '\' - skipping to end of line stdin(158:0): error: Missing ')'? Extra '('? parsing failed: 4 errors encountered

Yes, the ABNF is broken (a couple of dyslexic backslashes where there should be slashes, I think). I didn’t look at that yet.

Grüße, Carsten

cabo commented 2 years ago

I think the "slugified" confusion comes from a numbered syntax section and an unnumbered syntax section in the same document. We will fix this editorially, so we can ignore the xml2rfc misbehavior. I pushed an ABNF update.

cabo commented 2 years ago

Now https://github.com/ietf-tools/xml2rfc/issues/890

glyn commented 2 years ago

I think the "slugified" confusion comes from a numbered syntax section and an unnumbered syntax section in the same document. We will fix this editorially, so we can ignore the xml2rfc misbehavior. I pushed an ABNF update.

make now renders the HTML version (with some warnings). Thanks @cabo.

gregsdennis commented 2 years ago

In regard to ba6d370, if we're going to change things like this, can we please be sure to apply it everywhere? There are still //PICKER//s throughout the document.

__PICKER__ renders as PICKER (bold), just FYI.

gregsdennis commented 2 years ago

draft-ietf-jsonpath-base.txt draft-ietf-jsonpath-base.html.txt

cabo commented 2 years ago

In regard to ba6d370, if we're going to change things like this, can we please be sure to apply it everywhere? There are still //PICKER//s throughout the document.

__PICKER__ renders as PICKER (bold), just FYI.

I just changed the anchors/idrefs, which are not shown. But, yes, this is certainly a simplifying change, as we probably want to use the final names for the anchors/idrefs as well.

glyn commented 2 years ago

The draft PR https://github.com/ietf-wg-jsonpath/draft-ietf-jsonpath-base/pull/259 makes a start at extracting the syntactic advantages of this PR with less disruption to the document structure or semantics.

cabo commented 1 year ago

I believe we will choose "selector" as the new name for picker, and thus need a new name for selector. These are different things (each is defined by the interaction with the other) and conflating them doesn't help.

glyn commented 1 year ago

I believe we will choose "selector" as the new name for picker, and thus need a new name for selector. These are different things (each is defined by the interaction with the other) and conflating them doesn't help.

Disjoint terms are not the only option. We could use an umbrella term "selector" to denote something which maps a value to a nodelist of reflexive (i.e. including the value itself, to accommodate $ and @) descendants of the value. Then we could use an adjectival phrases such as "sub-selector" (since they always occur inside another selector) for *pickers and "aggregate selector" or "composite selector" for the child and descendant selectors. I'm not wedded to those particular phrases, but at least they avoid giving entirely disjoint names to closely related concepts.

However, I suppose disjoint terms could be made to work. E.g. we could name *selectors "compositors", "aggregates", or such like...

cabo commented 1 year ago

The point of disjoint terms is to have two different terms for things that are completely different. A selector needs one or more pickers, and a picker needs to be used inside a selector. Calling both with the same name means that no statement can be made that applies to both sub-meanings of the term.

glyn commented 1 year ago

Calling both with the same name means that no statement can be made that applies to both sub-meanings of the term.

That's a good point. Let's try disjoint terms then.

gregsdennis commented 1 year ago

And remove the current definition "A single item within a bracketed ([]) child selector that matches the values which are to be selected." which is focused on one example of how PICKERS are used. - @glyn

I don't understand how this focuses on a single use case? A *picker matches/identifies child elements that are to be selected. That's all it does.

What other function do you see for *pickers?

glyn commented 1 year ago

And remove the current definition "A single item within a bracketed ([]) child selector that matches the values which are to be selected." which is focused on one example of how PICKERS are used. - @glyn

I don't understand how this focuses on a single use case? A *picker matches/identifies child elements that are to be selected. That's all it does.

The phrase "A single item within a bracketed ([]) child selector" seems to be focus on the child selector and ignores the descendant selectors.

What other function do you see for *pickers?

I think we need to move away from "matching" language and start to use selector language with *pickers taking a value and producing a nodelist.

gregsdennis commented 1 year ago

@glyn I've realized that we may have had a conflict of communication earlier regarding iteration. I'd like to expand on that.

There are two kinds of iteration happening:

The way I see it, the child selector will perform the first kind, input nodelist iteration. It iterates over the nodelist resulting from the previous selector, passing each node to each of the *pickers. (This is the iteration I now think you may have been referring to.)

Each picker then identifies which children of that node that the child selector should return. Filter pickers, specifically, must iterate over the children of the node in order to make this determination. The other *pickers do not need to iterate of the children; they can merely use the bounds. (This is the iteration I was referring to.).

A couple examples:

The picker identifies which children to select from a single node as the child selector iterates over the previous nodelist. The picker then identifies which children of that node to select. Finally, the child selector aggregates those children (via concatenation) and outputs them in its own nodelist for consumption by the next selector (or for path output).

Ordering

The order of the resulting node list is the full selection of each picker across the entire nodelist concatenated in the order of the pickers.

[
  [ "a1", "b1", "c1", "d1" ],
  [ "a2", "b2", { "foo": 2 }, "d2" ],
  [ "a3", "b3", "c3", { "foo": 3 } ],
  [ "a4", "b4", "c4", "d4" ]
]
$[*][1,?@.foo]

The [*] returns the arrays in its nodelist:

Then the child selector passes each one of those to the *pickers.

But we need this in *picker order, so we take all of the results from 1 followed by all of the results from ?@.foo. The resulting nodelist from the child selector is:

The resulting list could also be achieved using multiple iterations of the input nodelist, passing all of them to each picker in turn. This would avoid the re-sorting step at the end. Either algorithm is fine with me, they're both O(n²). If you think one is easier to comprehend, let's use that.

I'm not sure how much of this you want laid out in the document.

glyn commented 1 year ago

@gregsdennis I suspect we have quite different ideas of what constitutes a spec. From my perspective, the purpose of the JSONPath spec is to describe what the results (plural because of non-determinism) can be, given a JSON value and a JSONPath, but without prescribing how the results are computed.

Occasionally, it's necessary to write parts of the spec as an algorithm, such as the details of slicing, because the behaviour is just too "fiddly" to write as a non-algorithmic spec. But this is not meant to dictate how all implementations should be written - any implementation which produces equivalent results is acceptable.

With that in mind, let me try to answer address your comment.

@glyn I've realized that we may have had a conflict of communication earlier regarding iteration. I'd like to expand on that.

There are two kinds of iteration happening:

* input nodelist iteration

* child iteration

The way I see it, the child selector will perform the first kind, input nodelist iteration. It iterates over the nodelist resulting from the previous selector, passing each node to each of the *pickers. (This is the iteration I now think you may have been referring to.)

Each picker then identifies which children of that node that the child selector should return. Filter pickers, specifically, must iterate over the children of the node in order to make this determination. The other *pickers do not need to iterate of the children; they can merely use the bounds. (This is the iteration I was referring to.).

A couple examples:

* `0:5:2` would be given a node (as part of the nodelist iteration from the selector).  If that node is an array, then it just returns the values at indices 0, 2, and 4, if those indices exist.  I see this as direct access (think array access in C), not iteration.

* `?@.foo==1` would also be given a node.  This *picker would need to iterate over and evaluate all of the children of the node in order to identify which ones are objects where the `foo` property is `1`.

The picker identifies which children to select from a single node as the child selector iterates over the previous nodelist. The picker then identifies which children of that node to select. Finally, the child selector aggregates those children (via concatenation) and outputs them in its own nodelist for consumption by the next selector (or for path output).

Ordering

The order of the resulting node list is the full selection of each picker across the entire nodelist concatenated in the order of the pickers.

[
  [ "a1", "b1", "c1", "d1" ],
  [ "a2", "b2", { "foo": 2 }, "d2" ],
  [ "a3", "b3", "c3", { "foo": 3 } ],
  [ "a4", "b4", "c4", "d4" ]
]
$[*][1,?@.foo]

The [*] returns the arrays in its nodelist:

* `[ "a1", "b1", "c1", "d1" ]`

* `[ "a2", "b2", { "foo": 2 }, "d2" ]`

* `[ "a3", "b3", "c3", { "foo": 3 } ]`

* `[ "a4", "b4", "c4", "d4" ]`

Then the child selector passes each one of those to the *pickers.

* From `[ "a1", "b1", "c1", "d1" ]`

  * `1` identifies `"b1"`
  * `?@.foo` identifies nothing

* From `[ "a2", "b2", { "foo": 2 }, "d2" ]`

  * `1` identifies `"b2"`
  * `?@.foo` identifies `{ "foo": 2 }`

* From `[ "a3", "b3", "c3", { "foo": 3 } ]`

  * `1` identifies `"b3"`
  * `?@.foo` identifies `{ "foo": 3 }`

* From `[ "a4", "b4", "c4", "d4" ]`

  * `1` identifies `"b4"`
  * `?@.foo` identifies nothing

But we need this in *picker order, so we take all of the results from 1 followed by all of the results from ?@.foo. The resulting nodelist from the child selector is:

* `"b1"`

* `"b2"`

* `"b3"`

* `"b4"`

* `{ "foo": 2 }`

* `{ "foo": 3 }`

The resulting list could also be achieved using multiple iterations of the input nodelist, passing all of them to each picker in turn. This would avoid the re-sorting step at the end. Either algorithm is fine with me, they're both O(n²). If you think one is easier to comprehend, let's use that.

I'm not sure how much of this you want laid out in the document.

The current draft has a top-level semantics section which describes, admittedly in somewhat algorithmic terms, the nodelists passed between the selectors constituting a JSONPath. This section is unmodified in the current PR. I think this is really the only place which needs to deal with what you describe as iterating over the nodelist. Individual selectors just need to say what their output nodelist can be for any input value (i.e. just one element of the input nodelist).

The other iteration you describe involves the children of one of the input values of the child selector. That's the one I had in mind when I described two options for how to specify the selector-picker interface. I believe those options are equivalent as specifications of what the results of a child selector can be. We don't want to mandate either option as the way implementations should be written. Indeed, some implementations could be structured as selectors and pickers and other implementations could have only selectors.

So how do we choose which option to use in the spec? Not by the performance of a naive implementation which mirrors the structure of the spec closely, since more optimal implementations are perfectly acceptable so long as they produce results which conform to the spec.

We should choose the option which gives the clearest specification. This is a value judgement. I preferred option 2 because its interface is narrower in the sense that it deals with a single child of the input value. The ordering of the children is then not a concern for pickers. Of course, a naive implementation of option 2 could perform badly, e.g. when applying an index picker to a value with a great many children, but other implementations could avoid unnecessary iteration.

However, you preferred option 1 and I was comfortable going along with that, so long as we understand that the goal is to write a spec rather than prescribe how implementations should be written. With option 1, each picker is really a selector because it takes a value (from its input nodelist) and produces a nodelist. This means that the current selector text can be largely reused. Instead of the picker descriptions being reworked to talk about matching, I think it's fine (in option 1) to simply say which children each *picker selects, and the order in which these occur in the output nodelist.

With option 2, the multi-child aspects can all be described in the child selector spec. Each *picker spec just needs to describe whether or not a given child is matched, which from my perspective is quite a lot simpler.

Does this help? Sorry for a long answer, but as you appreciate, it's easy to miscommunicate.

Edited to correct the option numbering.

cabo commented 1 year ago

Option 1, very very obviously. Filter pickers iterate over children of each of the nodes given, others don't. The difference between the child and the descendants selector is which nodes are presented to the pickers (the current node, or that and all of its descendants).