⚡ Bolt: Replace func.sum(case) with GROUP BY for single column aggregation#760
⚡ Bolt: Replace func.sum(case) with GROUP BY for single column aggregation#760RohanExploit wants to merge 1 commit into
Conversation
…ation Replaced expensive `func.sum(case(...))` logic with standard `GROUP BY` queries in `backend/closure_service.py` and `backend/routers/grievances.py` when calculating statistics over the single `confirmation_type` categorical column. This optimization utilizes database indexes/hashing natively instead of forcing row-by-row condition evaluations. Results are transformed into Python dictionaries for O(1) value lookups. Performance impact (1000 iterations): - sum_case: ~0.73s - group_by: ~0.44s (Approx. ~40% faster execution on single column aggregates)
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
✅ Deploy Preview for fixmybharat canceled.
|
🙏 Thank you for your contribution, @RohanExploit!PR Details:
Quality Checklist:
Review Process:
Note: The maintainers will monitor code quality and ensure the overall project flow isn't broken. |
📝 WalkthroughWalkthroughThis PR implements a performance optimization for SQLAlchemy analytics by replacing conditional ChangesSQLAlchemy Aggregation Optimization
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsStopped waiting for pipeline failures after 30000ms. One of your pipelines takes longer than our 30000ms fetch window to run, so review may not consider pipeline-failure results for inline comments if any failures occurred after the fetch window. Increase the timeout if you want to wait longer or run a Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
This PR optimizes closure confirmation counting by replacing conditional func.sum(case(...)) aggregates with GROUP BY queries and dictionary lookups, aligning closure status/finalization paths with the repository’s performance-focused database access patterns.
Changes:
- Replaces closure confirmation/dispute aggregate logic in the router and closure service with grouped counts.
- Removes the temporary benchmark script for closure status aggregation.
- Adds a Bolt learning note documenting the
GROUP BYoptimization.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
backend/routers/grievances.py |
Updates closure status count aggregation and applies formatting cleanup. |
backend/closure_service.py |
Updates closure finalization count aggregation and applies formatting cleanup. |
backend/tests/benchmark_closure_status.py |
Removes the benchmark script used for local measurement. |
.jules/bolt.md |
Documents the aggregation optimization guidance. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| raise HTTPException(status_code=404, detail="Grievance not found") | ||
|
|
||
| # Optimized: Use a single aggregate query to calculate total followers, confirmations and disputes in one database roundtrip |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.jules/bolt.md:
- Line 93: Update the learning entry header "## 2026-05-19 - Replace
func.sum(case(...)) with GROUP BY" to the PR's actual opening date (change
2026-05-19 to 2026-05-14) so the entry date matches this PR's timeline and
avoids future-dating the log; locate the header line with the exact phrase
"Replace func.sum(case(...)) with GROUP BY" and edit only the date portion.
In `@backend/closure_service.py`:
- Around line 65-67: The calculation for required_confirmations in
ClosureService uses int(...) which floors and can undercount confirmations;
change the logic in the places using int(follower_count *
ClosureService.CONFIRMATION_THRESHOLD) (e.g., where required_confirmations is
computed) to use math.ceil(...) to round up instead, and add the math import if
missing so the required_confirmations = max(1, math.ceil(follower_count *
ClosureService.CONFIRMATION_THRESHOLD)) enforces the 60% rule; update both the
occurrence around the required_confirmations assignment shown and the similar
calculation at the later occurrence (lines ~187-189).
In `@backend/routers/grievances.py`:
- Around line 529-531: The calculation for required_confirmations uses int(...),
which floors and can undercount; change it to use math.ceil(total_followers *
ClosureService.CONFIRMATION_THRESHOLD) (still wrapped with max(1, ...)) so the
60% threshold rounds up correctly, and add the necessary import for math; update
the expression where required_confirmations is assigned (referencing
required_confirmations, total_followers, and
ClosureService.CONFIRMATION_THRESHOLD).
- Line 604: The verification endpoints use insecure direct string equality; in
verify_escalation_audit_blockchain and verify_grievance_blockchain replace the
direct comparisons that set is_valid (computed_hash == audit.integrity_hash and
computed_hash == grievance.integrity_hash) with constant-time comparisons using
hmac.compare_digest(computed_hash, audit.integrity_hash) and
hmac.compare_digest(computed_hash, grievance.integrity_hash), and ensure the
module imports hmac at the top of the file so the function is available.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 786b89ef-537d-4b35-9aca-5749b4e47818
📒 Files selected for processing (4)
.jules/bolt.mdbackend/closure_service.pybackend/routers/grievances.pybackend/tests/benchmark_closure_status.py
💤 Files with no reviewable changes (1)
- backend/tests/benchmark_closure_status.py
| **Learning:** In retrieval loops calculating Jaccard similarity (e.g. RAG), explicitly building a union set `A.union(B)` is expensive due to memory allocation and population. | ||
| **Action:** Use the inclusion-exclusion principle $|A \cup B| = |A| + |B| - |A \cap B|$ to calculate union size in O(1) arithmetic time after calculating the intersection. Pre-calculate $|B|$ (token count) to further reduce overhead. Use `isdisjoint()` for fast early-exit. | ||
|
|
||
| ## 2026-05-19 - Replace func.sum(case(...)) with GROUP BY |
There was a problem hiding this comment.
Fix the learning entry date to match this PR’s actual timeline.
This note is dated 2026-05-19, but this PR was opened on 2026-05-14. Future-dating makes the learning log harder to trust and search.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.jules/bolt.md at line 93, Update the learning entry header "## 2026-05-19 -
Replace func.sum(case(...)) with GROUP BY" to the PR's actual opening date
(change 2026-05-19 to 2026-05-14) so the entry date matches this PR's timeline
and avoids future-dating the log; locate the header line with the exact phrase
"Replace func.sum(case(...)) with GROUP BY" and edit only the date portion.
| required_confirmations = max( | ||
| 1, int(follower_count * ClosureService.CONFIRMATION_THRESHOLD) | ||
| ) |
There was a problem hiding this comment.
Use ceil, not int, for confirmation threshold math.
int() floors the requirement and under-enforces the 60% rule (e.g., 3 followers requires 1 confirmation). This can approve closures prematurely.
Suggested patch
+import math
...
- required_confirmations = max(
- 1, int(follower_count * ClosureService.CONFIRMATION_THRESHOLD)
- )
+ required_confirmations = max(
+ 1, math.ceil(follower_count * ClosureService.CONFIRMATION_THRESHOLD)
+ )
...
- required_confirmations = max(
- 1, int(total_followers * ClosureService.CONFIRMATION_THRESHOLD)
- )
+ required_confirmations = max(
+ 1, math.ceil(total_followers * ClosureService.CONFIRMATION_THRESHOLD)
+ )Also applies to: 187-189
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/closure_service.py` around lines 65 - 67, The calculation for
required_confirmations in ClosureService uses int(...) which floors and can
undercount confirmations; change the logic in the places using
int(follower_count * ClosureService.CONFIRMATION_THRESHOLD) (e.g., where
required_confirmations is computed) to use math.ceil(...) to round up instead,
and add the math import if missing so the required_confirmations = max(1,
math.ceil(follower_count * ClosureService.CONFIRMATION_THRESHOLD)) enforces the
60% rule; update both the occurrence around the required_confirmations
assignment shown and the similar calculation at the later occurrence (lines
~187-189).
| required_confirmations = max( | ||
| 1, int(total_followers * ClosureService.CONFIRMATION_THRESHOLD) | ||
| ) |
There was a problem hiding this comment.
required_confirmations is undercounted due to floor conversion.
This endpoint also uses int(...), which floors and can report a lower approval threshold than the configured 60%.
Suggested patch
+import math
...
- required_confirmations = max(
- 1, int(total_followers * ClosureService.CONFIRMATION_THRESHOLD)
- )
+ required_confirmations = max(
+ 1, math.ceil(total_followers * ClosureService.CONFIRMATION_THRESHOLD)
+ )📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| required_confirmations = max( | |
| 1, int(total_followers * ClosureService.CONFIRMATION_THRESHOLD) | |
| ) | |
| required_confirmations = max( | |
| 1, math.ceil(total_followers * ClosureService.CONFIRMATION_THRESHOLD) | |
| ) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/routers/grievances.py` around lines 529 - 531, The calculation for
required_confirmations uses int(...), which floors and can undercount; change it
to use math.ceil(total_followers * ClosureService.CONFIRMATION_THRESHOLD) (still
wrapped with max(1, ...)) so the 60% threshold rounds up correctly, and add the
necessary import for math; update the expression where required_confirmations is
assigned (referencing required_confirmations, total_followers, and
ClosureService.CONFIRMATION_THRESHOLD).
| message = "No integrity hash present for this audit log; cryptographic integrity cannot be verified." | ||
| else: | ||
| is_valid = (computed_hash == audit.integrity_hash) | ||
| is_valid = computed_hash == audit.integrity_hash |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify hash-comparison consistency in blockchain verification endpoints
rg -n -C2 'computed_hash\s*==\s*.*integrity_hash|compare_digest\(' backend/routers/grievances.pyRepository: RohanExploit/VishwaGuru
Length of output: 1976
🏁 Script executed:
#!/bin/bash
# Try to clone the repository and verify hash comparison consistency
cd /tmp && git clone https://github.com/RohanExploit/VishwaGuru.git 2>&1 | head -20Repository: RohanExploit/VishwaGuru
Length of output: 158
🏁 Script executed:
#!/bin/bash
# Read the file around line 604 to see the context
cd /tmp/VishwaGuru
head -n 620 backend/routers/grievances.py | tail -n 25Repository: RohanExploit/VishwaGuru
Length of output: 1214
🏁 Script executed:
#!/bin/bash
# Get more context around line 604 to see the actual comparison
cd /tmp/VishwaGuru
sed -n '600,620p' backend/routers/grievances.pyRepository: RohanExploit/VishwaGuru
Length of output: 1026
🏁 Script executed:
#!/bin/bash
# Get the exact line 604 and surrounding context
cd /tmp/VishwaGuru
sed -n '600,610p' backend/routers/grievances.py | cat -nRepository: RohanExploit/VishwaGuru
Length of output: 619
🏁 Script executed:
#!/bin/bash
# Look for the is_valid comparison mentioned in the review
cd /tmp/VishwaGuru
grep -n "is_valid.*computed_hash" backend/routers/grievances.pyRepository: RohanExploit/VishwaGuru
Length of output: 296
🏁 Script executed:
#!/bin/bash
# Check what's actually at line 604 specifically
cd /tmp/VishwaGuru
sed -n '604p' backend/routers/grievances.pyRepository: RohanExploit/VishwaGuru
Length of output: 118
🏁 Script executed:
#!/bin/bash
# Find the function names for each hash comparison
cd /tmp/VishwaGuru
for line in 514 572 630; do
echo "=== Around line $line ==="
sed -n "$((line-15)),$((line+2))p" backend/routers/grievances.py | grep -E "^def |^async def |is_valid|compare_digest"
doneRepository: RohanExploit/VishwaGuru
Length of output: 443
🏁 Script executed:
#!/bin/bash
# Get function context for each comparison line
cd /tmp/VishwaGuru
for line in 514 572 630; do
echo "=== Line $line ==="
awk -v target=$line 'NR <= target && /^(def|async def)/ { func=$0 } NR == target { print func; print $0 }' backend/routers/grievances.py
doneRepository: RohanExploit/VishwaGuru
Length of output: 655
🏁 Script executed:
#!/bin/bash
# Get function context - find the nearest function definition before each line
cd /tmp/VishwaGuru
python3 << 'EOF'
with open('backend/routers/grievances.py', 'r') as f:
lines = f.readlines()
target_lines = [514, 572, 630]
for target in target_lines:
print(f"\n=== Line {target} ===")
# Find nearest function def before this line
for i in range(target-1, -1, -1):
if lines[i].startswith('def ') or lines[i].startswith('async def '):
print(f"Function: {lines[i].strip()}")
break
# Print the target line
print(f"Code: {lines[target-1].strip()}")
EOFRepository: RohanExploit/VishwaGuru
Length of output: 467
🏁 Script executed:
#!/bin/bash
# Verify the exact context for line 514 and 572 - where the actual unsafe comparisons are
cd /tmp/VishwaGuru
echo "=== Lines 510-520 (around line 514) ==="
sed -n '510,520p' backend/routers/grievances.py
echo ""
echo "=== Lines 568-578 (around line 572) ==="
sed -n '568,578p' backend/routers/grievances.pyRepository: RohanExploit/VishwaGuru
Length of output: 1204
🏁 Script executed:
#!/bin/bash
# Check if hmac is imported and compare_digest is used elsewhere
cd /tmp/VishwaGuru
echo "=== Import statements ==="
head -50 backend/routers/grievances.py | grep -E "^import|^from"
echo ""
echo "=== All compare_digest usage ==="
grep -n "compare_digest" backend/routers/grievances.pyRepository: RohanExploit/VishwaGuru
Length of output: 911
Use hmac.compare_digest() for constant-time hash comparison in verification endpoints.
Two of three sibling blockchain verification endpoints use unsafe direct comparison (==) instead of constant-time digest comparison. Lines 514 and 572 in verify_escalation_audit_blockchain() and verify_grievance_blockchain() should match the pattern used at line 630 in verify_closure_confirmation_blockchain().
Replace:
is_valid = (computed_hash == audit.integrity_hash) # Line 514
is_valid = (computed_hash == grievance.integrity_hash) # Line 572With:
is_valid = hmac.compare_digest(computed_hash, audit.integrity_hash)
is_valid = hmac.compare_digest(computed_hash, grievance.integrity_hash)This prevents timing attacks that could leak information about valid integrity hashes.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/routers/grievances.py` at line 604, The verification endpoints use
insecure direct string equality; in verify_escalation_audit_blockchain and
verify_grievance_blockchain replace the direct comparisons that set is_valid
(computed_hash == audit.integrity_hash and computed_hash ==
grievance.integrity_hash) with constant-time comparisons using
hmac.compare_digest(computed_hash, audit.integrity_hash) and
hmac.compare_digest(computed_hash, grievance.integrity_hash), and ensure the
module imports hmac at the top of the file so the function is available.
What:
Replaced the
func.sum(case(...))logic with standard SQLGROUP BYqueries when aggregating statistics over a single categorical column (confirmation_type) inClosureConfirmation. The results are converted to a Python dictionary (dict(counts)) to perform O(1) lookups forconfirmedanddisputedcounts.Why:
Executing multiple
func.sum(case(...))aggregate statements forces the database engine to evaluate conditional statements on every single row instead of utilizing hashing or indexing optimizations available for basic grouped sets. As the number of grievance confirmations grows, this becomes a recurring bottleneck in the database access layer.Impact:
casedirectly inside route logic.Measurement Steps:
Benchmarking script (
benchmark_closure_status.py) verified the performance difference with 50 followers, 30 confirmations, and 10 disputes populated into a SQLite memory database.Documentation: Added the learning regarding "Replace func.sum(case(...)) with GROUP BY" to
.jules/bolt.md.PR created automatically by Jules for task 2756740780356283571 started by @RohanExploit
Summary by cubic
Optimized closure confirmation counting by replacing conditional aggregates with a single GROUP BY query, reducing database work and speeding closure checks and status retrieval.
Performance
func.sum(case(...))to GROUP BY in closure confirmation queries (ClosureService.check_and_finalize_closureandGET /grievances/{id}/closure-status).Refactors
caseimports in routes.backend/tests/benchmark_closure_status.py..jules/bolt.md.Written for commit 90f3c30. Summary will update on new commits.
Summary by CodeRabbit
Performance
Documentation