SERVER-79136 Block $group min/max rewrite in TS if there is a non-meta filterr6.0.9-rc0

(cherry picked from commit 1a6d2cc2f17e91f81714b009ee7840c1099f5440)
(cherry picked from commit 5cedf9a6870900e558b838414a3d0a436a4b0944)

6 files changed, 306 insertions, 101 deletions

diff --git a/etc/backports_required_for_multiversion_tests.yml b/etc/backports_required_for_multiversion_tests.yml
index 8aa6449197f..844109d304b 100644
--- a/etc/backports_required_for_multiversion_tests.yml
+++ b/etc/backports_required_for_multiversion_tests.yml
@@ -304,6 +304,8 @@ last-continuous:
     ticket: SERVER-67699
   - test_file: jstests/core/query/elemmatch/elemmatch_or_pushdown_paths.js
     ticket: SERVER-74954
+  - test_file: jstests/core/timeseries/timeseries_groupby_reorder.js
+    ticket: SERVER-79136
   suites: null
 last-lts:
   all:
@@ -681,4 +683,6 @@ last-lts:
     ticket: SERVER-60466
   - test_file: jstests/core/query/elemmatch/elemmatch_or_pushdown_paths.js
     ticket: SERVER-74954
+  - test_file: jstests/core/timeseries/timeseries_groupby_reorder.js
+    ticket: SERVER-79136
   suites: null
diff --git a/jstests/core/timeseries/timeseries_groupby_reorder.js b/jstests/core/timeseries/timeseries_groupby_reorder.js
index 71c4b87fa3b..ade24a59470 100644
--- a/jstests/core/timeseries/timeseries_groupby_reorder.js
+++ b/jstests/core/timeseries/timeseries_groupby_reorder.js
@@ -1,5 +1,7 @@
 /**
- * Test the behavior of $group on time-series collections.
+ * Test the behavior of $group on time-series collections. Specifically, we are targeting rewrites
+ * that replace bucket unpacking with $group over the buckets collection. Currently, only $min/$max
+ * are supported for the rewrites.
  *
  * @tags: [
  *   directly_against_shardsvrs_incompatible,
@@ -17,38 +19,199 @@ load("jstests/core/timeseries/libs/timeseries.js");
 const coll = db.timeseries_groupby_reorder;
 coll.drop();
 
-assert.commandWorked(
-    db.createCollection(coll.getName(), {timeseries: {metaField: "meta", timeField: "t"}}));
+// We will only check correctness of the results here as checking the plan in JSTests is brittle and
+// is better done in document_source_internal_unpack_bucket_test/group_reorder_test.cpp. For the
+// cases when the re-write isn't applicable, the used datasets should yield wrong result if the
+// re-write is applied.
+function runGroupRewriteTest(docs, pipeline, expectedResults) {
+    coll.drop();
+    db.createCollection(coll.getName(), {timeseries: {metaField: "meta", timeField: "time"}});
+    coll.insertMany(docs);
+    assert.docEq(expectedResults, coll.aggregate(pipeline).toArray(), () => {
+        return `Pipeline: ${tojson(pipeline)}. Explain: ${
+            tojson(coll.explain().aggregate(pipeline))}`;
+    });
+}
 
-const t = new Date();
-assert.commandWorked(coll.insert({_id: 0, t: t, b: 1, c: 1}));
-assert.commandWorked(coll.insert({_id: 0, t: t, b: 2, c: 2}));
-assert.commandWorked(coll.insert({_id: 0, t: t, b: 3, c: 3}));
+// Test with measurement group key -- a rewrite in this situation would be wrong.
+(function testNonMetaGroupKey() {
+    const t = new Date();
+    const docs = [
+        {time: t, meta: 1, key: 2, val: 1},  // global min
+        {time: t, meta: 1, key: 1, val: 3},  // min for key = 1
+        {time: t, meta: 1, key: 1, val: 5},  // max for key = 1
+        {time: t, meta: 1, key: 2, val: 7},  // global max
+    ];
+    runGroupRewriteTest(docs,
+                        [{$group: {_id: "$key", min: {$min: "$val"}}}, {$match: {_id: 1}}],
+                        [{"_id": 1, "min": 3}]);
+    runGroupRewriteTest(docs,
+                        [{$group: {_id: "$key", max: {$max: "$val"}}}, {$match: {_id: 1}}],
+                        [{"_id": 1, "max": 5}]);
+})();
 
-// Test reordering the groupby and internal unpack buckets.
-if (!isMongos(db)) {
-    const res = coll.explain("queryPlanner").aggregate([
-        {$group: {_id: '$meta', accmin: {$min: '$b'}, accmax: {$max: '$c'}}}
-    ]);
+// While a group with const group key can be re-written in terms of a group on the buckets, we don't
+// currently do it. However, when/if we start doing it, it should work.
+(function testConstGroupKey_NoFilter() {
+    const t = new Date();
+    const docs = [
+        {time: t, meta: 1, val: 0},
+        {time: t, meta: 1, val: 1},
+        {time: t, meta: 1, val: 5},
+    ];
+    runGroupRewriteTest(
+        docs, [{$group: {_id: null, min: {$min: "$val"}}}], [{"_id": null, "min": 0}]);
+    runGroupRewriteTest(
+        docs, [{$group: {_id: null, max: {$max: "$val"}}}], [{"_id": null, "max": 5}]);
+})();
 
-    assert.docEq(res.stages[1], {
-        "$group":
-            {_id: "$meta", accmin: {"$min": "$control.min.b"}, accmax: {"$max": "$control.max.c"}}
-    });
-}
+// With a filter on meta the group re-write would still apply if the group key is const.
+(function testConstGroupKey_WithFilterOnMeta() {
+    const t = new Date();
+    const docs = [
+        {time: t, meta: 1, val: 0},
+        {time: t, meta: 2, val: 1},
+        {time: t, meta: 2, val: 3},
+        {time: t, meta: 1, val: 5},
+    ];
+    runGroupRewriteTest(docs,
+                        [{$match: {meta: 2}}, {$group: {_id: null, min: {$min: "$val"}}}],
+                        [{"_id": null, "min": 1}]);
+    runGroupRewriteTest(docs,
+                        [{$match: {meta: 2}}, {$group: {_id: null, max: {$max: "$val"}}}],
+                        [{"_id": null, "max": 3}]);
+})();
+
+// In presense of a non-meta filter the group re-write doesn't apply even if the group key is const.
+(function testConstGroupKey_WithFilterOnMeasurement() {
+    const t = new Date();
+    const docs = [
+        {time: t, meta: 1, val: 0, include: false},
+        {time: t, meta: 1, val: 1, include: true},
+        {time: t, meta: 1, val: 5, include: false},
+    ];
+    runGroupRewriteTest(docs,
+                        [{$match: {include: true}}, {$group: {_id: null, min: {$min: "$val"}}}],
+                        [{"_id": null, "min": 1}]);
+    runGroupRewriteTest(docs,
+                        [{$match: {include: true}}, {$group: {_id: null, max: {$max: "$val"}}}],
+                        [{"_id": null, "max": 1}]);
+})();
 
-let res = coll.aggregate([{$group: {_id: '$meta', accmin: {$min: '$b'}, accmax: {$max: '$c'}}}])
-              .toArray();
-assert.docEq([{"_id": null, "accmin": 1, "accmax": 3}], res);
+// Test with meta group key. The group re-write applies.
+(function testMetaGroupKey_NoFilter() {
+    const t = new Date();
+    const docs = [
+        {time: t, meta: 1, val: 5},
+        {time: t, meta: 2, val: 4},
+        {time: t, meta: 2, val: 3},
+        {time: t, meta: 1, val: 1},
+    ];
+    runGroupRewriteTest(docs,
+                        [{$group: {_id: "$meta", min: {$min: "$val"}}}, {$match: {_id: 2}}],
+                        [{"_id": 2, "min": 3}]);
+    runGroupRewriteTest(docs,
+                        [{$group: {_id: "$meta", max: {$max: "$val"}}}, {$match: {_id: 2}}],
+                        [{"_id": 2, "max": 4}]);
+})();
+
+// Test with meta group key preceeded by a filter on the meta key. The re-write still applies.
+(function testMetaGroupKey_WithFilterOnMeta() {
+    const t = new Date();
+    const docs = [
+        {time: t, meta: 1, val: 5},
+        {time: t, meta: 2, val: 4},
+        {time: t, meta: 2, val: 3},
+        {time: t, meta: 1, val: 1},
+    ];
+    runGroupRewriteTest(docs,
+                        [{$match: {meta: 2}}, {$group: {_id: "$meta", min: {$min: "$val"}}}],
+                        [{"_id": 2, "min": 3}]);
+    runGroupRewriteTest(docs,
+                        [{$match: {meta: 2}}, {$group: {_id: "$meta", max: {$max: "$val"}}}],
+                        [{"_id": 2, "max": 4}]);
+})();
+
+// Test with meta group key preceeded by a filter on a measurement key. The re-write doesn't apply.
+(function testMetaGroupKey_WithFilterOnMeasurement() {
+    const t = new Date();
+    const docs = [
+        {time: t, meta: 1, val: 3, include: false},
+        {time: t, meta: 1, val: 4, include: true},
+        {time: t, meta: 1, val: 5, include: false},
+    ];
+    runGroupRewriteTest(docs,
+                        [{$match: {include: true}}, {$group: {_id: "$meta", min: {$min: "$val"}}}],
+                        [{"_id": 1, "min": 4}]);
+    runGroupRewriteTest(docs,
+                        [{$match: {include: true}}, {$group: {_id: "$meta", max: {$max: "$val"}}}],
+                        [{"_id": 1, "max": 4}]);
+})();
 
 // Test SERVER-73822 fix: complex $min and $max (i.e. not just straight field refs) work correctly.
-res = coll.aggregate([{
-              $group: {
-                  _id: '$meta',
-                  accmin: {$min: {$add: ["$b", "$c"]}},
-                  accmax: {$max: {$add: ["$b", "$c"]}}
-              }
-          }])
-          .toArray();
-assert.docEq(res, [{"_id": null, "accmin": 2, "accmax": 6}]);
+(function testMetaGroupKey_WithNonPathExpressionUnderMinMax() {
+    const t = new Date();
+    // min(a+b) != min(a) + min(b) and max(a+b) != max(a) + max(b)
+    const docs = [
+        {time: t, meta: 1, a: 1, b: 20},  // max(a + b)
+        {time: t, meta: 1, a: 2, b: 10},
+        {time: t, meta: 1, a: 3, b: 1},  // min(a + b)
+    ];
+    runGroupRewriteTest(docs,
+                        [{$group: {_id: "$meta", min: {$min: {$add: ["$a", "$b"]}}}}],
+                        [{"_id": 1, "min": 4}]);
+    runGroupRewriteTest(docs,
+                        [{$group: {_id: "$meta", max: {$max: {$add: ["$a", "$b"]}}}}],
+                        [{"_id": 1, "max": 21}]);
+})();
+
+// Test with meta group key and a non-min/max accumulator that doesn't use any fields. The buckets
+// still have to be unpacked because we don't know the number of events in uncompressed buckets.
+(function testMetaGroupKey_WithAccumulatorNotUsingAnyFields() {
+    const t = new Date();
+    const docs = [
+        {time: t, meta: 1, val: 3},
+        {time: t, meta: 3, val: 4},
+        {time: t, meta: 1, val: 5},
+    ];
+    runGroupRewriteTest(
+        docs, [{$group: {_id: "$meta", x: {$sum: 1}}}, {$match: {_id: 1}}], [{"_id": 1, "x": 2}]);
+})();
+
+// Test with meta group key and a non-min/max accumulator that uses only the meta field. This query
+// is _not_ eligible for the re-write w/o bucket unpacking because while it doesn't depend on any
+// fields of the individual events it still depends on the number of events in a bucket.
+(function testMetaGroupKey_WithNonMinMaxAccumulatorOnMeta() {
+    const t = new Date();
+    const docs = [
+        {time: t, meta: 1, val: 3},
+        {time: t, meta: 3, val: 4},
+        {time: t, meta: 1, val: 5},
+    ];
+    runGroupRewriteTest(docs,
+                        [{$group: {_id: "$meta", x: {$sum: "$meta"}}}, {$match: {_id: 1}}],
+                        [{"_id": 1, "x": 2}]);
+})();
+
+// In presence of a filter $min and $max on the meta field cannot be re-written because a filter
+// might end up selecting nothing in buckets with a particular meta.
+(function testMetaGroupKey_WithAccumulatorOnMeta_WithFilterOnMeasurement() {
+    const t = new Date();
+    const docs = [
+        {time: t, meta: 1, include: false},
+    ];
+    runGroupRewriteTest(
+        docs, [{$match: {include: true}}, {$group: {_id: "$meta", x: {$min: "$meta"}}}], []);
+})();
+
+// Test min/max on the time field (cannot rewrite $min because the control.time.min is rounded
+// down).
+(function testMetaGroupKey_WithMinMaxOnTime() {
+    const docs = [
+        {time: ISODate("2023-07-20T23:16:47.683Z"), meta: 1},
+    ];
+    runGroupRewriteTest(docs,
+                        [{$group: {_id: "$meta", min: {$min: "$time"}}}],
+                        [{_id: 1, min: ISODate("2023-07-20T23:16:47.683Z")}]);
+})();
 })();
diff --git a/src/mongo/db/exec/bucket_unpacker.h b/src/mongo/db/exec/bucket_unpacker.h
index d4167369a2d..29f2f1f30d2 100644
--- a/src/mongo/db/exec/bucket_unpacker.h
+++ b/src/mongo/db/exec/bucket_unpacker.h
@@ -152,29 +152,32 @@ public:
     };
 
     /**
-     * Takes a predicate after $_internalUnpackBucket on a bucketed field as an argument and
-     * attempts to map it to new predicates on the 'control' field. There will be a 'loose'
-     * predicate that will match if some of the event field matches, also a 'tight' predicate that
-     * will match if all of the event field matches. For example, the event level predicate {a:
-     * {$gt: 5}} will generate the loose predicate {control.max.a: {$_internalExprGt: 5}}, and the
-     * tight predicate {control.min.a: {$_internalExprGt: 5}}. The loose predicate will be added
-     * before the
-     * $_internalUnpackBucket stage to filter out buckets with no match. The tight predicate will
-     * be used to evaluate predicate on bucket level to avoid unnecessary event level evaluation.
+     * Takes a predicate after $_internalUnpackBucket as an argument and attempts to rewrite it as
+     * new predicates on the 'control' field. There will be a 'loose' predicate that will match if
+     * some of the event field matches, also a 'tight' predicate that will match if all of the event
+     * field matches.
+     *
+     * For example, the event level predicate {a: {$gt: 5}} will generate the loose predicate
+     * {control.max.a: {$_internalExprGt: 5}}. The loose predicate will be added before the
+     * $_internalUnpackBucket stage to filter out buckets with no match.
+     *
+     * Ideally, we'd like to add a tight predicate such as {control.min.a: {$_internalExprGt: 5}} to
+     * evaluate the filter on bucket level to avoid unnecessary event level evaluation. However, a
+     * bucket might contain events with missing fields that are skipped when computing the controls,
+     * so in reality we only add a tight predicate on timeField which is required to exist.
      *
      * If the original predicate is on the bucket's timeField we may also create a new loose
-     * predicate on the '_id' field to assist in index utilization. For example, the predicate
-     * {time: {$lt: new Date(...)}} will generate the following predicate:
+     * predicate on the '_id' field (as it incorporates min time for the bucket) to assist in index
+     * utilization. For example, the predicate {time: {$lt: new Date(...)}} will generate the
+     * following predicate:
      * {$and: [
      *      {_id: {$lt: ObjectId(...)}},
      *      {control.min.time: {$_internalExprLt: new Date(...)}}
      * ]}
      *
-     * If the provided predicate is ineligible for this mapping, the function will return a nullptr.
-     * This should be interpreted as an always-true predicate.
-     *
-     * When using IneligiblePredicatePolicy::kIgnore, if the predicate can't be pushed down, it
-     * returns null. When using IneligiblePredicatePolicy::kError it raises a user error.
+     * If the provided predicate is ineligible for this mapping and using
+     * IneligiblePredicatePolicy::kIgnore, both loose and tight predicates will be set to nullptr.
+     * When using IneligiblePredicatePolicy::kError it raises a user error.
      */
     static BucketPredicate createPredicatesOnBucketLevelField(
         const MatchExpression* matchExpr,
diff --git a/src/mongo/db/pipeline/document_source_internal_unpack_bucket.cpp b/src/mongo/db/pipeline/document_source_internal_unpack_bucket.cpp
index 2fa4da8f284..1c0dce7ef34 100644
--- a/src/mongo/db/pipeline/document_source_internal_unpack_bucket.cpp
+++ b/src/mongo/db/pipeline/document_source_internal_unpack_bucket.cpp
@@ -751,99 +751,125 @@ DocumentSourceInternalUnpackBucket::rewriteGroupByMinMax(Pipeline::SourceContain
     const auto& metaField = *_bucketUnpacker.bucketSpec().metaField();
 
     const auto& idFields = groupPtr->getIdFields();
+
+    // Currently, we only support simple group key. TODO: SERVER-68811. Allow rewrites of object
+    // group key if all its fields depend on the metaField only.
     if (idFields.size() != 1) {
         return {};
     }
 
     const auto& exprId = idFields.cbegin()->second;
-    // TODO: SERVER-68811. Allow rewrites if expression is constant.
     const auto* exprIdPath = dynamic_cast<const ExpressionFieldPath*>(exprId.get());
+
+    // Currently, we only support group key expression of the form {_id : "<path>"}.
+    // TODO: SERVER-68811. Allow rewrites if expression is constant.
     if (exprIdPath == nullptr) {
         return {};
     }
 
     const auto& idPath = exprIdPath->getFieldPath();
+    // {The path must be at or under the metaField for this re-write to be correct (and the zero
+    // component is always CURRENT).}
     if (idPath.getPathLength() < 2 || idPath.getFieldName(1) != metaField) {
         return {};
     }
 
-    std::vector<AccumulationStatement> accumulationStatements;
+    std::vector<AccumulationStatement> accumulationStatementsBucket;
     for (const AccumulationStatement& stmt : groupPtr->getAccumulatedFields()) {
-        const auto* exprArg = stmt.expr.argument.get();
-        if (const auto* exprArgPath = dynamic_cast<const ExpressionFieldPath*>(exprArg)) {
-            const auto& path = exprArgPath->getFieldPath();
-            if (path.getPathLength() <= 1) {
-                return {};
-            }
+        const auto& op = stmt.expr.name;
+        // If _any_ of the accumulators aren't $min/$max we won't perform the re-write (some other
+        // accs might be re-writable in terms of the bucket controls, we just haven't invested into
+        // implementing them).
+        if (op != "$min" && op != "$max") {
+            return {};
+        }
 
-            const auto& rootFieldName = path.getFieldName(1);
-            if (rootFieldName == _bucketUnpacker.bucketSpec().timeField()) {
-                // Rewrite not valid for time field. We want to eliminate the bucket
-                // unpack stage here.
-                return {};
-            }
+        const auto* exprArgPath =
+            dynamic_cast<const ExpressionFieldPath*>(stmt.expr.argument.get());
 
-            std::ostringstream os;
-            if (rootFieldName == metaField) {
-                // Update aggregates to reference the meta field.
-                os << timeseries::kBucketMetaFieldName;
+        // This is either a const or a compound expression. While some such expressions (e.g: {$min:
+        // {$add:
+        // ['$a', 2]}}) could be re-written in terms of the min/max on the control fields, in
+        // general we cannot do it (e.g. {$min: {$add: ['$a', '$b']}}), so we block the re-write.
+        if (!exprArgPath) {
+            return {};
+        }
 
-                for (size_t index = 2; index < path.getPathLength(); index++) {
-                    os << "." << path.getFieldName(index);
-                }
+        // Path can have a single component if it's using $$CURRENT or a similar variable. We don't
+        // support these.
+        const auto& path = exprArgPath->getFieldPath();
+        if (path.getPathLength() <= 1) {
+            return {};
+        }
+        const auto& accFieldName = path.getFieldName(1);
+
+        // Rewrite not valid for the timeField because control.min.time contains a rounded-down time
+        // and not the actual min time of events in the bucket.
+        if (accFieldName == _bucketUnpacker.bucketSpec().timeField()) {
+            return {};
+        }
+
+        // Build the paths for the bucket-level fields.
+        std::ostringstream os;
+        if (accFieldName == metaField) {
+            // Update aggregates to reference the meta field.
+            os << timeseries::kBucketMetaFieldName;
+
+            for (size_t index = 2; index < path.getPathLength(); index++) {
+                os << "." << path.getFieldName(index);
+            }
+        } else {
+            // Update aggregates to reference the control field.
+            const auto op = stmt.expr.name;
+            if (op == "$min") {
+                os << timeseries::kControlMinFieldNamePrefix;
+            } else if (op == "$max") {
+                os << timeseries::kControlMaxFieldNamePrefix;
             } else {
-                // Update aggregates to reference the control field.
-                const auto op = stmt.expr.name;
-                if (op == "$min") {
-                    os << timeseries::kControlMinFieldNamePrefix;
-                } else if (op == "$max") {
-                    os << timeseries::kControlMaxFieldNamePrefix;
-                } else {
-                    // Rewrite is valid only for min and max aggregates.
-                    return {};
-                }
+                MONGO_UNREACHABLE;
+            }
 
-                for (size_t index = 1; index < path.getPathLength(); index++) {
-                    if (index > 1) {
-                        os << ".";
-                    }
-                    os << path.getFieldName(index);
+            for (size_t index = 1; index < path.getPathLength(); index++) {
+                if (index > 1) {
+                    os << ".";
                 }
+                os << path.getFieldName(index);
             }
+        }
 
-            const auto& newExpr = ExpressionFieldPath::createPathFromString(
-                pExpCtx.get(), os.str(), pExpCtx->variablesParseState);
+        // Re-create the accumulator using the bucket-level paths.
+        const auto& newExpr = ExpressionFieldPath::createPathFromString(
+            pExpCtx.get(), os.str(), pExpCtx->variablesParseState);
 
-            AccumulationExpression accExpr = stmt.expr;
-            accExpr.argument = newExpr;
-            accumulationStatements.emplace_back(stmt.fieldName, std::move(accExpr));
-        } else {
-            return {};
-        }
+        AccumulationExpression accExpr = stmt.expr;
+        accExpr.argument = newExpr;
+        accumulationStatementsBucket.emplace_back(stmt.fieldName, std::move(accExpr));
     }
 
+    // Re-create the group key using the bucket-level path.
     std::ostringstream os;
     os << timeseries::kBucketMetaFieldName;
     for (size_t index = 2; index < idPath.getPathLength(); index++) {
         os << "." << idPath.getFieldName(index);
     }
-    auto exprId1 = ExpressionFieldPath::createPathFromString(
+    auto exprIdBucket = ExpressionFieldPath::createPathFromString(
         pExpCtx.get(), os.str(), pExpCtx->variablesParseState);
 
     auto newGroup = DocumentSourceGroup::create(pExpCtx,
-                                                std::move(exprId1),
-                                                std::move(accumulationStatements),
+                                                std::move(exprIdBucket),
+                                                std::move(accumulationStatementsBucket),
                                                 groupPtr->getMaxMemoryUsageBytes());
 
-    // Erase current stage and following group stage, and replace with updated group.
+    // Replace the current stage (DocumentSourceInternalUnpackBucket) and the following group stage
+    // with the new group.
     container->erase(std::next(itr));
     *itr = std::move(newGroup);
 
     if (itr == container->begin()) {
-        // Optimize group stage.
+        // Optimize the new group stage.
         return {true, itr};
     } else {
-        // Give chance of the previous stage to optimize against group stage.
+        // Give chance to the previous stage to optimize against the new group stage.
         return {true, std::prev(itr)};
     }
 }
@@ -1295,7 +1321,8 @@ Pipeline::SourceContainer::iterator DocumentSourceInternalUnpackBucket::doOptimi
             return itr;
         }
     }
-    {
+
+    if (!_eventFilter) {
         // Check if we can avoid unpacking if we have a group stage with min/max aggregates.
         auto [success, result] = rewriteGroupByMinMax(itr, container);
         if (success) {
diff --git a/src/mongo/db/pipeline/document_source_internal_unpack_bucket.h b/src/mongo/db/pipeline/document_source_internal_unpack_bucket.h
index 4e785907075..4d20a22f107 100644
--- a/src/mongo/db/pipeline/document_source_internal_unpack_bucket.h
+++ b/src/mongo/db/pipeline/document_source_internal_unpack_bucket.h
@@ -221,8 +221,8 @@ public:
 
     /**
      * Helper method which checks if we can avoid unpacking if we have a group stage with min/max
-     * aggregates. If a rewrite is possible, 'container' is modified, and we returns result value
-     * for 'doOptimizeAt'.
+     * aggregates. If the rewrite is possible, 'container' is modified, bool in the return pair is
+     * set to 'true' and the iterator is set to point to the new group.
      */
     std::pair<bool, Pipeline::SourceContainer::iterator> rewriteGroupByMinMax(
         Pipeline::SourceContainer::iterator itr, Pipeline::SourceContainer* container);
@@ -284,7 +284,15 @@ private:
     int _bucketMaxCount = 0;
     boost::optional<long long> _sampleSize;
 
-    // Filters pushed from the later $match stages
+    // It's benefitial to do as much filtering at the bucket level as possible to avoid unpacking
+    // buckets that wouldn't contribute to the results anyway. There is a generic mechanism that
+    // allows to swap $match stages with this one (see 'getModifiedPaths()'). It lets us split out
+    // and push down a filter on the metaField "as is". The remaining filters might cause creation
+    // of additional bucket-level filters (see 'createPredicatesOnBucketLevelField()') that are
+    // inserted before this stage while the original filter is incorporated into this stage as
+    // '_eventFilter' (to be applied to each unpacked document) and/or '_wholeBucketFilter' for the
+    // cases when _all_ events in a bucket would match (currently, we only do this for the
+    // timeField).
     std::unique_ptr<MatchExpression> _eventFilter;
     BSONObj _eventFilterBson;
     DepsTracker _eventFilterDeps;
diff --git a/src/mongo/db/pipeline/document_source_internal_unpack_bucket_test/group_reorder_test.cpp b/src/mongo/db/pipeline/document_source_internal_unpack_bucket_test/group_reorder_test.cpp
index 6dbc68d3616..78b2931cdbc 100644
--- a/src/mongo/db/pipeline/document_source_internal_unpack_bucket_test/group_reorder_test.cpp
+++ b/src/mongo/db/pipeline/document_source_internal_unpack_bucket_test/group_reorder_test.cpp
@@ -125,7 +125,7 @@ TEST_F(InternalUnpackBucketGroupReorder, MinMaxGroupOnMetafield) {
     auto unpackSpecObj = fromjson(
         "{$_internalUnpackBucket: { include: ['a', 'b', 'c'], metaField: 'meta1', timeField: 't', "
         "bucketMaxSpanSeconds: 3600}}");
-    auto groupSpecObj = fromjson("{$group: {_id: '$meta1.a.b', accmin: {$sum: '$meta1.f1'}}}");
+    auto groupSpecObj = fromjson("{$group: {_id: '$meta1.a.b', accmin: {$min: '$meta1.f1'}}}");
 
     auto pipeline = Pipeline::parse(makeVector(unpackSpecObj, groupSpecObj), getExpCtx());
     pipeline->optimizePipeline();
@@ -133,7 +133,7 @@ TEST_F(InternalUnpackBucketGroupReorder, MinMaxGroupOnMetafield) {
     auto serialized = pipeline->serializeToBson();
     ASSERT_EQ(1, serialized.size());
 
-    auto optimized = fromjson("{$group: {_id: '$meta.a.b', accmin: {$sum: '$meta.f1'}}}");
+    auto optimized = fromjson("{$group: {_id: '$meta.a.b', accmin: {$min: '$meta.f1'}}}");
     ASSERT_BSONOBJ_EQ(optimized, serialized[0]);
 }