Putting together scopes with OR #142

Closed
dgm opened this Issue Jun 22, 2012 · 15 comments

Comments

Projects
None yet
5 participants

dgm commented Jun 22, 2012

I see this question a lot on stack overflow, but never have seen a useful answer. I like to break down complex queries into more manageable bits and then compose them, which works great for chaining together 'AND' conditions. But I'm at a loss how to handle it when it needs to be an OR condition.

Using squeel got me part of the way there, at least OR became possible within one query scope, ie.

where{(last_name == 'foo') | (first_name == 'bar)}

but when those two parts become long queries, I'd like to break them into parts:

scope :by_last_name, lambda {|n| where{last_name==n}}
scope :by_first_name, lambda {|n| where{first_name==n}}

I can chain them into a combined query with

.by_last_name('foo').merge(by_first_name('bar'))

But that uses AND.

I have also discovered that a sifter approach works:

sifter :by_last_name do |n|
  last_name==n
end

sifter :by_first_name do |n|
  first_name==n
end

.where{sift(:by_last_name, "foo") | sift(:by_first_name, "bar")}

So close, but all that sifting syntax is getting in the way, and it appears that sifters and scopes cannot be directly mixed, so I'll have to duplicate some scope as sifters in order to use them this way. Is there a way to convert a sifter to a scope or vice versa? Am I missing another obvious syntax?

the8472 commented Jun 22, 2012

The problem with scopes is that they can contain joins. And combining (inner) joins generally narrows the set of records, which is OK if you're combining ANDs which also narrows the set.

If you want to combine ORs you're widening the set of returned records, that only has a chance to work with outer joins and might not do exactly what you expect. And if you start automagically converting inner to outer joins things get even more problematic.

I've done something like that with AREL, but that was under controlled conditions. A general "combine any scope you throw at it" feature would start doing things you don't expect it to very very quickly.

dgm commented Jun 22, 2012

Yes, and writing the sql by hand can get the same unexpected results, that doesn't meant it wouldn't be handy for squeel to have a way to do it.

dgm commented Jun 22, 2012

Here's the full excruciating detail. :) I am trying to write an autocomplete query, that has to look in several fields, and do both a soundex match and a %foo% match. (soundex only matches on a complete name match. type in 'dav' wouldn't match 'david')

I think I see where I should factor out the regex and call smaller functions. Never mind that. The point is, by breaking out into smaller sifters, the logic is much easier to follow than making one big monster query. The only problem is, I can't reuse any existing scopes and had to duplicate several scopes in sifter syntax, which isn't very DRY. (I added s_ to the name to avoid clashes with existing scopes)

  sifter :s_sounds_like_term_in_any_columns do |term, *search_columns|
      search_columns.map {|col| __send__(col).op('sounds like', term)}.inject(&:|)
  end

  sifter :s_has_name_like do |search|
    sift(:s_sounds_like_term_in_any_columns, search, :last_name, :first_name, :middle_name, :maiden_name, :common_name)
  end

  sifter :s_contains_term_in_any_columns do |term, *search_columns|
      search_columns.map {|col| __send__(col).op('like', "%#{term}%")}.inject(&:|)
  end

  sifter :s_has_name_containing do |search|
    sift(:s_contains_term_in_any_columns, search, :last_name, :first_name, :middle_name, :maiden_name, :common_name)
  end

  sifter :s_soundex_search_by_name, do |q|
    if q
      case q
      when  /^(.+),\s*(.*?)$/
        sift(:s_sounds_like_term_in_any_columns, $1, :last_name, :maiden_name) & 
        sift(:s_sounds_like_term_in_any_columns, $2, :first_name, :common_name, :middle_name)
      when /^(.+)\s+(.*?)$/
        sift(:s_sounds_like_term_in_any_columns, $2, :last_name, :maiden_name) &
        sift(:s_sounds_like_term_in_any_columns, $1, :first_name, :common_name, :middle_name)
      else
        sift(:s_has_name_like, q)
      end
    else
      id > 0
    end
  end

  sifter :s_search_by_name, do |q|
    if q
      case q
      when  /^(.+),\s*(.*?)$/
        sift(:s_contains_term_in_any_columns, $1, :last_name, :maiden_name) & 
        sift(:s_contains_term_in_any_columns, $2, :first_name, :common_name, :middle_name)
      when /^(.+)\s+(.*?)$/
        sift(:s_contains_term_in_any_columns, $2, :last_name, :maiden_name) &
        sift(:s_contains_term_in_any_columns, $1, :first_name, :common_name, :middle_name)
      else
        sift(:s_has_name_containing, q)
      end
    else
      id > 0
    end
  end

  def self.ultimate_autocomplete(search)
    self.where{sift(:s_search_by_name, search) | sift(:s_soundex_search_by_name, search)}
  end

the resulting SQL:

SELECT `people`.* FROM `people` 
WHERE ((((`people`.`last_name` like '%Dokes%' OR `people`.`maiden_name` like '%Dokes%') 
                   AND ((`people`.`first_name` like '%Joe%' OR `people`.`common_name` like '%Joe%') 
OR `people`.`middle_name` like '%Joe%')) 
OR ((`people`.`last_name` sounds like 'Dokes' OR `people`.`maiden_name` sounds like 'Dokes') 
      AND ((`people`.`first_name` sounds like 'Joe' OR `people`.`common_name` sounds like 'Joe') 
                 OR `people`.`middle_name` sounds like 'Joe'))))

Good luck trying to sort out the sql version. :D

This logic is only in one table, I have even more complicated queries and scopes. I just don't like having to duplicate them :(

the8472 commented Jun 22, 2012

Ah yes. Autogenerated query madness, I'm familiar with that. There's a workaround: Subselects and (less well supported by rails) Unions.

Try something like Person.where{id.in(Person.my_scope.select(id) | id.in(Person.my_other_scope.select(id))) and see if you can take the performance hit.

dgm commented Jun 22, 2012

Hmm. That went from 20ms to 60ms. I'll keep the general idea around though.

Owner

ernie commented Jun 26, 2012

So, I do have one takeaway from reading this discussion, which is that I'm not the only one who might find it useful to have sifters generate a matching scope.

How do you gents feel about something along the lines of:

sifter :some_sifter_name do |q|
  squeel{attr_name == q}
end

Generating a sifter named sifter_some_sifter_name and a scope some_sifter_name that does a:

scope :some_sifter_name do |q|
  where{sift(:sifter_some_sifter_name, q)}
end
Owner

ernie commented Jun 26, 2012

Obviously, this would be in Squeel 1.1, not 1.0.x.

dgm commented Jun 26, 2012

yes please! Then I could simply convert some of my existing scopes into sifters but continue to use them as scopes. :)

dgm commented Jun 26, 2012

One question: in the current version, I think I ran into problems when a sifter and scope shared the same name... Can that be worked around?

Owner

ernie commented Jun 26, 2012

No, it can't -- they're implemented in terms of methods, so just like two methods can't have the same name, scopes/sifters can't have the same name.

dgm commented Jun 26, 2012

ah, I misread, you had the same solution I had: name the sifter: sifter_foo and the scope just: foo

It would be neat if the method could detect which environment was called for and then delegate to scope or sifter as needed...

dgm commented Jun 26, 2012

Similarly, is there a way to shorten the sifter syntax from where{sift(:sifter_some_sifter_name, q)} to where{sifter_some_sifter_name, q} ?

Owner

ernie commented Jun 26, 2012

Not really. It'd impose a bunch of overhead on dsl calls for somewhat limited improvements. Especially if you consider the change I just suggested, which would add "sifter" to the method name to begin with (but wouldn't require you to type the "sifter" part when calling sift(:whatever)

ansel1 commented Nov 29, 2012

Just adding my voice to the chorus. Ran up against a use case like this:

User :has_many SecondaryEmails

User has an email field too, which is the primary email field. Both User and SecondaryEmail have a "verified" flag.

I wanted to find a verified User with a verified email address (either primary or secondary).

Ended up with something like:

# in SecondaryEmail
sifter(:verified_sifter) { verified == true }
scope :verified, where{sift(:verified_sifter)}
sifter(:verified_with_email_sifter) {|an_email| sift(:verified_sifter) & (email == an_email)}
scope :verified_with_email, lambda { |an_email| where{sift(:verified_with_email_sifter, an_email)}}

# in User
scope :verified, where(verified: true)
def self.with_verified_email an_email
  User.verified.joins{secondary_emails.outer}
    .where{ (email == an_email) | (secondary_emails.sift(:verified_with_email_sifter, an_email))}
end

This works. Would have been great (but perhaps too magical to ever work) to be like:

# in SecondaryEmail
seive(:verified) { verified == true }
seive(:verified_with_email) { | e | verified.seive{email == e}}

# in User
seive(:verified) {verified == true}
def self.with_verified_email an_email
  User.verified.joins{secondary_emails.outer}
    .where{ (email == an_email) | (user_accounts.verified.email == an_email)}
end

Something like that...seive is like a synonym for :where. The resulting object is a Relation. But because it only accepts the same arguments as :where, it is always easily convertable to a sifter when necessary. Because of this easy convertability, it gets two added features over a normal Relation: a) it acts like a squeel node when evaluated by DSL, and b) it response to :seive, which lets you chain them like you can :where clauses.

If you chained a :seive to :where or :joins or any of the other relations, the result would be a normal Relation, which would lose the special abilities of a seive.

@ernie ernie added a commit that referenced this issue Mar 24, 2013

@ernie ernie Sifters must be prefixed with sifter.
This is to eliminate confusion between sifters and scopes. They aren't
the same thing. Related to #142. Still on the fence about having the
sifter macro define a scope to match, but I'm leaning toward it being
too much unnecessary magic for now.
bbbce19

ernie closed this Mar 25, 2013

kawikak commented Apr 13, 2013

It would be really handy if the sifter defined a matching scope, or if there was at least the option to do so. Currently I often do the same thing ansel1 is doing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment