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

Pr/more aggressive do map simplification #1429

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

pinetops
Copy link
Contributor

@pinetops pinetops commented Sep 2, 2024

Test of more aggressive simplification of do_map:

This passes all tests, but I expected that we could also skip calling func on the root node, as do_map would recurse/process leaves appropriately. But that's not the case.

Basically, all values passed to do_map in the test suite look like something like this, and func turns the input expression into an empty array and doesn't walk the tree. (and hundreds of similar examples)
expression: author.id == "7a18baa9-40fc-4c9e-a903-d700a8031309"
func.(expression): []
expression: exists(author.friends, id == "7a18baa9-40fc-4c9e-a903-d700a8031309")
func.(expression): []

Is that right?

@zachdaniel
Copy link
Contributor

🤔 pretty strange. Ash.Filter.map is a general purpose function, so it has to retain certain semantics. Basically you can traverse a filter and replace any value with any other new value. We have to check if the new value has been touched, because we don't have guarantees that the value inserted has been mapped over accordingly. We could attempt to break those semantics, but I worry about the downstream implications.

What I think might be a better choice is to make an entirely new module, and create new implementations for these, and deprecate Ash.Filter.map. The reason that would be nice is that there are other new filter analyzer/manipulator functions, specifically Ash.Filter.split/2, i.e

Ash.Filter.split(a and (b or c), fn x -> is_a(x) or is_b(x) end)

{a, b or c}

Ash.Filter.split gives you only a complete statement matching the predicate, never splitting an or on the left, and the rest on the right.

What might also be better is to move these over to be Expr focused, since these things aren't only used for filters any more. So something like Ash.Expr.Tools.map and Ash.Expr.Tools.split, etc.

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.

2 participants