Allow making operations on sifters and having them in a group

[odedniv]

Operators don't work on sifters, so you always have to wrap them like so: _(sift(:my_sifter)).
This is a patch I made that always returns the Sifter object wrapped withing Grouping.

Given the following model:

class User < ActiveRecord::Base
  sifter(:my_sifter) { id + 5 }
end

Here is some examples of the results before the patch:

User.where{sift(:my_sifter) > 5}.to_sql
 => "NoMethodError: undefined method `>' for #<Squeel::Nodes::Sifter:0x00000009bffbb8>"
User.select{sift(:my_sifter).as(column)}.to_sql
 => "NoMethodError: undefined method `as' for #<Squeel::Nodes::Sifter:0x00000009c50928>"
Account.joins(:user).where{user.sift(:my_sifter) > 2}.to_sql
 => "NoMethodError: undefined method `>' for #<Squeel::Nodes::KeyPath:0x00000009190218>"

Examples after the patch (and the expected result for the above examples):

User.where{sift(:my_sifter) > 5}.to_sql
 => "SELECT `users`.* FROM `users`  WHERE (`users`.`id` + 5) > 5"
User.select{sift(:my_sifter).as(column)}.to_sql
 => "SELECT (`users`.`id` + 5) AS column FROM `users` "
Account.joins(:user).where{user.sift(:my_sifter) > 2}.to_sql
 => "SELECT `accounts`.* FROM `accounts` INNER JOIN `users` ON `users`.`id` = `accounts`.`user_id` WHERE (`users`.`id` + 5) > 2"

Also, you can't have sifter within #group, so I added sifter visitation.

Before:

User.group{sift(:my_sifter)}.to_sql
 => "TypeError: Cannot visit Squeel::Nodes::Sifter"

After:

User.group{sift(:my_sifter)}.to_sql
 => "SELECT `users`.* FROM `users`  GROUP BY (`users`.`id` + 5)"

[the8472]

Alternatively you could just simply create a grouping inside the sifter, since it gets instance-evaled in the DSL. There could be cases where adding a grouping could be harmful, e.g. in SQL function evaluation.

[odedniv]

This doesn't work, still getting the error when doing the following:

class User < ActiveRecord::Base
  sifter(:my_sifter) { _(id + 5) }
end

If only it was that simple...

[the8472]

Oh right, it gets lazy-evaluated. I guess in that case the modules and operator aliases should be included in the sifter node? Like https://github.com/activerecord-hackery/squeel/blob/master/lib/squeel/nodes/grouping.rb#L6-22

My main concern is that always wrapping things in a grouping might break in some edge cases.

[odedniv]

I myself was more afraid of copying code from one place to another, felt safer to wrap with a grouping. I bet @ernie would have a much better idea on how to implement this

[ernie]

Probably reasonable to include the operators on the sifters. I am trying to think of a situation in which (apart from a stylistic opposition to unnecessary parens) a user would actively want to avoid parentheses around the result of a sifter. If I can't come up with any, then it would make sense to update the visitor to wrap the result in an Arel grouping node, without putting a grouping node into the Squeel AST.

[the8472]

Functions that take keyword operators as part of their syntax would explode with extra parens.

Consider COUNT(DISTINCT id) as foo vs. COUNT((DISTINCT id)) as foo. So if you use a sifter inside a function node, combined with operator nodes it would fail.

[ernie]

Bah. Good call, @the8472. So, I'd say the better option really would be to simply wrap the sifter in a grouping node inside the Squeel DSL itself, and do whatever comparisons you want to afterward, @odedniv. That wouldn't require any changes to Squeel and would make the intent more obvious in your own code (I think).

[odedniv]

@ernie, this means I have to wrap every use of that sifter with _(sift(...)), while grouping is not really what I wanted to achieve. I just want to be able to create a complicated equation in the model that I can later use anywhere. Example:

class User < ActiveRecord::Base
  sifter(:c) { a + b }
  def c
    a + b
  end
end

User.where{sift(:c) == 5}.count
User.first.c == 5

I don't want the outside world to know how c is calculated. Wrapping every use of c with a grouping is very tedious... (I would actually prefer it didn't know it was calculated at all and just use c == 5 withing the DSL)

[the8472]

How is writing _() in every query any more "tedious" than writing .where{} for every query? It might not be pretty, but from my experience writing complex queries rarely is pretty. I often end up with multiple lines of arr.reduce(self){|a,b| a.__send__(b)}, subselects, literals, aliasing and what-not.

[odedniv]

For the above example, I don't count where{c > 5} as a complex query, and the "outside" world doesn't know how c is implemented and therefore shouldn't need to know that it needs _() around it. If c wanted it around it should have added it itself (within the sifter).

I don't see a reason not to add the operator methods to Sifter, just like you have it for anything else (Literal etc).

[the8472]

and the "outside" world doesn't know how c is implemented

well, squeel is an AST-abstraction over SQL/Arel/active-record relations. It's still relatively close to the database. I don't think you should try to hide logic/pretend that it behaves like a regular column when it isn't.

Of course you could monkey-patch some short-hand into the DSL that uses some operator character that's not in use or something like that. E.g. def !(sifter) ... end could map to _(sift(sifter))

Another idea would be to make squeel aware of aliased columns in the select clause, that way you could write:

User.select{_(complex_calculation).as(c)}.where{c == 3} and have a scope on the class level simply include the select clause. right now you have to write where{c == 3}, that could be improved i guess.