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

#last and #first finders should use query_constraints for ordering #46676

Merged

Conversation

nvasilevski
Copy link
Contributor

Since the newly introduced query_constraints config is a higher abstraction for the existing ActiveRecord::Base#primary_key and can be treated as a "virtual primary key", we should be moving towards substituting primary_key usages with query_constraints_list on the Active Record models level.

This PR changes #last and #first finders which both internally call into the ordered_relation method to use query_constraints_list to build the ORDER BY clause. This is a non-breaking change as all existing Active Record models will have query_constraints_list implicitly configured as [primary_key] which for most of these models will be the same as [id]

Possible extensions

While doing this refactoring I realized that it would be relatively easy to turn implicit_order_column into implicit_order_columns and allow defining multiple columns for the ORDER BY clause. I don't see an immediate need for doing this, just wanted to mention that it is easily achievable

Comment on lines 587 to 590
# remove the implicit order column from the query constraints list
order_columns = query_constraints_list - [implicit_order_column]
# put the implicit order column at the beginning of the order list
order_columns.unshift(implicit_order_column)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
# remove the implicit order column from the query constraints list
order_columns = query_constraints_list - [implicit_order_column]
# put the implicit order column at the beginning of the order list
order_columns.unshift(implicit_order_column)
# put the implicit order column at the beginning of the order list
[implicit_order_column] | query_constraints_list

What do you think about this? I think it does the same thing and may even be faster:

Benchmark.ips do |x|
  x.report("unshift") do
    list = ["order_id", "shop_id", "id"] - ["id"]
    list.unshift("id")
  end
  x.report("or") { ["id"] | ["order_id", "shop_id", "id"] }
  x.compare!
end
$ ruby bench.rb
Fetching gem metadata from https://rubygems.org/.
Resolving dependencies...
Using benchmark-ips 2.10.0
Using bundler 2.3.10
Warming up --------------------------------------
             unshift   269.860k i/100ms
                  or   330.147k i/100ms
Calculating -------------------------------------
             unshift      2.779M (± 4.2%) i/s -     14.033M in   5.059285s
                  or      3.335M (± 2.6%) i/s -     16.837M in   5.053048s

Comparison:
                  or:  3334503.0 i/s
             unshift:  2778866.2 i/s - 1.20x  (± 0.00) slower

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also it looks better 👍 Made a change, thanks!

else
self
end
return self unless order_values.empty? && (implicit_order_column || !query_constraints_list.empty?)
Copy link
Member

Choose a reason for hiding this comment

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

I generally like early returns but I'm finding this one harder to read than the previous implementation. I think I'd rather this use the if/else than the early return.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fair, I brought the if else back 👍

@nvasilevski nvasilevski force-pushed the ordered-relation-respects-query-constraints branch from 926ef18 to 1635152 Compare December 9, 2022 14:59
@eileencodes eileencodes merged commit 2c86139 into rails:main Dec 9, 2022
@eileencodes eileencodes deleted the ordered-relation-respects-query-constraints branch December 9, 2022 16:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants