Skip to content

fix(driver-mongodb): a boolean aggregand answers the ruled values on this face too - #12819

Merged
os-zhuang merged 5 commits into
mainfrom
claude/issue-11151-mongodb-boolean-aggregand
Aug 28, 2026
Merged

fix(driver-mongodb): a boolean aggregand answers the ruled values on this face too#12819
os-zhuang merged 5 commits into
mainfrom
claude/issue-11151-mongodb-boolean-aggregand

Conversation

@claude

@claudeclaudeBot commented Aug 27, 2026

Copy link
Copy Markdown
Contributor

Fixes#11151

driver-mongodb answered all four ruled boolean-aggregand cells wrong. This brings the face onto a contract that was already ruled and that every other face already implements. Two independent halves, in the same package, that are not the same fix — applying either to the other half is wrong in the opposite direction.

1. The premise, re-measured on this branch's own base

The readings on the card were taken on older bases (5a916c4d4d, 811a3c2b). Re-measured on origin/main @ 23843d3f4 before any edit, through buildAggregationPipeline + postProcessAggregation driven by the in-process runPipeline evaluator, on AGGREGATION_ROWS plus a boolean flag column.

Fixture distribution used: FLAG_BY_ID — rows 1..6 = true,false,false,false,true,true, so west is [T,F,F,F] and east is [T,T]. This is the one already landed on main in driver-sql's sql-driver-11635-boolean-aggregand-answers.test.ts, chosen over the other distribution in this card's record (true,false,true,true,false,false) so the two faces' grouped numbers are comparable and not merely both 3-true/3-false.

callbase 23843d3f4ruledafter
sum(flag)033
sum(flag) BY regionwest 0 / east 0west 1 / east 2west 1 / east 2
avg(flag)null0.50.5
avg(flag) BY regionwest null / east nullwest 0.25 / east 1west 0.25 / east 1
min(flag)nullfalsefalse
min(flag) BY regionwest null / east nullwest false / east truewest false / east true
max(flag)nulltruetrue
max(flag) BY regionwest null / east nullwest true / east truewest true / east true
count(flag)666 (agrees, unchanged)
count(flag) BY regionwest 4 / east 2sameunchanged
count_distinct(flag)222 (agrees, unchanged)
count_distinct(flag) BY regionwest 2 / east 1sameunchanged

Positive controls on the same run, proving the harness measured a real tree rather than answering null structurally: sum(score) = 210, avg(score) = 35, min(score) = 10, max(score) = 60 — all correct on the base, all unchanged after.

No cell already answered correctly. All four were wrong; both controls already agreed.

2. Half 1 — mongodb-aggregation.ts, $sum / $avg ONLY

These lowered straight to the arithmetic accumulators, which ignore every non-numeric value. With no numeric value $sum folds to its identity 0 and $avg answers null. Emitted lowering on the base, for the record:

sum: [{"$group":{"_id":null,"measure":{"$sum":"$flag"}}}]
avg: [{"$group":{"_id":null,"measure":{"$avg":"$flag"}}}]

Both arms now wrap the aggregand in the boolean-only coercion #11065 landed on driver-memory:

{ $cond: [{ $eq: [{ $type: path }, 'bool'] }, { $cond: [path, 1, 0] }, path] }

The coercion is deliberately narrow: null, missing and a non-numeric string reach the accumulator exactly as before and stay excluded. A control pins that (sum(stage) over the string column is still 0, avg(stage) still null).

3. Half 2 — mongodb-pipeline-evaluator.testkit.ts, $min / $max

A defect in the instrument, not in the lowering.{ $min: '$flag' } is correct for a real mongod: $min / $max are order statistics over BSON canonical comparison order, which ranks booleans and returns one. The null came from accumulate, which computed its "arithmetic accumulators ignore non-numeric values" filter once for the whole switch and let $min / $max consume it — one arm too far; the comment was accurate the whole time.

Those arms now ignore only null and missing, compare what is left by BSON canonical order, and return a member of the input. bsonRank is the single place that order is written down (null, number, string, boolean, in the manual's order); bsonLte was refactored onto it, and the three pre-existing ranks keep their relative order exactly, so $lte — the date-bucket label path, its only other caller — is unchanged.

$type is modelled in the evaluator for the first time, because half 1's coercion emits it. Without it the strict evaluator would have thrown on the new lowering, which is the evaluator working as designed.

4. The coercion was NOT applied to $min / $max — confirmed, and pinned

Applying half 1's $cond to the order statistics would answer 0 / 1 where #11249 ruled false / true. It was not applied. The emitted stages after this change:

min: [{"$group":{"_id":null,"measure":{"$min":"$flag"}}}]
max: [{"$group":{"_id":null,"measure":{"$max":"$flag"}}}]

byte-identical to the base. A test block reads the emitted stages rather than the values to keep it that way, and this is deliberate: once the evaluator ranks booleans, the values alone can no longer tell the two spellings apart — a $min over a coerced aggregand would answer 0, which a loose assertion accepts. Only the stage distinguishes them, so the pin asserts the stage and additionally that no $cond appears in either arm.

5. The testkit's "refuses every shape it does not model" promise

The head note promises the evaluator refuses rather than answers, and bsonLte already honoured it by throwing UnsupportedShape on an unranked type. The $min / $max arms did not — they answered null silently, which is the worse failure, because the resulting red reads as a defect in the driver under test. That is exactly how this card's min/max half was first misattributed to the lowering.

What was done for types beyond booleans: bsonRank refuses everything it does not rank, and both arms route through it, so an unmodelled aggregand now raises instead of collapsing to null. The rank check runs over every candidate before the fold, so the refusal is a property of the aggregand's type and not of the group's cardinality — a bare fold never compares a one-element group and would have handed back an unmodelled value unexamined, recreating the same silence one layer down. Two tests pin the refusal on a Date column, and a third pins that it is not blanket (number, string and boolean all still answer), so "it refuses" cannot degrade into "it refuses everything".

Deliberately not done: dates, ObjectIds and embedded documents are not ranked. Ranking them is modelling work with no card behind it, and the honest state is a loud refusal that names itself.

6. Two independent ablations

Predictions were committed before either mutation (b87754eb1, empty commit). Each leg mutated one file only, proved the mutation on disk with anchored greps in both directions, ran, restored via git checkout HEAD -- with an absolute path from a trap ... EXIT INT TERM, and proved the restore by git hash-object against the HEAD blob plus an empty git diff HEAD.

No rebuild is involved in either leg, and that is a property of the wiring rather than an omission: the pin suite imports ./mongodb-aggregation.js and ./mongodb-pipeline-evaluator.testkit.js as in-package relative specifiers, which vitest resolves to src/*.ts, never to this package's dist/. The only built dependency in the closure is @objectstack/spec/data, which neither leg mutates.

Ablation 1 — revert only the sum/avg coercion. Anchors: numericAggregandExpr(fieldRef) 2 to 0; $sum: fieldRef ?? 0 1 to 2; $avg: fieldRef ?? 0 0 to 1; blob de6a296e to 7fa018a7.

Result: 4 failed | 13 passed, and the 4 are exactly the predicted 4 — sum(flag) answers 3, avg(flag) answers 0.5, grouped sum/avg answer per group, sum and avg wrap the aggregand in the boolean-only coercion. Every min/max test stayed green, including the bare-lowering pin and the refusal block.

Ablation 2 — revert only the $min/$max arms to the number-filtered form. Anchors: for (const v of present) bsonRank(v); 1 to 0; Math.min(...numbers) 0 to 1; Math.max(...numbers) 0 to 1; blob a268c296 to 505c4b4a.

Result: 6 failed | 11 passed, and the 6 are exactly the 6 tests the prediction itemised — min(flag) answers false, max(flag) answers true, grouped min/max, both unmodelled BSON type raises UnsupportedShape cases, and the types it DOES rank all answer. Every sum/avg test stayed green, as did the entire emitted-lowering block (it reads stages, and the lowering is not mutated in this leg).

One correction, recorded rather than tidied away: the prediction commit itemised 6 tests for ablation 2 but wrote "Expected red count: 5" — an arithmetic slip in the summary line, not a wrong prediction. The named set matched exactly in both directions; no unnamed test went red, and every test predicted green stayed green. The prediction commit is left unamended so the slip stays visible.

Also predicted green in both legs and observed green in both: min/max over a column that is null or absent everywhere answer null. Both the old and new forms answer null there, so that case cannot discriminate — recorded so it is not read as evidence.

7. A first ablation attempt that did NOT land, recorded

The first ablation-1 script used perl -0pi -e 's/\Q...\E/.../' and exited 0 having changed nothing — the anchored counts came back identical, the on-disk blob equalled the HEAD blob. The script's own refuse-on-no-op guard caught it and voided the readings rather than running the suite against an unmutated tree, which would have reported a fully green ablation and read as "the assertions cannot fail". Replaced with a substitution helper that refuses unless the anchor occurs exactly once. Recorded because a zero-hit edit at exit 0 is invisible in every artifact except the count.

8. Fixture triage on the two pins the coercion reds

mongodb-aggregation.test.ts had two pins spelling the sum/avg accumulator literally: builds $group with groupBy fields and builds multiple aggregations. Their actual subject is alias routing and stage shape — which accumulator lands under which key — so the disposition is change the spelling, not replace the pin. The wrapper is written once as a local helper with a note recording that min/max deliberately do not take it; the builds min/max aggregations case immediately below reads their bare field path and is the pin that says so from that file.

Scanned for other consumers by the rule's radius rather than by package: buildAggregationPipeline / postProcessAggregation have no consumers outside packages/drivers/driver-mongodb/src (the only other repo hits are prose in packages/spec/src/data/aggregation-conformance.ts and its built .d.ts).

9. Standing caveat, carried not solved

runPipeline holds the lowering to the table. It does not answer whether a real mongod agrees, and nothing here claims it does — not in the PR body, not in a test name. This fleet cannot fetch a mongod binary at all (#5517), so every operator it models is read from the MongoDB manual rather than observed, $type and the BSON-order $min/$max added here included. The suite's head note says so.

10. Verification

Union re-run on the final commit dcead01a9, after merging origin/main (which moved by one commit touching only scripts/pm/dispatch-gates.mjs — no overlap with this diff, no generated artifact):

  • pnpm --filter @objectstack/driver-mongodb test20 passed | 5 skipped (25) files; 455 passed | 143 skipped (598) tests
  • pnpm --filter @objectstack/driver-mongodb typecheck — clean
  • pnpm check:type-check-debt — its own verdict lines: check-type-check-coverage: OK — 65/78 workspace packages type-checked ... and check-type-check-coverage --re-measure: OK — 31 ledger entr(ies) re-measured in 209.3s, 1570 raw tsc error(s) total, none above its recorded number. / surplus: none
  • pnpm lint (eslint . --no-inline-config, whole repo, not narrowed) — exit 0

Gate family derived from the real changeset with node scripts/pm/dispatch-gates.mjs --repo objectstack-ai/objectstack rather than from a hand-built diff: 22 path-matched families plus 5 convention-triggered ones. All ran green. One line in that list, a bare node scripts/pm/check-half-states.mjs, exited 3 = PREREQUISITE NOT MET (no GitHub credential in that tool's environment) — its own text says "no reading at all", and the gate CI actually runs is check:pm-half-states (the --self-test form), which passed.

One measurement that would otherwise have been silently false: this package's typecheck script excludes **/*.test.ts via its tsconfig, so a green pnpm typecheck says nothing about either test file in this diff. Verified with tsc --listFiles: 10 source files in the program, neither test file among them. Measured instead with an ad-hoc program that includes them — it found one real error in the new suite (an AGGREGATION_ROWS cast), now fixed by routing through unknown, the spelling mongodb-aggregation-translation.test.ts already uses. The 10 remaining errors are pre-existing, in 8 test files this diff does not touch, and match the TEST_DEBT ledger entry for @objectstack/driver-mongodb exactly (recorded 10). The ratchet above confirms it mechanically: surplus: none. That exclusion is already documented in the ledger entry's own note, so it is not filed as a new finding.

11. Bump: patch, argued

The changeset is patch, and the tension is real: this changes what an existing operation returns, which ordinarily argues for minor.

It is graded patch because the returned values were already ruled before this change#11065 for the arithmetic pair, #11249 for the order statistics — and are stated as shared values in @objectstack/spec/data. Every other face already produced them; driver-memory's sibling repair of this exact cell shipped as a patch (17.2.0, Patch Changes). There is no new API, no option, and no opt-out to describe: nothing here is a feature. The only behaviour a consumer could have depended on is a value this project has ruled wrong and that no other driver produces. Grading it minor would advertise a capability that does not exist and imply the old answer had standing.

The instrument half is a .testkit.ts file with no published surface, so it contributes nothing to the bump either way.

12. Scope

Draft, and it stays draft: this seat neither merges nor flips ready.


Generated by Claude Code

Two independent halves of one cell, in the same package:
1. `mongodb-aggregation.ts` lowered `sum` / `avg` as bare `$sum` / `$avg`
over the field path. Those are arithmetic accumulators and ignore every
non-numeric value, so a boolean column summed to `0` and averaged to
`null`. They now wrap the aggregand in the #11065 boolean-only coercion,
answering 3 / 0.5 on the 3-true/3-false fixture.
2. `mongodb-pipeline-evaluator.testkit.ts` applied its "arithmetic
accumulators ignore non-numeric values" filter one arm too far: `$min` /
`$max` consumed it too and answered `null` over a boolean column. They are
order statistics over BSON canonical order, so they now ignore only null
and missing, compare by type-then-value, and return a member of the input
— `false` / `true`, the #11249 ruling. `bsonRank` is the single place that
order is written down and refuses any type it does not model, so the arms
raise rather than silently answering `null`, which is what the file's head
note has always promised. `$type` is modelled for the same reason: the
coercion above emits it.
The coercion in (1) is deliberately NOT applied to `$min` / `$max` — that
would answer 0 / 1 where #11249 ruled false / true.
Part of #11151
…s controls
Ungrouped and grouped, on the `FLAG_BY_ID` distribution already landed on
`main` in driver-sql's #11635 suite (west [T,F,F,F], east [T,T]), so the two
faces' grouped numbers are comparable.
The load-bearing pin is the emitted-lowering block: it reads the stages rather
than the values to assert the `sum`/`avg` coercion did NOT reach `$min`/`$max`.
Once the evaluator ranks booleans both spellings produce an answer, so the
values alone can no longer tell them apart.
Controls kept beside them: `count` / `count_distinct` (which already agreed),
all four functions over the numeric column, and sum/avg over a string column
to pin that the coercion stays boolean-only.
Part of #11151
Two independent halves, ablated separately, because a single ablation that
reds everything proves neither.
ABLATION 1 — revert ONLY `mongodb-aggregation.ts`'s sum/avg coercion
(`{ $sum: numericAggregandExpr(fieldRef) }` -> `{ $sum: fieldRef ?? 0 }`,
same for `$avg`).
PREDICTED DIRECTION: RED, and NARROWLY.
- RED: 'sum(flag) answers 3' (expect 0), 'avg(flag) answers 0.5'
(expect null), 'grouped sum/avg answer per group', and
'sum and avg wrap the aggregand in the boolean-only coercion'.
- GREEN, untouched: every min/max test, including
'min and max are left BARE', the empty-window nulls, the refusal block,
and all controls.
Expected red count: 4.
ABLATION 2 — revert ONLY `mongodb-pipeline-evaluator.testkit.ts`'s $min/$max
arms to the number-filtered form
(`numbers.length === 0 ? null : Math.min(...numbers)`, same for max).
PREDICTED DIRECTION: RED, and NARROWLY.
- RED: 'min(flag) answers false' (expect null), 'max(flag) answers true'
(expect null), 'grouped min/max answer per-group members',
'min over an unmodelled BSON type raises UnsupportedShape' and the max
twin (the arms would answer null instead of raising), and
'the types it DOES rank all answer' (min over the string column would
answer null rather than 'lost').
- GREEN, untouched: every sum/avg test, the emitted-lowering block in
full (it reads stages, not values, and the lowering is not mutated),
'min/max over a column that is null or absent everywhere answer null'
(both forms answer null there — this one CANNOT discriminate and is
predicted green in both legs), and the numeric-column control
(min(score)=10 / max(score)=60 survive a number-only filter).
Expected red count: 5.
No rebuild is involved in either leg: the pin suite imports
`./mongodb-aggregation.js` and `./mongodb-pipeline-evaluator.testkit.js` as
in-package relative specifiers, which vitest resolves to `src/*.ts`, not to
this package's `dist/`. The only built dependency in the closure is
`@objectstack/spec/data`, which neither leg mutates.
Part of #11151
…cion reds
`builds $group with groupBy fields` and `builds multiple aggregations` spelled
the `sum`/`avg` accumulator literally while their actual subject is alias
routing and stage shape. Disposition: change the spelling — the accumulator's
own internals are pinned, with reasons, in the new #11151 suite. The wrapper is
written once as a local helper so the two pins stay readable as routing pins,
and its note records that `min`/`max` deliberately do NOT take it (the
`builds min/max aggregations` case below it is the pin that says so).
Also: the new suite's `AGGREGATION_ROWS` cast goes through `unknown`, matching
the spelling `mongodb-aggregation-translation.test.ts` already uses. Measured
rather than assumed — this package's `typecheck` script EXCLUDES `**/*.test.ts`
via its tsconfig, so `pnpm typecheck` was green over a program that had never
read either test file. Checked with an ad-hoc program that includes them: my
two files are clean; ten pre-existing errors in eight untouched test files are
filed separately, not fixed here.
Part of #11151
@github-actions

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

6 anchor(s) derived from 1 changed package(s); no hand-written page names any of them, so this run has nothing to listnot a clean bill of health. This check sees only pages that NAME a derived anchor: one that documents this change in prose, or enumerates it in an authoring dialect, names none and stays invisible to it on every run.

What this run could not see
  • 1 name(s) were too generic to anchor anything (single lowercase words)
  • the SDK route bridge reached 47 of 219 client-bound route-ledger rows — the other 172 have no registrar path: tail to select them, so pages documenting THEIR client methods cannot appear above, on this or any run. Of those 172: 14 are remediable by widening that discovery convention (an in-repo file declares the path; the convention did not scan it); 56 are structural — on a ledger where NOT ONE row is declared in-repo, so no discovery change reaches them at any price; 102 are undecided (no in-repo declaration, on a ledger that has other in-repo registrars — absence and an unreadable spelling are not distinguishable here). The rows themselves: node scripts/docs-audit/affected-docs.mjs --bridge-coverage
  • a page that states a rule by its inputs shares no identifier with the emitter that implements the rule, so an emitter-only diff cannot list it — not on this run and not on any run. Measured on fix(driver-sql): emit varchar(maxLength) for a text field a declared index keys on #11430: content/docs/protocol/objectql/types.mdx documents the text-family column mapping by the ObjectQL type names it maps FROM (text / textarea / html) while the diff changed createColumn; it went unlisted, and it was the page that diff falsified, in four places. No shared token exists to detect this on, so a rule your change carries has to be re-read by hand in the pages that restate it.

Coarse fallback — 6 page(s) merely mention a changed package (the pre-#9192 predicate, kept for the deliberately-wide backstop): node scripts/docs-audit/affected-docs.mjs --json 632e862d14497e882885e0a75f5c31cc61c5186dpackageMentionDocs.

Which tree this was computed on

This run read content/docs from 0ec3407f69f9d6fb9c28b43ad722133a45972eb6 — the merge of head dcead01a9ee88f9ebdf5cf8676288cc1caaa0cc3 into base 632e862d14497e882885e0a75f5c31cc61c5186d, which is what actions/checkout gives a pull_request run. Not the PR head.

A worktree cut from an older main holds a different content/docs, so re-deriving there can legitimately return a different list — that is a different tree, not a wrong row. To answer on the same tree:

# while this PR is open — GitHub drops the merge commit once it closes
git fetch origin 0ec3407f69f9d6fb9c28b43ad722133a45972eb6 && git checkout 0ec3407f69f9d6fb9c28b43ad722133a45972eb6
# afterwards, rebuild it from the two parents, which stay fetchable
git fetch origin 632e862d14497e882885e0a75f5c31cc61c5186d dcead01a9ee88f9ebdf5cf8676288cc1caaa0cc3 && git checkout -B drift-repro 632e862d14497e882885e0a75f5c31cc61c5186d && git merge --no-ff dcead01a9ee88f9ebdf5cf8676288cc1caaa0cc3
node scripts/docs-audit/affected-docs.mjs --json 632e862d14497e882885e0a75f5c31cc61c5186d

⚠️ That checkout carried uncommitted changes, so the commit above does not fully identify what was read.

@github-actionsgithub-actionsBot added documentation Improvements or additions to documentation tests tooling labels Aug 27, 2026
@os-zhuang
os-zhuang marked this pull request as ready for review August 28, 2026 00:01
@os-zhuang
os-zhuang enabled auto-merge August 28, 2026 00:01
@os-zhuang
os-zhuang added this pull request to the merge queueAug 28, 2026
Merged via the queue into main with commit c4ecf0cAug 28, 2026
34 checks passed
@os-zhuang
os-zhuang deleted the claude/issue-11151-mongodb-boolean-aggregand branch August 28, 2026 00:17
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentationImprovements or additions to documentationsize/lteststooling

Projects

None yet

Development

Successfully merging this pull request may close these issues.

driver-mongodb lowers avg / sum over a boolean column to null / 0 — the same cell as #11065, one driver over

2 participants

@os-zhuang@claude