Repository navigation
fix(core): drive the taxonomy pivot query from the most selective taxonomy, not the first where key - #3266
fix(core): drive the taxonomy pivot query from the most selective taxonomy, not the first where key#3266eisenbruch wants to merge 6 commits into
Conversation
🦋 Changeset detectedLatest commit: 7db3e96 The changes in this PR will be included in the next version bump. This PR includes changesets to release 12 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 |
@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-test
@emdash-cms/plugin-types
@emdash-cms/registry-client
@emdash-cms/registry-lexicons
@emdash-cms/registry-loader
@emdash-cms/registry-moderation
@emdash-cms/registry-verification
upgrade-emdash
@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.
The approach is sound: reordering the taxonomy sets so the most selective one drives the pivot seek fixes the key-order dependency described in #3259 and keeps results identical. The code is otherwise correct — the count query is parameterized through Kysely and the locale-agnostic pivot rows mean omitting a locale filter is appropriate.
The main concern is that the "optimization" introduces one extra round-trip per additional taxonomy filter on the public-collection hot path, which is exactly what EmDash’s performance conventions warn against ("one query beats two" and "the logged-out hot path only ratchets down"). The selectivity probe should be batched into a single GROUP BY taxonomy_id count instead of Promise.all per set. The new regression test also only asserts result equality, which was already true before the fix; it does not protect against the reported rows-read regression. The changeset and the new inline comment are usable but drift toward describing internal query mechanics rather than the user-facing behavior.
I did not run the test suite, builds, or query-count tooling; those claims are unverified.
| if (groupSets.length > 1) { | ||
| const sizes = await Promise.all( | ||
| groupSets.map((groups) => | ||
| db | ||
| .selectFrom("content_taxonomies") | ||
| .select((eb) => eb.fn.countAll<number>().as("n")) | ||
| .where("collection", "=", type) | ||
| .where("taxonomy_id", "in", groups) | ||
| .executeTakeFirst() | ||
| .then((r) => Number(r?.n ?? 0)), | ||
| ), | ||
| ); | ||
| const ordered = groupSets |
There was a problem hiding this comment.
[needs fixing] The new selectivity probe issues one query per taxonomy set in Promise.all. On a logged-out collection route filtered by two or more taxonomies, this adds N extra round-trips to D1 for every request that takes this path, partially undermining a PR whose goal is to reduce query cost. EmDash’s conventions prefer batching over multiple similar queries.
Use a single GROUP BY taxonomy_id count over the union of all sets, then sum per set locally:
| if (groupSets.length > 1) { | |
| const sizes = await Promise.all( | |
| groupSets.map((groups) => | |
| db | |
| .selectFrom("content_taxonomies") | |
| .select((eb) => eb.fn.countAll<number>().as("n")) | |
| .where("collection", "=", type) | |
| .where("taxonomy_id", "in", groups) | |
| .executeTakeFirst() | |
| .then((r) => Number(r?.n ?? 0)), | |
| ), | |
| ); | |
| const ordered = groupSets | |
| if (groupSets.length > 1) { | |
| const allGroups = groupSets.flat(); | |
| const rows = await db | |
| .selectFrom("content_taxonomies") | |
| .select(["taxonomy_id", (eb) => eb.fn.countAll<number>().as("n")]) | |
| .where("collection", "=", type) | |
| .where("taxonomy_id", "in", allGroups) | |
| .groupBy("taxonomy_id") | |
| .execute(); | |
| const countById = new Map(rows.map((r) => [r.taxonomy_id, Number(r.n ?? 0)])); | |
| const sizes = groupSets.map((groups) => | |
| groups.reduce((sum, g) => sum + (countById.get(g) ?? 0), 0), | |
| ); | |
| const ordered = groupSets | |
| .map((groups, i) => ({ groups, n: sizes[i] })) | |
| .toSorted((a, b) => a.n - b.n) | |
| .map((x) => x.groups); | |
| groupSets.splice(0, groupSets.length, ...ordered); | |
| } |
| // The pivot query walks the FIRST set through `content_taxonomies` and checks the rest with a | ||
| // correlated EXISTS per candidate row, so the rows read depend on which set drives. Until now that | ||
| // was the caller's first `where` key: the same query read 3,131 rows one way round and 539 the | ||
| // other. Drive from the set with the fewest pivot rows; the counts are an index-only read. |
There was a problem hiding this comment.
[suggestion] This comment reads like the PR description (specific observed row counts, historical ordering behavior) rather than a concise invariant for future readers. AGENTS.md asks comments to explain why the code must do something, not to summarize the change or PR.
| // The pivot query walks the FIRST set through `content_taxonomies` and checks the rest with a | |
| // correlated EXISTS per candidate row, so the rows read depend on which set drives. Until now that | |
| // was the caller's first `where` key: the same query read 3,131 rows one way round and 539 the | |
| // other. Drive from the set with the fewest pivot rows; the counts are an index-only read. | |
| // The pivot query walks the first set through content_taxonomies and | |
| // correlates the rest with an EXISTS per candidate row, so choose the | |
| // smallest set to minimize rows read. |
| expect(titles).toContain("Sports + Featured"); | ||
| }); | ||
|
|
||
| it("gives the same result whichever taxonomy is written first (#3259)", async () => { |
There was a problem hiding this comment.
[suggestion] This test asserts that both key orders return the same IDs, but #3259 reports a rows-read asymmetry, not a correctness bug — the result set was already identical before the fix. A test that cannot fail on the reported regression gives false confidence. Consider either instrumenting rows_read via the DB (e.g., D1 meta.rows_read, or an EXPLAIN/plan-cost assertion on SQLite) or removing this case if measuring the plan is impractical in unit tests.
| "emdash": patch | ||
| --- | ||
|
|
||
| A collection query filtered by two or more taxonomies now drives the pivot query from the taxonomy with the fewest tagged entries instead of whichever key the caller wrote first. Results are unchanged; on a large collection the rows read no longer depend on key order (3,131 versus 539 for the same query in one measurement). |
There was a problem hiding this comment.
[suggestion] The changeset describes the internal “pivot query” mechanics rather than leading with the user-facing behavior. Per .changeset/README.md, readers should be able to tell whether the release affects them without knowing the loader’s internal query shape.
| A collection query filtered by two or more taxonomies now drives the pivot query from the taxonomy with the fewest tagged entries instead of whichever key the caller wrote first. Results are unchanged; on a large collection the rows read no longer depend on key order (3,131 versus 539 for the same query in one measurement). | |
| A collection query filtered by two or more taxonomies now evaluates those filters in the order that minimizes rows read, instead of the order they were written in `where`. Results are unchanged; on large collections the rows read no longer depend on key order (3,131 versus 539 for the same query in one measurement). |
|
Agreed on "one query beats two", and done in On the test: it asserts result equality across key orders, which is the contract, and I have not added a rows-read assertion because the unit harness has no statement or row counter to assert against (the figures in #3259 are D1's own |
050cbb9 to
d7afc6c
Compare
|
Rebased onto The statement
The |
…onomy, not the first where key
The selectivity probe ran one COUNT per taxonomy set; a GROUP BY over all the sets' ids is one round trip on the public render path.
d7afc6c to
df2f562
Compare
|
Brought up to date with |
github-actions
Bot
added
review/needs-rereview
github-actions
Bot
removed
the
ci:run
What does this PR do?
buildTaxonomyPivotQuerywalksgroupSets[0]throughcontent_taxonomiesand checks every other set with a correlatedEXISTSper candidate row.groupSetsfollowed the caller'swherekey order, so the rows read depended on how the object was written: the same listings query read 3,131 rows as{ business_type, places }and 539 as{ places, business_type }(D1meta.rows_read, #3259).With two or more taxonomy filters, the resolved term groups are now counted in
content_taxonomies (collection, taxonomy_id)(an index-only read per set) and sorted ascending, so the most selective taxonomy drives. One taxonomy is untouched. Results are identical; only the plan changes.The second half of #3259 (any field filter abandons the pivot path) is the documented limitation in the comment above this branch and is not changed here.
Tests: a new key-order test (many posts in the category, one with the tag) asserts both orders return the same ids; the existing two-taxonomy tests still pass.
Refs #3259.
Type of change
Checklist
pnpm typecheckpasses (core)pnpm lintpassespnpm testpasses (loader-taxonomy-filter.test.tsandloader-field-filters.test.ts, 23 of 23)pnpm formathas been runAI-generated code disclosure
Screenshots / test output