Skip to content

feat: generalize accounts prefiltering logic to any filter shape#1207

Open
Azorlogh wants to merge 13 commits intorelease/v2.2from
feat/v2.2/optimisation-aggregate-generalize
Open

feat: generalize accounts prefiltering logic to any filter shape#1207
Azorlogh wants to merge 13 commits intorelease/v2.2from
feat/v2.2/optimisation-aggregate-generalize

Conversation

@Azorlogh
Copy link
Contributor

@Azorlogh Azorlogh commented Dec 23, 2025

based on #1206

Makes the logic work for any filter, simplifying it & giving us more choice for when to enable/disable it


Open with Devin

flemzord and others added 11 commits December 22, 2025 17:44
- Introduced a more efficient method for filtering accounts based on address segments by using INNER JOIN instead of LATERAL JOIN.
- Enhanced the query logic to support multiple OR conditions for partial address filters.
- Added comprehensive tests for various scenarios involving partial address filters, including PIT and OOT conditions.
- Updated the query in resource_volumes.go to include the 'moves' table alias, resolving issues with missing aliases in specific query scenarios.
- Added a test case to validate the fix, ensuring proper functionality when filtering using PIT and metadata without a partial address filter.
- Updated the volume query logic in resource_aggregated_balances.go to ensure the 'moves' table alias is correctly applied when filtering using PIT and metadata without a partial address filter.
- Added a new test case in balances_test.go to validate the fix and ensure proper functionality in the specified scenario.
- Updated the query optimization in resource_aggregated_balances.go and resource_volumes.go to handle cases where partial address filters are combined with metadata filters, ensuring accurate results without excluding relevant accounts.
- Added new test cases in balances_test.go and volumes_test.go to validate the functionality of the updated query logic, specifically for scenarios involving $or conditions with partial addresses and metadata.
- Updated the query logic in resource_aggregated_balances.go and resource_volumes.go to optimize filtering with single partial address conditions, ensuring accurate results when combined with other filters.
- Added new test cases in balances_test.go and volumes_test.go to validate the functionality of the updated query logic, specifically for scenarios involving $or conditions with partial addresses and balance filters.
- Enhanced the handling of partial address filtering in resource_aggregated_balances.go and resource_volumes.go to ensure accurate results when combined with complex query operators.
- Updated the test cases in balances_test.go to reflect changes in expected results for address filtering scenarios.
- Introduced a new utility function to analyze query complexity, ensuring optimizations are applied safely without excluding relevant accounts.
- Modified test cases in balances_test.go and volumes_test.go to reflect changes in address filtering, ensuring accurate expectations for partial address queries.
- Enhanced utility functions in utils.go to clarify the semantics of address patterns, improving the handling of segment counts for filtering logic.
… edge cases

- Introduced new test cases in volumes_test.go to validate the handling of special characters and long patterns in account addresses.
- Added scenarios for UTF-8 characters, patterns with wildcards, and edge cases to ensure accurate filtering and matching behavior.
- Enhanced tests for complex query combinations, including $or and $and conditions, to verify expected results across various address formats.
…fixes

- Introduced a new test case in volumes_test.go to validate the behavior of $or queries when combining partial and exact address prefixes.
- Ensured that the tests cover scenarios with both matching and non-matching exact addresses, confirming correct volume retrieval.
- Enhanced parallel execution of tests for improved performance and reliability.
@Azorlogh Azorlogh requested a review from a team as a code owner December 23, 2025 14:37
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Dec 23, 2025

Walkthrough

Adds a skipFilter hook to the ledger repositoryHandler interface and implements it across handlers; refactors dataset construction in volumes and aggregated_balances to introduce address-based optimizations and conditional metadata handling for PIT/OOT queries; and adds extensive tests for complex address, metadata, and logical filter combinations.

Changes

Cohort / File(s) Summary
Interface & core builder
internal/storage/ledger/resource.go
Added skipFilter(query repositoryHandlerBuildContext[Opts]) bool to repositoryHandler; buildFilteredDataset now constructs a buildContext and applies Builder-based WHERE only when q.Builder != nil and skipFilter(buildContext) is false.
No-op handler implementations
internal/storage/ledger/resource_accounts.go, internal/storage/ledger/resource_logs.go, internal/storage/ledger/resource_transactions.go
Implemented skipFilter on each handler returning false (no behavioral change; satisfies new interface).
Aggregated balances (PIT optimizations)
internal/storage/ledger/resource_aggregated_balances.go
Added skipFilter and refactored buildDataset: introduced address-segment optimization, eligibleAccounts subquery join, conditional dateFilterColumn (insertion_date vs effective_date), optional metadata subquery joins, and alternate window-function path for non-optimized cases; preserved feature-flag checks.
Volumes (optimized & fallback flows)
internal/storage/ledger/resource_volumes.go
Added skipFilter with PIT/OOT awareness; implemented optimized branch that pre-filters eligible accounts and inner-joins moves (when MovesHistory enabled) plus metadata lateral joins; retained fallback aggregation path when optimization disabled; adjusted date constraint handling and address/metadata join logic.
Query helpers
internal/storage/ledger/utils.go
Added filterInvolvedAccounts(builder, addressColumnName) helper to route address/account predicates into filterAccountAddress via Builder/ContextFn pattern.
Tests — balances
internal/storage/ledger/balances_test.go
Added tests (≈+121 lines) covering PIT with metadata filters, OR/NOT combinations involving partial/exact addresses, and aggregated volume assertions exercising edge cases around account inclusion/exclusion.
Tests — volumes
internal/storage/ledger/volumes_test.go
Added extensive tests (≈+1008 lines) for PIT/OOT scenarios, multi-segment and wildcard address patterns, metadata existence/value checks, logical operator combinations, group/asset aggregations, UTF‑8 and edge cases, and a PIT metadata-regression case.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Poem

🐇 I dug a tunnel through the query tree,
Skipping tags where filters need not be,
PIT and volumes hop in line,
Addresses trimmed like vine,
Tests nibble edges — the ledger's glee 🥕

🚥 Pre-merge checks | ✅ 2 | ❌ 1
❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 6.67% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately describes the main change: generalizing accounts prefiltering logic to work with any filter shape, which aligns with the core refactoring across multiple resource handlers.
Description check ✅ Passed The PR description accurately describes the changeset: generalizing accounts prefiltering logic to work for any filter shape, simplifying implementation, and providing more control over optimization enablement.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ 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 feat/v2.2/optimisation-aggregate-generalize

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.

❤️ Share

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

@Azorlogh Azorlogh changed the base branch from feat/v2.2/optimisation-aggregate to main January 20, 2026 13:44
@Azorlogh Azorlogh changed the base branch from main to release/v2.2 January 20, 2026 13:44
@Azorlogh Azorlogh changed the title generalize accounts prefiltering logic to any filter shape feat: generalize accounts prefiltering logic to any filter shape Jan 21, 2026
Copy link
Contributor

@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: 5

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
internal/storage/ledger/utils.go (1)

26-53: Add nil guard and safe type assertion to prevent runtime panics.

filterInvolvedAccounts calls builder.Build() unconditionally and uses value.(string) without type checking. Throughout the codebase (resource.go, legacy/balances.go, legacy/volumes.go, legacy/logs.go), nil guards precede .Build() calls. Additionally, defensive type assertions are safer than direct casting.

♻️ Proposed fix
 func filterInvolvedAccounts(builder query.Builder, addressColumnName string) (string, []any, error) {
+	if builder == nil {
+		return "true", nil, nil
+	}
 	return builder.Build(query.ContextFn(func(property, operator string, value any) (string, []any, error) {
 		if property == "address" || property == "account" {
-			return filterAccountAddress(value.(string), addressColumnName), nil, nil
+			s, ok := value.(string)
+			if !ok {
+				return "", nil, fmt.Errorf("expected string for %s filter", property)
+			}
+			return filterAccountAddress(s, addressColumnName), nil, nil
 		} else {
 			return "true", nil, nil
 		}
 	}))
 }
🤖 Fix all issues with AI agents
In `@internal/storage/ledger/resource_aggregated_balances.go`:
- Around line 91-118: The optimized path building the select (inside the
useAddressOptimization branch creating ret via store.newScopedSelect()) applies
only the PIT filter (query.PIT) but omits the OOT filter (query.OOT); update
that branch to also apply the OOT constraint by adding a Where(...) using the
same date column variable (dateFilterColumn) and query.OOT when query.OOT is set
(similar to how resource_volumes.go handles both PIT and OOT), so ret includes
both Where("moves."+dateFilterColumn+" <= ?", query.PIT) and the appropriate OOT
Where clause (e.g., Where("moves."+dateFilterColumn+" >= ?", query.OOT) or the
correct operator used elsewhere) when needed.
- Around line 105-116: The metadata lateral join block guarded by
query.useFilter("metadata") (the subQuery built via store.newScopedSelect(),
DistinctOn("accounts_address"), ColumnExpr("first_value(metadata) ... as
metadata"), and the Join(`left join lateral (?) accounts_metadata on true`) plus
Column("metadata")) is dead because useAddressOptimization is defined as
needAddressSegments && !query.useFilter("metadata"), so when this optimization
path runs query.useFilter("metadata") is always false; remove this unreachable
metadata join and Column("metadata") addition from the optimized branch (i.e.,
delete the entire if query.useFilter("metadata") { ... } block inside the
useAddressOptimization path) to eliminate dead code.
- Around line 50-53: The skipFilter in
aggregatedBalancesResourceRepositoryHandler currently only checks
query.UsePIT(), causing the PIT-only optimization to miss OOT queries; update
aggregatedBalancesResourceRepositoryHandler.skipFilter (method on type
aggregatedBalancesResourceRepositoryHandler taking
repositoryHandlerBuildContext[ledgercontroller.GetAggregatedVolumesOptions]) to
mirror volumesResourceHandler.skipFilter by using query.UsePIT() ||
query.UseOOT() (i.e., change the first condition to include UseOOT) while
keeping the existing needAddressSegments and !query.useFilter("metadata") checks
so buildDataset optimization applies for both PIT and OOT.

In `@internal/storage/ledger/resource_volumes.go`:
- Around line 144-154: The metadata lateral join block guarded by
query.useFilter("metadata") inside resource_volumes.go is dead because the
surrounding useAddressOptimization path already enforces
!query.useFilter("metadata"); remove the entire metadata-handling block (the if
query.useFilter("metadata") { ... } that builds subQuery and appends the lateral
join to selectVolumes) to eliminate unreachable code and avoid confusion; ensure
no other references to the removed subQuery or accounts_metadata join remain in
the same function.
- Around line 187-197: The metadata lateral subquery in resource_volumes.go (the
block that runs when query.useFilter("metadata") and builds subQuery via
store.newScopedSelect() with DistinctOn/ModelTableExpr/ColumnExpr and is joined
into selectVolumes) lacks point-in-time filtering; update that subQuery to
include a Where("date <= ?", query.PIT) condition in addition to the existing
Where("accounts_metadata.accounts_address = moves.accounts_address") so the
lateral join only returns metadata as of the requested PIT. Also apply the same
fix to the optimized-path metadata handling block that builds/joins metadata
(the other code path that constructs a metadata subquery or array_agg and
attaches "(array_agg(metadata))[1] as metadata") to ensure consistent PIT/OOT
behavior.
🧹 Nitpick comments (1)
internal/storage/ledger/volumes_test.go (1)

1035-1055: Verify filter field name consistency.

This test uses "address" as the filter field name, while other tests use "account". Both should work since "account" is defined as an alias for "address" in volumesResourceHandler.filters(), but consider using consistent field names across tests for clarity.

Copy link

@devin-ai-integration devin-ai-integration bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Devin Review: No Issues Found

Devin Review analyzed this PR and found no potential bugs to report.

View in Devin Review to see 5 additional flags.

Open in Devin Review

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.

2 participants