fix(orm): read a wide index union from its probes instead of scanning - #456
fix(orm): read a wide index union from its probes instead of scanning#456MikeyZhang75 wants to merge 5 commits into
Conversation
A compiled index union past MAX_INDEX_UNION_PROBES declined to a full table scan. Give the executor a second option: concatenate the probe ranges when they are provably disjoint and ordered on the index's leading field, so the cap picks merged vs sequential rather than index vs scan. Also drop the compiler-side cap, which refused to compile a union at all for an `in` beside another AND term once the list passed 64 values. Fixes #445
🦋 Changeset detectedLatest commit: 65002ee The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 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 |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c6dd8d244b
ℹ️ 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".
| const probePlan = this.tryCompileInArray(term as BinaryExpression); | ||
| // A wide `in` opens one index range per value, which past some width | ||
| // costs more than the single scan it replaces. Leave those alone. | ||
| if ( | ||
| !probePlan || | ||
| probePlan.probeFilters.length > MAX_INDEX_UNION_PROBES | ||
| ) { | ||
| if (!probePlan) { | ||
| continue; | ||
| } | ||
| return { ...probePlan, postFilters: [expression] }; |
There was a problem hiding this comment.
When an AND combines a wide in with a JavaScript-only residual such as contains, this unconditional promotion enters the non-cursor multi-probe branch in query.ts. Because the residual makes probeBound undefined, that branch concurrently calls collect() for every probe before applying the residual and global limit. For example, if the list includes a common status and limit: 1 matches an early row, the previous residual stream stopped after that match, while this plan reads the entire matching-status population and can exceed Convex read limits. Route this shape through the sequential filtered stream instead of the eager probe collector.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in f34dc17 — thanks, this was a real regression.
Measured it on 400 rows all sharing status: 'active' with the first row already matching the residual, limit: 1:
| width | before | after |
|---|---|---|
| 64 | 400 scanned | 1 scanned |
| 65 | 400 scanned | 1 scanned |
Two notes on the diagnosis. The eager collect() was not new — width 64 already read all 400, so this branch has always had the problem; what my compiler change did was route the wide case into it too, where it had previously fallen out to the lazy scan path. And select() was already lazy (1 read at both widths), which is what made the asymmetry obvious.
Rather than restore the cap, I took your suggestion and routed the shape through the stream: _takeResidualProbeUnion builds the same compiled union, applies postFilters in filterWith, and takes it, so the bound counts matches instead of probed rows. It only fires when a residual is the sole reason the per-probe bound was dropped — post-fetch membership and a sort the index cannot serve still need every row, so those keep collecting. That also fixes the pre-existing width-64 case.
Regression test added: an index union with a residual filter sizes the read by matches, asserting scanned <= 4 at widths 64 and 65. Verified it fails with expected 400 to be less than or equal to 4 when the new path is disabled.
| return ( | ||
| compareValues(a.lower.value, b.lower.value) || | ||
| Number(!a.lower.inclusive) - Number(!b.lower.inclusive) |
There was a problem hiding this comment.
For an indexed temporal column in Date mode, probe filters retain public Date values, but _applyFilterToQuery later normalizes those bounds to their stored number or string representation. Sorting the raw bounds with Convex's compareValues therefore compares values outside its Value contract rather than the keys the streams actually emit. A wide in containing Date values can consequently reject a valid disjoint union and require maxScan, or order probes differently from their normalized keys and trigger ConcatStreams in wrong order; normalize each bound with _normalizeComparableValue before sorting and overlap checks.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Applied the normalization in f34dc17, but flagging that I could not reproduce the failure — this is hardening, not a repair.
I built the case you describe: a timestamp() column (default date mode) with an index on it, and a 70-value in of Date objects. Instrumenting the bound as it reaches _orderDisjointProbes:
PROBE-BOUND raw= [object Number] 1577836800000 -> norm= number 1577836800000
The operands are already in stored representation. _buildColumnFilterExpression normalizes every value at filter-expression build time (query.ts:1773), so a raw Date never reaches the compiler, let alone the ordering. The query reads 3 documents for limit: 3 both with and without the normalization.
Worth recording what the failure would have been, since it is not the one described: compareValues(a, b) returns 0 for any two Dates in both directions, so raw temporal bounds would make every probe look like it overlapped its neighbour and the union would be declined — falling back to the bounded scan. It fails closed. It could not order probes wrongly or trigger ConcatStreams in wrong order, because a consistent 0 comparator leaves the sort a no-op and the overlap check rejects before anything is concatenated.
Keeping the change anyway: it costs one idempotent call per bound and it stops the ordering proof from depending on a caller three layers up, which is the same reason _applyFilterToQuery re-normalizes rather than trusting its input. Comment updated to say that rather than claiming a fix.
…opulation The non-cursor multi-probe branch drops its per-probe bound when a residual filter is present, then collects every probe before applying it. For a common probed value that reads most of the table to answer a one-row page. Read that shape as one ordered union stream instead, so the residual runs in filterWith while rows are pulled and take() counts matches. 400 reads -> 1 on the dense case, at every probe width. Also compare probe bounds in their stored representation when ordering them, matching what _applyFilterToQuery pushes into the range. Addresses review feedback on #456.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f34dc171d3
ℹ️ 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".
| - Fix `select()` reading the whole table when its `where` compiled to an index union of more than 64 values. On a 120-row table with a single match, `select().where({ status: { in: [...65 values] } }).limit(1)` read 120 documents where the equivalent `findMany` read 1. Both read 1. | ||
| - Fix an `in` next to another condition — `where: { status: { in: [...] }, name: { contains: 'x' } }` — falling back to a table scan once the list passed 64 values. | ||
| - Fix a `limit` on that same shape reading every row carrying a probed value before applying the other condition. On 400 rows sharing one status where the first row already matched, `limit: 1` read 400 documents; it reads 1. | ||
| - Lengthening an `in` list no longer changes which plan it gets. Up to 64 values the ranges are read as one merged stream, and past that they are read one after another, so the read stays index-bounded at any length. An `orderBy` that sorts across values, such as `createdAt`, still needs a merge and so still asks for `maxScan` past 64 values. |
There was a problem hiding this comment.
When release tooling publishes this changeset, this bullet exposes private executor details (merged versus sequential streams) and begins with “Lengthening” rather than an action verb. The repository’s changeset contract requires action-verb, user-facing bullets and forbids private algorithm notes, so describe only the concrete user-visible outcome of wide indexed filters. .agents/rules/changeset.mdcL36-L41
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in the latest push — the bullet broke two of the rules you cite.
It opened with "Lengthening" instead of an action verb, it described merged versus sequential stream reading, which is an executor detail no user can observe, and it packed three outcomes into one bullet. Replaced with two:
- Keep an indexed `in`, `notIn`, `ne`, or same-field `OR` read index-bounded however long its value list is.
- Require `maxScan` past 64 values only when the sort has to interleave them, such as `orderBy: { createdAt: 'desc' }`.The merged-versus-sequential explanation still lives in www/content/docs/orm/queries/pagination.mdx and the kitcn skill reference, where the reader is choosing a query shape and the threshold is actionable — the changeset just says what changes for them.
I left the ## Breaking changes bullet leading with a noun. That matches the shipped precedent in CHANGELOG.md for the same feature ("Cursor pages for an index-union filter with no orderBy are now in the order of the index the read walks", 0.32.0, from #425), where the subject is the affected read rather than an action the user takes. Happy to reword if you would rather the rule apply uniformly to breaking bullets too.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 65002eebdc
ℹ️ 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".
|
|
||
| ## Breaking changes | ||
|
|
||
| - A long `in`, `notIn`, `ne`, or same-field equality `OR` filter — past 64 values — now pages from its index ranges instead of scanning the table. Those pages are in the order of the index the union walks, grouped by the probed value, rather than in creation order, and they no longer need `maxScan`. Add `orderBy` to keep newest-first paging. |
There was a problem hiding this comment.
The final tree still has a separate changeset bullet beginning with “A long” rather than one of the required action verbs. When this changeset becomes release notes, it violates the repository’s explicit changeset format; rewrite it to begin with an action such as Fix or Improve. This is fresh evidence distinct from the previously corrected “Lengthening” patch bullet. .agents/rules/changeset.mdcL36-L41
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: afb94ac15d
ℹ️ 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".
| - Fix a `limit` on that same shape reading every row carrying a probed value before applying the other condition. On 400 rows sharing one status where the first row already matched, `limit: 1` read 400 documents; it reads 1. | ||
| - Keep an indexed `in`, `notIn`, `ne`, or same-field `OR` read index-bounded however long its value list is. |
There was a problem hiding this comment.
When release tooling publishes this changeset, these separate patch bullets still begin with Keep and Require, neither of which is in the required action-verb list. Rewrite them to begin with an allowed verb such as Fix or Improve. Fresh evidence: lines 32–33 are distinct from the previously flagged breaking-change bullet on line 7. .agents/rules/changeset.mdcL36-L41
Useful? React with 👍 / 👎.
854fc51 to
65002ee
Compare
1 participant
🐛 Fixes #445
🧭 Task plan: docs/plans/445-index-union-bounded-on-pipeline-path.md
🟢 95-100% confidence
expected 120 to be less than or equal to 8✅ Outcome
A compiled index union stays index-bounded at any probe width — on the
select()pipeline path, on the cursor path, and beside anotherANDterm. The issue's case drops from 120 document reads to 1, matching what the equivalentfindManyalready cost, and a wide union no longer demandsmaxScanto page. Alimiton a union carrying a residual filter now sizes the read by matching rows rather than by the probed population: 400 reads become 1 on a table where every row shares the probed value.Past 64 probes the ranges are read one after another, trading one serialized round trip per probe for the table scan it replaces; reads are never worse. An
orderBythat sorts across values, such ascreatedAt, still needs the merge and so still asks formaxScanpast 64 probes. CI'sfixtures:checklane is red on this branch and onmainalike: upstream shadcn replaced the scaffoldedlib/utils.tswith thecnpackage and bumped@base-ui/react,lucide-reactandexpo. That drift is unrelated to this diff, which touches no scaffold source, and is being fixed in its own PR.🏗️ Design
The pipeline path had exactly one union executor.
mergedStreamholds every probe open at once, so it declined pastMAX_INDEX_UNION_PROBES, and every fallback rung in_buildPlanStreamis gated on!hasProbeUnionPlan— leaving the read anchored to no index at all.findManynever had the problem because it runs the probes directly, uncapped, so this is an executor asymmetry rather than a missing guard._buildProbeUnionStreamgains a second executor: past the cap it concatenates the probe ranges instead of merging them, so the cap now picks fan-out versus sequential and never index versus table scan._orderDisjointProbesproves the probes are pairwise-disjoint ordered ranges on the index's leading field rather than trusting the compiler, and declines otherwise. Concatenation is restricted tomergeOffset === 0, where theOrderByStreama merge wraps each probe in is an identity — which is what keeps merged and concatenated unions interchangeable for cursors andnarrow(). The cap stays at 64 because the fan-out it refuses is real and measured.The issue's suggested fall-through was discarded: every multiProbe plan carries
indexFilters: [], so anchoring its index is a rangeless full index scan that bounds nothing and silently reorders rows. MakingmaxScanreachable on the non-cursor path was also rejected — non-cursor reads are sized bylimit/defaultLimit, not by a scan budget. Two problems the issue did not name are fixed too: crossing the cap changed emitted row order, and a second enforcement site intryCompileAndInArraymadewhere: { status: { in: [...65] }, name: { contains: 'x' } }scan on every path.🧪 Verified
bunx vitest run convex/orm/index-union-pagination.test.ts: 22/22, 5 of them verified red before the fixbunx vitest run convex/orm/: 554 passed, 0 failedbun run test: 990 vitest + 1400 bun, 0 failedbun typecheck: 5/5bun lint:fix: cleanbun --cwd packages/kitcn build: 72 filesbun check: green except the unrelatedfixtures:checkdrift noted aboveautoreview --mode branch --base origin/main --engine claude: no accepted/actionable findings,overall: patch is correctDatenormalization was applied as hardening after measuring that it does not reproduce.Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.