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.