Skip to content

Commit 2095040

Browse files
os-zhuangclaude
andauthored
fix(driver-memory): count a boolean aggregand as 1/0 in avg and sum (#11153)
* fix(driver-memory): count a boolean aggregand as 1/0 in avg and sum `avg` over a boolean column returned `null` on driver-memory while SQLite answered the arithmetically correct rate over the same rows, and objectql's in-memory fallback answered that same rate — driver-memory was the lone outlier of the three. Both of this package's faces carried the divergence independently: the data face filtered aggregands with `typeof v === 'number'`, and the analytics face emitted a bare mingo `$avg`, which ignores non-numeric values as MongoDB's does. `sum` shares the data face's arm and had the same defect one function over (`SUM(bool)` is "how many true" on every SQL face), so it is aligned with `avg` rather than left behind. The coercion is boolean-only: null, missing and non-numeric strings reach the accumulators unchanged and stay excluded, rather than being folded to 0. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RfyXxZ2WPjcjhuXpiQQc3y * test(driver-memory): record the reverse verification, and add the changeset The ablation was run after the fix was committed so it had a restore point: both coercions reverted to origin/main's expressions, the mutation confirmed on disk by marker counts in both directions, then restored by a trap. Measured 11 failed / 4 passed against a predicted 10 — the direction held exactly (every failure on a value, none on a throw) and the predicted count was off by one. Both numbers are recorded rather than the prediction being quietly rewritten to match. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RfyXxZ2WPjcjhuXpiQQc3y --------- Co-authored-by: Claude <noreply@anthropic.com>
1 parent 8cc8401 commit 2095040

4 files changed

Lines changed: 416 additions & 3 deletions

File tree

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,31 @@
1+
---
2+
"@objectstack/driver-memory": patch
3+
---
4+
5+
**Bug fix (one query, two answers):** `avg` and `sum` over a **boolean** column now answer the same numbers on `driver-memory` that every SQL face and objectql's in-memory fallback already answered, instead of `null` and `0` (#11065).
6+
7+
Measured on one `AnalyticsService.queryDataset` call, one dataset, one set of rows, run twice — five `crm_case` records, four closed (three with `is_sla_violated: false`, one `true`), one open with `true`:
8+
9+
```
10+
[memory] unfiltered {"avg_sla_violated":null, "closed_count":4}
11+
[memory] closed-filter {"avg_sla_violated":null, "closed_count":4}
12+
[sqlite] unfiltered {"avg_sla_violated":0.4, "closed_count":4}
13+
[sqlite] closed-filter {"avg_sla_violated":0.25, "closed_count":4}
14+
```
15+
16+
SQLite's are the arithmetically correct numbers (2/5 unfiltered, 1/4 over the closed cases). The `count` measures beside them agreed on both drivers, and so did a `derived` ratio built on those counts — the divergence was specific to averaging a boolean.
17+
18+
**There were three implementations of "average a column" in play, and this driver was the lone outlier.** SQLite lowers `AVG(col)`; objectql's in-memory fallback (`in-memory-aggregation.ts`) coerces with `Number(v)`, and `Number(true) === 1`; `driver-memory` selected its aggregands with `typeof v === 'number'`, which drops every boolean, leaving `nums.length === 0` and returning `null`. So this is an alignment to the two faces that already agreed, not a new convention.
19+
20+
**Both of this package's faces carried the defect independently**, and both are fixed:
21+
22+
- the **data face** (`computeAggregate`, reached by `engine.aggregate` pushdown — the door the report's own repro takes) selected with `typeof v === 'number'`;
23+
- the **analytics face** (`buildAggregator`) emitted a bare mingo `$avg`, and mingo ignores a non-numeric value exactly as MongoDB does — measured at `{avg: null, sum: 0}` over the same five rows.
24+
25+
Fixing one alone would have left the other free to keep its own answer, which is the shape of this package's recurring defect (#5374, #6814).
26+
27+
**`sum` is aligned with `avg` deliberately.** The two share the data face's arm, and `SUM(bool)` is "how many true" on every SQL face and in the objectql fallback; correcting only the function the report named would have left the identical defect alive one function over, answering `0` for a column with two `true` rows.
28+
29+
**The coercion is boolean-only, and the narrowness is the point.** `null`, a missing key and a non-numeric string reach the accumulators unchanged and stay excluded. Adopting the wider half of objectql's `toNumber` — which maps a non-numeric string to `0` — would average garbage as zero rather than excluding it; whether that is right is a separate question this change does not open, and a regression row pins the exclusion on both faces so it cannot arrive by accident.
30+
31+
**Why it mattered beyond the number.** Neither face errors, so a dashboard tile bound to a rate measure rendered a percentage under SQL and a blank here, indistinguishable from "no matching rows". The test-facing half is worse: a suite pinning such a measure on the in-memory driver asserted against `null` and could not fail in the direction that matters. Nothing in the repo pinned the old value — the shared cross-driver fixture (`AGGREGATION_ROWS`) carries no boolean column at all, which is why every face could disagree here unobserved.

‎packages/drivers/driver-memory/src/memory-analytics.ts‎

Lines changed: 48 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -480,6 +480,52 @@ function sizeDistinctSet(values: readonly unknown[]): number {
480480
return new Set(values.filter((v) => v !== null && v !== undefined)).size;
481481
}
482482

483+
/**
484+
* [#11065] `path`, with a BOOLEAN rendered as the number it is worth — the
485+
* aggregand expression `$sum` and `$avg` consume on this face.
486+
*
487+
* ## What it is for
488+
*
489+
* mingo's `$avg` mirrors MongoDB's and IGNORES a non-numeric value, so a
490+
* whole boolean column averaged to `null` and summed to `0` (measured: five
491+
* bools in, `{avg: null, sum: 0}` out). Under SQL the same rows answer
492+
* `AVG(col)` = 0.4 and `SUM(col)` = 2, and objectql's in-memory fallback
493+
* (`in-memory-aggregation.ts`) answers those numbers too, because its
494+
* `toNumber` is `Number(v)` and `Number(true) === 1`. A rate measure over a
495+
* flag column — an SLA-violation rate, a win rate — is the ordinary shape of
496+
* that query, and the two answers are not two spellings of one: a tile bound
497+
* to the measure renders a percentage on one driver and a blank on the other,
498+
* with no error on either path.
499+
*
500+
* ## Why an EXPRESSION and not post-processing
501+
*
502+
* The `count_distinct` neighbour above collects with `$addToSet` and sizes the
503+
* array after `driver.aggregate` returns ({@link sizeDistinctSet}). Sum and
504+
* average must NOT be built that way: the post-processing step runs after the
505+
* pipeline's own `$sort` and `$limit` stages, so a measure left as an array
506+
* until then would be SORTED as an array — `order` over a `sum` or `avg`
507+
* measure is an ordinary analytics query, unlike ordering by `count_distinct`.
508+
* Keeping the rule inside the `$group` expression leaves every later stage
509+
* looking at the number it expects.
510+
*
511+
* ## The narrowness is deliberate
512+
*
513+
* Only `bool` is rewritten. Everything else — null, missing, a non-numeric
514+
* string — reaches mingo exactly as before and is ignored by the accumulator
515+
* exactly as before (measured: a numeric column carrying a null, a string and
516+
* a missing key answers identically with and without this wrapper). Coercing
517+
* wider would mean adopting `toNumber`'s other half, which maps a non-numeric
518+
* string to `0` and so averages garbage as zero rather than excluding it —
519+
* a separate question from this one.
520+
*
521+
* The data face carries the same rule in JavaScript (`memory-driver.ts`,
522+
* `computeAggregate`); `memory-boolean-aggregand.test.ts` drives both, because
523+
* one face aligned alone is how this package's faces come to disagree.
524+
*/
525+
function numericAggregandExpr(path: string): Record<string, unknown> {
526+
return { $cond: [{ $eq: [{ $type: path }, 'bool'] }, { $cond: [path, 1, 0] }, path] };
527+
}
528+
483529
/**
484530
* [#7853] A `JSON.stringify` replacer that renders a `RegExp` operand instead of
485531
* dropping it — the one value type the pipeline dump carries that
@@ -1192,9 +1238,9 @@ export class MemoryAnalyticsService implements IAnalyticsService {
11921238
case 'count':
11931239
return { $sum: 1 };
11941240
case 'sum':
1195-
return { $sum: `$${fieldPath}` };
1241+
return { $sum: numericAggregandExpr(`$${fieldPath}`) };
11961242
case 'avg':
1197-
return { $avg: `$${fieldPath}` };
1243+
return { $avg: numericAggregandExpr(`$${fieldPath}`) };
11981244
case 'min':
11991245
return { $min: `$${fieldPath}` };
12001246
case 'max':

0 commit comments

Comments
 (0)