Skip to content

Commit f62ddf7

Browse files
committed
Merge remote-tracking branch 'origin/main' into claude/issue-12488-changeset-repo-name
2 parents ea489f7 + c4ecf0c commit f62ddf7

5 files changed

Lines changed: 503 additions & 22 deletions

File tree

Lines changed: 48 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,48 @@
1+
---
2+
"@objectstack/driver-mongodb": patch
3+
---
4+
5+
fix(driver-mongodb): a boolean aggregand answers the ruled values (#11151)
6+
7+
`sum` and `avg` over a **boolean** column answered `0` and `null` on this
8+
driver, where every SQL dialect (#11635), `driver-memory` (#11065) and
9+
objectql's in-memory fallback already answered `3` and `0.5` over the same
10+
3-true/3-false rows. The lowering passed the boolean straight to MongoDB's
11+
`$sum` / `$avg`, which are arithmetic accumulators and ignore every non-numeric
12+
value: with nothing numeric to fold, `$sum` returns its identity `0` and `$avg`
13+
returns `null`. Both arms now wrap the aggregand in the boolean-only `$cond`
14+
coercion #11065 landed, so a rate measure over a flag column reads the same on
15+
this driver as on the others.
16+
17+
**`min` / `max` are deliberately NOT coerced.** They are order statistics
18+
over BSON canonical comparison order, which ranks booleans and returns a member
19+
of the input domain — #11249 ruled they answer `false` / `true`, and coercing
20+
them would have answered `0` / `1`, breaking that contract in the opposite
21+
direction from the defect being fixed. Their lowering is unchanged; a pin reads
22+
the emitted stages to keep it that way.
23+
24+
**The coercion stays boolean-only.** `null`, a missing key and a non-numeric
25+
string reach the accumulators exactly as before and stay excluded. Widening to
26+
the other half of objectql's `toNumber` — which maps a non-numeric string to
27+
`0` — would average garbage as zero rather than excluding it, a separate
28+
question this change does not open; a control pins the exclusion.
29+
30+
**Why `patch` and not `minor`.** This changes what an existing operation
31+
returns, which ordinarily argues for `minor`. It is graded `patch` because the
32+
returned values were **already ruled** before this change (#11065 for the
33+
arithmetic pair, #11249 for the order statistics) and are stated as shared
34+
values in `@objectstack/spec/data`; every other face already produced them, and
35+
the sibling repair on `driver-memory` shipped as a patch. There is no new API,
36+
no option, and no opt-out to describe — nothing here is a feature, and the only
37+
behaviour a consumer could have depended on is a value this project has ruled
38+
wrong and that no other driver produces. Calling it `minor` would advertise a
39+
capability that does not exist and imply the old answer had standing.
40+
41+
Not user-visible, and shipped in the same change because the two are one cell:
42+
`mongodb-pipeline-evaluator.testkit.ts` — the server-free instrument that holds
43+
this lowering to the shared table — applied its "arithmetic accumulators ignore
44+
non-numeric values" filter to `$min` / `$max` as well, one arm too far, and so
45+
answered `null` for them over a boolean column while the lowering under test was
46+
correct. Those arms now ignore only null and missing, compare by BSON canonical
47+
order, and refuse a type the evaluator does not rank instead of silently
48+
answering `null`.
Lines changed: 272 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,272 @@
1+
// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license.
2+
3+
/**
4+
* [#11151] Boolean aggregands answer the RULED values on this face too — all
5+
* four cells, ungrouped and grouped.
6+
*
7+
* ## The ruling this suite pins
8+
*
9+
* - **`sum` / `avg` answer arithmetic** — `3` / `0.5` over a 3-true/3-false
10+
* fixture. The #11065 family shape, landed on `driver-memory` and on every
11+
* SQL dialect (#11635).
12+
* - **`min` / `max` answer `false` / `true`** — #11249 (maintainer 2026-08-23,
13+
* recorded on that card's comment 5386670755, verbatim and untranslated:
14+
* 「10950 不考虑存量,其他接受你的建议」). Order statistics return a member of
15+
* the input domain, so the JSON boolean IS the contract and `0` / `1` is not
16+
* a spelling of it.
17+
*
18+
* Measured on `origin/main` @ `23843d3f4` before the fix, through the harness
19+
* below: `sum` = `0`, `avg` = `null`, `min` = `null`, `max` = `null` — all four
20+
* wrong, whole-table and per group, while `count` = 6 and `count_distinct` = 2
21+
* already agreed.
22+
*
23+
* ## The two independent defects behind those four cells
24+
*
25+
* They are NOT one fix applied twice, and each is asserted here against the
26+
* half it governs:
27+
*
28+
* 1. **The lowering** (`mongodb-aggregation.ts`) emitted a bare
29+
* `{$sum: '$flag'}` / `{$avg: '$flag'}`. MongoDB's arithmetic accumulators
30+
* ignore non-numeric values, so with no numeric value `$sum` folds to its
31+
* identity `0` and `$avg` answers `null`. Fixed by the boolean-only `$cond`
32+
* coercion.
33+
* 2. **The instrument** (`mongodb-pipeline-evaluator.testkit.ts`) applied that
34+
* same "ignore non-numeric" rule to `$min` / `$max`, which are order
35+
* statistics over BSON canonical order and rank booleans perfectly well. The
36+
* `{$min: '$flag'}` lowering was, and remains, correct.
37+
*
38+
* ⛔ Applying (1)'s coercion to `$min` / `$max` would answer `0` / `1` and break
39+
* #11249 in the opposite direction. {@link describe} block "the emitted lowering
40+
* keeps the two halves apart" pins that it was not, reading the emitted stages
41+
* rather than trusting the values — the values alone cannot tell a `$min` over a
42+
* boolean from a `$min` over a coerced `1`/`0` once the evaluator ranks both.
43+
*
44+
* ## ⚠️ What this suite deliberately does NOT answer
45+
*
46+
* Whether a real mongod agrees. `runPipeline` is the in-process evaluator
47+
* `mongodb-aggregation-translation.test.ts` uses: it holds the LOWERING to the
48+
* shared table by MongoDB's documented semantics, and this fleet cannot fetch a
49+
* mongod binary at all (#5517). Every operator it models is read from the
50+
* manual, not observed — `$type` and the BSON-order `$min`/`$max` this card
51+
* added included. No test name here claims otherwise.
52+
*
53+
* ## The fixture
54+
*
55+
* `AGGREGATION_ROWS` — the shared aggregate-vocabulary fixture — plus a boolean
56+
* `flag` column. The distribution is {@link FLAG_BY_ID}, the one already landed
57+
* on `main` in `driver-sql`'s #11635 suite, chosen over the other distribution
58+
* in this card's record (`true,false,true,true,false,false`) so the two faces'
59+
* grouped numbers are comparable rather than merely both 3-true/3-false.
60+
*/
61+
62+
import { describe, it, expect } from 'vitest';
63+
import { AGGREGATION_ROWS } from '@objectstack/spec/data';
64+
import {
65+
buildAggregationPipeline,
66+
postProcessAggregation,
67+
type AggregationInput,
68+
} from './mongodb-aggregation.js';
69+
import {
70+
runPipeline,
71+
UnsupportedShape,
72+
type Doc,
73+
} from './mongodb-pipeline-evaluator.testkit.js';
74+
75+
/** The alias every measure is projected under — never a fixture column. */
76+
const MEASURE = 'measure';
77+
78+
/**
79+
* The `flag` column, keyed by fixture row id: 3 true / 3 false, with `east`
80+
* (rows 5–6) all-true. The per-group split is deliberately asymmetric —
81+
* `west` is `[T,F,F,F]` and `east` `[T,T]` — so `east`'s grouped `min` is
82+
* `true`: a measure computed over the whole table, or a sticky per-column
83+
* constant, goes red on that cell rather than passing by symmetry.
84+
*/
85+
const FLAG_BY_ID: Record<string, boolean> = {
86+
'1': true,
87+
'2': false,
88+
'3': false,
89+
'4': false,
90+
'5': true,
91+
'6': true,
92+
};
93+
94+
/**
95+
* The six shared rows plus `flag`, and two columns that exist only to drive the
96+
* empty-input branch of an order statistic: `voidcol` is an explicit `null` on
97+
* every row, and no row carries `absent` at all.
98+
*/
99+
const ROWS: Doc[] = (AGGREGATION_ROWS as unknown as Doc[]).map((row) => ({
100+
...row,
101+
flag: FLAG_BY_ID[row.id as string],
102+
voidcol: null,
103+
}));
104+
105+
/** Run one aggregation end to end and answer the whole-table measure. */
106+
function measure(func: string, field: string): unknown {
107+
const aggregations = [{ function: func, field, alias: MEASURE }] as AggregationInput[];
108+
const pipeline = buildAggregationPipeline({ aggregations });
109+
return postProcessAggregation(runPipeline(ROWS, pipeline), aggregations)[0]?.[MEASURE];
110+
}
111+
112+
/** Run one aggregation grouped by `region`, keyed by group value. */
113+
function byRegion(func: string, field: string): Record<string, unknown> {
114+
const aggregations = [{ function: func, field, alias: MEASURE }] as AggregationInput[];
115+
const pipeline = buildAggregationPipeline({ aggregations, groupBy: ['region'] });
116+
const rows = postProcessAggregation(runPipeline(ROWS, pipeline), aggregations);
117+
return Object.fromEntries(rows.map((row) => [String(row.region), row[MEASURE]]));
118+
}
119+
120+
describe('[#11151] driver-mongodb — the fixture this suite measures', () => {
121+
// The fixture read back rather than trusted: a seed that dropped a row or
122+
// folded the flags would turn every value below into a test of another table.
123+
it('is six rows, 3 true / 3 false, with east all-true', () => {
124+
expect(ROWS).toHaveLength(6);
125+
expect(ROWS.filter((r) => r.flag === true), 'true rows').toHaveLength(3);
126+
expect(ROWS.filter((r) => r.region === 'east').map((r) => r.flag)).toEqual([true, true]);
127+
expect(ROWS.filter((r) => r.region === 'west').map((r) => r.flag)).toEqual([
128+
true,
129+
false,
130+
false,
131+
false,
132+
]);
133+
});
134+
});
135+
136+
describe('[#11151] the ruled arithmetic half — sum / avg count a boolean as 1 or 0', () => {
137+
it('sum(flag) answers 3, not $sum’s identity 0', () => {
138+
expect(measure('sum', 'flag')).toBe(3);
139+
});
140+
141+
it('avg(flag) answers 0.5, not null', () => {
142+
expect(measure('avg', 'flag')).toBe(0.5);
143+
});
144+
145+
it('grouped sum/avg answer per group: west [T,F,F,F], east [T,T]', () => {
146+
expect(byRegion('sum', 'flag')).toEqual({ west: 1, east: 2 });
147+
expect(byRegion('avg', 'flag')).toEqual({ west: 0.25, east: 1 });
148+
});
149+
});
150+
151+
describe('[#11151] the ruled order-statistic half — min / max answer JSON booleans', () => {
152+
// Asserted STRICTLY. `0` / `1` satisfies a `Number()` reading and is exactly
153+
// the answer #11249 ruled against, so a loose comparison here would pass on
154+
// the one wrong value this half exists to exclude.
155+
it('min(flag) answers false — the boolean, not 0 and not null', () => {
156+
expect(measure('min', 'flag')).toBe(false);
157+
});
158+
159+
it('max(flag) answers true — the boolean, not 1 and not null', () => {
160+
expect(measure('max', 'flag')).toBe(true);
161+
});
162+
163+
it('grouped min/max answer per-group members, and east’s min is true', () => {
164+
// `east` is the load-bearing cell: all-true, so its `min` is `true`. A
165+
// whole-table computation or a sticky `false` fails here and only here.
166+
expect(byRegion('min', 'flag')).toEqual({ west: false, east: true });
167+
expect(byRegion('max', 'flag')).toEqual({ west: true, east: true });
168+
});
169+
170+
it('min/max over a column that is null or absent everywhere answer null', () => {
171+
// The manual's rule: null and missing are IGNORED, and a group left with
172+
// nothing answers `null`. Not folded to `false`, which is what a boolean
173+
// face that manufactured a default would do.
174+
expect(measure('min', 'voidcol'), 'explicit null on every row').toBeNull();
175+
expect(measure('max', 'voidcol'), 'explicit null on every row').toBeNull();
176+
expect(measure('min', 'absent'), 'a column no row carries').toBeNull();
177+
expect(measure('max', 'absent'), 'a column no row carries').toBeNull();
178+
});
179+
});
180+
181+
describe('[#11151] the emitted lowering keeps the two halves apart', () => {
182+
const emit = (func: string): unknown =>
183+
buildAggregationPipeline({
184+
aggregations: [{ function: func, field: 'flag', alias: MEASURE }] as AggregationInput[],
185+
})[0];
186+
187+
const COERCED = {
188+
$cond: [{ $eq: [{ $type: '$flag' }, 'bool'] }, { $cond: ['$flag', 1, 0] }, '$flag'],
189+
};
190+
191+
it('sum and avg wrap the aggregand in the boolean-only coercion', () => {
192+
expect(emit('sum')).toEqual({ $group: { _id: null, [MEASURE]: { $sum: COERCED } } });
193+
expect(emit('avg')).toEqual({ $group: { _id: null, [MEASURE]: { $avg: COERCED } } });
194+
});
195+
196+
it('⛔ min and max are left BARE — the coercion is not applied to them', () => {
197+
// The load-bearing pin of this file. `$min`/`$max` over a coerced aggregand
198+
// would answer `0`/`1` — arithmetic where #11249 ruled for a member of the
199+
// input domain — and the VALUES cannot catch it once the evaluator ranks
200+
// booleans, because both spellings then produce an answer. Only the emitted
201+
// stage distinguishes them.
202+
expect(emit('min')).toEqual({ $group: { _id: null, [MEASURE]: { $min: '$flag' } } });
203+
expect(emit('max')).toEqual({ $group: { _id: null, [MEASURE]: { $max: '$flag' } } });
204+
expect(JSON.stringify(emit('min')), 'no $cond reached the min arm').not.toContain('$cond');
205+
expect(JSON.stringify(emit('max')), 'no $cond reached the max arm').not.toContain('$cond');
206+
});
207+
208+
it('a fieldless sum/avg is unchanged — the coercion needs a path to coerce', () => {
209+
const fieldless = buildAggregationPipeline({
210+
aggregations: [{ function: 'sum', alias: MEASURE }] as AggregationInput[],
211+
})[0];
212+
expect(fieldless).toEqual({ $group: { _id: null, [MEASURE]: { $sum: 0 } } });
213+
});
214+
});
215+
216+
describe('[#11151] CONTROLS — what neither half was allowed to move', () => {
217+
// These two already agreed with every other face before this card. A suite
218+
// holding only the broken cells cannot show that the fix was targeted.
219+
it('count(flag) / count_distinct(flag) are unchanged', () => {
220+
expect(measure('count', 'flag'), 'count over the boolean column').toBe(6);
221+
expect(measure('count_distinct', 'flag'), 'count_distinct over the boolean column').toBe(2);
222+
expect(byRegion('count', 'flag')).toEqual({ west: 4, east: 2 });
223+
expect(byRegion('count_distinct', 'flag')).toEqual({ west: 2, east: 1 });
224+
});
225+
226+
it('all four functions over the NUMERIC column are untouched', () => {
227+
expect(measure('sum', 'score'), 'sum(score)').toBe(210);
228+
expect(measure('avg', 'score'), 'avg(score)').toBe(35);
229+
expect(measure('min', 'score'), 'min(score)').toBe(10);
230+
expect(measure('max', 'score'), 'max(score)').toBe(60);
231+
});
232+
233+
it('sum/avg still IGNORE a non-numeric string — the coercion is boolean-only', () => {
234+
// `stage` is a string column with two explicit nulls. Coercing wider would
235+
// mean adopting `Number('won') === NaN` or a `toNumber` that maps it to 0;
236+
// both are separate questions from this card, and neither is adopted.
237+
expect(measure('sum', 'stage'), 'sum over a string column').toBe(0);
238+
expect(measure('avg', 'stage'), 'avg over a string column').toBeNull();
239+
});
240+
});
241+
242+
describe('[#11151] the evaluator REFUSES a type it does not rank, rather than answering null', () => {
243+
// The head note of `mongodb-pipeline-evaluator.testkit.ts` promises this
244+
// instrument "refuses every shape it does not model". The `$min`/`$max` arms
245+
// were the exception: they filtered to numbers and answered `null` for
246+
// everything else, silently. A wrong answer from a strict evaluator is worse
247+
// than a refusal, because the red it produces reads as a defect in the driver
248+
// under test — which is how this card's own `min`/`max` half was first
249+
// misattributed to the lowering.
250+
const withDate: Doc[] = ROWS.map((row) => ({ ...row, when: new Date('2026-01-01T00:00:00Z') }));
251+
252+
for (const func of ['min', 'max'] as const) {
253+
it(`${func} over an unmodelled BSON type raises UnsupportedShape`, () => {
254+
const aggregations = [
255+
{ function: func, field: 'when', alias: MEASURE },
256+
] as AggregationInput[];
257+
const pipeline = buildAggregationPipeline({ aggregations });
258+
expect(() => runPipeline(withDate, pipeline)).toThrow(UnsupportedShape);
259+
expect(() => runPipeline(withDate, pipeline)).toThrow(/unmodelled type/);
260+
});
261+
}
262+
263+
// The refusal is a property of the evaluator's coverage, NOT a statement
264+
// about MongoDB: a real mongod ranks dates fine. Extending `bsonRank` is the
265+
// way to model one, and until someone does the instrument says so out loud
266+
// instead of answering `null`.
267+
it('the types it DOES rank all answer, so the refusal above is not blanket', () => {
268+
expect(measure('min', 'score'), 'number').toBe(10);
269+
expect(measure('min', 'stage'), 'string').toBe('lost');
270+
expect(measure('min', 'flag'), 'boolean').toBe(false);
271+
});
272+
});

packages/drivers/driver-mongodb/src/mongodb-aggregation.test.ts

Lines changed: 19 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,22 @@
33
import { describe, it, expect } from 'vitest';
44
import { buildAggregationPipeline, postProcessAggregation } from './mongodb-aggregation.js';
55

6+
/**
7+
* [#11151] The aggregand `sum` and `avg` now consume: the field path, with a
8+
* BOOLEAN rendered as the number it is worth. Spelled once here because the
9+
* pins below are about ALIAS ROUTING and stage shape — which accumulator lands
10+
* under which key — and the accumulator's own internals are pinned, with the
11+
* reasons, in `mongodb-11151-boolean-aggregand-answers.test.ts`.
12+
*
13+
* ⛔ `min` / `max` deliberately do NOT take this wrapper: they are order
14+
* statistics and #11249 ruled they answer `false` / `true`, not `0` / `1`. The
15+
* `builds min/max aggregations` case below reads their bare field path and is
16+
* the pin that says so from this file.
17+
*/
18+
const coerced = (path: string) => ({
19+
$cond: [{ $eq: [{ $type: path }, 'bool'] }, { $cond: [path, 1, 0] }, path],
20+
});
21+
622
describe('MongoDB Aggregation Pipeline Builder', () => {
723
it('builds empty pipeline for no options', () => {
824
expect(buildAggregationPipeline({})).toEqual([]);
@@ -32,7 +48,7 @@ describe('MongoDB Aggregation Pipeline Builder', () => {
3248
groupBy: ['region'],
3349
});
3450
expect(pipeline).toEqual([
35-
{ $group: { _id: { region: '$region' }, total_amount: { $sum: '$amount' } } },
51+
{ $group: { _id: { region: '$region' }, total_amount: { $sum: coerced('$amount') } } },
3652
{ $project: { _id: 0, region: '$_id.region', total_amount: 1 } },
3753
]);
3854
});
@@ -50,8 +66,8 @@ describe('MongoDB Aggregation Pipeline Builder', () => {
5066
const groupStage = pipeline[0];
5167
expect(groupStage.$group._id).toEqual({ customer_id: '$customer_id' });
5268
expect(groupStage.$group.order_count).toEqual({ $sum: 1 });
53-
expect(groupStage.$group.total).toEqual({ $sum: '$amount' });
54-
expect(groupStage.$group.average).toEqual({ $avg: '$amount' });
69+
expect(groupStage.$group.total).toEqual({ $sum: coerced('$amount') });
70+
expect(groupStage.$group.average).toEqual({ $avg: coerced('$amount') });
5571
});
5672

5773
it('adds $sort stage', () => {

0 commit comments

Comments
 (0)