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

Don't apply sorting to collection until after scoping #7205

Merged

Conversation

agrobbin
Copy link
Contributor

Consider sorting that requires a JOIN as part of the SQL query (or really anything that was raised in #3085):

ActiveAdmin.register Foo do
  config.order_clause = FooOrderClause

  index do
    column :latest_bar, sortable: 'bars.created_at' do
      ...
    end
  end
end

class FooOrderClause < ActiveAdmin::OrderClause
  def apply(chain)
    case table_column
    when 'bars.created_at'
      chain.joins(:bars).select(table_column)
    else
      super
    end
  end
end

Oftentimes, including that JOIN is not a huge deal in terms of performance, however, there have been situations where that table/column can increase the time it takes for a query to run by a couple hundred milliseconds, for various reasons.

Increasing the time to load the page slightly (0.1s) is not a big deal, in an admin, but imagine if you had 7-10 different scopes on that page as well? Because scopes leverage collection_before_scope, they will proceed to include the JOIN in their query, even though it is only necessary to do sorting of a collection that is not actually being sorted, only counted.

Applying sorting of the collection after scoping, instead of before, should have no bearing on the end-result, while ensuring that any custom sorting logic is not needlessly applied to all scope count queries.

Consider sorting that requires a `JOIN` as part of the SQL query (or really anything that was raised in activeadmin#3085):

```ruby
ActiveAdmin.register Foo do
  config.order_clause = FooOrderClause

  index do
    column :latest_bar, sortable: 'bars.created_at' do
      ...
    end
  end
end

class FooOrderClause < ActiveAdmin::OrderClause
  def apply(chain)
    case table_column
    when 'bars.created_at'
      chain.joins(:bars).select(table_column)
    else
      super
    end
  end
end
```

Oftentimes, including that `JOIN` is not a huge deal in terms of performance, however, there have been situations where that table/column can increase the time it takes for a query to run by a couple hundred milliseconds, for various reasons.

Increasing the time to load the page slightly (0.1s) is not a big deal, in an admin, but imagine if you had 7-10 different scopes on that page as well? Because scopes leverage `collection_before_scope`, they will proceed to include the `JOIN` in their query, even though it is only necessary to do sorting of a collection that is not actually being sorted, only counted.

Applying sorting of the collection *after* scoping, instead of before, should have no bearing on the end-result, while ensuring that any custom sorting logic is not needlessly applied to all scope count queries.
Copy link
Member

@deivid-rodriguez deivid-rodriguez left a comment

Choose a reason for hiding this comment

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

This makes sense to me!

Copy link
Member

@Fivell Fivell left a comment

Choose a reason for hiding this comment

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

thanks

@Fivell Fivell merged commit 72336a1 into activeadmin:master Feb 3, 2022
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.

None yet

4 participants