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(remap): Add push and append functions #5750
Merged
Merged
Changes from all commits
Commits
Show all changes
33 commits
Select commit
Hold shift + click to select a range
7c4ec31
Add append function
e69b2aa
Add CUE docs sources for append function
b7d4aa0
Add TOML unit test for append function
40b22f1
Fix formatting
fb0c35e
Fix CUE formatting
62957dc
Resolve md5/sha1 import ambiguity
7dc4407
Clarify append semantics in docs
0c80b9d
Merge branch 'master' into remap-append-function
f766a9f
Update function description re: return value
217f3de
Merge branch 'master' into remap-append-function
1b69a0f
Rename function to push
986a828
Add append function
ca017c3
Add docs for append function
9f7b832
Update behavior tests
336ff86
Fix formatting issue
1c7a4ed
Update docs
97d0758
Use lit and array macros
0dd8284
Make behavior test naming and placement consistent with other tests
14b28c9
Add inner type definitions plus tests
fe987c0
Add inner type definitions for push function
6c34790
Fix merge conflict in behavior tests
9da4ed5
Reformat
8095cf7
Add inner type defs back into push and append expressions
531934a
Fix test naming issue created by merge
f8ae749
Fix merge conflicts
e9cbf97
Merge branch 'master' into remap-append-function
e06dc3f
Fix behavior test array equality statements
58e8dcf
Fix merge conflict
85c0c9c
Fix array declaration issue in behavior test
1211fcd
Fix behavior test outputs
9f6c3ad
Fix merge conflict
95f4919
Merge branch 'master' into remap-append-function
7282233
Fix merge conflict
File filter
Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,40 @@ | ||
package metadata | ||
|
||
remap: functions: append: { | ||
arguments: [ | ||
{ | ||
name: "value" | ||
description: "The array" | ||
required: true | ||
type: ["array"] | ||
}, | ||
{ | ||
name: "items" | ||
description: "The items to append" | ||
required: true | ||
type: ["array"] | ||
}, | ||
] | ||
return: ["array"] | ||
category: "Array" | ||
description: """ | ||
Adds each item from an array to the end of another array. The expression | ||
`append([1, 2, 3], [4, 5, 6])`, for example, would produce the array `[1, 2, 3, 4, 5, 6]`. | ||
The items in both arrays can be of any VRL type. | ||
""" | ||
examples: [ | ||
{ | ||
title: "Mixed array" | ||
input: { | ||
kitchen_sink: [72.5, false, [1, 2, 3]] | ||
items: ["booper", "bopper"] | ||
} | ||
source: """ | ||
.kitchen_sink = append(.kitchen_sink, .items) | ||
""" | ||
output: { | ||
kitchen_sink: [72.5, false, [1, 2, 3], "booper", "bopper"] | ||
} | ||
}, | ||
] | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,58 @@ | ||
package metadata | ||
|
||
remap: functions: push: { | ||
arguments: [ | ||
{ | ||
name: "value" | ||
description: "The array" | ||
required: true | ||
type: ["array"] | ||
}, | ||
{ | ||
name: "item" | ||
description: "The item to push" | ||
required: true | ||
type: ["any"] | ||
}, | ||
] | ||
return: ["array"] | ||
category: "Array" | ||
description: """ | ||
Adds the specified item to the end of an array and returns the resulting array. The item | ||
can be of any VRL type and is added even if an item with the same value is already present | ||
in the array. | ||
|
||
The `push` function does *not* change the array in place. In this example, the `push` | ||
function would return an array with `apple`, `orange`, and `banana`, but the value of | ||
`fruits` would be unchanged: | ||
|
||
```js | ||
.fruits = ["apple", "orange"] | ||
push(.fruits, "banana") | ||
.fruits | ||
["apple", "orange"] | ||
``` | ||
|
||
In order to change the value of `fruits`, you would need to store the resulting array in | ||
the field: | ||
|
||
```js | ||
.fruits = push(.fruits, "banana") | ||
``` | ||
""" | ||
examples: [ | ||
{ | ||
title: "Mixed array" | ||
input: { | ||
kitchen_sink: [72.5, false, [1, 2, 3]] | ||
item: "booper" | ||
} | ||
source: """ | ||
.kitchen_sink = push(.kitchen_sink, .item) | ||
""" | ||
output: { | ||
kitchen_sink: [72.5, false, [1, 2, 3], "booper"] | ||
} | ||
}, | ||
] | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,143 @@ | ||
use remap::prelude::*; | ||
|
||
#[derive(Clone, Copy, Debug)] | ||
pub struct Append; | ||
|
||
impl Function for Append { | ||
fn identifier(&self) -> &'static str { | ||
"append" | ||
} | ||
|
||
fn parameters(&self) -> &'static [Parameter] { | ||
&[ | ||
Parameter { | ||
keyword: "value", | ||
accepts: |v| matches!(v, Value::Array(_)), | ||
required: true, | ||
}, | ||
Parameter { | ||
keyword: "items", | ||
accepts: |v| matches!(v, Value::Array(_)), | ||
required: true, | ||
}, | ||
] | ||
} | ||
|
||
fn compile(&self, mut arguments: ArgumentList) -> Result<Box<dyn Expression>> { | ||
let value = arguments.required("value")?.boxed(); | ||
let items = arguments.required("items")?.boxed(); | ||
|
||
Ok(Box::new(AppendFn { value, items })) | ||
} | ||
} | ||
|
||
#[derive(Debug, Clone)] | ||
struct AppendFn { | ||
value: Box<dyn Expression>, | ||
items: Box<dyn Expression>, | ||
} | ||
|
||
impl Expression for AppendFn { | ||
fn execute(&self, state: &mut state::Program, object: &mut dyn Object) -> Result<Value> { | ||
let mut value = self.value.execute(state, object)?.try_array()?; | ||
let mut items = self.items.execute(state, object)?.try_array()?; | ||
|
||
value.append(&mut items); | ||
|
||
Ok(value.into()) | ||
} | ||
|
||
fn type_def(&self, state: &state::Compiler) -> TypeDef { | ||
use value::Kind; | ||
|
||
let items_type = self | ||
.items | ||
.type_def(state) | ||
.fallible_unless(Kind::Array) | ||
.with_inner_type(self.items.type_def(state).inner_type_def); | ||
|
||
self.value | ||
.type_def(state) | ||
.fallible_unless(Kind::Array) | ||
.merge(items_type) | ||
.with_constraint(Kind::Array) | ||
.with_inner_type( | ||
self.items | ||
.type_def(state) | ||
.merge(self.value.type_def(state)) | ||
.inner_type_def, | ||
) | ||
lucperkins marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
} | ||
|
||
#[cfg(test)] | ||
mod tests { | ||
use super::*; | ||
use value::Kind; | ||
|
||
test_type_def![ | ||
value_array_items_array_infallible { | ||
expr: |_| AppendFn { | ||
value: array!["foo", "bar", 142].boxed(), | ||
items: array!["baq", "baz", true].boxed(), | ||
}, | ||
def: TypeDef { | ||
fallible: false, | ||
kind: Kind::Array, | ||
inner_type_def: Some(TypeDef { | ||
kind: Kind::Boolean | Kind::Bytes | Kind::Integer, | ||
..Default::default() | ||
}.boxed()) | ||
}, | ||
} | ||
|
||
value_non_array_fallible { | ||
expr: |_| AppendFn { | ||
value: lit!(27).boxed(), | ||
items: array![1, 2, 3].boxed(), | ||
}, | ||
def: TypeDef { | ||
fallible: true, | ||
kind: Kind::Array, | ||
inner_type_def: Some(TypeDef { | ||
kind: Kind::Integer, | ||
..Default::default() | ||
}.boxed()) | ||
}, | ||
} | ||
|
||
items_non_array_fallible { | ||
expr: |_| AppendFn { | ||
value: array![1, 2, 3].boxed(), | ||
items: lit!(27).boxed(), | ||
}, | ||
def: TypeDef { | ||
fallible: true, | ||
kind: Kind::Array, | ||
inner_type_def: Some(TypeDef { | ||
kind: Kind::Integer, | ||
..Default::default() | ||
}.boxed()) | ||
}, | ||
} | ||
]; | ||
|
||
test_function![ | ||
append => Append; | ||
|
||
both_arrays_empty { | ||
args: func_args![value: array![], items: array![]], | ||
want: Ok(value!([])), | ||
} | ||
|
||
one_array_empty { | ||
args: func_args![value: array![], items: array![1, 2, 3]], | ||
want: Ok(value!([1, 2, 3])), | ||
} | ||
|
||
neither_array_empty { | ||
args: func_args![value: array![1, 2, 3], items: array![4, 5, 6]], | ||
want: Ok(value!([1, 2, 3, 4, 5, 6])), | ||
} | ||
]; | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
This might warrant a larger discussion, but I think it would be clearer that
append
modifies the array in-place if it didn't return anything. The fact that it returns the modified array could lead users to believe it is returning a new array that is a copy of the old array plus the new element.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 do know that Jean et al would eventually like to make
del
, for example, return either the deleted value ornull
if the field doesn't exist. Returning the new array would enable you to do things likeunique(append(.fruits, "mango"))
(mentioned in my other comment) and feels more consistent with some programming languages (Go et al) but not others (Rust et al). I don't have strong feelings there, especially on this first iteration. Happy to defer. For now I'll leave the current behavior in place (return the new array) and update the docs.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 Go is a bit special because it actually returns a new "struct" representing the array. Usually this is pointing to the same underlying memory, but it'll actually be different if it had to extend the array; this is a common gotcha for people coming to the language. Consider that Go's in-place sorting routines do not return the sorted array. I think this makes it clearer that it is happening in-place.
That being said, maybe I'm in the minority here. I know Ruby returns the same array to make it easier to chain though they also have
sort
create a copy whilesort!
sorts in-place.I guess the most important thing is consistency in VRL. We can choose to always return the modified value, even if it happens in-place, and hopefully that consistency will make it easy to reason about.
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 would update the docs to make it clear that the array itself is modified though, the current wording:
Might lead people to imagine it returns a new array. Specifically the word
new
there. Maybe: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.
Ah, I see what you're saying now. I'll update.
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.
@jszwedko Every VRL function except
del
is a "pure" function that generates a value but doesn't modify the object. In this case, the value of thelist
and theitem
are merely extracted from the function arguments and used to produce a new array. Compare with thedel
function logic:The
Object
type has methods that give you hooks into the event itself. If you don't invoke those then the underlying event is unchanged. So I'm not sure that tests verifying that the event is unchanged are any more necessary here than in all of the other functions (exceptdel
, of course).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.
True, I'm probably missing something in the plumbing of
remap-lang
. I would have assumed that complex types like theVec
andBTreeMap
would have been accessed by reference to avoid unnecessary clones (though we do want the clone here).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're not missing anything 😄. We opted to accept the clone, as a trade-off to keep the initial implementation simpler.
Since we're using the
bytes
crate, the cloning is actually relatively cheap, since it uses reference-counting internally, and other most other value types implement copy, and are thus cheap to clone (cloning a map is probably most expensive, since we're storing the keys as strings on the heap).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.
Aha 🤔 thanks for the explanation!
@lucperkins I think it'd still be nice to add a test that asserts the original array was unchanged in case we change the behavior of always cloning values and need to add an explicit clone here.
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.
merge
is another one.merge(., .foo)
will merge.foo
into the root.