-
Notifications
You must be signed in to change notification settings - Fork 132
backport: feat/conditional-where-on-selections #1102
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
backport: feat/conditional-where-on-selections #1102
Conversation
gfyrag
commented
Nov 6, 2025
- feat: add utils function to centralize the creation of select queries
- feat: optimize queries on ledgers alone in their buckets
WalkthroughThe PR refactors database query construction across multiple resource types in the ledger storage layer. A new Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## release/v2.3 #1102 +/- ##
================================================
+ Coverage 80.52% 81.76% +1.23%
================================================
Files 186 186
Lines 10205 8836 -1369
================================================
- Hits 8218 7225 -993
+ Misses 1565 1188 -377
- Partials 422 423 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
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.
Actionable comments posted: 0
🧹 Nitpick comments (4)
internal/storage/ledger/resource_transactions.go (1)
131-136: Note: Redundant ledger filtering in nested subquery.The
newScopedSelect()is used here within a moves subquery that's already correlated to transactions filtered by the outer scoped select. While not incorrect, this applies the ledger check redundantly:
- Outer query filters by ledger via the scoped select
- This inner query also filters by ledger via another scoped select
- The correlation
WHERE("transactions_id in (select id from dataset)")already ensures ledger scopeThis redundancy shouldn't cause incorrect results, but adds unnecessary overhead to the subquery evaluation.
Consider using
h.store.db.NewSelect()here instead since the ledger scope is already established by the correlation to the dataset. However, the current approach is safer and maintains consistency, so this is optional.internal/storage/ledger/resource_volumes.go (1)
94-108: Note: Redundant ledger filtering in correlated lateral join.The
accountsQueryat line 95 usesnewScopedSelect()with a correlationWHERE("accounts.address = accounts_address"). Since the outer query already filters by ledger and the join correlates toaccounts_addressfrom the filtered moves, the inner ledger check is redundant.Similar pattern at line 111 with the metadata subquery. While not incorrect, this adds overhead.
These correlated subqueries could use
h.store.db.NewSelect()instead, relying on the correlation for ledger scope. However, usingnewScopedSelect()is more defensive and maintains consistency across the codebase.internal/storage/ledger/resource_accounts.go (2)
66-91: Note: Redundant ledger filtering in balance calculation filter.The
selectBalancequery at line 66 usesnewScopedSelect()and is correlated todataset.address. Since this subquery is used within a filter expression (line 88) where the dataset is already ledger-scoped, the inner ledger check is redundant.This could use
h.store.db.NewSelect()since the correlation todataset.addressalready provides ledger scope. However, the current approach is defensive and consistent with the broader refactoring.
122-153: Note: Redundant ledger filtering in volumes expansion.The
selectRowsQueryat line 122 usesnewScopedSelect()with a correlationWHERE("accounts_address in (select address from dataset)"). Since the dataset is already ledger-scoped, the inner ledger check is redundant.This correlated subquery could use
h.store.db.NewSelect(), relying on the correlation for ledger scope. However, maintainingnewScopedSelect()is more defensive and consistent.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (6)
internal/storage/ledger/resource_accounts.go(4 hunks)internal/storage/ledger/resource_aggregated_balances.go(3 hunks)internal/storage/ledger/resource_logs.go(1 hunks)internal/storage/ledger/resource_transactions.go(4 hunks)internal/storage/ledger/resource_volumes.go(4 hunks)internal/storage/ledger/store.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
internal/storage/ledger/resource_accounts.go (2)
pkg/features/features.go (1)
FeatureAccountMetadataHistory(22-22)internal/storage/common/resource.go (1)
JoinCondition(35-38)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Tests
- GitHub Check: Dirty
🔇 Additional comments (9)
internal/storage/ledger/resource_logs.go (1)
24-26: LGTM! Clean migration to scoped select.The change correctly replaces the direct database select with the new scoped select helper, removing the explicit ledger filter as intended. The query structure remains intact.
internal/storage/ledger/resource_transactions.go (2)
33-47: LGTM! Consistent migration to scoped select.The main transactions dataset correctly uses
newScopedSelect()to establish ledger-scoped querying. The column selections remain intact and the structure is preserved.
58-63: LGTM! Metadata history subquery correctly scoped.The transactions metadata history subquery properly uses the scoped select helper. The additional WHERE clause for the PIT filter is correctly applied alongside the scoped filtering.
internal/storage/ledger/resource_volumes.go (2)
37-65: LGTM! Volumes dataset correctly migrated to scoped select.The main volumes query properly uses
newScopedSelect()for both the base case and when additional filters require lateral joins with accounts. The structure and logic are preserved.
70-93: LGTM! PIT/OOT branch correctly uses scoped select.The moves-based query for point-in-time or out-of-time scenarios correctly applies the scoped select with appropriate date filtering. The aggregation logic remains intact.
internal/storage/ledger/resource_accounts.go (1)
29-55: LGTM! Accounts dataset correctly migrated to scoped select.The main accounts query properly uses
newScopedSelect()for both the base case and when metadata history is involved. The structure, column selections, and PIT filtering logic are preserved.internal/storage/ledger/resource_aggregated_balances.go (2)
26-74: LGTM! PIT branch correctly migrated to scoped select.The point-in-time query path properly uses
newScopedSelect()for the main moves query and all lateral-joined subqueries (accounts and accounts_metadata). The logic and structure are preserved, with appropriate date filtering for metadata history.
76-103: LGTM! Non-PIT branch correctly migrated to scoped select.The accounts_volumes query path properly uses
newScopedSelect()for both the main query and the lateral-joined accounts subquery. The conditional inclusion of metadata and address arrays is correctly maintained.internal/storage/ledger/store.go (1)
166-187: Optimization is validated and intentional—no further action needed.This is an established performance optimization (commit 04805c0, Nov 6 2025) deliberately designed to improve query plans for multi-ledger buckets. The developer chose the OR + bucket-check approach after considering the trade-off between uncorrelated subquery evaluation and improved index usage on the
bucketcolumn. The implementation is already integrated across all resource query handlers and working in production paths. The documented comment explains the rationale for the specific approach taken.