-
Notifications
You must be signed in to change notification settings - Fork 244
Convert SQL queries to Arel where feasible #453
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
Conversation
- Add ArelHelpers module for building Arel queries - Convert simple queries in numeric_deterministic_ordering.rb - Convert queries in finders.rb and hash_tree_support.rb - Fix table access patterns to use model context properly - Apply RuboCop corrections to all modified files All multi-database tests passing
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.
Pull Request Overview
This PR converts hardcoded SQL queries to Arel for improved type safety, database portability, and maintainability. The changes focus on query builders for hierarchical tree operations while preserving complex database-specific optimizations as raw SQL.
- Added ArelHelpers module with utility methods for building hierarchy queries using Arel
- Converted multiple SQL query methods to use Arel including stats queries, complex joins, and generation-based finders
- Updated table access patterns to properly use hierarchy_class and model tables with proper aliasing
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
File | Description |
---|---|
lib/closure_tree/support.rb | Includes the new ArelHelpers module in the Support class |
lib/closure_tree/arel_helpers.rb | New helper module providing Arel query building utilities |
lib/closure_tree/numeric_deterministic_ordering.rb | Converts ordering and stats queries from SQL to Arel |
lib/closure_tree/hierarchy_maintenance.rb | Updates hierarchy deletion to use Arel helper method |
lib/closure_tree/hash_tree_support.rb | Converts tree scope generation depth queries to Arel |
lib/closure_tree/finders.rb | Converts generation-based and leaves finder queries to Arel |
Comments suppressed due to low confidence (2)
lib/closure_tree/hash_tree_support.rb:10
- [nitpick] The variable name
hierarchy_table_arel
is redundant since it's already clear it's an Arel table. Consider renaming tohierarchy_table
to match the pattern used in other methods.
hierarchy_table_arel = if scope.respond_to?(:hierarchy_class)
lib/closure_tree/hash_tree_support.rb:18
- [nitpick] The variable name
model_table_arel
is redundant since it's already clear it's an Arel table. Consider renaming tomodel_table
to match the pattern used in other methods.
model_table_arel = scope.klass.arel_table
|
||
query = hierarchy_table | ||
.project( | ||
hierarchy_table[:ancestor_id].count.as('total_descendants'), |
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.
The count operation should count all rows, not just ancestor_id values. This should be hierarchy_table.project(Arel.star).count.as('total_descendants')
or use a literal count(*) to match the original SQL behavior.
hierarchy_table[:ancestor_id].count.as('total_descendants'), | |
hierarchy_table.project(Arel.star).count.as('total_descendants'), |
Copilot uses AI. Check for mistakes.
lib/closure_tree/arel_helpers.rb
Outdated
# Build the middle subquery with DISTINCT | ||
middle_subquery = Arel::SelectManager.new | ||
middle_subquery.from(inner_subquery) | ||
middle_subquery.project(Arel.sql('DISTINCT descendant_id')) |
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.
Using raw SQL for DISTINCT defeats the purpose of using Arel. This should use Arel's distinct method: middle_subquery.project(inner_subquery[:descendant_id]).distinct
.
middle_subquery.project(Arel.sql('DISTINCT descendant_id')) | |
middle_subquery.project(inner_subquery[:descendant_id]).distinct |
Copilot uses AI. Check for mistakes.
join_roots = model_table | ||
.join(roots_subquery) | ||
.on(Arel.sql('1 = 1')) |
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.
[nitpick] Using raw SQL '1 = 1' for a cross join condition is not idiomatic Arel. Consider using a proper cross join or documenting why this cartesian product is intentional.
join_roots = model_table | |
.join(roots_subquery) | |
.on(Arel.sql('1 = 1')) | |
join_roots = Arel::Nodes::JoinSource.new( | |
model_table, | |
roots_subquery | |
) |
Copilot uses AI. Check for mistakes.
- Use Arel.star.count instead of counting specific column for SQL COUNT(*) - Replace raw SQL 'DISTINCT' with Arel's .distinct method - Document intentional use of cartesian product join in find_all_by_generation These changes improve code consistency while maintaining the same functionality. All tests pass with these modifications.
Summary
This PR converts hardcoded SQL queries to Arel where it makes sense, improving type safety, database portability, and maintainability.
Changes
_ct_sum_order_by
to use Arelself_and_descendants_preordered
androots_and_descendants_preordered
find_all_by_generation
(instance and class methods) to Arelleaves
method to use Arel joinsdefault_tree_scope
to use Arel for subqueries and joinshierarchy_class
and model tablesmodel_class
to proper context in instance methodsNotes
find_by_path
kept as SQL due to complexityBenefits