-
Notifications
You must be signed in to change notification settings - Fork 126
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
Introduce cond/1
support in queries
#706
Introduce cond/1
support in queries
#706
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.
For me it looks amazing! 😍
But let's wait to hear from José :)
lib/explorer/query.ex
Outdated
defmacro select(do: clauses) do | ||
conditions = | ||
clauses | ||
|> Enum.map(fn {:->, _, [[condition], on_true]} -> [condition, on_true] end) |
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 can do the reverse here, so we don't do it at runtime, and you can use tuples (semantically better):
|> Enum.map(fn {:->, _, [[condition], on_true]} -> [condition, on_true] end) | |
|> Enum.map(fn {:->, _, [[condition], on_true]} -> {condition, on_true} end) | |
|> Enum.reverse() |
lib/explorer/query.ex
Outdated
|
||
unquote(conditions) | ||
|> Enum.reverse() | ||
|> Enum.reduce(nil, fn [condition, truthy], acc -> |
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.
Something you could optimize is to check if condition == true
. If that's the case, you know it will always be true and you can skip the whole select. Something like this:
{true, truthy}, _acc -> Explorer.Shared.lazy_series!(truthy)
{condition, truthy}, acc -> ...
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 is great @sasikumar87!
The only question I have is if we want to call it cond do
instead of select
. I would prefer to keep it closer to Elixir, so cond
is my vote.
In that case, instead of defining a defmacro select(...) do
, you should match on defp traverse({:cond, _meta, clauses}) do
. Everything else should remain the same. The documentation would go in the module docs.
PS: Now that I think about it, we could also support case ... do
but an initial version would require all patterns to be literals (i.e. integers or strings). But that's for another PR. :)
@josevalim I read the following in #515,
Do you think this argument still has merit to use |
I see @josevalim from the past disagrees with @josevalim from the future. Let me think about it, haha. |
@sasikumar87 let's go with |
cond/1
support in queries
💚 💙 💜 💛 ❤️ |
Referring to #515, this PR introduces multi-clause select as a macro, which in-turn expand to nested
Series.select/3
queries. This makes nested selects easy to write in a flat structure and improves readability.Below are the list of assumptions made in this PR
nil
Creating the PR for initial feedback, will add the docs if the approach is good.