Skip to content
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

Multiple fields for ordering by when using previous, next #48

Closed
pnomolos opened this issue Feb 20, 2015 · 13 comments
Closed

Multiple fields for ordering by when using previous, next #48

pnomolos opened this issue Feb 20, 2015 · 13 comments
Milestone

Comments

@pnomolos
Copy link
Contributor

Right now you can only set a single field for ordering by when using previous and next. Most of the time this isn't an issue, but I've run into an issue where things are breaking down for me.

Here's the code for previous for reference:

    def previous(options={})
      field = self.class.by_star_start_field
      value = self.send(field.split(".").last)
      self.class.by_star_scope(options.merge(scope_args: self)).where("#{field} < ?", value).reorder("#{field} DESC").first
    end

Part of my issue is the use of where("#{field} < ?", value) rather than something like where("#{field} <= ? AND id != ?", value, self.id) because my field is month-based (and can't be changed) and on very rare occasions I have two values with the same month, so previous/next will always skip to a different month. I do realize that my proposed fix is over-simplified and falls apart when dealing with legacy schemas, composite PKs, etc, but it's for the sake of example.

Following up from that is then the inability to order records by some criteria within the same month, should the previous issue be fixable.

I don't know if this is something that you want to deal with, or if it's too niche to be a problem. I'm more than willing to create a full pull request should you be OK with trying to solve it, and would welcome any suggestions, criticism, or thoughts.

Thanks!

@johnnyshields
Copy link
Collaborator

I don't understand what you mean by "month based". How is your data stored in the DB? As a string? YYYY-MM ?

@pnomolos
Copy link
Contributor Author

As a Postgres date field, with the day always set to the 1st.

@johnnyshields
Copy link
Collaborator

And so the problem is that since two dates are identical values, it considers them as one?

You fix may not work by the way (i.e. excluding the id), since it can lead to toggling between records A and B in the same month and you can never progress forward.

The best solution I can think of is to slightly randomize the time of the month data in your DB, so that the values are very slightly different (i.e. by a few seconds) and the function will behave consistently.

The only other way would be if Postgres has some native query method that can jump to a given ID in a SELECT statement then get the next record, I don't know if this is supported however.

@pnomolos
Copy link
Contributor Author

This is why my proposed solution would allow you to order via multiple fields, my data doesn't even include a timestamp on the date field so adding random seconds would require reworking a bunch of code (as a lot of comparison checks based on the date could be affected). My idea was something like the following for the previous method (totally untested):

def previous(options={})
    fields = [*self.class.by_star_start_fields]
    query = self.class.by_star_scope(options.merge(scope_args: self))
    fields.each do |field|
        query = query.where("#{field} <= ?", self.send(field.split(".").last))
    end
    query.where("id != ?", self.id).reorder("#{fields.join(' DESC, ')} DESC").first
end

@radar
Copy link
Owner

radar commented Feb 20, 2015

Perhaps you could define a different previous / next method that worked based solely on the id column. Would that work?

@pnomolos
Copy link
Contributor Author

@radar Unfortunately not. IDs are UUIDs so they aren't orderable.

@johnnyshields
Copy link
Collaborator

Suppose I have the following data

record id date
a 1 2015-01-01
b 2 2015-01-01
c 3 2015-01-02

records.where("date <= ?", a.date).where("id != ?", a.id).reorder("date ASC")

This will return:

record id date
b 2 2015-01-01
c 3 2015-01-02

The first being b.

Then I do:

records.where("date <= ?", b.date).where("id != ?", b.id).reorder("date ASC")

This will return:

record id date
a 1 2015-01-01
c 3 2015-01-02

The first being a. So we have an infinite loop between a and b.

@johnnyshields
Copy link
Collaborator

One alternative would be to make previous_batch and next_batch (or something like that) methods which remove the .first specifier hence can return more than one result. I think that might be the best in this case.

@pnomolos
Copy link
Contributor Author

This is why I was suggesting being able to order by multiple fields, so you wouldn't end up in an infinite loop

@johnnyshields
Copy link
Collaborator

@pnomolos I don't think that would make a difference. Please try it yourself and see what happens.

@pnomolos
Copy link
Contributor Author

@johnnyshields I'll try this out sometime this week, just haven't had time due to some deadlines.

@johnnyshields johnnyshields modified the milestone: 3.0 May 5, 2015
@johnnyshields
Copy link
Collaborator

johnnyshields commented May 8, 2016

I thought a bit about this. I think what you need to do is an OR query to handle the "equals" and "greater than" cases separately.

(
   where("date = ?", x.date).where("#{field} > ?", x.send(:field))
   OR 
   where("date <= ?", x.date).where("id != ?", x.id)
).reorder("date ASC")

In general ID is a safe to use here, because UUIDs ARE orderable and guaranteed to be present (the existing logic already uses ID). But we could also give the option for tiebreaker_field and tiebreaker_sort or something.

@johnnyshields
Copy link
Collaborator

closing as this is out-of-scope for this gem

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

No branches or pull requests

3 participants