-
Notifications
You must be signed in to change notification settings - Fork 1.5k
Add array.flatten built-in function
#8232
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
base: main
Are you sure you want to change the base?
Conversation
Originally meant to be `array.concat_n`, but this name is better as the behavior of this function differs from `array.concat` — namely that `array.flatten` accepts any type of valued in the input array. Only arrays are however flattened, and the rest are appended directly to the flattened output. Note that this function only flattens at the topmost level of the inpu array — not recursively! Which a cursory look at a few other languages suggested was the common case. But if others feel we should flstten more, I'm happy to make an update. The C code for a Wasm implementstion here is cowboy coded, and I did not msnage to run the tests on my machine due to some `docker` <-> `container` differences. I mostly just imitated the existing code in the array category. I doubt it'll work on the first try, but only CI can judge me. Also: - Remove `opa fmt` step from the Rego CI step, as this is done by Regal anyway a little later in the list of tasks. - Replace some hsrd-coded `docker` nmes in the `Makefile` with `$(DOCKER)` - Added name of built-in function missing to the unsupportedBuiltinErr error, as it has happened a few times now that I've used `:=` in a query, and had no clue what built-in it referred to. Fixes open-policy-agent#8226 Signed-off-by: Anders Eknert <anders.eknert@apple.com>
✅ Deploy Preview for openpolicyagent ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
srenatus
left a comment
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.
LGTM! Thanks
|
|
||
| var ArrayFlatten = &Builtin{ | ||
| Name: "array.flatten", | ||
| Description: "Non-recursively unpacks array items in arr into the flattened array. Other types are appended as-is.", |
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.
Do you think we'll want a recursive variant, too? 🤔
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.
Possibly. So we're back to calling it array.concat_n then? 🙂 I do like not having to ceremonially wrap things in array literals to have them appended.
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.
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 fine with either. Let's flip a mobile phone. (cc @johanfylling)
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.
It came up screen-up📱
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.
Meaning? 😄

Originally meant to be
array.concat_n, but this name is better as the behavior of this function differs fromarray.concat— namely thatarray.flattenaccepts any type of valued in the input array. Only arrays are however flattened, and the rest are appended directly to the flattened output.Note that this function only flattens at the topmost level of the inpu array — not recursively! Which a cursory look at a few other languages suggested was the common case. But if others feel we should flatten more, I'm happy to make an update.
The C code for a Wasm implementstion here is cowboy coded, and I did not manage to run the tests on my machine due to some
docker<->containerdifferences. I mostly just imitated the existing code in the array category. I doubt it'll work on the first try, but only CI can judge me.Also:
opa fmtstep from the Rego CI step, as this is done by Regal anyway a little later in the list of tasks.dockernmes in theMakefilewith$(DOCKER):=in a query, and had no clue what built-in it referred to.Fixes #8226