-
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
Improve boolean support in query #515
Comments
@sasikumar87 is it correct to say your PR does the third item in the list above? I also think the first item is already done (but not the second). |
@josevalim yes, third item in the list above. |
@sasikumar87 do you want to send a PR for the second item? The idea is to introduce functions for def left and right when Kernel.and(is_boolean(left), is_boolean(right)), do: Kernel.and(left, right)
def left and right, do: Explorer.Series.and(boolean!(left), boolean!(right))
defp boolean!(%Series{dtype: :boolean} = series) do
series
end
defp boolean!(other) do
raise ArgumentError, "boolean operators require either a boolean (true/false) or a boolean series, got: #{inspect(other)}"
end It is untested but that's the general idea. :) |
@josevalim will do |
@josevalim to confirm for |
Ah yes, I forgot a clause in boolean! That accepts a Boolean and converts it a series! |
The goal is to support this:
As well as multi-clause:
We chose
select do
instead ofcond
because, opposite tocond
which only evaluates the clauses that matches, queries always evaluates all clauses in order to build the expression.In order to support this, we will need:
select
(the first argument will always be a list). In this case, it should be enough to callfrom_list([...])
on arguments that are not listsand/2
,or/2
, andnot/1
select/1
macro defined aboveThe reason why we decided to handle this at the query level is because
Series.and(series, false)
does not really have a purpose and we should keepand/2
and inExplorer.Series
remain strict about expecting series. We may want to lift this restriction in the future but, at a first glance, it sounds reasonable.The text was updated successfully, but these errors were encountered: