Skip to content

perf(macros): gate sargable multi-filter matrix behind opt-in - #168

Merged
nicolasburtey merged 1 commit into
mainfrom
perf/gate-sargable-filters
Jul 30, 2026
Merged

perf(macros): gate sargable multi-filter matrix behind opt-in#168
nicolasburtey merged 1 commit into
mainfrom
perf/gate-sargable-filters

Conversation

@nicolasburtey

@nicolasburtey nicolasburtey commented Jul 30, 2026

Copy link
Copy Markdown
Member

Problem

The sargable per-state/per-combination SQL matrix added in #162 emits one sqlx::query! per filter combination × cursor state × direction. For an entity with N list_for columns that grows as 2^N (3^N with optional columns), and across many repos this exploded downstream compile times.

Measured impact on lana-bank (the build-rc-release CI job)

cc9f625fc (the es-entity 0.11.8 → 0.11.9 bump) is the only code change between CI build 67 (last fast) and build 68 (first slow):

builds wall time
49–67 (es-entity 0.11.8) 34–45 min (median ~37)
68+ (es-entity 0.11.9) 49–59 min (median ~54)

The bump is a pure dep bump + regenerated .sqlx cache (no .rs changes in lana-bank), and it took lana-bank's offline sqlx query cache from 1319 → 3373 descriptors (+2054, ×2.5). With the deps layer fetched from cache (apples-to-apples), the lana-cli-release release compile phase went 32m44s → 48–51 min. Each new descriptor is a sqlx::query! that must be compile-time-checked and LLVM-optimized in release — that is the ~16 min.

Fix

Make the multi-filter specialization matrix opt-in via a new #[es_repo(..., sargable_filters)] attribute (default off). With it off, list_for_filters emits only the existing catch-all COALESCE query (pre-#162 behavior).

Importantly, the cheap queries stay sargable regardless of the flag:

  • no-filter (list_by_{sort}) — always sargable
  • single-filter (list_for_{col}_by_{sort}) — always sargable, O(N)

Only the multi-filter combination matrix (the 2^N explosion) is gated. So repos opt in only when their multi-filter list queries are genuinely hot paths:

#[es_repo(
    entity = "Transfer",
    columns(
        account_id(ty = "AccountId", list_for(by(created_at))),
        status(ty = "String", list_for(by(created_at))),
    ),
    sargable_filters,
)]
pub struct Transfers { pool: PgPool }

Changes

  • options/mod.rs: add sargable_filters: Option<bool> (default false) + accessor
  • list_for_filters_fn.rs: short-circuit is_specialized_combo when disabled
  • existing specialization tests opt in (sargable_filters: true)
  • new test proving default-off emits the catch-all only (fewer es_query calls, COALESCE present) while opt-in emits the full specialized matrix (no COALESCE)
  • book/src/repo-list-for-filters.md: document the attribute + the compile-time tradeoff

Verification

  • 96 macro unit tests pass (cargo test -p es-entity-macros --lib)
  • cargo build -p es-entity --features event-context,tracing-context,instrument clean
  • Integration tests (tests/sargable_list_queries.rs) opt in to keep exercising the matrix

Predicted downstream impact

A conservative classifier flags ≥1010 of the 3373 lana-bank descriptors as multi-predicate (definitely removed); in practice the gate removes the entire multi-filter matrix, bringing the descriptor count back toward the 1319 baseline and recovering most of the ~16 min. (The single-filter and no-filter queries that 0.11.9 added for new columns remain, so the count won't be exactly 1319.

Notes

  • This is a behavior change: consumers that relied on the matrix being on by default will silently fall back to the catch-all for multi-filter queries. Correctness is preserved (same results); only index-sargability for multi-filter queries is lost unless they opt in. Given the compile-time tax, opt-in is the safer default.
  • Versioning handled by the repo's release CI.
    EOF
    )

Note

Medium Risk
Default-off changes runtime SQL for multi-filter list_for_filters (COALESCE vs specialized plans)—correctness preserved but index usage may regress until consumers opt in. Macro/API default is a silent behavior shift for anyone who depended on the matrix without setting the flag.

Overview
Multi-filter list_for_filters codegen is now opt-in via sargable_filters on #[es_repo(...)] (default off). Without it, only the catch-all COALESCE SQL is emitted for two-or-more active filters—same results as before #162’s per-combination matrix, but far fewer sqlx::query! descriptors at compile time.

No-filter (list_by_*) and single-filter (list_for_*_by_*) sargable queries are still generated unconditionally; only the exponential multi-filter combination matrix is gated.

Repos that need index-friendly multi-filter lists (e.g. hot UI table paths) set sargable_filters on the repo attribute; the book documents the compile-time tradeoff. Macro tests and tests/sargable_list_queries opt in so the full matrix stays covered in CI.

Reviewed by Cursor Bugbot for commit 07d319d. Bugbot is set up for automated code reviews on this repo. Configure here.

@nicolasburtey
nicolasburtey marked this pull request as ready for review July 30, 2026 18:20
The sargable per-state/per-combination SQL matrix introduced in #162
emits one sqlx::query! per filter combination × cursor state ×
direction. For an entity with N list_for columns that grows as 2^N
(3^N with optional columns). Across many repos this exploded lana-bank's
.sqlx offline cache from 1319 → 3373 query descriptors (+2054), adding
~16 min of release LLVM codegen to every CI build (build-rc-release
went from ~37 min to ~54 min at the 0.11.9 bump).

Make the multi-filter specialization matrix opt-in via a new
`#[es_repo(..., sargable_filters)]` attribute, defaulting to off. With
it off, list_for_filters emits only the existing catch-all COALESCE
query (pre-#162 behavior); single-filter (list_for_{col}_by_{sort}) and
no-filter (list_by_{sort}) queries stay sargable and cheap (O(N)), so
they are generated regardless of the flag.

Only repos whose multi-filter list queries are hot paths need to opt in.

- options: add `sargable_filters: Option<bool>` (default false) + accessor
- list_for_filters_fn: short-circuit is_specialized_combo when disabled
- tests: existing specialization tests opt in; add a test proving
  default-off emits the catch-all only (fewer es_query calls, COALESCE
  present) while opt-in emits the full specialized matrix
- book: document the attribute and the compile-time tradeoff
@nicolasburtey
nicolasburtey force-pushed the perf/gate-sargable-filters branch from b16d894 to 07d319d Compare July 30, 2026 18:22
@nicolasburtey

Copy link
Copy Markdown
Member Author

@cursor review

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

✅ Bugbot reviewed your changes and found no new issues!

Comment @cursor review or bugbot run to trigger another review on this PR

Reviewed by Cursor Bugbot for commit 07d319d. Configure here.

@nicolasburtey
nicolasburtey merged commit dfa753f into main Jul 30, 2026
7 checks passed
@bodymindarts

Copy link
Copy Markdown
Member

Should this be a per-repo setting? Not a global compile time flag?

@nicolasburtey

Copy link
Copy Markdown
Member Author

It is per-repo already — it's an attribute on the #[es_repo(...)] derive, not a cargo feature flag. Each repo decides independently:

#[es_repo(
    entity = "Transfer",
    columns(
        account_id(ty = "AccountId", list_for(by(created_at))),
        status(ty = "String", list_for(by(created_at))),
    ),
    sargable_filters,   // ← opt in for this repo only
)]
pub struct Transfers { pool: PgPool }

Default is off, so a repo that doesn't set it emits only the catch-all COALESCE query for the multi-filter case. There's nothing in any Cargo.toml [features] — it's parsed straight off the derive in RepositoryOptions (options/mod.rs) and read by ListForFiltersFn::is_specialized_combo.

So you can keep the matrix on for the handful of repos whose multi-filter list queries are hot paths (e.g. Transfers above) and leave it off everywhere else.

nicolasburtey added a commit to GaloyMoney/job that referenced this pull request Jul 30, 2026
* chore(deps): bump es-entity to 0.11.11

Rebuild against es-entity 0.11.11 (GaloyMoney/es-entity#168), which gates
the sargable multi-filter query matrix behind an opt-in per-repo attribute
(default off). Backward compatible.

* chore(deps): rebuild sqlx cache for es-entity 0.11.11

es-entity 0.11.11 gates the sargable multi-filter query matrix behind an
opt-in per-repo attribute, defaulting to off. The EsRepo macro now
emits a different (smaller) set of queries for JobRepo, so the cached
query hashes no longer match and offline builds fail with:

  error: `SQLX_OFFLINE=true` but there is no cached data for this query

Regenerate the cache via `cargo sqlx prepare --workspace` against a DB
with the latest migration applied.
nicolasburtey added a commit to GaloyMoney/cala that referenced this pull request Jul 30, 2026
* chore(deps): bump es-entity to 0.11.11

Rebuild against es-entity 0.11.11 (GaloyMoney/es-entity#168), which gates
the sargable multi-filter query matrix behind an opt-in per-repo
attribute (default off). Backward compatible; cala's EsRepo repos emit
the catch-all COALESCE query for the multi-filter case unless a repo opts
in with #[es_repo(..., sargable_filters)].

* chore(deps): bump job to 0.6.36 and rebuild sqlx cache for es-entity 0.11.11

es-entity 0.11.11 gates the sargable multi-filter query matrix behind an
opt-in per-repo attribute (default off), so the EsRepo derive now emits a
different (smaller) set of list queries. The committed .sqlx cache (built
against es-entity 0.11.9) and the job 0.6.35 dependency both carried stale
query hashes, causing offline builds to fail with:

  error: `SQLX_OFFLINE=true` but there is no cached data for this query

- bump job 0.6.35 -> 0.6.36 (rebuilt against es-entity 0.11.11)
- regenerate cala-ledger/.sqlx via `cargo sqlx prepare -- --all-features`

* chore: replace .err().expect() with expect_err (new clippy err_expect lint)

Rust 1.93 clippy (stable toolchain) flags .err().expect() as err_expect;
the workspace-clippy check runs with --deny warnings, so resolve the two
pre-existing occurrences in account_set tests.

* chore(deps): sync vendored job_setup migration with job 0.6.36

job 0.6.36 modified the released 20250904065521_job_setup.sql migration
(added autovacuum_vacuum_cost_delay = 0 to unthrottle autovacuum on
job_executions). cala vendors this migration so its own sqlx::migrate!()
is self-contained, but job 0.6.36 also runs its migrator against the same
DB. With cala still on the old checksum and job on the new one, the second
migrator fails:

  Error: migration 20250904065521 was previously applied but has been modified

Sync cala-ledger/migrations/20250904065521_job_setup.sql to match job 0.6.36
exactly.
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