perf(spp_programs): replace cycle computed fields with SQL aggregation#119
perf(spp_programs): replace cycle computed fields with SQL aggregation#119kneckinator wants to merge 1 commit into19.0from
Conversation
total_amount, total_entitlements_count, show_approve_entitlements_button, and all_entitlements_approved now use SQL queries instead of iterating over loaded entitlement recordsets. This avoids loading all entitlements into memory just to compute summary statistics.
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly improves performance in the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request provides a significant performance improvement by replacing Python-based iterations with more efficient SQL aggregations for several computed fields. The changes are well-implemented and accompanied by a comprehensive new test suite that covers various scenarios, including multi-cycle batching. My review includes one suggestion to adopt a more robust and standardized pattern for these computed fields to enhance readability and ensure correct behavior with new or empty recordsets.
| if not self.ids: | ||
| for rec in self: | ||
| rec.total_amount = 0 | ||
| return | ||
| self.env.cr.execute( | ||
| """ | ||
| SELECT cycle_id, COALESCE(SUM(initial_amount), 0) | ||
| FROM spp_entitlement | ||
| WHERE cycle_id IN %s | ||
| GROUP BY cycle_id | ||
| """, | ||
| (tuple(self.ids),), | ||
| ) | ||
| totals = dict(self.env.cr.fetchall()) | ||
| for rec in self: | ||
| rec.total_amount = sum(entitlement.initial_amount for entitlement in rec.entitlement_ids) | ||
| rec.total_amount = totals.get(rec.id, 0) |
There was a problem hiding this comment.
The current implementation for handling new records (without IDs) is a bit complex. A more robust and common pattern in Odoo for computed fields that use SQL is to first set a default value for all records, then filter for records with IDs to perform the query. This makes the code clearer and safer, especially for empty recordsets. This same feedback applies to _compute_total_entitlements_count, _compute_show_approve_entitlement, and _compute_all_entitlements_approved as well.
| if not self.ids: | |
| for rec in self: | |
| rec.total_amount = 0 | |
| return | |
| self.env.cr.execute( | |
| """ | |
| SELECT cycle_id, COALESCE(SUM(initial_amount), 0) | |
| FROM spp_entitlement | |
| WHERE cycle_id IN %s | |
| GROUP BY cycle_id | |
| """, | |
| (tuple(self.ids),), | |
| ) | |
| totals = dict(self.env.cr.fetchall()) | |
| for rec in self: | |
| rec.total_amount = sum(entitlement.initial_amount for entitlement in rec.entitlement_ids) | |
| rec.total_amount = totals.get(rec.id, 0) | |
| self.total_amount = 0 | |
| recs_with_ids = self.filtered('id') | |
| if not recs_with_ids: | |
| return | |
| # Query for records with IDs | |
| self.env.cr.execute( | |
| """ | |
| SELECT cycle_id, COALESCE(SUM(initial_amount), 0) | |
| FROM spp_entitlement | |
| WHERE cycle_id IN %s | |
| GROUP BY cycle_id | |
| """, | |
| (tuple(recs_with_ids.ids),), | |
| ) | |
| totals = dict(self.env.cr.fetchall()) | |
| for rec in recs_with_ids: | |
| rec.total_amount = totals.get(rec.id, 0) |
Codecov Report❌ Patch coverage is
❌ Your patch check has failed because the patch coverage (66.66%) is below the target coverage (70.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## 19.0 #119 +/- ##
==========================================
- Coverage 70.14% 69.96% -0.19%
==========================================
Files 739 783 +44
Lines 43997 46622 +2625
==========================================
+ Hits 30863 32619 +1756
- Misses 13134 14003 +869
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
1 participant
Summary
Replace Python iteration over entitlement recordsets with SQL aggregation for 4 cycle computed fields, eliminating the need to load all entitlements into memory just to compute summary statistics.
Changes
_compute_total_amountsum(ent.initial_amount for ent in rec.entitlement_ids)— loads every entitlement recordSELECT cycle_id, SUM(initial_amount) ... GROUP BY cycle_id_compute_total_entitlements_countlen(rec.entitlement_ids) + len(rec.inkind_entitlement_ids)— loads both recordsetsSELECT cycle_id, COUNT(*) ... GROUP BY cycle_idqueries_compute_show_approve_entitlementany(ent.state == "pending_validation" for ent in ...)— loads all, iterates in PythonSELECT DISTINCT cycle_id ... WHERE state = 'pending_validation'withUNIONfor inkind_compute_all_entitlements_approvedall(ent.state == "approved" for ent in ...)— loads all, iterates in PythonAll SQL queries accept multiple cycle IDs (
WHERE cycle_id IN %s) so Odoo's batch recomputation is efficient across cycles.Context
Phase 3 of 9 in the
spp_programsperformance optimization effort. Independent of other phases.Test Plan
./scripts/test_single_module.sh spp_programspassestest_cycle_computed_fields.pycovering:INclause correctly)