-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
enhancement(vrl): add filter_array
#7908
Conversation
Signed-off-by: prognant <pierre.rognant@datadoghq.com>
Signed-off-by: prognant <pierre.rognant@datadoghq.com>
I haven't reviewed the PR itself yet, but I did want to note that this gets close to a "wait until we have iteration" use-case (#6031). That is to say, with iteration, presumably, filtering would be a matter of iterating and then filtering individual fields in an array or object, which would make a function like this obsolete. I'm not against merging this, but I do think we need some form of iteration sooner or later, at which point we'll likely deprecate functions that like this one. |
Very much agreed that this seems like a great case for iteration. I'm also ok with merging this if it is blocking other work, but it will (hopefully!) be deprecated shortly. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, @prognant!
Agree with @JeanMertz and @jszwedko regarding iteration.
Does this PR have a matching issue? If so, would be great to add it in the PR description as "Closes #XXX".
&[ | ||
Parameter { | ||
keyword: "value", | ||
kind: kind::ARRAY, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm wondering if we can restrict the input type further to only accept arrays of Value::Bytes
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I used the the same input kind as match_array
, I don't think we can enforce that kind of restriction here, I guess we could do that in resolve
but after having a quick look to existing function I did not find any previous similar implementation.
&[ | ||
Example { | ||
title: "some item match", | ||
source: r#"filter_array(["foobar", "bazqux"], r'foo')"#, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure if I would expect this to match if the regex doesn't match exhaustively. Maybe we can add an option for that behavior?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
r'foo'
matches foobar, so I'd expect the current behaviour.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is fine. We don't provide an exhaustive option elsewhere, and you can always use ^foo$
if need be.
Not really, that work is related to easily enable datadog tags manipulation in VRL, e.g.:
That being said I agree that adding enumeration/looping/mapping would make this PR essentially useless. |
Co-authored-by: Pablo Sichert <mail@pablosichert.com> Signed-off-by: prognant <pierre.rognant@datadoghq.com>
f8e8606
to
adceb51
Compare
filter_array
filter_array
@prognant what's the status here? Should we close since it seems to be stale? |
This was a provision in case this was needed, as VRL iteration is planned, this is irrelevant. |
Allow to remove elements from an array based on a regexp.
Non-byte type are removed.
Note: as this PR was mostly anticipating a usecase (datadog tags manipulation) I suggest we don't merge it unless there is a real need for the usecase and enumeration/iteration is not available in VRL.