Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 31 additions & 0 deletions .changeset/memory-boolean-aggregand.md
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,31 @@
---
"@objectstack/driver-memory": patch
---

**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).

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`:

```
[memory] unfiltered {"avg_sla_violated":null, "closed_count":4}
[memory] closed-filter {"avg_sla_violated":null, "closed_count":4}
[sqlite] unfiltered {"avg_sla_violated":0.4, "closed_count":4}
[sqlite] closed-filter {"avg_sla_violated":0.25, "closed_count":4}
```

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.

**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.

**Both of this package's faces carried the defect independently**, and both are fixed:

- the **data face** (`computeAggregate`, reached by `engine.aggregate` pushdown — the door the report's own repro takes) selected with `typeof v === 'number'`;
- 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.

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).

**`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.

**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.

**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.
50 changes: 48 additions & 2 deletions packages/drivers/driver-memory/src/memory-analytics.ts
Original file line numberDiff line numberDiff line change
Expand Up@@ -480,6 +480,52 @@ function sizeDistinctSet(values: readonly unknown[]): number {
return new Set(values.filter((v) => v !== null && v !== undefined)).size;
}

/**
* [#11065] `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
*
* mingo's `$avg` mirrors MongoDB's and IGNORES a non-numeric value, so a
* whole boolean column averaged to `null` and summed to `0` (measured: five
* bools in, `{avg: null, sum: 0}` out). Under SQL the same rows answer
* `AVG(col)` = 0.4 and `SUM(col)` = 2, and objectql's in-memory fallback
* (`in-memory-aggregation.ts`) answers those numbers 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 tile bound
* to the measure renders a percentage on one driver and a blank on the other,
* with no error on either path.
*
* ## Why an EXPRESSION and not post-processing
*
* The `count_distinct` neighbour above collects with `$addToSet` and sizes the
* array after `driver.aggregate` returns ({@link sizeDistinctSet}). Sum and
* average must NOT be built that way: the post-processing step runs after the
* pipeline's own `$sort` and `$limit` stages, so a measure left as an array
* until then would be SORTED as an array — `order` over a `sum` or `avg`
* measure is an ordinary analytics query, unlike ordering by `count_distinct`.
* Keeping the rule inside the `$group` expression leaves every later stage
* looking at the number it expects.
*
* ## The narrowness is deliberate
*
* Only `bool` is rewritten. Everything else — null, missing, a non-numeric
* string — reaches mingo exactly as before and is ignored by the accumulator
* exactly as before (measured: a numeric column carrying a null, a string and
* a missing key answers identically with and without this wrapper). 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.
*
* The data face carries the same rule in JavaScript (`memory-driver.ts`,
* `computeAggregate`); `memory-boolean-aggregand.test.ts` drives both, because
* one face aligned alone is how this package's faces come to disagree.
*/
function numericAggregandExpr(path: string): Record<string, unknown> {
return { $cond: [{ $eq: [{ $type: path }, 'bool'] }, { $cond: [path, 1, 0] }, path] };
}

/**
* [#7853] A `JSON.stringify` replacer that renders a `RegExp` operand instead of
* dropping it — the one value type the pipeline dump carries that
Expand DownExpand Up@@ -1192,9 +1238,9 @@ export class MemoryAnalyticsService implements IAnalyticsService {
case 'count':
return { $sum: 1 };
case 'sum':
return { $sum: `$${fieldPath}` };
return { $sum: numericAggregandExpr(`$${fieldPath}`) };
case 'avg':
return { $avg: `$${fieldPath}` };
return { $avg: numericAggregandExpr(`$${fieldPath}`) };
case 'min':
return { $min: `$${fieldPath}` };
case 'max':
Expand Down
Loading
Loading