Adding on-demand support for a subset of json path. by FranciscoThiesen · Pull Request #2127 · simdjson/simdjson

FranciscoThiesen · 2024-02-11T06:08:30Z

Implementation for #2070

Added at_path() method + documentation
Created a new ondemand_json_path_tests file to stress test the newly introduced method

After some reviews I can also introduce this in the DOM (since it won't be much different)

- Not yet tested - Not yet compiled - Not yet used in ondemand array/object/etc...

Pending: - Building - Adding tests for each of the on-demand classes usage of at_path - Properly documenting the subset of json path that is currently supported.

…_path() call

FranciscoThiesen · 2024-02-11T06:35:26Z

Will check the failures tomorrow

…n function. Now json_path_to_pointer_conversion returns a std::string.

include/simdjson/generic/ondemand/array-inl.h

… -O3 flag.

tests/ondemand/ondemand_json_path_tests.cpp

doc/basics.md

lemire · 2024-02-12T17:05:25Z

@FranciscoThiesen Looks very good to me. Please consider my comments. (All minor stuff so far.)

lemire · 2024-02-12T17:06:24Z

@mbasmanova Would you have a look? At least at the documentation?

There is still time to clarify the documentation, adjust the API and so forth.

FranciscoThiesen · 2024-02-12T17:19:55Z

Thanks for the review @lemire, I didn't know about the string suffix format so I learned something new (:

Will make the adjustments later today!

lemire · 2024-02-13T00:07:22Z

I didn't know about the string suffix format so I learned something new

Look at my PR, it should become clear...

#2128

Let me repeat myself: we can't use it inside the library because we require that the library be compatible with C++11... but I fully expect nearly everyone that uses simdutf to be at C++17 right now... or better.

mbasmanova

@FranciscoThiesen @lemire Francisco, thank you for working on this. Daniel, thank you for enabling this work. I read the docs and found them very nice and easy to understand. I made a few tiny comments.

doc/basics.md

mbasmanova · 2024-02-13T06:18:21Z

doc/basics.md

-A JSON Path is a sequence of segments each starting with the '/' character. Within arrays, an integer
+A JSON Pointer path is a sequence of segments each starting with the '/' character. Within arrays, an integer
 index allows you to select the indexed node. Within objects, the string value of the key allows you to
 select the value. If your keys contain the characters '/' or '~', they must be escaped as '~1' and


If your keys contain the characters '/' or '~'

I understand that / is used as a separator, hence, needs escaping, but I wonder why ~ needs escaping.

Curious, do we allow unicode characters?

This comes from the json pointer spec: https://datatracker.ietf.org/doc/html/rfc6901#section-4

Since we use ~1 to escape '/', there has to be a way for the user to specify ~1 as part of a valid string. Imagine that we want to specify the following path "/someField~1/...", without having the ~1 being replaced by "/". For this to happen, we need to escape the ~ as ~0 and then the string can be represented as /someField~01/

@lemire can you clarify regarding unicode character support? (I see it in the JSON pointer spec, but you have a lot more context here)

Since we use '~1' to escape '/', there has to be a way for the user to specify...

Got it. Thanks.

Oh, the markdown text for the message got super weird from using ~. Just updated the comment and it seems that the strikethrough effect is now gone from the markdown format.

@lemire can you chime in here regarding @mbasmanova question on unicode characters?

(Regarding my comment, I know we mean JSON Path, but I was answering regarding what exists currently, prior to this PR.)

@FranciscoThiesen @mbasmanova My statement was incorrect. We will support unicode (UTF-8) just fine, but not if there are escaped Unicode sequences.

So the key "école" would be matched. No problem. But only if it appears byte-by-byte identical in the path and in the JSON document.

It has to be UTF-8 throughout. We don't do normalization. We don't escape.

@lemire Goti it. Thank you for clarifying. If not too much trouble, it would be nice to document this.

It will be documented for the next release.

doc/basics.md

lemire · 2024-02-13T18:18:50Z

@FranciscoThiesen I suggest you have a look at @mbasmanova's comments. If needed, I can chime in/help.

FranciscoThiesen · 2024-02-13T19:22:46Z

Thank you @mbasmanova for the review! Will address all the comments later today.

lemire · 2024-02-14T13:33:44Z

@FranciscoThiesen Do you recommend merging?

FranciscoThiesen · 2024-02-14T17:36:29Z

@lemire yes

Regarding improvements that I can do in a follow-up PR, so far I have:

Adding better/more informative error handling during path -> pointer conversion

Also, do you think that adding at_path support for DOM makes sense, or it would be more interesting for me to work on any other issues?

lemire · 2024-02-15T05:18:52Z

DOM makes sense

Sure !!!

If you have lots of talent and energy, the most sought after issue is #1386

lemire · 2024-02-15T14:47:22Z

Merging. I will improve the documentation before the release.

lemire · 2024-02-15T16:35:46Z

The documentation regarding Unicode characters is coming: #2132

Note that I changed "JSON Path" to "JSONPath" throughout as the in-progress RFC uses JSONPath and we tend to follow the conventions present in the RFCs.

FranciscoThiesen added 13 commits February 7, 2024 21:48

First implementation of the conversion logic.

bdec27e

- Not yet tested - Not yet compiled - Not yet used in ondemand array/object/etc...

Adding at_path to ondemand arrays

91b4668

Adding at_path to array, document and value.

c955ecd

Adding at_path to ondemand object as well.

b8b942e

Pending: - Building - Adding tests for each of the on-demand classes usage of at_path - Properly documenting the subset of json path that is currently supported.

Adding a simple json_path test to on_demand

0b0a609

Still trying to compile the ondemand_readme_examples test with the at…

5904d12

…_path() call

Fixing linking issues with array::at_path

fa5dcbe

Fixing issues with the path -> pointer conversion and removing comments

ef04491

Adding a clone of json_pointer to test json_path extensively

e70e925

Adding ondemand_json_path_tests (all tests passing)

9f7a635

Merge branch 'simdjson:master' into master

aba6c1b

Adding documentation for at_path()

1289559

Removing some newlines

e0c8f6d

Removing simdjson_result from the return type of the string conversio…

cea8b4f

…n function. Now json_path_to_pointer_conversion returns a std::string.

lemire reviewed Feb 11, 2024

View reviewed changes

include/simdjson/generic/ondemand/array-inl.h Outdated Show resolved Hide resolved

FranciscoThiesen added 2 commits February 11, 2024 18:11

Fixing typo

8840a38

Making string concatenation explicit to avoid ubuntu gcc12 issue with…

3440669

… -O3 flag.

lemire reviewed Feb 12, 2024

View reviewed changes

tests/ondemand/ondemand_json_path_tests.cpp Show resolved Hide resolved

lemire reviewed Feb 12, 2024

View reviewed changes

doc/basics.md Outdated Show resolved Hide resolved

lemire reviewed Feb 12, 2024

View reviewed changes

doc/basics.md Show resolved Hide resolved

lemire reviewed Feb 12, 2024

View reviewed changes

doc/basics.md Outdated Show resolved Hide resolved

lemire mentioned this pull request Feb 12, 2024

adopting string literals #2128

Merged

Addressing latest reviews

afb4435

mbasmanova reviewed Feb 13, 2024

View reviewed changes

Addressing latest reviews

6cf504a

lemire merged commit 2b53248 into simdjson:master Feb 15, 2024

lemire mentioned this pull request Feb 16, 2024

Add full support for JSONPath #2070

Open

Conversation

FranciscoThiesen commented Feb 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

FranciscoThiesen commented Feb 11, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

lemire commented Feb 12, 2024

Uh oh!

lemire commented Feb 12, 2024

Uh oh!

FranciscoThiesen commented Feb 12, 2024

Uh oh!

lemire commented Feb 13, 2024

Uh oh!

mbasmanova left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

FranciscoThiesen Feb 13, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

lemire commented Feb 13, 2024

Uh oh!

FranciscoThiesen commented Feb 13, 2024

Uh oh!

lemire commented Feb 14, 2024

Uh oh!

FranciscoThiesen commented Feb 14, 2024

Uh oh!

lemire commented Feb 15, 2024

Uh oh!

lemire commented Feb 15, 2024

Uh oh!

lemire commented Feb 15, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

FranciscoThiesen commented Feb 11, 2024 •

edited

Loading

FranciscoThiesen Feb 13, 2024 •

edited

Loading

lemire commented Feb 15, 2024 •

edited

Loading