diff --git a/.changeset/mongodb-boolean-aggregand-ruled-answers.md b/.changeset/mongodb-boolean-aggregand-ruled-answers.md new file mode 100644 index 0000000000..6512270601 --- /dev/null +++ b/.changeset/mongodb-boolean-aggregand-ruled-answers.md @@ -0,0 +1,48 @@ +--- +"@objectstack/driver-mongodb": patch +--- + +fix(driver-mongodb): a boolean aggregand answers the ruled values (#11151) + +`sum` and `avg` over a **boolean** column answered `0` and `null` on this +driver, where every SQL dialect (#11635), `driver-memory` (#11065) and +objectql's in-memory fallback already answered `3` and `0.5` over the same +3-true/3-false rows. The lowering passed the boolean straight to MongoDB's +`$sum` / `$avg`, which are arithmetic accumulators and ignore every non-numeric +value: with nothing numeric to fold, `$sum` returns its identity `0` and `$avg` +returns `null`. Both arms now wrap the aggregand in the boolean-only `$cond` +coercion #11065 landed, so a rate measure over a flag column reads the same on +this driver as on the others. + +**⛔ `min` / `max` are deliberately NOT coerced.** They are order statistics +over BSON canonical comparison order, which ranks booleans and returns a member +of the input domain — #11249 ruled they answer `false` / `true`, and coercing +them would have answered `0` / `1`, breaking that contract in the opposite +direction from the defect being fixed. Their lowering is unchanged; a pin reads +the emitted stages to keep it that way. + +**The coercion stays boolean-only.** `null`, a missing key and a non-numeric +string reach the accumulators exactly as before and stay excluded. Widening to +the other half of objectql's `toNumber` — which maps a non-numeric string to +`0` — would average garbage as zero rather than excluding it, a separate +question this change does not open; a control pins the exclusion. + +**Why `patch` and not `minor`.** 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, and +the sibling repair on `driver-memory` shipped as a patch. There is no new API, +no option, and no opt-out to describe — nothing here is a feature, and the only +behaviour a consumer could have depended on is a value this project has ruled +wrong and that no other driver produces. Calling it `minor` would advertise a +capability that does not exist and imply the old answer had standing. + +Not user-visible, and shipped in the same change because the two are one cell: +`mongodb-pipeline-evaluator.testkit.ts` — the server-free instrument that holds +this lowering to the shared table — applied its "arithmetic accumulators ignore +non-numeric values" filter to `$min` / `$max` as well, one arm too far, and so +answered `null` for them over a boolean column while the lowering under test was +correct. Those arms now ignore only null and missing, compare by BSON canonical +order, and refuse a type the evaluator does not rank instead of silently +answering `null`. diff --git a/packages/drivers/driver-mongodb/src/mongodb-11151-boolean-aggregand-answers.test.ts b/packages/drivers/driver-mongodb/src/mongodb-11151-boolean-aggregand-answers.test.ts new file mode 100644 index 0000000000..fb8f2ff788 --- /dev/null +++ b/packages/drivers/driver-mongodb/src/mongodb-11151-boolean-aggregand-answers.test.ts @@ -0,0 +1,272 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#11151] Boolean aggregands answer the RULED values on this face too — all + * four cells, ungrouped and grouped. + * + * ## The ruling this suite pins + * + * - **`sum` / `avg` answer arithmetic** — `3` / `0.5` over a 3-true/3-false + * fixture. The #11065 family shape, landed on `driver-memory` and on every + * SQL dialect (#11635). + * - **`min` / `max` answer `false` / `true`** — #11249 (maintainer 2026-08-23, + * recorded on that card's comment 5386670755, verbatim and untranslated: + * 「10950 不考虑存量,其他接受你的建议」). Order statistics return a member of + * the input domain, so the JSON boolean IS the contract and `0` / `1` is not + * a spelling of it. + * + * Measured on `origin/main` @ `23843d3f4` before the fix, through the harness + * below: `sum` = `0`, `avg` = `null`, `min` = `null`, `max` = `null` — all four + * wrong, whole-table and per group, while `count` = 6 and `count_distinct` = 2 + * already agreed. + * + * ## The two independent defects behind those four cells + * + * They are NOT one fix applied twice, and each is asserted here against the + * half it governs: + * + * 1. **The lowering** (`mongodb-aggregation.ts`) emitted a bare + * `{$sum: '$flag'}` / `{$avg: '$flag'}`. MongoDB's arithmetic accumulators + * ignore non-numeric values, so with no numeric value `$sum` folds to its + * identity `0` and `$avg` answers `null`. Fixed by the boolean-only `$cond` + * coercion. + * 2. **The instrument** (`mongodb-pipeline-evaluator.testkit.ts`) applied that + * same "ignore non-numeric" rule to `$min` / `$max`, which are order + * statistics over BSON canonical order and rank booleans perfectly well. The + * `{$min: '$flag'}` lowering was, and remains, correct. + * + * ⛔ Applying (1)'s coercion to `$min` / `$max` would answer `0` / `1` and break + * #11249 in the opposite direction. {@link describe} block "the emitted lowering + * keeps the two halves apart" pins that it was not, reading the emitted stages + * rather than trusting the values — the values alone cannot tell a `$min` over a + * boolean from a `$min` over a coerced `1`/`0` once the evaluator ranks both. + * + * ## ⚠️ What this suite deliberately does NOT answer + * + * Whether a real mongod agrees. `runPipeline` is the in-process evaluator + * `mongodb-aggregation-translation.test.ts` uses: it holds the LOWERING to the + * shared table by MongoDB's documented semantics, and this fleet cannot fetch a + * mongod binary at all (#5517). Every operator it models is read from the + * manual, not observed — `$type` and the BSON-order `$min`/`$max` this card + * added included. No test name here claims otherwise. + * + * ## The fixture + * + * `AGGREGATION_ROWS` — the shared aggregate-vocabulary fixture — plus a boolean + * `flag` column. The distribution is {@link FLAG_BY_ID}, the one already landed + * on `main` in `driver-sql`'s #11635 suite, chosen over the other distribution + * in this card's record (`true,false,true,true,false,false`) so the two faces' + * grouped numbers are comparable rather than merely both 3-true/3-false. + */ + +import { describe, it, expect } from 'vitest'; +import { AGGREGATION_ROWS } from '@objectstack/spec/data'; +import { + buildAggregationPipeline, + postProcessAggregation, + type AggregationInput, +} from './mongodb-aggregation.js'; +import { + runPipeline, + UnsupportedShape, + type Doc, +} from './mongodb-pipeline-evaluator.testkit.js'; + +/** The alias every measure is projected under — never a fixture column. */ +const MEASURE = 'measure'; + +/** + * The `flag` column, keyed by fixture row id: 3 true / 3 false, with `east` + * (rows 5–6) all-true. The per-group split is deliberately asymmetric — + * `west` is `[T,F,F,F]` and `east` `[T,T]` — so `east`'s grouped `min` is + * `true`: a measure computed over the whole table, or a sticky per-column + * constant, goes red on that cell rather than passing by symmetry. + */ +const FLAG_BY_ID: Record = { + '1': true, + '2': false, + '3': false, + '4': false, + '5': true, + '6': true, +}; + +/** + * The six shared rows plus `flag`, and two columns that exist only to drive the + * empty-input branch of an order statistic: `voidcol` is an explicit `null` on + * every row, and no row carries `absent` at all. + */ +const ROWS: Doc[] = (AGGREGATION_ROWS as unknown as Doc[]).map((row) => ({ + ...row, + flag: FLAG_BY_ID[row.id as string], + voidcol: null, +})); + +/** Run one aggregation end to end and answer the whole-table measure. */ +function measure(func: string, field: string): unknown { + const aggregations = [{ function: func, field, alias: MEASURE }] as AggregationInput[]; + const pipeline = buildAggregationPipeline({ aggregations }); + return postProcessAggregation(runPipeline(ROWS, pipeline), aggregations)[0]?.[MEASURE]; +} + +/** Run one aggregation grouped by `region`, keyed by group value. */ +function byRegion(func: string, field: string): Record { + const aggregations = [{ function: func, field, alias: MEASURE }] as AggregationInput[]; + const pipeline = buildAggregationPipeline({ aggregations, groupBy: ['region'] }); + const rows = postProcessAggregation(runPipeline(ROWS, pipeline), aggregations); + return Object.fromEntries(rows.map((row) => [String(row.region), row[MEASURE]])); +} + +describe('[#11151] driver-mongodb — the fixture this suite measures', () => { + // The fixture read back rather than trusted: a seed that dropped a row or + // folded the flags would turn every value below into a test of another table. + it('is six rows, 3 true / 3 false, with east all-true', () => { + expect(ROWS).toHaveLength(6); + expect(ROWS.filter((r) => r.flag === true), 'true rows').toHaveLength(3); + expect(ROWS.filter((r) => r.region === 'east').map((r) => r.flag)).toEqual([true, true]); + expect(ROWS.filter((r) => r.region === 'west').map((r) => r.flag)).toEqual([ + true, + false, + false, + false, + ]); + }); +}); + +describe('[#11151] the ruled arithmetic half — sum / avg count a boolean as 1 or 0', () => { + it('sum(flag) answers 3, not $sum’s identity 0', () => { + expect(measure('sum', 'flag')).toBe(3); + }); + + it('avg(flag) answers 0.5, not null', () => { + expect(measure('avg', 'flag')).toBe(0.5); + }); + + it('grouped sum/avg answer per group: west [T,F,F,F], east [T,T]', () => { + expect(byRegion('sum', 'flag')).toEqual({ west: 1, east: 2 }); + expect(byRegion('avg', 'flag')).toEqual({ west: 0.25, east: 1 }); + }); +}); + +describe('[#11151] the ruled order-statistic half — min / max answer JSON booleans', () => { + // Asserted STRICTLY. `0` / `1` satisfies a `Number()` reading and is exactly + // the answer #11249 ruled against, so a loose comparison here would pass on + // the one wrong value this half exists to exclude. + it('min(flag) answers false — the boolean, not 0 and not null', () => { + expect(measure('min', 'flag')).toBe(false); + }); + + it('max(flag) answers true — the boolean, not 1 and not null', () => { + expect(measure('max', 'flag')).toBe(true); + }); + + it('grouped min/max answer per-group members, and east’s min is true', () => { + // `east` is the load-bearing cell: all-true, so its `min` is `true`. A + // whole-table computation or a sticky `false` fails here and only here. + expect(byRegion('min', 'flag')).toEqual({ west: false, east: true }); + expect(byRegion('max', 'flag')).toEqual({ west: true, east: true }); + }); + + it('min/max over a column that is null or absent everywhere answer null', () => { + // The manual's rule: null and missing are IGNORED, and a group left with + // nothing answers `null`. Not folded to `false`, which is what a boolean + // face that manufactured a default would do. + expect(measure('min', 'voidcol'), 'explicit null on every row').toBeNull(); + expect(measure('max', 'voidcol'), 'explicit null on every row').toBeNull(); + expect(measure('min', 'absent'), 'a column no row carries').toBeNull(); + expect(measure('max', 'absent'), 'a column no row carries').toBeNull(); + }); +}); + +describe('[#11151] the emitted lowering keeps the two halves apart', () => { + const emit = (func: string): unknown => + buildAggregationPipeline({ + aggregations: [{ function: func, field: 'flag', alias: MEASURE }] as AggregationInput[], + })[0]; + + const COERCED = { + $cond: [{ $eq: [{ $type: '$flag' }, 'bool'] }, { $cond: ['$flag', 1, 0] }, '$flag'], + }; + + it('sum and avg wrap the aggregand in the boolean-only coercion', () => { + expect(emit('sum')).toEqual({ $group: { _id: null, [MEASURE]: { $sum: COERCED } } }); + expect(emit('avg')).toEqual({ $group: { _id: null, [MEASURE]: { $avg: COERCED } } }); + }); + + it('⛔ min and max are left BARE — the coercion is not applied to them', () => { + // The load-bearing pin of this file. `$min`/`$max` over a coerced aggregand + // would answer `0`/`1` — arithmetic where #11249 ruled for a member of the + // input domain — and the VALUES cannot catch it once the evaluator ranks + // booleans, because both spellings then produce an answer. Only the emitted + // stage distinguishes them. + expect(emit('min')).toEqual({ $group: { _id: null, [MEASURE]: { $min: '$flag' } } }); + expect(emit('max')).toEqual({ $group: { _id: null, [MEASURE]: { $max: '$flag' } } }); + expect(JSON.stringify(emit('min')), 'no $cond reached the min arm').not.toContain('$cond'); + expect(JSON.stringify(emit('max')), 'no $cond reached the max arm').not.toContain('$cond'); + }); + + it('a fieldless sum/avg is unchanged — the coercion needs a path to coerce', () => { + const fieldless = buildAggregationPipeline({ + aggregations: [{ function: 'sum', alias: MEASURE }] as AggregationInput[], + })[0]; + expect(fieldless).toEqual({ $group: { _id: null, [MEASURE]: { $sum: 0 } } }); + }); +}); + +describe('[#11151] CONTROLS — what neither half was allowed to move', () => { + // These two already agreed with every other face before this card. A suite + // holding only the broken cells cannot show that the fix was targeted. + it('count(flag) / count_distinct(flag) are unchanged', () => { + expect(measure('count', 'flag'), 'count over the boolean column').toBe(6); + expect(measure('count_distinct', 'flag'), 'count_distinct over the boolean column').toBe(2); + expect(byRegion('count', 'flag')).toEqual({ west: 4, east: 2 }); + expect(byRegion('count_distinct', 'flag')).toEqual({ west: 2, east: 1 }); + }); + + it('all four functions over the NUMERIC column are untouched', () => { + expect(measure('sum', 'score'), 'sum(score)').toBe(210); + expect(measure('avg', 'score'), 'avg(score)').toBe(35); + expect(measure('min', 'score'), 'min(score)').toBe(10); + expect(measure('max', 'score'), 'max(score)').toBe(60); + }); + + it('sum/avg still IGNORE a non-numeric string — the coercion is boolean-only', () => { + // `stage` is a string column with two explicit nulls. Coercing wider would + // mean adopting `Number('won') === NaN` or a `toNumber` that maps it to 0; + // both are separate questions from this card, and neither is adopted. + expect(measure('sum', 'stage'), 'sum over a string column').toBe(0); + expect(measure('avg', 'stage'), 'avg over a string column').toBeNull(); + }); +}); + +describe('[#11151] the evaluator REFUSES a type it does not rank, rather than answering null', () => { + // The head note of `mongodb-pipeline-evaluator.testkit.ts` promises this + // instrument "refuses every shape it does not model". The `$min`/`$max` arms + // were the exception: they filtered to numbers and answered `null` for + // everything else, silently. A wrong answer from a strict evaluator is worse + // than a refusal, because the red it produces reads as a defect in the driver + // under test — which is how this card's own `min`/`max` half was first + // misattributed to the lowering. + const withDate: Doc[] = ROWS.map((row) => ({ ...row, when: new Date('2026-01-01T00:00:00Z') })); + + for (const func of ['min', 'max'] as const) { + it(`${func} over an unmodelled BSON type raises UnsupportedShape`, () => { + const aggregations = [ + { function: func, field: 'when', alias: MEASURE }, + ] as AggregationInput[]; + const pipeline = buildAggregationPipeline({ aggregations }); + expect(() => runPipeline(withDate, pipeline)).toThrow(UnsupportedShape); + expect(() => runPipeline(withDate, pipeline)).toThrow(/unmodelled type/); + }); + } + + // The refusal is a property of the evaluator's coverage, NOT a statement + // about MongoDB: a real mongod ranks dates fine. Extending `bsonRank` is the + // way to model one, and until someone does the instrument says so out loud + // instead of answering `null`. + it('the types it DOES rank all answer, so the refusal above is not blanket', () => { + expect(measure('min', 'score'), 'number').toBe(10); + expect(measure('min', 'stage'), 'string').toBe('lost'); + expect(measure('min', 'flag'), 'boolean').toBe(false); + }); +}); diff --git a/packages/drivers/driver-mongodb/src/mongodb-aggregation.test.ts b/packages/drivers/driver-mongodb/src/mongodb-aggregation.test.ts index b3388635f5..6dfdcf8851 100644 --- a/packages/drivers/driver-mongodb/src/mongodb-aggregation.test.ts +++ b/packages/drivers/driver-mongodb/src/mongodb-aggregation.test.ts @@ -3,6 +3,22 @@ import { describe, it, expect } from 'vitest'; import { buildAggregationPipeline, postProcessAggregation } from './mongodb-aggregation.js'; +/** + * [#11151] The aggregand `sum` and `avg` now consume: the field path, with a + * BOOLEAN rendered as the number it is worth. Spelled once here because the + * pins below are about ALIAS ROUTING and stage shape — which accumulator lands + * under which key — and the accumulator's own internals are pinned, with the + * reasons, in `mongodb-11151-boolean-aggregand-answers.test.ts`. + * + * ⛔ `min` / `max` deliberately do NOT take this wrapper: they are order + * statistics and #11249 ruled they answer `false` / `true`, not `0` / `1`. The + * `builds min/max aggregations` case below reads their bare field path and is + * the pin that says so from this file. + */ +const coerced = (path: string) => ({ + $cond: [{ $eq: [{ $type: path }, 'bool'] }, { $cond: [path, 1, 0] }, path], +}); + describe('MongoDB Aggregation Pipeline Builder', () => { it('builds empty pipeline for no options', () => { expect(buildAggregationPipeline({})).toEqual([]); @@ -32,7 +48,7 @@ describe('MongoDB Aggregation Pipeline Builder', () => { groupBy: ['region'], }); expect(pipeline).toEqual([ - { $group: { _id: { region: '$region' }, total_amount: { $sum: '$amount' } } }, + { $group: { _id: { region: '$region' }, total_amount: { $sum: coerced('$amount') } } }, { $project: { _id: 0, region: '$_id.region', total_amount: 1 } }, ]); }); @@ -50,8 +66,8 @@ describe('MongoDB Aggregation Pipeline Builder', () => { const groupStage = pipeline[0]; expect(groupStage.$group._id).toEqual({ customer_id: '$customer_id' }); expect(groupStage.$group.order_count).toEqual({ $sum: 1 }); - expect(groupStage.$group.total).toEqual({ $sum: '$amount' }); - expect(groupStage.$group.average).toEqual({ $avg: '$amount' }); + expect(groupStage.$group.total).toEqual({ $sum: coerced('$amount') }); + expect(groupStage.$group.average).toEqual({ $avg: coerced('$amount') }); }); it('adds $sort stage', () => { diff --git a/packages/drivers/driver-mongodb/src/mongodb-aggregation.ts b/packages/drivers/driver-mongodb/src/mongodb-aggregation.ts index 876623b8f5..de6a296ef5 100644 --- a/packages/drivers/driver-mongodb/src/mongodb-aggregation.ts +++ b/packages/drivers/driver-mongodb/src/mongodb-aggregation.ts @@ -467,6 +467,60 @@ export function buildAggregationPipeline(opts: { return pipeline; } +/** + * [#11151] `path`, with a BOOLEAN rendered as the number it is worth — the + * aggregand expression `$sum` and `$avg` consume on this face. + * + * ## What it is for + * + * MongoDB's `$sum` and `$avg` are ARITHMETIC accumulators and ignore every + * non-numeric value, a boolean included. So a whole boolean column summed to + * `$sum`'s identity `0` and averaged to `null` here, while `SUM(col)` / + * `AVG(col)` answer `3` / `0.5` over the same 3-true/3-false rows on every SQL + * dialect (#11635), `driver-memory` answers those numbers on both of its faces + * (#11065), and objectql's in-memory fallback answers them too because its + * `toNumber` is `Number(v)` and `Number(true) === 1`. A rate measure over a + * flag column — an SLA-violation rate, a win rate — is the ordinary shape of + * that query, and the two answers are not two spellings of one: a dashboard + * tile bound to the measure renders a percentage under SQL and a blank here, + * indistinguishable from "no matching rows". `sum`'s `0` is the worse half, + * being a plausible number rather than a visible hole. + * + * The expression is the one #11065 landed on `driver-memory`'s analytics face + * (`memory-analytics.ts`, `numericAggregandExpr`), reproduced rather than + * imported: this driver shares no line of code with that one, and the shared + * contract between them is the VALUES in `@objectstack/spec/data`, not a + * helper. + * + * ## Why an EXPRESSION and not post-processing + * + * {@link postProcessAggregation} runs after the pipeline's own `$sort` and + * `$limit` stages, so a measure left unresolved until then would be sorted as + * whatever it was — and `order` over a `sum` or `avg` measure is an ordinary + * analytics query. Keeping the rule inside the `$group` expression leaves every + * later stage looking at the number it expects. + * + * ## ⛔ Why it is NOT applied to `min` / `max` + * + * `$min` / `$max` are ORDER STATISTICS over BSON canonical comparison order, + * not arithmetic accumulators: they rank booleans and return a MEMBER of the + * input domain. #11249 ruled (maintainer 2026-08-23) that a boolean aggregand + * answers `false` / `true` there — the JSON boolean, not `0` / `1` — so + * wrapping those two arms in this coercion would break the ruled contract in + * the opposite direction from the defect it fixes. + * + * ## The narrowness is deliberate + * + * Only `bool` is rewritten. Null, missing and a non-numeric string reach the + * accumulator exactly as before and are ignored by it exactly as before. + * Coercing wider would mean adopting `toNumber`'s other half, which maps a + * non-numeric string to `0` and so averages garbage as zero rather than + * excluding it — a separate question from this one. + */ +function numericAggregandExpr(path: string): Document { + return { $cond: [{ $eq: [{ $type: path }, 'bool'] }, { $cond: [path, 1, 0] }, path] }; +} + /** * Build a single MongoDB accumulator expression from an aggregation descriptor. */ @@ -490,11 +544,14 @@ function buildAccumulator(agg: AggregationInput): Document { ? { $sum: 1 } : { $sum: { $cond: [{ $eq: [{ $ifNull: [fieldRef, null] }, null] }, 0, 1] } }; + // [#11151] `sum` / `avg` coerce a BOOLEAN aggregand; see + // {@link numericAggregandExpr} for why, and for why `min` / `max` below + // deliberately do NOT. case 'sum': - return { $sum: fieldRef ?? 0 }; + return { $sum: fieldRef === null ? 0 : numericAggregandExpr(fieldRef) }; case 'avg': - return { $avg: fieldRef ?? 0 }; + return { $avg: fieldRef === null ? 0 : numericAggregandExpr(fieldRef) }; case 'min': return { $min: fieldRef ?? 0 }; diff --git a/packages/drivers/driver-mongodb/src/mongodb-pipeline-evaluator.testkit.ts b/packages/drivers/driver-mongodb/src/mongodb-pipeline-evaluator.testkit.ts index d2363bed55..a268c29696 100644 --- a/packages/drivers/driver-mongodb/src/mongodb-pipeline-evaluator.testkit.ts +++ b/packages/drivers/driver-mongodb/src/mongodb-pipeline-evaluator.testkit.ts @@ -30,7 +30,8 @@ * (proxy 403 on the download — #5517), and a suite nobody has executed against * the real thing is a claim, not a check. The date operators added for #7580 * (`$convert` → date, `$dateToString`, `$concat`, `$switch`, `$lte`) carry that - * bound exactly as the #6850/#6814 ones do. + * bound exactly as the #6850/#6814 ones do, and so do `$type` and the BSON-order + * `$min` / `$max` added for #11151. * * The single most important discipline that makes the bound survivable: this * file models the DOCUMENTED semantics, never the behaviour the lowering @@ -180,25 +181,72 @@ function formatDate(d: Date, format: string): string { } /** - * BSON canonical sort order, for the two types `$lte` meets here. Null sorts - * BELOW every string — which is why the `quarter` lowering can let its `$switch` - * run on a null instant without a guard: it answers a digit, and the surrounding - * `$concat` has already decided the whole label is null. + * BSON canonical TYPE order, for the types this evaluator models. The manual's + * order runs MinKey, Null, Numbers, String, Object, Array, BinData, ObjectId, + * **Boolean**, Date, Timestamp, Regex, MaxKey — so a boolean ranks ABOVE every + * number and every string. That ranking is the whole reason `$min` / `$max` can + * answer over a boolean column where `$sum` / `$avg` cannot: order statistics + * compare by type-then-value and return a MEMBER of the input, while the + * arithmetic accumulators ignore what they cannot add. + * + * ⛔ Every type not listed here is a thrown {@link UnsupportedShape}, never a + * silent rank. This function is the one place the order is written down, so a + * type it does not model refuses in BOTH of its callers rather than being ranked + * one way by `$lte` and dropped by `$min`. + * + * [#11151] Boolean was added here. The three ranks that existed before keep + * their relative order exactly, so `$lte` — whose only operands are the null + * and the string a date-bucket label reaches it as — is unchanged. + */ +function bsonRank(v: unknown): number { + if (v === MISSING || v === null) return 0; + if (typeof v === 'number') return 1; + if (typeof v === 'string') return 2; + if (typeof v === 'boolean') return 3; + throw new UnsupportedShape(`BSON canonical order over an unmodelled type: ${typeof v}`); +} + +/** + * `left <= right` under BSON canonical order — type rank first, value second. + * Null sorts BELOW every string, which is why the `quarter` lowering can let its + * `$switch` run on a null instant without a guard: it answers a digit, and the + * surrounding `$concat` has already decided the whole label is null. */ function bsonLte(left: unknown, right: unknown): boolean { - const rank = (v: unknown): number => { - if (v === MISSING || v === null) return 0; - if (typeof v === 'number') return 1; - if (typeof v === 'string') return 2; - throw new UnsupportedShape(`$lte over an unmodelled BSON type: ${typeof v}`); - }; - const lr = rank(left); - const rr = rank(right); + const lr = bsonRank(left); + const rr = bsonRank(right); if (lr !== rr) return lr < rr; if (lr === 0) return true; // null <= null + // `false` sorts below `true`; TypeScript refuses a relational operator on two + // booleans, so the comparison is spelled through their numeric images. + if (lr === 3) return Number(left) <= Number(right); return (left as string | number) <= (right as string | number); } +/** + * The manual's `$type`: the BSON type NAME of a value, and the string + * `'missing'` for a field path that resolved to nothing — the one type name + * that is not a type. Modelled because the `sum` / `avg` boolean coercion + * (#11151) emits `$type`, and an evaluator that guessed here would be blessing + * a lowering nobody checked. + * + * A JS number is reported as `'double'`: every unboxed number crosses the wire + * as a BSON double, and this evaluator has no boxed integers to distinguish, so + * `'double'` is the honest answer rather than a size-dependent guess between + * `'int'`, `'long'` and `'double'`. + */ +function bsonTypeName(v: unknown): string { + if (v === MISSING) return 'missing'; + if (v === null) return 'null'; + if (typeof v === 'boolean') return 'bool'; + if (typeof v === 'number') return 'double'; + if (typeof v === 'string') return 'string'; + if (v instanceof Date) return 'date'; + if (Array.isArray(v)) return 'array'; + if (typeof v === 'object') return 'object'; + throw new UnsupportedShape(`$type over an unmodelled BSON type: ${typeof v}`); +} + /** MongoDB's expression truthiness: `false`, `null`, `0` and missing are false. */ function truthy(v: unknown): boolean { return !(v === false || v === null || v === MISSING || v === 0 || v === undefined); @@ -247,6 +295,17 @@ export function evalExpr(doc: Doc, expr: unknown): unknown { if (!Array.isArray(arg) || arg.length !== 2) throw new UnsupportedShape('$lte takes two operands'); return bsonLte(evalExpr(doc, arg[0]), evalExpr(doc, arg[1])); } + case '$type': { + // [#11151] `$type` takes exactly ONE operand, unwrapped or wrapped in a + // one-element array; the manual accepts both spellings and the + // boolean-aggregand lowering emits the unwrapped one. A longer array is + // an error on a real server, so it is refused here rather than reported + // as `'array'`. + if (Array.isArray(arg) && arg.length !== 1) { + throw new UnsupportedShape(`$type takes one operand, got ${arg.length}`); + } + return bsonTypeName(evalExpr(doc, Array.isArray(arg) ? arg[0] : arg)); + } case '$cond': { if (!Array.isArray(arg) || arg.length !== 3) throw new UnsupportedShape('$cond takes [if, then, else]'); return truthy(evalExpr(doc, arg[0])) ? evalExpr(doc, arg[1]) : evalExpr(doc, arg[2]); @@ -319,7 +378,21 @@ export function accumulate(rows: Doc[], acc: unknown): unknown { const [op] = keys; const arg = (acc as Doc)[op]; const values = rows.map((row) => evalExpr(row, arg)); - /** MongoDB's arithmetic accumulators ignore missing and non-numeric values. */ + /** + * MongoDB's ARITHMETIC accumulators ignore missing and non-numeric values. + * + * [#11151] ⛔ This filter belongs to `$sum` and `$avg` and to nothing else. It + * used to be computed once for the whole switch and consumed by `$min` and + * `$max` as well — one arm too far, and the comment above it was accurate the + * whole time. `$min` / `$max` are ORDER STATISTICS over BSON canonical order + * (see {@link bsonRank}), not arithmetic: they rank every type, so a boolean + * column has a real minimum and a real maximum. Filtering to numbers left + * them with nothing and they answered `null` — SILENTLY, which is the one + * thing this file's head note promises it never does. A wrong answer from a + * strict evaluator is worse than a refusal, because the red it produces reads + * as a defect in the lowering under test; that misreading really happened, and + * cost a card's dispatch a wrong diagnosis. + */ const numbers = values.filter((v): v is number => typeof v === 'number'); switch (op) { @@ -328,9 +401,24 @@ export function accumulate(rows: Doc[], acc: unknown): unknown { case '$avg': return numbers.length === 0 ? null : numbers.reduce((a, b) => a + b, 0) / numbers.length; case '$min': - return numbers.length === 0 ? null : Math.min(...numbers); - case '$max': - return numbers.length === 0 ? null : Math.max(...numbers); + case '$max': { + // The manual's rule for both: null and missing are IGNORED, whatever is + // left is compared by BSON canonical order, and a group in which every + // value is null or missing answers `null`. The result is a MEMBER of the + // input — a boolean in, a boolean out (the #11249 contract) — never a + // number derived from one. `bsonRank` refuses any type it does not model, + // so an unmodelled aggregand raises rather than collapsing to `null`. + const present = values.filter((v) => v !== MISSING && v !== null); + if (present.length === 0) return null; + // Rank every candidate BEFORE folding, so the refusal is a property of + // the aggregand's TYPE and not of the group's cardinality: a fold alone + // never compares a one-element group, and would hand back an unmodelled + // value unexamined — the same silence this arm was fixed to stop. + for (const v of present) bsonRank(v); + return present.reduce((best, v) => + (op === '$min' ? bsonLte(v, best) : bsonLte(best, v)) ? v : best, + ); + } case '$addToSet': { // `$addToSet` skips a MISSING field and keeps an explicit `null` — the // whole of #6814 lives in that second half.