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 filter operation #1588

Merged
merged 7 commits into from Mar 8, 2023
Merged

Add filter operation #1588

merged 7 commits into from Mar 8, 2023

Conversation

rbren
Copy link
Contributor

@rbren rbren commented Mar 6, 2023

Fixes #1586

Comment on lines 26 to 36
childCtx := context.SingleReadonlyChildContext(result)
include, err := d.GetMatchingNodes(childCtx, expressionNode.RHS)
if err != nil {
return Context{}, err
}
var includeResult bool
var errDecoding error
includeEl := include.MatchingNodes.Front()
if includeEl.Next() != nil {
return Context{}, errors.New("Only expected one child")
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was the best way I could find to turn each value into a truthy value to determine if it should be included. LMK if there's a better way

Copy link
Owner

Choose a reason for hiding this comment

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

hmm, yeah there should be a cleaner way. What this function is basically doing is a map(select(X)).

You could create a new select expression node and give the expression.RHS to it.

something like:

    selectNodesOp := &ExpressionNode{
      Operation: selectOp,
      RHS:       expression.RHS,
      }
      
      includedNodes, err := d.GetMatchingNodes(splatted, selectNodesOp.RHS)

Pretty sure this could be extended to create a map(select) expression and just delegate the logic to those functions entirely...

@rbren
Copy link
Contributor Author

rbren commented Mar 6, 2023

One thing I'm not super happy with is arrays-of-arrays. But it works the same for both map and filter.

❯ cat arr.yaml
- - 1
  - 2
  - 3
- - 4
  - 5
  - 6

❯ go run yq.go e '.[] | filter(. > 1)' arr.yaml
- 2
- 3
- 4
- 5
- 6

❯ go run yq.go e '.[] | map(. + 1)' arr.yaml
- 2
- 3
- 4
- 5
- 6
- 7

I'd rather see the latter output

- - 2
  - 3
  - 4
- - 5
  - 6
  - 7

@mikefarah
Copy link
Owner

Yep - the first expression splats the array of arrays, which becomes the new context- and then it maps (or filters) that context.

This is like the difference between: .a | .b = 3 (which will set the root context to .a before updating .b, and doing .a.b = 3 which will deeply update b without changing the root context.

To get what you want - you'd need to run the expression without changing the context. You can use 'with' to that:

yq 'with(.[]; map(. += 1))'

Hope that makes sense!

expression: `filter(. < 3)`,
expected: []string{
"D0, P[], (!!seq)::- 1\n",
"D0, P[], (!!seq)::- 2\n",
Copy link
Owner

Choose a reason for hiding this comment

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

this isn't actually the right result - this means the filter function is return two arrays - each with a single element. It should return a single array with two elements

"D0, P[], (!!seq)::- 1\n- 2\n",

I think if you refactor to use map and select under the hood - it would do the right thing.

@rbren
Copy link
Contributor Author

rbren commented Mar 8, 2023

Awesome, I didn't know about with! Interesting that it needs += instead of just +

I also got it to do what I want with this expression:

go run yq.go e '.[] |= map(. + 1) | .[] |= filter(. < 4)' arr.yaml

@mikefarah mikefarah merged commit 9539877 into mikefarah:master Mar 8, 2023
@fearphage
Copy link

Isn't filter(...) just shorthand for map(select(...))?

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.

Inconsistency between map and select; proposal for a new filter function
3 participants