Skip to content

Added #or to ActiveRecord::Relation#16052

Closed
matthewd wants to merge 1 commit into
rails:masterfrom
matthewd:relation-or
Closed

Added #or to ActiveRecord::Relation#16052
matthewd wants to merge 1 commit into
rails:masterfrom
matthewd:relation-or

Conversation

@matthewd

@matthewd matthewd commented Jul 4, 2014

Copy link
Copy Markdown
Member

To complement the default and behaviour while building relations, allow two very similar relations to be combined with an or.

Post.where('id = 1').or(Post.where('id = 2'))
# => SELECT * FROM posts WHERE (id = 1) OR (id = 2)

Unlike previous attempts, we avoid any ambiguity about "how much" we're ORing by exclusively accepting a second relation -- thus, we're ORing whatever conditions differ between the two.

This is a much stricter variant of @gaelmuller's #9052; see also @oelmekki's #10891.

/cc @dhh @jeremy @tenderlove @rafaelfranca

    Post.where('id = 1').or(Post.where('id = 2'))
    # => SELECT * FROM posts WHERE (id = 1) OR (id = 2)

[Matthew Draper & Gael Muller]
@dhh

dhh commented Jul 4, 2014

Copy link
Copy Markdown
Member

API-wise, this seems reasonable to me. Others can have a look at the implementation.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd extract these conditionals to smaller private methods. I know what they are doing but it would be better if they were easily to nay contributor understand what they are doing.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, this ended up a bit uglier than I'd originally anticipated, and I didn't go back and clean it up. Will do.

@egilburg

egilburg commented Jul 4, 2014

Copy link
Copy Markdown
Contributor
  1. Is this chainable?

a.or(b).or(c)

  1. If yes, does it combine with and and what's the evaluation order for something like:

a.or(b).and(c).or(d)

@rafaelfranca

Copy link
Copy Markdown
Member

@egilburg they only work with relations so they can't be combined with and. We don't even have a and method 😄.

They can be chained but with other relation so b and c need to be something like:

User.where('something').or(User.where('something')).or(User.where('something'))

But I believe something like this is still valid:

User.where('something').or(User.where('something')).where(bar: true)

@oelmekki

oelmekki commented Jul 4, 2014

Copy link
Copy Markdown

I love this solution, thanks.

I don't see in tests anything specific to handling where.not, does it
works without problems ?

Something like :

Post.where.not('id = 1').or(Post.where.not('id = 2'))

Not that it would be very good code (it's quite hard to understand),
but there probably will be people to try it.

@egilburg

egilburg commented Jul 4, 2014

Copy link
Copy Markdown
Contributor

Sorry, instead of and I meant just .where().where().where() which of course acts like AND.

In below example what would be evaluation order be?:

User.where(a).where(b).or(User.where(c))

(A && B) || C

or

A && (B || C)

And in below example:

User.where(a).or(User.where(b)).where(c)

Is it:

(A || B) && C

or

A || (B && C)

@gaelmuller

Copy link
Copy Markdown

Obviously I think this is needed, but I don't see what is wrong with my
proposition.

I personnaly find this notation heavier.

Gael Muller

@rafaelfranca

Copy link
Copy Markdown
Member
User.where(a).where(b).or(User.where(c))

(A && B) || C

User.where(a).or(User.where(b)).where(c)

(A || B) && C

@egilburg

egilburg commented Jul 4, 2014

Copy link
Copy Markdown
Contributor

Ok, so it's always left-to-right. In this case, what ambiguity is solved by using the heavier:

User.where(field1: 'value1').or(User.where(field2: 'value2')).where(field3: 'value3')

As opposed to lighter:

User.where(field1: 'value1').or(field2: 'value2').where(field3: 'value3')

@matthewd

matthewd commented Jul 4, 2014

Copy link
Copy Markdown
Member Author

and to get the other interpretations:

User.where(a).where(b).or(User.where(a).where(c)))
# or: z = User.where(a); z.where(b).or(z.where(c))

A && (B || C)

User.where(a).or(User.where(b).where(c))

A || (B && C)


In general, I anticipate this being used mostly to combine named scopes, likely often inside another named scope (like this): if you're already dealing with direct where conditions, you're probably still better off using straight SQL.

@dhh

dhh commented Jul 4, 2014

Copy link
Copy Markdown
Member

User.where(a).or(User.where(b).where(c))
A || (B && C)

On Jul 4, 2014, at 11:05 AM, Eugene Gilburg notifications@github.com wrote:

Ok, so it's always left-to-right. In this case, what ambiguity is solved by using the heavier:

User.where(field1: 'value1').or(User.where(field2: 'value2')).where(field3: 'value3')

As opposed to lighter:

User.where(field1: 'value1').or(field2: 'value2').where(field3: 'value3')


Reply to this email directly or view it on GitHub.

@oelmekki

oelmekki commented Jul 4, 2014

Copy link
Copy Markdown

@rafaelfranca :

User.where(a).or(User.where(b)).where(c)
(A || B) && C

I think the point is precisely to avoid such order consideration and
only use OR on specific fields :

User.where( foo: 'a' ).or( User.where( foo: 'b' ) ).where( bar: 'c' )

and

User.where( foo: 'a' ).where( bar: 'c' ).or( User.where( foo: 'b' ) )

are both : ( foo = 'a' or foo = 'b' ) and bar = 'c'

Edit : or maybe not, given Matthew answer.

@gaelmuller :

This ordering thing was the problem with your implementation.

@matthewd

matthewd commented Jul 4, 2014

Copy link
Copy Markdown
Member Author

The main ambiguity in that syntax, I think, is:

User.where(a: 1).where(b: 2).or(c: 3)

That will presumably resolve as "(a = 1 AND b = 2) OR c = 3", as the heavier one would... but how do I express the (arguably) more likely "a = 1 AND (b = 2 OR c = 3)"? (given that I'm in a method on the relation User.where(a: 1))

@egilburg

egilburg commented Jul 4, 2014

Copy link
Copy Markdown
Contributor

In latter case you'd use:

User.where(a: 1).where(User.where(b: 2).or(c: 3))

So where would allow both a hash syntax or accept a sub-scope inside, allowing combining simple left-to-right interpretations with manual priority order if needed.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a particular reason for this? Shouldn't

WHERE a = 1 AND (b = 2 OR c = 3)

be equivalent to

WHERE ((a = 1 AND b = 2) OR (a = 1 AND c = 3))

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. legibility / length of the generated query
  2. keeps the query the same "shape" as the set of relation calls that created it

So, yes.. but it seemed like a nice thing to do, and not unreasonably expensive.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in your example @sgrif if a != 1 wouldn't the first fail after one check whereas the second would still make two checks?

@al2o3cr

al2o3cr commented Jul 6, 2014

Copy link
Copy Markdown
Contributor

This seems to avoid some of the issues from the previous or attempts (for instance, chaining on an existing where doesn't produce unexpected SQL) but introduces a distinction between bound params and SQL strings:

      # works
      base_rel = Post.where(id: 1)
      base_rel.where("title = 'bar'").or(base_rel.where("title = 'baz'")).to_a

      # fails with "ArgumentError: Relation passed to #or must be structurally compatible"
      base_rel = Post.where(id: 1)
      base_rel.where(title: 'bar').or(base_rel.where(title: 'baz')).to_a

This appears to be caused by the two relations being ord having different bind_values in the second case. I'm not sure how to deal with merging bind_values, but this should work or it's going to be very hard to reliably or relations.

@ghost

ghost commented Jul 7, 2014

Copy link
Copy Markdown

Maybe you can use a code block as argument to make it simpeler in use:

Post.where(id: 1).or({ where(id: 2) })

@oelmekki

Copy link
Copy Markdown

Btw @matthewd , by far the biggest problem I had with
activerecord_any_of was the lost of atypical information (not WHERE,
HAVING or JOIN) from more elaborated queries (that's why I have scoped
it behind WhereChain to make clear you're supposed to use where like
relations, and not just any kind of relation you want).

I've toyed for a time with the idea of using UNION instead of OR for
those queries (which remain fairly rare compared to usual queries):
better dropping perfs (provided everyone is aware of it) than failing.

I've hit two limitations making this a hard task :

  • arel implementation of UNION is tricky and probably needs refactoring
    for it to become a first class keyword
  • sqlite's support of UNION is problematic, for example you can't use a
    dedicated LIMIT for each statement union'd

In case it helps.

@sgrif

sgrif commented Nov 5, 2014

Copy link
Copy Markdown
Contributor

It looks like this will break on queries which use bound parameters, we should probably merge the bind_values and add a test for hash style where, as well.

@lucascaton

Copy link
Copy Markdown
Contributor

Nice one! 👍

@duduribeiro

Copy link
Copy Markdown

👍

@brunojabs

Copy link
Copy Markdown

Great! 👍 🎉

@flowerett

Copy link
Copy Markdown

🎉

@deniskorobicyn

Copy link
Copy Markdown

👍

@nurey

nurey commented Feb 3, 2015

Copy link
Copy Markdown
Contributor

✨ 👍
any idea which version of Rails this will land in?

@dhh

dhh commented Feb 3, 2015

Copy link
Copy Markdown
Member

Rails 5.0.

On Feb 3, 2015, at 11:35, Ilia Lobsanov notifications@github.com wrote:

any idea which version of Rails this will land in?


Reply to this email directly or view it on GitHub.

@EppO

EppO commented Feb 6, 2015

Copy link
Copy Markdown

Awesome ! 👏

@dalpo

dalpo commented Feb 16, 2015

Copy link
Copy Markdown

❤️

@dgilperez

Copy link
Copy Markdown
Contributor

Bravo! ❤️

@pinak1180

Copy link
Copy Markdown

Great..!!! 👍

@sergiotapia

Copy link
Copy Markdown

Fanastic!

@davidgeere

Copy link
Copy Markdown

It's wonderful but I feel like I didn't see an example above that allows for the following example:

 @person = Person.where("(first_name = :forename OR middle_name = :forename OR nickname = :forename) AND last_name = :last_name AND gender = :gender AND birthday = :birthday", forename: first_name, last_name: last_name, gender: gender, birthday: birthday).first

So I would reuse the first name passed in to try an prepare for the chance that someone could have used the persons middle or nickname instead of their full name. Stupid example but on contracts I am David (first name), to the general public I am Mitchell (middle) and to friends I am Mitch (nickname) and I have been searched for on all of them.

Looking forward to getting the or relation!

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you create a method like this?

def validate_strutucture_for(attr)
  lambda { |name| send("#{name}_#{attr}") == other.send("#{name}_#{attr}") }
end

And with that method, you can write

Relation::SINGLE_VALUE_METHODS.all? &validate_strutucture_for("value")

and

(Relation::MULTI_VALUE_METHODS - [allowed_to_vary, :extending]).all? &validate_strutucture_for("values")

What do you think?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that's a bit harder to read in Ruby than what we have now (though it could be broken up)

@bf4

bf4 commented Sep 1, 2015

Copy link
Copy Markdown
Contributor

FWIW, I've written up a backport of ActiveRelation#or to Rails 4.2.3

@Eric-Guo

Copy link
Copy Markdown
Contributor

I just copy @bf4 file to a new gems called where-or

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.