Skip to content

fix(orm): index-lower pipeline union and flatMap stage wheres - #449

Merged
zbeyens merged 6 commits into
mainfrom
fix/orm-pipeline-where-index-lowering
Sep 7, 2026
Merged

zbeyens merged 6 commits into
mainfrom
fix/orm-pipeline-where-index-lowering

Conversation

@MikeyZhang75

@MikeyZhang75 MikeyZhang75 commented Sep 6, 2026 •

Copy link
Copy Markdown
Collaborator
  • Auto release

🐛 Fixes #447

🧭 Task plan: docs/plans/447-orm-pipeline-where-index-lowering.md

🟡 80-94% confidence: core cases verified; four P2 limitations deferred.

Phase 🧪 Tests 🌐 Browser
Reproduced 🔴 Union 21 reads; flatMap 21 reads ➖ N/A
Verified 🟢 41/41 pipeline; package build; full bun check; P0/P1 review clean 🟢 Filters, pagination and API docs: desktop and 390px

✅ Outcome

Object filters in union sources and relation-targeted flatMap stages narrow compatible index reads. The regressions read 1 and 3 documents respectively. Each callback filter resolves once, and the same expression controls the read and filtering.

⚠️ Caveat

Lowered flatMap and single-source union ranges follow the selected index order; existing cursors for those queries are incompatible. Auto release is disabled for this merge.

Four P2 findings are deferred: implicit creation-time suffix lowering, interleave-aware index selection, narrowing a pinned index when a shorter index competes, and the union cursor changeset classification. For custom interleave ordering, pin the compatible index and range explicitly; the implicit selection can throw. These remain open review threads.

🏗️ Design

Pipeline reads use the shared query-plan compiler. Explicit ranges remain scoped, and union lowering preserves the order required by interleave. FlatMap selects only indexes whose leading fields match the relation keys. Complete predicates still filter the resulting streams.

🧪 Verified

  • 41 pipeline tests, including callback single-resolution, compatible/incompatible pinned indexes, pinned ranges and interleave fallback.
  • Package build, source/mirror agreement, lint and full repository check.
  • All three docs routes rendered on desktop and mobile without page overflow or captured console errors. Walkthrough waived by the user.
  • Branch autoreview through P1: no accepted/actionable findings.

`_buildUnionSourceStream` and `_applyFlatMapStage` built raw streams and
post-filtered, so an object `where` there never reached the plan compiler the
chain-level read uses. 21 documents scanned to return 1 (union) and 2
(flatMap); the same `where` on `findMany` scanned 1.

Union sources compile through `_compileQueryPlan` + `_buildPlanStream`, keeping
the existing rule that a compiled plan may not displace a pinned index range,
and falling back to the unanchored read when the lowered one cannot supply
`interleaveBy`. flatMap stages take an index that extends the join keys, chosen
once outside the mapper so `mappedIndexFields` keeps one arity. The `where`
stays applied in JS either way.

`_assertWhereIndexRequirement` compiled a `whereExpression` and returned past
it; it is now predicate-only, and names an anchor each call site can actually
add. flatMap has no `.withIndex(...)` option, so it names the relation index.

Children now arrive in the lowered index's order, so a range field orders them
ahead of creation time and outstanding cursors do not carry over.
@changeset-bot

changeset-bot Bot commented Sep 6, 2026 •

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: 6c882e2

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 2 packages
Name Type
kitcn Minor
@kitcn/resend Minor

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

@vercel

vercel Bot commented Sep 6, 2026 •

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated
better-convex Ready Ready Preview Sep 7, 2026 8:30am UTC

Request Review

@chatgpt-codex-connector chatgpt-codex-connector 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.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 3131bb5ce8

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread packages/kitcn/src/orm/query.ts Resolved
`_resolvePipelineWhere` resolved the `where` to choose the index, then
`_buildTableFilterPredicate` resolved it again to build the row filter. A
callback that closes over changing state could therefore bound the read with
one expression and filter it with another, dropping rows the second expression
matched but the first never fetched.

The resolved shape now carries the predicate clause, and the row filter is
built from that same shape. Drops `_buildTableFilterPredicate`, whose only two
callers were these sites, and with it a per-row `_buildFilterExpression`
rebuild for object wheres.

@chatgpt-codex-connector chatgpt-codex-connector 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.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: cdd52030bd

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

`pagination.mdx` still said a source `where` always filters after the read and
that anchoring every source is what bounds it. The first is stale. The second
is still true when the pinned index cannot be narrowed, so it is now stated as
the three cases it actually covers: no index pinned, a pinned index the `where`
narrows, and a pinned index it cannot.

Pins the two untested cases: a `where` narrows a pinned index left open, and a
pinned range is never widened by one.
@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Sep 7, 2026 •

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-09-07T08:40:11.577082Z 6c882e2 New commits
ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@chatgpt-codex-connector chatgpt-codex-connector 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.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 6c882e220f

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment on lines +3410 to +3412
(index) =>
index.fields.length > targetFields.length &&
targetFields.every((field, i) => index.fields[i] === field)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Include implicit creation-time suffix in flatMap candidates

When a stage filters createdAt and the relation has only its normal foreign-key index (for example, by_author(authorId)), the effective Convex index is [authorId, _creationTime, _id] as constructed by stream.ts:getIndexFields, but this candidate check considers only explicitly declared fields and rejects an index whose declared length equals targetFields.length. Consequently _resolveFlatMapStageIndex returns null and the query still reads every child before applying a time-window filter, missing the advertised index lowering for a common paginated relation query; build candidates from the effective index fields, including Convex's implicit suffix.

Useful? React with 👍 / 👎.

Comment on lines +3348 to +3351
// that actually gets built, rather than fed to the compiler as a
// scoring hint — `interleaveBy` names normalized key fields, which
// are not the declared index fields the scorer ranks.
orderSpecs: [],

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Pass interleave order into union index selection

When multiple indexes share the filtered prefix, this empty ordering hint can select an index that cannot supply interleaveBy even though another candidate can. For example, with by_status(status) and by_status_likes(status, numLikes), { where: { status: "active" } } selects the shorter exact-match index; streamCanOrderBy then rejects it for interleaveBy(["numLikes"]), falls back to a creation-time scan, and mergedStream throws because that scan also cannot provide numLikes order. Pass the normalized interleave fields into compiler scoring so it selects the compatible compound index.

Useful? React with 👍 / 👎.

Comment on lines +3344 to +3346
queryConfig: this._compileQueryPlan({
whereExpression,
configuredIndex: sourceIndex,

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Constrain pinned union planning to the pinned index

When a source pins an open compound index and another shorter index matches the same filter, the global compiler can select the shorter index before _compileQueryPlan notices it differs from sourceIndex; the plan is then discarded rather than retried against the pinned index. For example, pinning by_status_likes(status, numLikes) with where: { status: "active" } while by_status(status) also exists makes the source walk the entire pinned compound index and post-filter it, despite the pinned index being directly narrowable. Compile against the open pinned index, or retry it after rejecting a competing selection.

Useful? React with 👍 / 👎.


## Patches

- Compile a `select().union([{ where }])` source `where` against the table's indexes instead of filtering every scanned row, so an object `where` on an indexed field bounds the read. A source keeps its unanchored read when the lowered one could not supply `interleaveBy`, and a `where` never displaces an index the source or the chain pinned with a range.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Document union cursor changes as breaking

For a single union source with a range filter, lowering changes both order and cursor shape: { where: { status: { lt: "m" } } } previously walked [createdAt, id] but now walks [status, createdAt, id]. Outstanding cursors therefore do not carry over, just as with the flatMap change already listed above, yet this bullet classifies union lowering as a patch and supplies no migration example. Move this behavior into the breaking section and state its ordering/cursor consequence. .agents/rules/changeset.mdcL21-L32

Useful? React with 👍 / 👎.

@zbeyens

zbeyens commented Sep 7, 2026

Copy link
Copy Markdown
Member

Autoclosure terminal receipt

Head: 6c882e220fd6cb38237e73458429d13bf75b9af6.

Proof: final-head NO_PROXY=localhost,127.0.0.1 bunx vitest run convex/orm/pipeline.test.ts passed 41/41 with no type errors. Full local bun check, package build, lint and branch P0/P1 autoreview passed; CI run 34100814400 passed. Vercel Preview passed. Filters, pagination and API docs passed desktop/390px inspection; walkthrough waived. No P0/P1 item requires a separate replay.

Fresh feedback: helper 4 unresolved threads, 2 top-level comments, 3 review bodies. Unfiltered inventory: 3 top-level comments, 5 reviews, 6 threads (2 resolved / 4 unresolved), 8 inline comments. Helper omits 1 top-level item, 2 review bodies and 4 comments in resolved threads; all are included below. Zero actionable P0/P1; 2 P2 findings fixed and 4 P2 findings explicitly deferred, not resolved. Hosted review completed before this receipt.

Task plan: docs/plans/447-orm-pipeline-where-index-lowering.md. Versioned baseline ledger: docs/plans/2026-09-07-pr-449-autoclosure.md. This exact-head receipt records the late feedback URLs without a receipt-only branch push.

Auto release remains unchecked. Merge will use [skip release]; Version Packages is outside merge scope.

@zbeyens
zbeyens merged commit b8df2da into main Sep 7, 2026
10 checks passed

This branch was successfully deployed

1 active deployment
Preview — 6c882e22 Deployed Sep 7, 2026 by vercel[bot]
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.

ORM: _assertWhereIndexRequirement returns past its own check, so plain-object pipeline wheres silently full-scan

2 participants