commands: the $group accumulators

Nine of them -- `$sum`, `$avg`, `$min`, `$max`, `$first`, `$last`, `$push`,
`$addToSet`, `$count` -- and the corpus goes 9 pass / 10 fail to 18 pass /
1 fail. The one left is the compound `_id`, which needs the expression
evaluator.

Landed before that evaluator, against the tier order the design review set
out, and the corpus is why: every one of these takes a single value per
document, a path or a constant, so nine of its ten failures turned out to be
reachable without one. `classify_expr` already produced exactly that value.

What the recording caught, which is the argument for measuring expectations
rather than writing them:

  - `$avg` over a group with no numeric value is **null**, not `0`. A divisor
    that counted documents rather than numbers would pass every test anybody
    would think to write by hand, and be wrong on the one group that matters.
  - `$min`/`$max` compare across types in canonical BSON order, so the maximum
    of `30`, `7` and `"not a number"` is the string.
  - `$push` skips an absent field but would push an explicit null, so "resolved
    to nothing" and "resolved to null" cannot be the same value internally --
    which is why the accumulators take `?bson.Value` and not `.null`.
  - `$first`/`$last` follow input order, including when the value is absent:
    `$last` of a missing field is null, not the last present one.

`AccState` is one struct rather than a union: the fields are small and every
site already switches on the kind, so a union would add a tag test where a
switch was going to be anyway. Its arrays are the gpa's, the values inside them
the reply arena's -- they outlive the group and travel with the documents.

`numeric_value` is the int32-or-double narrowing MongoDB reports, shared now
between the accumulators and `cmd_aggregate`'s count fast path. It was written
twice before; a divergence between them would make `countDocuments` disagree
with the pipeline it is a shortcut for.

`$avg` and `$push` came out of the Tier 0 refusal test, replaced by
`$stdDevPop` and `$mergeObjects`. The refusal is a property of what is missing
rather than of a list, and the test should read that way.

191/191 unit tests in ReleaseFast and ReleaseSafe, 83/83 fuzz, e2e 49, e2e3 16,
e2e4 17, e2e6 72, e2e7 86, crud corpus unchanged at 201/90/196.

src/commands.zig

@@ -2749,12 +2749,63 @@ fn classify_expr(reply: *wire.Reply, v: bson.Value, what: []const u8) !?GroupExp
     return null;
 }
 
+/// A running total as MongoDB reports it: an integral value inside int32 range
+/// comes back an int32, anything else a double. The count fast path in
+/// `cmd_aggregate` mirrors this exactly, and a divergence between them would
+/// make `countDocuments` disagree with the pipeline it is a shortcut for.
+fn numeric_value(x: f64) bson.Value {
+    if (x == @floor(x) and x <= 2_147_483_647 and x >= -2_147_483_648) {
+        return .{ .int32 = @intFromFloat(x) };
+    }
+    return .{ .double = x };
+}
+
+/// The accumulators this server implements. Every one of them takes a single
+/// value per document -- a path or a constant -- so none of them needs the
+/// expression evaluator, which is why they land before it rather than after:
+/// the corpus said nine of its ten failures were reachable without one.
+const AccKind = enum { sum, avg, min, max, first, last, push, add_to_set, count };
+
+fn acc_kind(name: []const u8) ?AccKind {
+    const table = .{
+        .{ "$sum", AccKind.sum },       .{ "$avg", AccKind.avg },
+        .{ "$min", AccKind.min },       .{ "$max", AccKind.max },
+        .{ "$first", AccKind.first },   .{ "$last", AccKind.last },
+        .{ "$push", AccKind.push },     .{ "$addToSet", AccKind.add_to_set },
+        .{ "$count", AccKind.count },
+    };
+    inline for (table) |e| {
+        if (std.mem.eql(u8, name, e[0])) return e[1];
+    }
+    return null;
+}
+
 /// One output field of a `$group`, with its argument already classified.
 const Accumulator = struct {
     key: []const u8,
+    kind: AccKind,
     arg: GroupExpr,
 };
 
+/// What one accumulator has seen of one group so far.
+///
+/// One struct rather than a union: the fields are small, the branches are
+/// per-kind anyway, and a union would need a tag test at every site that a
+/// switch on `kind` already makes.
+const AccState = struct {
+    /// `$sum`'s running total, and `$avg`'s numerator.
+    total: f64 = 0,
+    /// Documents seen for `$count`; *numeric values* seen for `$avg`, which is
+    /// what makes `$avg` ignore the non-numbers rather than average them in as
+    /// zeroes.
+    n: u64 = 0,
+    /// `$min`/`$max`/`$first`/`$last`. Null means nothing qualified, which is
+    /// the answer mongod gives for all four.
+    value: ?bson.Value = null,
+    /// `$push` and `$addToSet`.
+    items: std.ArrayListUnmanaged(bson.Value) = .empty,
+};
+
 /// Minimal $group: `_id` of a constant or "$field", and `$sum` accumulators
 /// over the same. Everything else is refused rather than answered.
 fn run_group(
@@ -2802,11 +2853,10 @@ fn run_group(
             try reply.put_error(@intFromEnum(ErrorCode.location_one_accumulator), "Location40238", detail);
             return null;
         }
-        if (!std.mem.eql(u8, spec[0].key, "$sum")) {
+        const kind = acc_kind(spec[0].key) orelse {
-            // `$avg`, `$max`, `$push` and the rest are real MongoDB operators
+            // Still the honest answer for the ones that remain unimplemented --
-            // that this server does not implement; see the note on the error
+            // `$stdDevPop`, `$mergeObjects`, `$top` -- reported with MongoDB's
-            // codes for why they are reported as unknown rather than with an
+            // own code for "no such operator". See the note on the codes.
-            // invented code. They used to answer `0`.
             const detail = try std.fmt.allocPrint(
                 arena,
                 "unknown group operator '{s}'",
@@ -2814,15 +2864,22 @@ fn run_group(
             );
             try reply.put_error(@intFromEnum(ErrorCode.location_unknown_group_operator), "Location15952", detail);
             return null;
-        }
+        };
+        // `$count` takes `{}` and nothing else, so it never reaches the
+        // expression classifier -- an empty document is exactly what that
+        // refuses.
+        const arg: GroupExpr = if (kind == .count)
+            .{ .constant = .null }
+        else blk: {
             const what = try std.fmt.allocPrint(arena, "the argument of '{s}'", .{p.key});
-        const arg = (try classify_expr(reply, spec[0].value, what)) orelse return null;
+            break :blk (try classify_expr(reply, spec[0].value, what)) orelse return null;
-        try accs.append(arena, .{ .key = p.key, .arg = arg });
+        };
+        try accs.append(arena, .{ .key = p.key, .kind = kind, .arg = arg });
     }
 
     const Group = struct {
         id_value: bson.Value,
-        sums: []f64,
+        states: []AccState,
     };
     var groups: std.StringHashMapUnmanaged(Group) = .empty;
     defer groups.deinit(ctx.gpa);
@@ -2854,46 +2911,105 @@ fn run_group(
             const key = try ctx.gpa.dupe(u8, id_key_buf.items);
             try keys_owned.append(ctx.gpa, key);
             gop.key_ptr.* = key;
-            const sums = try ctx.gpa.alloc(f64, accs.items.len);
+            const states = try ctx.gpa.alloc(AccState, accs.items.len);
-            @memset(sums, 0);
+            @memset(states, .{});
-            gop.value_ptr.* = .{ .id_value = id_value, .sums = sums };
+            gop.value_ptr.* = .{ .id_value = id_value, .states = states };
         }
         for (accs.items, 0..) |acc, a| {
-            // A `$sum` over something that is not a number contributes nothing,
+            const st = &gop.value_ptr.states[a];
-            // which is MongoDB's rule and not a stand-in for an unimplemented
+            if (acc.kind == .count) {
-            // one: `{$sum: "$name"}` over strings really is zero.
+                st.n += 1;
-            const v: bson.Value = switch (acc.arg) {
+                continue;
-                .path => |path| (try stream_path(walk_arena.allocator(), coll, src, i, path)) orelse .null,
+            }
+            // A path that resolves to nothing is *absent*, which several of
+            // these treat differently from a present null: `$push` skips it
+            // where it would push an explicit null, and `$min` ignores it.
+            const found: ?bson.Value = switch (acc.arg) {
+                .path => |path| try stream_path(walk_arena.allocator(), coll, src, i, path),
                 .constant => |c| c,
             };
-            gop.value_ptr.sums[a] += switch (v) {
+            switch (acc.kind) {
-                .int32 => |n| @floatFromInt(n),
+                .count => unreachable,
-                .int64 => |n| @floatFromInt(n),
+                .sum, .avg => {
+                    // A number or nothing. `{$sum: "$name"}` over strings really
+                    // is zero -- MongoDB's rule, not a stand-in for something
+                    // unimplemented -- and `$avg`'s divisor counts only what it
+                    // added, which is what makes it ignore the rest rather than
+                    // average them in as zeroes.
+                    const num: ?f64 = switch (found orelse bson.Value.null) {
+                        .int32 => |n| @as(f64, @floatFromInt(n)),
+                        .int64 => |n| @as(f64, @floatFromInt(n)),
                         .double => |n| n,
-                else => 0,
+                        else => null,
                     };
+                    if (num) |x| {
+                        st.total += x;
+                        st.n += 1;
+                    }
+                },
+                .min, .max => {
+                    const v = found orelse continue;
+                    // Canonical BSON order, across types: the smaller of a
+                    // number and a string is the number, which is a type rule
+                    // rather than a value one.
+                    if (st.value) |cur| {
+                        const ord = bson.compare(v, cur);
+                        const take = if (acc.kind == .min) ord == .lt else ord == .gt;
+                        if (!take) continue;
+                    }
+                    st.value = try bson.copy_value(arena, v);
+                },
+                .first => {
+                    if (st.n == 0) {
+                        if (found) |v| st.value = try bson.copy_value(arena, v);
+                        st.n = 1;
+                    }
+                },
+                .last => {
+                    st.value = if (found) |v| try bson.copy_value(arena, v) else null;
+                },
+                .push => {
+                    const v = found orelse continue;
+                    try st.items.append(arena, try bson.copy_value(arena, v));
+                },
+                .add_to_set => {
+                    const v = found orelse continue;
+                    // Linear, because a set of BSON values has no cheap hash
+                    // that respects canonical equality, and because a group key
+                    // with thousands of distinct values in one set is not the
+                    // shape this is for.
+                    for (st.items.items) |seen| {
+                        if (bson.compare(seen, v) == .eq) break;
+                    } else try st.items.append(arena, try bson.copy_value(arena, v));
+                },
+            }
         }
     }
 
     var out: std.ArrayListUnmanaged(*const bson.Document) = .empty;
     errdefer out.deinit(ctx.gpa);
     var it = groups.iterator();
-    // free sum arrays
+    // The state arrays are the gpa's; the values inside them are the reply
+    // arena's and outlive this function with the documents they end up in.
     defer {
         var git = groups.iterator();
-        while (git.next()) |e| ctx.gpa.free(e.value_ptr.sums);
+        while (git.next()) |e| ctx.gpa.free(e.value_ptr.states);
     }
     while (it.next()) |entry| {
         const npairs = 1 + accs.items.len;
         const pairs = try arena.alloc(bson.Pair, npairs);
         pairs[0] = .{ .key = "_id", .value = try bson.copy_value(arena, entry.value_ptr.id_value) };
         for (accs.items, 0..) |acc, a| {
-            const sum: f64 = entry.value_ptr.sums[a];
+            const st = entry.value_ptr.states[a];
-            const sum_value: bson.Value = if (sum == @floor(sum) and sum <= 2_147_483_647 and sum >= -2_147_483_648)
+            pairs[1 + a] = .{ .key = acc.key, .value = switch (acc.kind) {
-                .{ .int32 = @intFromFloat(sum) }
+                .sum => numeric_value(st.total),
-            else
+                // Nothing numeric seen is `null`, not zero: an average of no
-                .{ .double = sum };
+                // values is not an average of zero.
-            pairs[1 + a] = .{ .key = acc.key, .value = sum_value };
+                .avg => if (st.n == 0) .null else numeric_value(st.total / @as(f64, @floatFromInt(st.n))),
+                .count => numeric_value(@floatFromInt(st.n)),
+                .min, .max, .first, .last => st.value orelse .null,
+                .push, .add_to_set => .{ .array = st.items.items },
+            } };
         }
         const doc = try arena.create(bson.Document);
         doc.* = bson.Document{ .arena = undefined, .pairs = pairs };
@@ -4629,19 +4745,22 @@ test "$group refuses what it cannot compute instead of answering zero" {
     const path_x = bson.Value{ .string = "$x" };
     const sum_one = bson.Value{ .doc = &.{.{ .key = "$sum", .value = .{ .int32 = 1 } }} };
     const cases = [_]Case{
+        // `$avg` and `$push` stood here until the accumulators landed. The
+        // ones that remain unimplemented answer the same way, which is the
+        // point: the refusal is a property of what is missing, not of a list.
         .{
-            .name = "$avg",
+            .name = "$stdDevPop",
             .group = &.{
                 .{ .key = "_id", .value = .{ .string = "$k" } },
-                .{ .key = "v", .value = .{ .doc = &.{.{ .key = "$avg", .value = path_x }} } },
+                .{ .key = "v", .value = .{ .doc = &.{.{ .key = "$stdDevPop", .value = path_x }} } },
             },
             .code = 15952,
         },
         .{
-            .name = "$push",
+            .name = "$mergeObjects",
             .group = &.{
                 .{ .key = "_id", .value = .{ .string = "$k" } },
-                .{ .key = "v", .value = .{ .doc = &.{.{ .key = "$push", .value = path_x }} } },
+                .{ .key = "v", .value = .{ .doc = &.{.{ .key = "$mergeObjects", .value = path_x }} } },
             },
             .code = 15952,
         },

tests/spec/aggregate/README.md

@@ -58,12 +58,15 @@ answer measures the version gap, not the engine.
 
 ## Where it stands
 
-Recorded against mongod 8.3.7, run against the M2 tip:
+Recorded against mongod 8.3.7. At the M2 tip it read 9 pass / 10 fail; with the
+accumulators in:
 
 ```
-group-accumulators.json    9 pass   10 fail   0 skip
+group-accumulators.json   18 pass    1 fail   0 skip
 ```
 
-The nine include the four refusals M2 added, which answer with mongod's own
+The one that remains is the compound `_id`, which needs the expression
-codes. The ten are M2.5's work: `$avg`, `$min`, `$max`, `$first`, `$last`,
+evaluator and is the next tier. The corpus found its first real disagreement on
-`$push`, `$addToSet`, `$count`, and a compound `_id`.
+the way there: `$avg` over a group with no numeric value is `null`, not `0`,
+and a divisor that counted documents rather than numbers would have passed
+every test anybody would think to write by hand.