Grouped query performance of total_count using SQL #1023
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This test's data was built using a hash keyed by a name which was included twice, so it would always pass as long as total_count builds a valid query. Fixed by using a 2D array instead.
Improved performance for total_count on grouped ActiveRecord::Relation. This re-implements kaminari#979 using plain SQL to avoid issues kaminari#1012 and kaminari#1015
|
I think we should prefer #1022 to keep benefit of prepared statements. See #979 (comment) |
|
I'm more than happy to ditch this in favour of the Arel solution. |
|
I just want to see if this version succeeds with activerecord 4.2 where #1022 does not. |
Only count non-null values of column_name if supplied. This meets the semantics of the SQL count function without requiring that column_name is one of the grouped columns. Also use the older-style heredoc syntax for backwards-compat.
...and it looks like activerecord 4.1 returns string representations from `select_value`
activerecord 4.1 doesn't like the use of a numeric value for `select`
|
Finally passing the tests. @guigs would you consider a SQL-based solution given that the Arel solution is not backwards-compatible with activerecord < 5? The raw SQL in this PR is functionally identical to the Arel-built query, and tbh I don't see how prepared statements factor into it, because the query is built, run and discarded immediately. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Improved performance for total_count on grouped ActiveRecord::Relation.
This re-implements #979 using plain SQL to avoid issues #1012 and #1015
This is an alternative solution to #1022