-
Notifications
You must be signed in to change notification settings - Fork 21.4k
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
[ActiveRecord] Add option filter
on in_order_of
#51761
base: main
Are you sure you want to change the base?
[ActiveRecord] Add option filter
on in_order_of
#51761
Conversation
This comment was marked as spam.
This comment was marked as spam.
This comment was marked as spam.
This comment was marked as spam.
I really like this idea, I had already the same, but you are faster ;) Maybe
|
@timoschilling, ty very much for your review! I liked your idea, @admsev , thanks for the bot suggestions as well! I made the grammar correction and the other consistency correction it suggested! |
only_values
on in_order_of
filter
on in_order_of
Co-authored-by: Timo Schilling <timo@schilling.io>
Should the matching enumerable method also be updated with a similar option? |
@louim i think you're right, but if that's the case, I believe the PRs should be separated for this update. I can work on that. |
filter
on in_order_of
filter
on in_order_of
assert_equal(order, posts.limit(3).map(&:id)) | ||
assert_equal(11, posts.count) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the order for the remaining elements?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's an excellent question. As I see it, arbitrarily, cause the remaining elements will be set with ELSE
value in CASE
clause.
Honestly, i don't know how DBMS defines behind the scenes the order for all the equal values setted with else value(if exists something like that).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
MySQL 8 is always returning the same order:
irb(main):001> User.order(Arel.sql("(CASE WHEN id = 8232465 THEN 1 WHEN id = 8232463 THEN 2 ELSE 3 END)")).limit(5).pluck(:id)
User Pluck (2.8ms) SELECT `users`.`id` FROM `users` ORDER BY (CASE WHEN id = 8232465 THEN 1 WHEN id = 8232463 THEN 2 ELSE 3 END) LIMIT 5
=> [8232465, 8232463, 8232464, 8232466, 8232468]
But I'd not rely in the order of the remaining elements in this scenario. 😬
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@willianveiga Yeah, definitely.
Motivation / Background
This Pull Request has been created because currently,
in_order_of
method always use where clause to filter the results only with the values specified invalues
. Sometimes, we only want to put some values as priority in the sorting but we want the entire search scope without caring about the rest of the sorting. The propose here is add an option to specify to filter scope by values or not.Detail
This Pull Request changes:
Checklist
Before submitting the PR make sure the following are checked:
[Fix #issue-number]