Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support to return multiple indexes when multiple matches are found #222

Merged
merged 5 commits into from Sep 1, 2021

Conversation

sspaink
Copy link
Contributor

@sspaink sspaink commented Jun 30, 2021

This pull request adds a new field to the Result structure that will contain multiple indexes when multiple matches are found. Currently only a single index is provided that is set to 0 when you provide a query to gather multiple matches (a query to access multiple child paths "friends.#.first" or to find all matches #(...)#). I called the new field HashtagIndexes because I believe this should only apply when a query uses a hashtag (but definitely open to any other name suggestions).

I am working on a project that would benefit from knowing all the indexes, I added a unit test called TestHashtagIndexesMatchesRaw that is very similar to what I would like to do with this feature. Basically run two separate queries and walk over the results of one and use the indexes to find matches. Thank you for looking over this pull request! Hopefully this is the right approach to achieve this.

@tidwall
Copy link
Owner

tidwall commented Jul 7, 2021

On the surface I like it and I can see various uses for this feature.

I'm not sure about the name either, it's kinda long and I don't think the term Hashtag is used anywhere in the docs. But that's a minor thing. Perhaps just HIndexes or simply Indexes would suffice.

One issue might be how it works with modifiers (or mulitpaths).

For example, with this document:

{
	"objectArray":[
		{"first": "Dale", "age": 44},
		{"first": "Roger", "age": 68},
	]
}

The path objectArray.#.first returns:

gjson.Result{
  Type: 5, 
  Raw: "[\"Dale\",\"Roger\"]", 
  Str: "",
  Num: 0, 
  Index: 0, 
  HashtagIndexes: []int{ 33, 66 },
}

While the path objectArray.@reverse.#.first returns:

gjson.Result{
  Type: 5, 
  Raw: "[\"Roger\",\"Dale\"]", 
  Str: "",
  Num: 0, 
  Index: 0, 
  HashtagIndexes: []int{ 11, 41 },
}

Because the modifier changed the order of the array, the 11 and 41 no longer applies to the original document.

We would either need to remap the indexes to the original document, or maybe we just return nil for the HashtagIndexes array.

Set to nil when modifiers used
@sspaink
Copy link
Contributor Author

sspaink commented Jul 9, 2021

Thank you for the feedback! I like the idea of just using the name Indexes, seems nicer. Good catch when using modifiers as well, seems the easiest solution would be just to set Indexes to nil. I am not sure how to remap the indexes without passing a unmodified json string around.

I've updated the pull request to use Indexes and set it to nil when using modifiers, added the @reverse example to the unit test as well.

@sspaink
Copy link
Contributor Author

sspaink commented Sep 1, 2021

@tidwall Is there anything else you would like me to do to help get this merged? Thanks!

@tidwall tidwall merged commit 52919fa into tidwall:master Sep 1, 2021
@tidwall
Copy link
Owner

tidwall commented Sep 1, 2021

LGTM. I just pushed a new version that includes your PR. :)

@sspaink
Copy link
Contributor Author

sspaink commented Sep 1, 2021

Awesome! Thanks :D

tidwall added a commit to tidwall/sjson that referenced this pull request Sep 4, 2021
This commit allows for updating values for more "complex" paths like:

	friends.#(last="Murphy")#.last

This is allowed because GJSON now tracks the origin positions of all
results (tidwall/gjson#222).

This new ability is limited to updating values only. Setting new
values that previously did not exist, or deleting values will
return an error.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants