Repository navigation
Conversation
…d listings (emdash-cms#1834) On SQLite/D1, a taxonomy-filtered listing ordered + paginated walks the whole collection in ORDER BY order applying the taxonomy filter as a correlated EXISTS per row. For a selective term (few matches) the LIMIT never fills early, so it scans the entire table — ~75k D1 rows read for a page returning one row. Adds an opt-in `taxonomyStrategy: "seek"` filter option that drives the query from content_taxonomies (materialized _matched CTE + CROSS JOIN, seeking by taxonomy_id and joining the collection by primary key). Default "scan" is unchanged and remains best for non-selective terms. SQLite-only; Postgres has statistics and is untouched. Also adds composite index content_taxonomies(taxonomy_id, collection, entry_id) (migration 051), which the seek plan relies on and which supersedes the single-column idx_content_taxonomies_term. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…onomy
The seek-strategy `_matched` CTE selected pivot entry ids without
DISTINCT. For a single taxonomy filtered by multiple slugs
(`{ category: ['news', 'sports'] }`), an entry tagged with more than one
slug had a pivot row per slug and no INTERSECT to dedupe it, so the outer
CROSS JOIN emitted the content row multiple times. The default EXISTS
scan is a semi-join and never duplicated.
Add DISTINCT to the per-taxonomy select and add seek-path correctness
tests covering OR-within-one-taxonomy (alone and combined with a second
taxonomy), cross-checked against the scan path.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Add a seek-path test covering a term shared across two collections, asserting the "post" listing stays scoped to posts and matches the scan path. Verifies the reviewer's cross-collection concern on emdash-cms#1843 does not manifest: the `+ct.collection` predicate keeps TEXT affinity (unary `+` is a value/type no-op in SQLite), so `_matched` never admits other collections' rows. Also switch three seek-strategy assertions from `Array#sort()` to `toSorted()` to clear the lint warnings introduced in 5840cf8. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The seek-path `_matched` CTE used `+ct.collection = ${type}` to disqualify
the pivot PK auto-index. The unary `+` forces only a single-column
`(taxonomy_id=?)` seek and applies `collection` as a residual filter, so a
term shared across collections reads that term's rows in *every*
collection, then discards the non-matching ones — a page returning one row
can read hundreds.
Replace it with an explicit `INDEXED BY idx_content_taxonomies_term_lookup`
plus a plain `ct.collection = ${type}` equality. This keeps the composite
index but seeks on both leading columns `(taxonomy_id=? AND collection=?)`,
reading only the queried collection's slice of the term. Measured against a
real D1 dataset, a shared selective term dropped from 704 rows read to 2.
Plain equality also removes the confusing unary `+`.
Tighten the plan test to assert the two-column seek so the residual-filter
form can't silently return.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Replace the manual `taxonomyStrategy: "scan" | "seek"` knob (never released publicly) with an automatic decision. The loader reads cached per-term entry counts that mirror the query's own collection/status/locale, then: - single term: seeks a selective term (drives from content_taxonomies via the composite index) or scans a non-selective one (LIMIT fills fast), chosen by a sqrt cost-model crossover; - multi-term AND: drives the seek from the smallest-count term and probes the rest as index-nested-loop EXISTS, retiring the INTERSECT (which could not reach the optimal plan). Counts are advisory — they change how rows are fetched, never which rows come back — and are cached under [content:<collection>, taxonomies] for free, correct invalidation. Postgres is unchanged (keeps EXISTS; it has statistics). Verified on real D1 (~24k published, ~76k pivot rows): a selective term reads 10 rows seeking vs 50,609 scanning; a ~50% term reads 68 scanning vs 51,099 seeking; multi-term smallest-first reads 14 vs 51,103. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
🦋 Changeset detectedLatest commit: 2b0d29f The changes in this PR will be included in the next version bump. This PR includes changesets to release 16 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Scope checkThis PR changes 821 lines across 9 files. Large PRs are harder to review and more likely to be closed without review. If this scope is intentional, no action needed. A maintainer will review it. If not, please consider splitting this into smaller PRs. See CONTRIBUTING.md for contribution guidelines. |
@emdash-cms/admin
@emdash-cms/auth
@emdash-cms/auth-atproto
@emdash-cms/blocks
@emdash-cms/cloudflare
@emdash-cms/contentful-to-portable-text
emdash
create-emdash
@emdash-cms/gutenberg-to-portable-text
@emdash-cms/plugin-cli
@emdash-cms/plugin-types
@emdash-cms/registry-client
@emdash-cms/registry-lexicons
@emdash-cms/sandbox-workerd
@emdash-cms/x402
@emdash-cms/plugin-ai-moderation
@emdash-cms/plugin-atproto
@emdash-cms/plugin-audit-log
@emdash-cms/plugin-color
@emdash-cms/plugin-embeds
@emdash-cms/plugin-field-kit
@emdash-cms/plugin-forms
@emdash-cms/plugin-webhook-notifier
commit: |
![emdashbot[bot]](https://avatars.githubusercontent.com/in/3255304?s=60&v=4){kind=small}
There was a problem hiding this comment.
I reviewed the auto-plan taxonomy-filtered listing PR. The approach is sound and well-scoped for #1834: it gives the SQLite/D1 loader an advisory selectivity signal, reuses the existing content:<collection> / taxonomies object-cache namespaces for invalidation, adds the composite pivot index via a forward-only migration, and leaves Postgres alone. It aligns with EmDash’s architecture.
One concrete bug needs fixing before merge: the cached term-counts shape uses a Map, but the object-cache codec does not preserve Map instances. Once an object-cache backend is configured, a cache hit deserializes terms to {}, so the planner silently skips the seek path and falls back to the slow scan, which is exactly the behavior this PR is trying to fix. I’ve inlined a minimal fix below.
I also flagged two smaller advisory issues: counts for status: "published" are time-sensitive via scheduled_at but the cache only invalidates on writes (not when a scheduled post becomes due), and the in-memory terms map is keyed by translation_group, which can overwrite per-slug counts for unlocalized queries with translated terms.
The migration, tests, changeset, SQL-safety, and locale/status scoping all look good otherwise. Note that the author’s checklist says the linked Discussion is still awaiting maintainer sign-off.
The cached CollectionTermCounts carried a Map, but the object-cache codec
is JSON-based and only preserves Date, so a cache hit deserialized `terms`
to `{}`. The seek planner gates on `terms.size > 0` (undefined on a plain
object), so a hit silently reverted to the slow scan the feature exists to
avoid. Cache a serializable Record and rebuild the Map at the boundary.
Add a regression test that forces the codec round-trip (memory backend +
two reads) — the existing taxonomy tests ran with no backend, so cachedQuery
short-circuited to load() and never crossed the codec.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
github-actions
Bot
removed
the
review/needs-review
github-actions
Bot
added
the
review/needs-rereview
There was a problem hiding this comment.
This is a sound, well-scoped change for #1834. The author has fixed my earlier blocking issue: term counts now survive an object-cache round-trip by storing a plain Record and rebuilding the Map at the cache boundary, with a dedicated regression test. The composite pivot index in migration 051, the EXPLAIN QUERY PLAN integration tests, and the locale/status-scoped count tests all look correct, and SQL construction uses sql.ref / parameters.
One concrete invalidation gap remains: term assignment / detachment in TaxonomyRepository only bumps the taxonomies object-cache namespace, but the new per-collection count cache lives under content:<collection>. After attaching or detaching a term, the cached selectivity counts stay stale until the next content write or the cache TTL, which directly undermines the plan-quality goal of this PR. The PR description claims attach → content:X, but the code path doesn't do that yet.
I also still see the two advisory items from my prior review:
status: "published"counts includescheduled_at <= now()but only invalidate on writes, so the plan can drift as scheduled posts go live.- The in-memory/in-cache
termsmap is keyed bytranslation_group, which overwrites per-slug variants for unlocalized queries with translated terms.
Please close the attach/detach invalidation gap before merge. The Discussion sign-off is also still pending, which the maintainer should confirm separately.
Findings
-
[needs fixing]
packages/core/src/database/repositories/taxonomy.ts:358Term assignment changes per-collection term counts, but
setTermsForEntryonly invalidates thetaxonomiesnamespace. The new count cache (getTermCountsForCollection) stores values undercontent:<collection>, so attaching/detaching terms leaves that cache stale until a content write or the TTL expires. The same applies toattachToEntry,detachFromEntry, andclearEntryTerms.Add
invalidateCollectionCache(collection)alongside the taxonomy invalidation:import { invalidateCollectionCache, invalidateTaxonomyObjectCache } from "../../object-cache/index.js"; ... if (toRemove.length > 0 || toAdd.length > 0) { invalidateCollectionCache(collection); invalidateTaxonomyObjectCache(); }
-
[suggestion]
packages/core/src/loader.ts:431The serializable
termsmap is keyed bytranslation_group. For a locale-less query, a term that has translations in multiple locales will emit multipleGROUP BYrows with the sametranslation_groupbut differentname/slug; the last row wins, so one slug/count overwrites the other. That makes the per-slugbySluglookup incomplete for unlocalized queries. Consider keying by a composite such asname\0slug(mirroring the planner lookup) or storing an array of variants per group. -
[suggestion]
packages/core/src/loader.ts:403buildStatusCondition(db, status, "e")forstatus === "published"includesscheduled_at <= now(). Because the count cache is invalidated only on writes, counts drift as scheduled posts cross their publish time. Since counts are only advisory this is a plan-quality issue, not a correctness issue, but it's worth documenting or capping the TTL for published-status counts.
Adds a regression test proving the term-count cache (keyed [content:<collection>, taxonomies]) is invalidated when a term is attached: the taxonomies epoch bump alone suffices because a cache hit requires every namespace epoch to match. Locks in the invariant so a future change can't leave selectivity counts stale after a term write. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
github-actions
Bot
removed
the
review/needs-rereview
|
Thanks for the re-review. Responses to the three items: Attach/detach invalidation gap [needs fixing] — this is a false positive; no change needed. The count cache is keyed under two namespaces:
Verified empirically and pinned it as a regression test in Published-status
|
github-actions
Bot
added
the
review/needs-rereview
There was a problem hiding this comment.
The blocking invalidation gap from my prior review is closed: the count cache is stamped under both content:<collection> and taxonomies, and the new regression test shows that a taxonomy write (which bumps the taxonomies namespace) correctly evicts it. The composite pivot index, the SQLite-only seek rewrite, the EXPLAIN-plan integration tests, and the locale/status/object-cache test coverage are all sound.
No correctness bugs remain. Three plan-quality advisory items survive this revision:
loadTermCountsForCollectiongroups bytranslation_group, name, slugbut stores the result in aRecordkeyed only bytranslation_group, so an unlocalized query overwrites per-slug variants. ThebySluglookup used by the planner can then miss a slug or use the wrong variant's count, making the seek/scan decision less accurate for multilingual sites queried without a locale.- Published counts include
scheduled_at <= now(), but the cache only invalidates on writes; as scheduled posts go live the count drifts from the query it is supposed to mirror. Since counts are advisory this only affects plan choice. - For a multi-slug filter within one taxonomy, the planner sums per-slug counts. If slugs overlap on the same entries, the summed count overstates the filter's true selectivity and can keep the
EXISTSscan when driving from the smallest distinct set would be cheaper.
Also note: the PR description says term attach/detach invalidates content:<collection>, but the code path relies on the folded taxonomies namespace instead. The practical effect is the same, so this is a documentation/implementation detail rather than a bug.
Finally, the PR checklist notes the linked Discussion is still awaiting maintainer sign-off; that should be confirmed before merge.
github-actions
Bot
added
review/awaiting-author
|
Closing in favor of a new approach |
What does this PR do?
On SQLite/D1, a taxonomy-filtered collection listing (
getEmDashCollection("posts", { where: { category }, orderBy, limit })) applies the taxonomy filter as a correlatedEXISTSon aSELECT * FROM ec_<collection> ... ORDER BY ... LIMIT ?. A stats-blind D1 planner drives from the collection's order index and probes the taxonomy per row. For a selective term theLIMITnever fills, so the query walks the whole collection — tens of thousands of D1 rows read for a page returning a handful (#1834). rows-read is the billed/throttled unit on D1, and the planner can't self-correct because choosing the fast plan needs the term's selectivity, which D1 doesn't track (sqlite_stat1is never maintained).This gives the loader that one missing fact — cached per-term entry counts — and lets it plan automatically:
content_taxonomies, seek the composite pivot index, join the collection by PK, sort the small candidate set) or keep theEXISTSscan for a non-selective one (theLIMITfills fast). The choice is a√(pageRows · total)cost-model crossover.EXISTS— theO(|small|·log|big|)plan.The counts inherit the query's own
collection+status+locale, so a term's count equals the rows that plan would actually fetch (a draft query counts drafts, a French query counts French, a single collection counts only its slice) — no fixed "published" semantics imposed. They're advisory: they change how rows are fetched, never which rows come back (seek and scan return identical results, tested). The map lives in the object-cache under[content:<collection>, taxonomies], so the existing epoch graph invalidates it for free on the writes that move a count (content create/delete/status-change/attach →content:X; term rename/delete →taxonomies) — no new table, no write-path hooks. It's keyed bytranslation_group(carrying name/slug/count + a collection total) so a future public term-count API can reuse the same load.Migration 051 adds the composite index
content_taxonomies(taxonomy_id, collection, entry_id); the seek forces it (INDEXED BY) with a plainct.collection = ?equality so it seeks on both leading columns, reading only the queried collection's slice even for a term shared across collections. Postgres is unchanged (keepsEXISTS; it has statistics).Verified against a real D1 dataset (~24k published rows in the collection, ~76k
content_taxonomiesrows), rows-read:The cost-model threshold picks correctly in both real single-term cases with margin (
√(21·24383) ≈ 716, between 2 and ~12,305).Out of scope (fast follow): the admin term-cloud / archive-nav counts still use the older status-agnostic, cross-collection aggregate that is only invalidated by taxonomy writes (not content writes). Rewiring those to this published, correctly-invalidated per-collection map is a separate change — it's a visible behavior change (public counts would drop drafts) and needs cross-collection aggregation.
Discussion: #1851
Closes #1834
Type of change
Checklist
pnpm typecheckpassespnpm lintpasses (0 diagnostics)pnpm testpasses (targeted: loader / query / taxonomy suites — 99 loader/query + 151 taxonomy tests green, both dialects)pnpm formathas been runAI-generated code disclosure
Screenshots / test output
tests/integration/loader-taxonomy-filter-plan.test.ts—EXPLAIN QUERY PLANon stats-blind SQLite: a selective term auto-seeks the composite index on both leading columns; a non-selective term keeps theEXISTSscan; a multi-term AND drives the seek from the smallest-count term.tests/unit/loader-term-counts.test.ts(new) — count correctness: status-follows-query (drafts vs published), per-locale, per-collection scoping.tests/unit/loader-taxonomy-filter.test.ts— OR-within dedupe, AND-across, localized-slug, shared-collection scoping, and a forced scan-path case, all via the auto-planner.Note:
scripts/query-counts.snapshot.*shows a pre-existing unrelated drift on this branch (CROSS JOIN → JOINin folded hydration, on non-taxonomy routes too); this change's real delta is the+1countGROUP BYon taxonomy-filtered routes. CI regenerates the snapshot.🤖 Generated with Claude Code