#1056 ✓resolved

Chaining finders loses conditions on merge

Reported by bodhi | September 22nd, 2009 @ 09:44 PM | in 0.10.2

A contrived example:

DataSet.all(:id.lt => 3).all(:id.lt => 5)
=> SELECT "id" FROM "data_sets" WHERE "id" < 5 ORDER BY "id"

The id < 3 condition disappeared. I expected to get something like

=> SELECT "id" FROM "data_sets" WHERE ("id" < 3) AND ("id" < 5) ORDER BY "id"

The use case is working on a subset of a query passed in from somewhere else:

def process values
   early_time = find_early_time values # defined somewhere
   early_values = values.all(:time.lt => early_time)

values = DataValues.all(:time.lt => 5.4)
process values

Comments and changes to this ticket

  • Kai Chen

    Kai Chen September 22nd, 2009 @ 11:03 PM

    I have the same problem

  • Dan Kubb (dkubb)

    Dan Kubb (dkubb) October 7th, 2009 @ 12:24 PM

    • Milestone set to 0.10.2
    • State changed from “new” to “confirmed”
    • Assigned user set to “Ted Han (knowtheory)”

    I can confirm this is a problem.

    @knowtheory: I am assigning this to you because you are working on making sure Query#merge works sanely, and this is an example of where our approach merging in the Query#options falls down.

    The simplest approach to this would be to AND the conditions together.

    The optimal approach (and probably not possible in the general case) would be to notice that { :id.lt => 5 } matches every record that { :id.lt => 3 } does, so the latter condition is unnecessary, and factor it out.

    @knowtheory: on another note, I know you said a Query#to_sql method seems like it doesn't belong, allowing people to "play" without actually executing any queries could aid in debugging and catching issues like this. For example, if people can drop down into IRB, and do:

    DataSet.all(:id.lt => 3).all(:id.lt => 5).query.to_sql # => sql query

    .. and see the query that is produced, it could provide a lower barrier to entry to making sure the queries are correct and optimized. For me to suggest this is pretty radical, because I'm so wary of tightly coupling to RDBMS, SQL does provice a nice compact representation of a Query that most developers understand.

  • Dan Kubb (dkubb)

    Dan Kubb (dkubb) October 23rd, 2009 @ 12:23 AM

    • State changed from “confirmed” to “resolved”

    I can confirm this is now working in edge dm-core. Conditions are now ANDed together by default in this situation, but no optimizations to factor out redundant conditionals are performed (as will likely occur at some point in the future). However, since the expected query is generated, and will return the expected results I am marking this ticket as closed.

Please Sign in or create a free account to add a new ticket.

With your very own profile, you can contribute to projects, track your activity, watch tickets, receive and update tickets through your email and much more.

New-ticket Create new ticket

Create your profile

Help contribute to this project by taking a few moments to create your personal profile. Create your profile »