diff --git a/docs/M2_DESIGN_REVIEW.md b/docs/M2_DESIGN_REVIEW.md new file mode 100644 index 0000000..0b8acc4 --- /dev/null +++ b/docs/M2_DESIGN_REVIEW.md @@ -0,0 +1,257 @@ +# M2 design review — aggregation expansion + +PLAN §3 gives M2 one line of scope ("pipeline stages and expression engine, +tiered scope defined at M2 design review") and one line of gate ("aggregate +spec suite green"), and §6 lists the three questions it deferred: the +stage/expression tiers, which spec files are the gate, and whether +`$lookup`/`$unwind`/`$facet` make the first cut. + +This document answers them, and reports one finding that has to be settled +before any of the rest is worth discussing. + +Everything below is measured against the M1 tip (`a748a3d`) unless it says +otherwise. Nothing here is recalled. + +--- + +## 1. The gate named in the plan does not exist + +`mongodb/specifications` has no aggregation suite. The full list of `source/` +directories at the pinned commit is: `auth`, `bson-*`, `causal-consistency`, +`change-streams`, `client-side-encryption`, `collation`, +`collection-management`, `command-logging-and-monitoring`, `compression`, +`connection-*`, `crud`, `gridfs`, `index-management`, `load-balancers`, +`logging`, `read-write-concern`, `retryable-*`, `run-command`, `sessions`, +`server-discovery-and-monitoring`, `server-selection`, `transactions`, +`unified-test-format`, `versioned-api`, and a handful of others. There is no +`aggregation`. + +The thirteen `aggregate-*.json` files we do run live inside `crud`, and they +test the aggregate **command surface** — cursor and `batchSize`, `readConcern` +and `readPreference` routing, the write semantics of `$out`/`$merge`, +`collation`, `let`, `allowDiskUse`, `rawData`. They barely touch stages, and +they touch expressions not at all: `$lookup`, `$unwind`, `$facet`, +`$addFields`, `$replaceRoot` do not appear anywhere in the pinned corpus. + +So "aggregate spec suite green" is not a gate for the milestone the scope line +describes. It is a gate for a much smaller milestone that happens to share the +command name. **Whichever option §4 picks, PLAN's M2 gate line has to be +rewritten**, and that is the first decision this review asks for. + +Where the current corpus stands, for reference: + +``` +aggregate-*.json overall 9 pass 13 fail 19 skip +``` + +| failure | cases | +|---|---| +| `$merge` unimplemented | 5 | +| `$out` unimplemented | 3 | +| `db.aggregate()` — `{aggregate: 1}`, no collection name | 3 | +| `collation` in aggregate | 1 | +| `let` | 1 | + +Of the 19 skips, 13 are environmental (`needs topology replicaset`, `needs +server >= 5.0`, `needs server >= 8.2.0`) and can never turn green on a +standalone. A literal reading of "green" is unreachable for that reason alone. + +--- + +## 2. Where the implementation actually is + +**Seven stages**, all in one `if/else if` chain in `cmd_aggregate`: +`$match`, `$sort`, `$skip`, `$limit`, `$project`, `$group`, `$count`. +Anything else answers `Location40324`, with the code verified against a real +mongod. + +(`$set` and `$unset` exist in the tree as **update operators** in +`src/update.zig`. They are not pipeline stages. An earlier count of nine +stages in this project's notes was wrong.) + +**There is no expression engine.** `$field` path resolution is open-coded in +two places inside `run_group`, and nowhere else. `$sum` is the only +accumulator. + +That last sentence is the important one, because of *how* the missing pieces +fail. Probed against a live server on three documents +(`{g:'a',x:10}, {g:'a',x:20}, {g:'b',x:30}`): + +| pipeline | answer | correct answer | +|---|---|---| +| `$group: {_id:'$g', s:{$sum:'$x'}}` | `[{a,30},{b,30}]` | ✅ | +| `$group: {_id:'$g', a:{$avg:'$x'}}` | `[{a,0},{b,0}]` | `[{a,15},{b,30}]` | +| `$group: {_id:'$g', m:{$max:'$x'}}` | `[{a,0},{b,0}]` | `[{a,20},{b,30}]` | +| `$group: {_id:'$g', p:{$push:'$x'}}` | `[{a,0},{b,0}]` | arrays | +| `$group: {_id:{g:'$g'}, n:{$sum:1}}` | one group, `_id` = `{"g":"$g"}` | two groups | +| `$project:{g:1}` then `$match:{x:10}` | matches | matches nothing | +| `$group: {_id:{$literal:1}, ...}` | `_id` = `{"$literal":1}` | `_id` = `1` | +| `$addFields` | `Location40324` | (unimplemented) | + +**Six of those eight are silent wrong answers with `ok: 1`.** Only +`$addFields` fails honestly. That is the shape of the risk in this area and it +should drive the milestone: an unrecognised stage is a bug report, an `$avg` +that returns `0` is a corrupted report that nobody files. It is also precisely +the class the M1 spec-runner work was about — and the corpus cannot see any of +it, because the corpus has no stage tests. + +**The pipeline model will not carry the milestone as it stands.** The stream is +a materialized window `[start, end)` over either slab offsets or generated +documents, and each stage moves the bounds. That works for the filtering and +reordering stages and nothing else: + +- `$project` is not a stage transform at all. It sets a variable that is + applied once, at emit — so only the last `$project` in a pipeline has any + effect, and a `$match` after a `$project` sees the unprojected document. +- `$unwind` is 1→N. There is no way to express that in a window over the input. +- `$lookup` needs a second collection under a second lock, inside a stage. +- `$facet` needs the same stream fanned into several independent sub-pipelines. +- `allowDiskUse` is accepted and ignored. `$group` and `$sort` are unbounded in + memory; the three passing `allowdiskuse` cases pass because the option is + parsed, not because anything spills. `src/spill.zig` exists and is used by + index builds, not here. + +So M2 is not "add stages to the chain". It is: build an expression evaluator, +change the stream model to a per-stage document iterator, and then stages +become cheap. Anything that adds stages before those two exist adds more +silent wrong answers. + +--- + +## 3. What that implies for tiers + +The dependency order is forced, not chosen: + +**Tier 0 — stop lying (no new features).** Refuse what is not implemented +instead of answering `0`: unknown accumulators, non-path expressions where a +path is expected, `$project` used anywhere but last. Every one of these becomes +an error with a measured mongod code. This is small, and it converts six silent +wrong answers into six honest ones. It is also the only tier that makes the +later ones measurable, for the same reason Stage 1 preceded Stage 3 in M1. + +**Tier 1 — the expression engine.** A real evaluator over `bson.Value`: +field paths, `$literal`, comparison and boolean operators, arithmetic, +`$cond`/`$ifNull`/`$switch`, string and date operators. Everything above +depends on it, including `$project`'s computed fields and `$group`'s `_id`. + +**Tier 2 — the stage iterator.** Replace the window with a per-stage pull +model, so a stage can emit more or fewer documents than it consumed. `$project` +becomes a real transform. Carries `$addFields`/`$set`, `$unset`, +`$replaceRoot`, `$unwind`, `$sortByCount`. + +**Tier 3 — the accumulators.** `$avg`, `$min`, `$max`, `$first`, `$last`, +`$push`, `$addToSet`, `$count`, `$mergeObjects`. Cheap once Tier 1 exists. + +**Tier 4 — the expensive stages.** `$lookup` (second collection, lock order), +`$facet` (sub-pipelines), `$unionWith`, `$graphLookup`. Each is a design +question of its own. + +**Tier 5 — the write stages.** `$out`, `$merge`. These are not aggregation +work at all — they are a write path that happens to be spelled as a stage, with +their own durability and atomicity questions. They are also 8 of the 13 current +`aggregate-*` failures. + +**Orthogonal, and cheap:** `db.aggregate()` (`{aggregate: 1}`), 3 failures. +**Orthogonal, and not cheap:** `allowDiskUse` meaning something, which is a +memory-bound question for `$group`/`$sort` and belongs with whichever tier +first makes those able to hold a lot. + +--- + +## 4. Gate options + +Each option is stated with what it would actually prove and what it costs. + +### Option A — "the aggregate command surface" + +Gate: the 13 `aggregate-*.json` files in the crud corpus, minus the 13 +environmental skips. Scope: Tier 5 + `db.aggregate()` + collation + `let`. + +- **Proves:** the aggregate *command* behaves — cursors, read concern, write + stages, database-level form. +- **Does not prove:** any stage or expression is correct. All six silent wrong + answers survive this gate untouched. +- **Cost:** small. Two write stages and three small fixes. +- **Honest name:** this is not "aggregation expansion". If chosen, M2 should be + renamed and the aggregation engine deferred to its own milestone. + +### Option B — a hand-built stage corpus, modelled on mongod's own + +Gate: a new `tests/spec/aggregate/` corpus in the unified format the runner +already reads, with cases ported from `mongod`'s `jstests/aggregation` and +every expected result **measured against mongod 8.3.7**, the way the cursor and +`lsid` error codes were. + +- **Proves:** exactly what the milestone claims to build, at whatever tier + depth the corpus is written to. +- **Cost:** the corpus is the milestone's second deliverable and a real one — + but the runner, the entity model, the matchers and `expectEvents` all exist + already, so the marginal cost is writing cases, not machinery. +- **Risk, stated plainly:** a corpus we author is a corpus we can accidentally + write to match our own bugs. The mitigation is the one this project already + uses: every expectation measured against a real mongod before it is + committed, never recalled. + +### Option C — differential against mongod + +Gate: a harness that runs the same pipelines against MultiforaDB and mongod +8.3.7 and diffs the results, over generated pipelines and a fixed corpus. + +- **Proves:** the most, by a distance, and it finds the silent-wrong-answer + class by construction rather than by someone thinking to test for it. +- **Cost:** mongod becomes a test dependency (it already is one for + measurement, but not for the gate). Result ordering and floating-point + formatting need normalising, and `$group` output order is genuinely + unspecified. +- **Note:** this is a *harness*, not a corpus, so it composes with B rather + than competing with it — B gives regression cases with committed + expectations, C finds new ones. + +### Option D — keep the plan's line and reinterpret "green" + +Gate: `aggregate-*.json` at 22 pass / 0 fail / 13 skip, and declare the skips +out of scope. + +- **Proves:** the same as A. +- **Why it is listed:** only to be rejected explicitly. It reads as if the + aggregation milestone were gated on aggregation tests, and it is not. If A is + the decision, say A. + +--- + +## 5. Recommendation + +**Tier 0 first, alone, before any scoping decision is final** — the same +argument that put `expectEvents` before the free list in M1. Six silent wrong +answers are six results nobody can measure past, and turning them into errors +is a day's work that changes what every later number means. It will move the +scorecard *down*, and that is the point. + +**Then B as the gate, with C as the harness that feeds it.** Option A is a real +piece of work and should happen — the `$merge`/`$out` failures are 8 of 13 — +but it should be named for what it is and probably belongs in M6 (admin/ops) or +its own small milestone, not as the gate for an engine. + +**Tiers 1→3 as M2 proper**, with Tier 4 explicitly out of the first cut. That +answers §6's third question: **`$unwind` yes** (it falls out of Tier 2 for +free), **`$lookup` and `$facet` no** — `$lookup` because a stage that takes a +second collection lock while the pipeline holds one is a lock-order design +question this milestone should not be carrying, and `$facet` because it needs +sub-pipeline execution that Tier 2's iterator should be allowed to settle +first. + +**One thing to decide that this review cannot decide alone:** whether +`allowDiskUse` must mean something by the end of M2. It is currently accepted +and ignored, which is the same category of lie as `$avg` returning `0` — a +client that passes it believes it is protected. If the answer is yes, the +memory bound on `$group`/`$sort` joins Tier 2 rather than trailing it. + +--- + +## 6. What this review does not cover + +- `$out`/`$merge` durability semantics (atomic swap? logged? crash behaviour?). + Needed before Tier 5 is scoped. +- Whether the expression engine is shared with `update`'s pipeline-update form, + which M3 will need. Probably yes; it changes where the code lives. +- Index use inside a pipeline beyond the existing leading-`$match` pushdown.