fix(orm): bound each index-union probe by matches, not scanned rows - #455
fix(orm): bound each index-union probe by matches, not scanned rows#455MikeyZhang75 wants to merge 7 commits into
Conversation
Non-paginated multiProbe findMany dropped its take() bound whenever RLS was enabled, a post-filter was not Convex-enforceable, or the where filtered through a relation. Each probe then collected its whole range: 500 scanned on a 500-row table for limit 3. Split the bound decision. probeBound now cancels only on an order no index serves; probeBoundedTake keeps the plain scanned-row take() for reads where every filter reached Convex; probeStreamed reads each probe as a stream whose filterWith runs postFilters + RLS + relation membership as rows are pulled, so take() counts survivors. Fan-out, _id dedupe, JS sort and slice unchanged. Per-probe truncation needs order only within a probe, which orderPushdownDirection already proves; the merged probe-union stream needs a global order it cannot express when the primary sort field is a probed one pointing the other way, and it does not dedupe. Hence no merge here. Fixes #442
🦋 Changeset detectedLatest commit: d2aa220 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: d9f7b967ea
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 37c386f9a4
ℹ️ 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".
…obes Streaming a relation `where` hands `_loadOneRelation` one key per call, so `_enforceRelationFanOutKeyCap` always compares keyCount 1 against the cap and a guard that fails fast today never fires. Measured with a cap of 5 and 40 distinct owners: main throws, the streamed path read 80 documents and returned silently. Batch key dedupe goes the same way — a non-_id relation target read 120 instead of 61 at 60 rows. Exclude a relation `where` from probeStreamed. That is exactly the two legs #442 reports (RLS, residual filter), and both guards stay batch-scoped where they work. Restoring them for a per-row caller needs an execution-scoped key ledger in the relation loader, which is not this branch's to own. Pin it: 40 distinct keys against a cap of 5 must still throw.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 636f2f85a8
ℹ️ 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".
1 participant
🐛 Fixes #442
🧭 Task plan: docs/plans/442-index-union-findmany-take-bound.md
🟢 95-100% confidence
query.ts— 200 scanned at 200 rows, 500 at 500convex/orm· 992 vitest · 1400bun test· 274 package · 124 CLI✅ Outcome
The non-paginated index-union (
multiProbe)findManylane dropped itstake()bound entirely whenever RLS was enabled or a post-filter was not Convex-enforceable (contains). Each probe fell back to.collect()and scanned its whole probed range.findMany({ where: { ownerId: { in: [a, b] } }, limit: 3 })now scans 6 rows at both 200 and 500 table rows — it was 200 and 500. Both legs the issue reports are fixed, and unions wider than the 64-probe merge cap are newly bounded too. Rows and their order are unchanged everywhere.🏗️ Design
The issue suggested routing the branch through
_buildResidualFilterStream. That was implemented and measured, and it is wrong:orderBy: [asc(type), desc(score)]returnsa10,a20,a30,b1where the fan-out returns the correcta30,a20,a10,b3.MergedStreamnever dedupes, andtryCompileOrRangeComplementcan emit overlapping probes, soOR: [{score: {lt: 8}}, {score: {gt: 3}}]returns the same document twice and short-fills the page._buildResidualFilterStreamdiscards_buildPlanStream'sprobeUnionflag, and everywithIndexrung there is gated on!hasProbeUnionPlan— so a declined union silently becomes an unanchored full-table scan.tryCompileInArrayapplies no probe cap, so a bareinwith 65+ values reaches exactly that.So this keeps the per-probe fan-out and replaces only each probe's
.collect()with a bounded per-probe stream. Truncating one probe needs order only within that probe, whichorderPushdownDirection !== nullalready proves; merging needs the strictly stronger global order.probeBound(is truncation legal at all) is split fromprobeBoundedTake(can a plain scanned-rowtake()carry it), so a residual filter or a membership pass moves the read onto a stream instead of cancelling the bound. The_iddedupe, the JS sort and the offset/limit slice are untouched, and a project withoutdefineSchema()keeps today's.collect().104 insertions in one branch. No shared helper touched, no signature changed, no path removed.
ne/notIn/isNotNullcombined with anorderByno index can serve still collect their whole complement range. That is the opposite cost regime and needs its own decision; it is stated in the changeset so it is not mistaken for covered.bun checkfails only atfixtures:check, on an upstreamexpo ~55.0.30 → ~55.0.31bump that is already owned byorigin/chore/sync-drifted-scaffold-fixtures. Every other lane is green, includingtest:verifyandtest:runtime.wherethat filters through a relation is deliberately excluded from the streamed path and keeps today's collect-then-filter behavior. Both guards that bound a relation load are scoped to the batch they are handed:_enforceRelationFanOutKeyCapcounts the distinct keys in one call, and_loadOneRelationde-duplicates source keys per call. Streaming that leg hands them one key at a time, which retires a guard that fails fast today. Measured with a cap of 5 and 40 distinct owners:mainthrowsrelationFanOutMaxKeys, the streamed version read 80 documents and returned silently. Restoring either for a per-row caller needs an execution-scoped key ledger inside the relation loader, which is not this branch's to own — so this PR fixes exactly the two legs ORM: index-unionfindManydrops itstake()bound entirely under RLS or a residual filter #442 reports._buildResidualFilterStreamdiscardingprobeUnion; the non-paginated pipeline lane computingrejectedProbeUnionunderisCursorPaginated &&while assigning the stream unconditionally; the cursor lane returning duplicate rows for overlapping range probes onmaintoday; and the single-index residual lane skipping the empty-result RLS policy-configuration assertion.🧪 Verified
npx vitest run convex/orm/index-union-read-bound.test.ts(11 passed) ·npx vitest run(991 passed) ·bun test(1400 pass / 0 fail) ·bun test packages/kitcn/src/orm(274 pass, covers the stub-db lane that has nostream()) ·bun --cwd packages/kitcn build·bun typecheck5/5 ·bun lintclean ·bun run test:verifyandbun run test:runtimegreen · autoreview--mode localclean.Red proof: stashing
packages/kitcn/src/orm/query.tsback tomainturns exactly the four read-bound assertions red while the order, dedupe, offset and no-schema guards stay green — those are behavior-preservation pins, not new behavior.Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.