M2: the aggregate command surface, and the refusals that had to come first #2
Reference in New Issue
Block a user
Delete Branch "m2-aggregate-command-surface"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
M2 as re-scoped by amendment A6: the
aggregatecommand surface, plus therefusals that had to come first. The engine itself (expression evaluator, stage
iterator, accumulators) is M2.5 and is untouched here.
Scorecard 194/97/196 → 201/90/196.
aggregate-*.jsongoes 9 pass / 13 failto 16 pass / 6 fail.
The milestone's gate was wrong twice, and both were found by measuring
The named corpus does not exist.
mongodb/specificationshas noaggregation suite; the thirteen
aggregate-*.jsonfiles this project runs liveinside
crudand test the aggregate command — cursors, readConcern, thewrite stages, collation,
let.$lookup,$unwind,$facet,$addFieldsand
$replaceRootappear nowhere in the pinned corpus. One milestone name wascovering two milestones, so they are split: M2 is the command surface, M2.5 is
the engine, and
$lookup/$facetare explicitly out of M2.5's first cut.The target on the corpus we kept was unreachable too, and that only showed
up when the 13 failures were priced one by one instead of counted: 6 of them
are blocked on M2.5, M4 and M8. Worth noting what the three
db.aggregate()cases really need — implementing
{aggregate: 1}moves none of them, becauseeach then fails on
$listLocalSessionsinstead. The design review had pricedthat line "orthogonal, and cheap"; it was cheap and it was worth nothing. The
gate now reads 0 fail among the seven reachable, and the seven that moved in
this branch are exactly those seven.
Tier 0: six silent wrong answers become errors
Measured on a live server, before:
{$avg: "$x"}answered0, and so did$maxand$push; a compound_idcollapsed every document into one groupkeyed by the unevaluated expression;
{$literal: 1}came back echoed; and a$matchafter a$projectstill matched on the field the projection hadremoved. Six of eight probed pipelines succeeded with a wrong result.
An unrecognised stage is a bug report; an
$avgthat returns0is acorrupted report nobody files — and the corpus cannot see either, which is why
this had to be measured against mongod rather than inferred from a test run.
$groupnow names its vocabulary and validates every accumulator before adocument is read.
$projectwas fixed rather than refused, since the streamalready knew how to materialize: it transforms where it stands, so
$match,$sortand$groupafter it read what it left. Nine error codes read offmongod 8.3.7, and the replies match it on code, codeName and message text.
A remote crash, pre-existing
$grouptook[]const u64and was handed the offset list whatever the streamwas made of. After a stage that materializes, that list is empty while the
bounds count trees. No authentication in front of it. Confirmed against the
binary at the previous commit rather than assumed, so it is committed on its
own as a pre-existing defect and not as a regression of this work.
$outand$mergewrite from the dispatch epilogueBoth write to a collection the pipeline is not reading, and three things stood
against doing that in the handler:
aggregateis a.readcommand, dispatchtakes locks from a static table keyed on the command name before the handler
runs, and
Collection.lockallows exactly one collection lock at a time. Sothe handler computes under the locks it has and leaves the output in
Context.pending_write; the epilogue applies it with nothing held. The.read/.writecontract is amended in its own comment rather than quietlybroken. The test's mutation is the argument in one line: apply the write inside
the stage and it deadlocks rather than fails.
$outis not atomic here, and that is stated in the code where somebodywill be standing when it matters. mongod replaces the target atomically; this
engine has no cross-collection atomicity and no rename to build one from, so a
crash between the drop and the last insert leaves part of the new output where
MongoDB would leave the whole of the old.
Verification
191/191 unit tests in ReleaseFast and ReleaseSafe, 83/83 fuzz, e2e 49, e2e2
concurrent, e2e3 16, e2e4 17, e2e6 72, e2e7 86. Every refusal has a mutation
recorded beside it.
Left open, recorded not fixed
query.projectis shared withfind'sprojection, where a nested inclusion(
{a: {b: 1}}) reads as falsy and returns the whole document minus that field.That is broken rather than unimplemented — narrowing is something a
projection should do — so it wants a fix, not a refusal, and it belongs with
whichever milestone takes
find's projection seriously.M2 Tier 0, and the first commit of the milestone: stop answering wrong numbers with `ok: 1`. `$group` had one accumulator, `$sum`, and one expression vocabulary, `$field` paths and constants -- open-coded twice and applied to whatever arrived. Everything outside that fell through to a zero. Measured on a live server before this commit: `{$avg: "$x"}` answered `0`, and so did `$max` and `$push`; a compound `_id` collapsed every document into one group keyed by the unevaluated expression; `{$literal: 1}` came back echoed; `{$sum: {$multiply: [...]}}` was `0`. Six of eight probed pipelines succeeded with a wrong result. That is a worse failure than an unimplemented one. An unrecognised stage is a bug report; an `$avg` that returns `0` is a corrupted report nobody files. It is also invisible to the gate: the corpus this project runs has no aggregation stage tests at all, which is what PLAN amendment A6 is about. So the vocabulary is now named -- `GroupExpr` is a path or a constant, and `classify_expr` is the single place the `$`-prefix distinction is made -- and every accumulator is validated before a document is read. A pipeline that cannot be answered is refused whole rather than half-answered. Five error codes added, every one read off mongod 8.3.7 rather than recalled, and the replies now match it on code and codeName for all six `$group` shapes probed: unknown group operator 15952 Location15952 a group specification needs _id 15955 Location15955 accumulator is not an object 40234 Location40234 two operators in one 40238 Location40238 unrecognized expression 168 InvalidPipelineOperator `$avg` and friends are reported as *unknown* operators, which is the choice `location_unrecognized_stage` already made for `$addFields`: this server reports what it does not implement using MongoDB's own code for "no such thing", because a code MongoDB never emits would break the error-code parity every milestone is held to. The message names the construct. `$sum` over a non-number still contributes nothing -- that is MongoDB's rule, not a stand-in for an unimplemented one, and the comment says so where it would otherwise read as another silent zero. The scorecard does not move: 194/97/196 before and after, byte-identical. That is not a disappointment, it is the design review's central finding arriving on schedule -- the corpus cannot see any of this, which is why M2.5 has to bring its own. Mutations: drop the accumulator-name check and `$avg` answers `ok: 1` again; drop the `_id` classification and the compound `_id` does. 188/188 unit tests in ReleaseFast and ReleaseSafe, 83/83 fuzz, e2e 49, e2e2 concurrent, e2e3 16, e2e4 17, e2e6 72, e2e7 86.A remote crash, present before this milestone and reachable by any client with a two-stage pipeline: aggregate: [{$group: {_id: "$k", n: {$sum: 1}}}, {$group: {_id: null, g: {$sum: 1}}}] thread panic: index out of bounds: index 2, len 0 `$group` took `[]const u64` and was handed `offs.items[start..end]` whatever the stream was made of. A pipeline starts as slab offsets -- matched and reordered in place, never materialized -- and flips to generated documents the moment a stage produces something the slab does not hold. After that `offs` is empty while `start`/`end` count trees, so the slice ran off an empty list and took the server thread down. There is no authentication in front of it. Found while making `$project` a real stage, which reaches the same branch; confirmed against the binary at the previous commit rather than assumed, so this is a pre-existing defect and not a regression of that work. It is committed on its own for that reason. `Stream` names the two forms the rest of the pipeline had been carrying implicitly in `in_trees`, and `$group` now reads whichever is live. The slab side stays byte-walked -- grouping a million documents does not build a million trees to read one field -- and `path_in_pairs` is the tree counterpart of `query_path_value_bytes` for the other half. The test groups by a key and then counts the groups, and sums `$n`, a path that only resolves against the generated document. Its mutation -- hand `run_group` the offsets unconditionally -- aborts the run rather than failing it, which is why this wanted a test and not a code read. Answers byte-identically to mongod 8.3.7 on the pipeline above. 189/189 unit tests in ReleaseFast and ReleaseSafe, 83/83 fuzz, e2e 49, e2e3 16, e2e4 17, e2e6 72, e2e7 86.It set a variable that the emit applied once. Three consequences, all measured on a live server before this changed rather than read off the code: - only the *last* `$project` in a pipeline had any effect; - a `$match` after one still matched on the field it had removed; - `{y: {$literal: 5}}` read as falsy, which flipped the whole projection into its exclusion branch and returned every document minus `y`, where mongod adds a computed `y`. Now it transforms the stream where it stands, materializing into the same tree form `$group` already produced. `$match`, `$sort` and `$group` after it read what it left, which is what "stage" means. What it cannot do is refused rather than mis-read. A computed field needs the expression evaluator M2.5 brings, and a nested spec (`{a: {b: 1}}`) needs a narrowing `query.project` does not do -- both were falsy, and falsy is the answer that quietly returned the whole document. Codes read off mongod 8.3.7, and the replies now match it on code, codeName and message text for the empty, the mixed and the computed forms: projection must have at least one field 51272 Location51272 cannot mix inclusion and exclusion 31254 Location31254 unknown expression in $project 31325 Location31325 Recorded, not fixed here: `query.project` is shared with `find`'s `projection`, where a nested inclusion has the same falsy reading and the same wrong answer. That is a real gap rather than an unimplemented feature -- narrowing is something this projection should do -- so it is its own fix, not a refusal, and it belongs with whichever milestone takes `find`'s projection seriously. Scorecard unchanged again at 194/97/196. The corpus has no `$project` stage tests either; A6 said so, and this is the second commit to confirm it. Mutations: drop the refusals and the empty projection answers `ok: 1`; put the projection back on the emit and the `$match`-after-`$project` case goes red. 190/190 unit tests in ReleaseFast and ReleaseSafe, 83/83 fuzz, e2e 49, e2e2 concurrent, e2e3 16, e2e4 17, e2e6 72, e2e7 86.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.