Skip to content

Conversation

@mgreenwood1001
Copy link
Contributor

@mgreenwood1001 mgreenwood1001 commented Jul 27, 2017

Issue #191 is an unintentional side-effect of the collection processing for parameters passed to functions. This change supports result set functions via patterns such as:

$.max($..allTheNumberThings)

$.max($.foo, $.bar) is already supported where foo and bar are numbers - consequently

$.max($..allTheNumberThings) shall also work also

$.avg($..allTheNumberThings, $.someCollection.max(), $..someOtherNumberThings)
will work too now with this change.

… as $.max($..allTheNumberThings)

$.max($.foo, $.bar) is already supported - consiquently $.max($..allTheNumberThings) should also work.
* Shows aggregation across fields rather than within a single entity.
*
*/
public class Issue191 extends com.jayway.jsonpath.internal.function.BaseFunctionTest {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IntelliJ was having a bad day I guess, its in the same package, ugh - I'll fix this...

@mgreenwood1001
Copy link
Contributor Author

the original question raised by #191 wanted to use the notation:

$..pathWithResultSetOutput.function()

so I've added support for that as well.

Normally, this won't work unless you make the graph stateful or introduce a pre-aggregation function (or emulate this in the evaluation steps) -- aggregate the results as a collection and passes it along to the function, i.e. $..path.group().min() -- I'm not fond of this approach.

Instead, The CompilePath function now recognizes the condition where you're scanning the elements and introducing a collection as a the result set and attempting to pass that along as input into the function, since that won't work (functions are stateless and require parameters that can be lazy evaluated or passed) the CompilePath now inverts the graph making the function the root token and the parameter to the function the original path which makes the whole thing sane such that we can lazy evaluate the path get the result then evaluate the function.

So now:

$..path.sum() and

$.sum($..path) are supported, it won't matter which you specify as both result in the same graph internally.

Since this CompilePath trick is in the constructor, its recursive, so

$.sum($..path.sum(), $..path.avg())

do what you'd hope it would do...

@mgreenwood1001 mgreenwood1001 changed the title Issue #191 is a bug - result set aggregation for text/numeric Issue #191 fixes both $..allThings.max() and $.max($..allTheThings) Aug 5, 2017
@mgreenwood1001 mgreenwood1001 changed the title Issue #191 fixes both $..allThings.max() and $.max($..allTheThings) Issue #191 fixes both $..allTheThings.max() and $.max($..allTheThings) Aug 5, 2017
@kallestenflo kallestenflo merged commit b4c26b2 into json-path:master Aug 16, 2017
cmunilla pushed a commit to cmunilla/JPath that referenced this pull request Oct 30, 2017
Issue json-path#191 fixes both $..allTheThings.max() and $.max($..allTheThings)
@rhavermans
Copy link

@kallestenflo Could you make a new release? We would like to have this functionality.

rhavermans added a commit to bolcom/JsonPath that referenced this pull request Jun 18, 2018
Change-Id: I605fe8fd74bb2a7d0a01edfe42b31e62480e4b97
@soberich
Copy link

soberich commented Dec 9, 2018

@mgreenwood1001 @kallestenflo Hey, guys, I just stumbled upon fact that this is really weird.. Why is it that many useful commits are on master but the artifact is still not released (for more than a year)? Anyone?

@mgreenwood1001
Copy link
Contributor Author

@kallestenflo should you need anything from me for a release please let me know.

@soberich
Copy link

soberich commented Dec 11, 2018

I've made pr with div() and mult() functions #511.
It's a master branch fork. So, would be nice to merge all that.

@sarod
Copy link

sarod commented Feb 12, 2020

@kallestenflo @mgreenwood1001 this feature would be really useful. It was merged in a year and a half ago. Is there a release planned soon?

@aabrams
Copy link

aabrams commented Feb 24, 2020

+1 Really useful to have this released, thanks!

@ericnesschrw
Copy link

This would be really useful. Any plans to release it?

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.

7 participants