Skip to content

Conversation

@gfyrag
Copy link
Contributor

@gfyrag 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

@gfyrag gfyrag requested a review from a team as a code owner November 6, 2025 11:59
@coderabbitai
Copy link

coderabbitai bot commented Nov 6, 2025

Walkthrough

The PR refactors database query construction across multiple resource types in the ledger storage layer. A new newScopedSelect() helper method is introduced that replaces simple ledger equality predicates with a conditional WHERE clause using a subquery to check if a ledger is the only one in its bucket. All five resource files are updated to use this new scoped select method instead of direct query builders, removing explicit ledger filters throughout their query paths.

Changes

Cohort / File(s) Summary
New scoped select helper
internal/storage/ledger/store.go
Adds new newScopedSelect() method that constructs a conditional WHERE clause via a subquery (checkLedgerAlone) joining _system.ledgers to check if current ledger is the only one in its bucket, avoiding simple ledger equality predicates for better PostgreSQL query plans
Resource query refactoring
internal/storage/ledger/resource_accounts.go, resource_aggregated_balances.go, resource_logs.go, resource_transactions.go, resource_volumes.go
All five resource files updated to replace h.store.db.NewSelect() with h.store.newScopedSelect() across BuildDataset and related query paths; explicit Where("ledger = ?", ...) filters removed from multiple query segments while preserving existing join structures and return values

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

  • newScopedSelect() logic: Verify the subquery construction and conditional WHERE clause correctly prevent query plan degradation without altering filtering semantics
  • Consistency across resource files: Confirm all five resource files apply the scoped select pattern uniformly and that no query paths were missed
  • Functional correctness: Ensure removal of explicit ledger filters doesn't inadvertently change dataset filtering or aggregation behavior in any code path
  • Subquery relationships: Validate that metadata joins, address filtering, and account relationships still function properly through the scoped context

Poem

🐰 Query plans were struggling, PostgreSQL would sigh,

So we scope and bucket-check as ledgers multiply,

No more simple WHERE clauses plain,

A conditional dance—performance gained! ✨

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The PR title 'backport: feat/conditional-where-on-selections' accurately describes the main changes: introducing conditional WHERE clauses in query selections and optimizing ledger query scoping.
Description check ✅ Passed The PR description directly relates to the changeset: it mentions adding a utils function for select queries and optimizing queries for ledgers, which aligns with the introduction of newScopedSelect() and removal of explicit ledger filters.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch backport/v2.3/conditional-where-on-selections

Comment @coderabbitai help to get the list of available commands and usage tips.

@codecov
Copy link

codecov bot commented Nov 6, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 81.76%. Comparing base (1d90239) to head (04805c0).
⚠️ Report is 2 commits behind head on release/v2.3.

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.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Copy link

@coderabbitai coderabbitai bot left a 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 scope

This 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 accountsQuery at line 95 uses newScopedSelect() with a correlation WHERE("accounts.address = accounts_address"). Since the outer query already filters by ledger and the join correlates to accounts_address from 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, using newScopedSelect() 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 selectBalance query at line 66 uses newScopedSelect() and is correlated to dataset.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 to dataset.address already 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 selectRowsQuery at line 122 uses newScopedSelect() with a correlation WHERE("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, maintaining newScopedSelect() 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.

📥 Commits

Reviewing files that changed from the base of the PR and between 663a3d0 and 04805c0.

📒 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 bucket column. 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.

@gfyrag gfyrag merged commit ce21381 into release/v2.3 Nov 6, 2025
8 of 9 checks passed
@gfyrag gfyrag deleted the backport/v2.3/conditional-where-on-selections branch November 6, 2025 13:49
@gfyrag gfyrag mentioned this pull request Nov 10, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants