From 76afa75efe3ac75891de78a208b662bb930b71b7 Mon Sep 17 00:00:00 2001 From: "A.Shakhmatov" Date: Sun, 9 Aug 2026 22:06:57 +0300 Subject: [PATCH] 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 | 191 ++++++++++++++++++++++++++------- tests/spec/aggregate/README.md | 13 ++- 2 files changed, 163 insertions(+), 41 deletions(-) diff --git a/src/commands.zig b/src/commands.zig index 225470e..3b058f0 100644 --- a/src/commands.zig +++ b/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")) { - // `$avg`, `$max`, `$push` and the rest are real MongoDB operators - // that this server does not implement; see the note on the error - // codes for why they are reported as unknown rather than with an - // invented code. They used to answer `0`. + const kind = acc_kind(spec[0].key) orelse { + // Still the honest answer for the ones that remain unimplemented -- + // `$stdDevPop`, `$mergeObjects`, `$top` -- reported with MongoDB's + // own code for "no such operator". See the note on the codes. 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; - } - 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; - try accs.append(arena, .{ .key = p.key, .arg = arg }); + }; + // `$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}); + break :blk (try classify_expr(reply, spec[0].value, what)) orelse return null; + }; + 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); - @memset(sums, 0); - gop.value_ptr.* = .{ .id_value = id_value, .sums = sums }; + const states = try ctx.gpa.alloc(AccState, accs.items.len); + @memset(states, .{}); + 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, - // which is MongoDB's rule and not a stand-in for an unimplemented - // one: `{$sum: "$name"}` over strings really is zero. - const v: bson.Value = switch (acc.arg) { - .path => |path| (try stream_path(walk_arena.allocator(), coll, src, i, path)) orelse .null, + const st = &gop.value_ptr.states[a]; + if (acc.kind == .count) { + st.n += 1; + continue; + } + // 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) { - .int32 => |n| @floatFromInt(n), - .int64 => |n| @floatFromInt(n), - .double => |n| n, - else => 0, - }; + switch (acc.kind) { + .count => unreachable, + .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 => 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 sum_value: bson.Value = if (sum == @floor(sum) and sum <= 2_147_483_647 and sum >= -2_147_483_648) - .{ .int32 = @intFromFloat(sum) } - else - .{ .double = sum }; - pairs[1 + a] = .{ .key = acc.key, .value = sum_value }; + const st = entry.value_ptr.states[a]; + pairs[1 + a] = .{ .key = acc.key, .value = switch (acc.kind) { + .sum => numeric_value(st.total), + // Nothing numeric seen is `null`, not zero: an average of no + // values is not an average of zero. + .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, }, diff --git a/tests/spec/aggregate/README.md b/tests/spec/aggregate/README.md index eb7f6de..56c4722 100644 --- a/tests/spec/aggregate/README.md +++ b/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 -codes. The ten are M2.5's work: `$avg`, `$min`, `$max`, `$first`, `$last`, -`$push`, `$addToSet`, `$count`, and a compound `_id`. +The one that remains is the compound `_id`, which needs the expression +evaluator and is the next tier. The corpus found its first real disagreement on +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.