89eae1cd9c
Priced the 13 aggregate failures one by one instead of counting them. Six
cannot turn green in M2 whatever is built: three need $listLocalSessions (M4)
*and* $addFields (M2.5), two need the expression engine, one needs a collation
(M8). So the gate reads '0 fail among the seven reachable', with the other six
named and attributed.
Note what the three db.aggregate() cases actually need. Implementing
{aggregate: 1} moves none of them, because each then fails on
$listLocalSessions instead. The review priced that line 'orthogonal, and
cheap'; it was cheap and worth nothing.
The seven that remain are $out and $merge, and they need a decision the review
had no authority to take. They write to a collection the pipeline is not
reading, and three things stand against that: aggregate is declared .read and
the dispatch contract says only .write commands may mutate; dispatch acquires
locks from a static table keyed on the command name, before the handler runs;
and Collection.lock's own comment states the invariant that decides it -- never
more than one collection lock at a time.
That is the same objection that kept $lookup out of M2.5's first cut, and the
review failed to apply it to the write stages in the same breath. Recorded as
the review's own error rather than quietly corrected. Two options set out in
§7, plus the durability question neither of them answers: mongod's $out
replaces the target atomically and this engine has no cross-collection
atomicity.
Nothing past Tier 0 is implemented until one is chosen.
344 lines
16 KiB
Markdown
344 lines
16 KiB
Markdown
# 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 |
|
|
|
|
All 19 skips are environmental -- 10 distinct requirements, some covering a
|
|
whole file at once (`needs topology replicaset`, `needs server >= 5.0`, `needs
|
|
server >= 8.2.0`, `needs server <= 4.2.99`). Not one can turn green on a
|
|
standalone, so a literal reading of "green" is unreachable for that reason
|
|
alone, and the gate has to be stated as *0 fail* rather than *all pass*.
|
|
|
|
---
|
|
|
|
## 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. Decision
|
|
|
|
**Taken: the command surface first (Option A as M2), the engine second (M2.5),
|
|
with Tier 0 folded into M2.** Recorded as PLAN amendment A6; the §3 rows are
|
|
rewritten and §6's deferred entry now points here.
|
|
|
|
The cost of that order is on the record: the engine is what gives wrong answers
|
|
today, and this leaves them alive for a further milestone. Tier 0 is what makes
|
|
the trade acceptable rather than merely convenient — every unimplemented
|
|
construct stops answering `0` and starts answering an error, so the wrong
|
|
answers become visible bug reports instead of quiet ones while the command
|
|
surface is built. It is not optional under this sequencing.
|
|
|
|
The recommendation the review arrived at independently is kept below, since a
|
|
decision is easier to revisit when the argument it overrode is still legible.
|
|
|
|
## 5.1 Recommendation as written before the decision
|
|
|
|
**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.
|
|
|
|
---
|
|
|
|
## 7. Post-decision measurement: the gate is not reachable either
|
|
|
|
Written after Tier 0 landed, when the 13 failures were priced one by one
|
|
instead of counted. Section 1 established that the *corpus* named in the plan
|
|
was the wrong instrument; this establishes that the *target* set on the
|
|
corpus we kept is unreachable too. Both were found by looking rather than by
|
|
assuming, and this one only by opening each case.
|
|
|
|
| cases | what they need | milestone |
|
|
|---|---|---|
|
|
| 5 `aggregate-merge.json` | `$merge` with a plain `into` | M2, blocked below |
|
|
| 2 `aggregate-out.json` | `$out` with a plain target | M2, blocked below |
|
|
| 3 `db-aggregate*.json` | `$listLocalSessions` **and** `$addFields` | M4 + M2.5 |
|
|
| 2 `aggregate-let.json` | `let`, `$$var`, `$expr` | M2.5 |
|
|
| 1 `aggregate-collation.json` | a case-insensitive collation | M8 |
|
|
|
|
So **6 of 13 can never turn green in M2**, whatever is built, and the gate has
|
|
to read *0 fail among the seven this milestone can reach*, with the other six
|
|
named and attributed. Note what the three `db.aggregate()` cases really need:
|
|
implementing `{aggregate: 1}` moves none of them, because each then fails on
|
|
`$listLocalSessions` instead. The design review priced that line "orthogonal,
|
|
and cheap"; it was cheap and it was worth nothing.
|
|
|
|
### And the seven need a decision this review has no authority to take
|
|
|
|
`$out` and `$merge` write to a collection the pipeline is not reading. Three
|
|
things in the current design stand against that, and none of them is a detail:
|
|
|
|
- `aggregate` is declared `.kind = .read`, and the dispatch contract is
|
|
explicit that "only `.write` commands may call engine mutation functions".
|
|
- dispatch acquires locks from a **static table keyed on the command name**,
|
|
before the handler runs, for the one collection named by the command.
|
|
- `Collection.lock`'s own comment states the invariant that decides this:
|
|
*never more than one collection lock at a time*. Taking the target's
|
|
exclusive lock while holding the source's shared one breaks it directly.
|
|
|
|
This is the same shape as the objection that kept `$lookup` out of M2.5's first
|
|
cut — "a stage that takes a second collection's lock while the pipeline holds
|
|
one is a lock-order question that should not ride along" — and the review
|
|
should have applied it to the write stages in the same breath. It did not,
|
|
which is the review's own error and is recorded here rather than quietly
|
|
corrected.
|
|
|
|
Two ways out, both real:
|
|
|
|
**(a) A pipeline that ends in a write stage is a write command.** Compute the
|
|
effective lock shape from the pipeline rather than the name: catalog exclusive,
|
|
no collection lock from dispatch, and the handler takes source-shared →
|
|
release → target-exclusive in that order. Heavier than mongod, and it
|
|
serializes such an aggregate against the whole catalog, but it keeps every
|
|
existing invariant intact and is honest about what the command is.
|
|
|
|
**(b) The write stages run in the dispatch epilogue,** after every lock is
|
|
released, next to the commit and the checkpoint that already live there. The
|
|
handler stashes what to write. Cheaper and better-behaved under concurrency,
|
|
but it amends the `.read`/`.write` contract, so the contract's comment has to
|
|
change with it and say why.
|
|
|
|
Either way the durability question is open and belongs with whichever is
|
|
chosen: mongod's `$out` replaces the target collection *atomically*, and this
|
|
engine has no cross-collection atomicity. A pipeline that fails after writing
|
|
half its output must not leave the target half-replaced.
|
|
|
|
**Nothing is implemented past Tier 0 until that is decided.**
|
|
|