-
Notifications
You must be signed in to change notification settings - Fork 59
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
feat(stdlib): add unflatten vrl function #993
feat(stdlib): add unflatten vrl function #993
Conversation
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.
You are on a roll :)
This looks like it is heading in the right direction to me. I appreciate the number of tests that were added.
If you are feeling inspired, this could be a good use-case for property testing where random inputs are fed into flatten
through unflatten
and then asserted to be the same. This would be the first time we have these sorts of tests in the stdlib, though, so might not be straight-forward; definitely not a requirement for merging this. There are some prop tests declared for the compiler here: https://github.com/vectordotdev/vrl/tree/main/lib/proptests
I have to admit that I kind of copied the ones from the I tried to implement proptests, but spent a few hours and couldn't get it working... I see that https://github.com/vectordotdev/vrl/tree/main/lib/proptests is not included in the workspace (and when I do so, compilation of the proptests crate fails). Are those tests currently being ran in the checks? I couldn't integrate property testing of vrl functions whithin the current testing framework I added a few vrl tests under |
I added a new |
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.
Nice, thanks @jorgehermo9 ! I left a few more comments, but this is looking good to me. No worries about the prop tests. I appreciate you giving those a shot, but definitely not required for this.
src/stdlib/unflatten.rs
Outdated
if values.len() == 1 { | ||
match values.pop().unwrap() { |
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.
if values.len() == 1 { | |
match values.pop().unwrap() { | |
if values.len() == 1 { | |
match values.pop().expect("exactly one element") { |
We typically expect
over unwrap
to provide more context about why the panic should never be hit.
I think one potential way to refactor this that might avoid this unwrap/expect and avoid the early returns is something like:
match values {
[value] => { ... what you currently have for handling if there is only one value ... }
values => { ... handle multiple values ... }
}
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 tried to do that, but the problem is values
is a Vec
and therefore, we should convert it to a slice in order to pattern match against it, using values[..]
, but then, at the first branch we would have the first element borrowed and we need it owned. The same with the second branch, we will have a borrowed slice of values, but not the owned values and we need owned values for the do_unflatten_entries
function.
Does this makes sense?
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.
Mmm, yeah, that makes sense. I wish there was a better way to model this that doesn't require the expect
but I'm not seeing it.
Hi @jszwedko I addressed the comments. I additionally added a more efficient way of constructing objects with "unique paths", for example: {
"a.b.c.d.e.f": 1,
"g.h.i.j.k": 2
} Since the two entries don't share any prefix, they can be constructed without the recursive calls to This may be a premature optimization, but seems reasonable to me to not construct a I also added a few more tests. Documentation in vectordotdev/vector#21142 |
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.
Great! Thanks for this contribution @jorgehermo9 !
Head branch was pushed to by a user without write access
Sorry, clippy action failed in CI. Fixed that. Forgot to ran Could you enable auto-merge again? @jszwedko |
Closes #81
Had no time to polish this PR, but I think it is good enough to be reviewed.I will do the things I've remaining asap, but pushing this if happens that someone can manage to review it in the meanwhile (no hurry)
Things I've left but couldn't do yet because had no time
unflatten
is the inverse offlatten
in the tests underlib/tests/tests/functions
If I miss anything, tell me please
I will address this as soon as I can, but I will be off these days.