SERVER-73822 Time-series $group rewrite ignores certain accumulators

3 files changed, 51 insertions, 10 deletions

diff --git a/jstests/core/timeseries/timeseries_groupby_reorder.js b/jstests/core/timeseries/timeseries_groupby_reorder.js
index 29d07f8b4fa..d3e52eaf2f9 100644
--- a/jstests/core/timeseries/timeseries_groupby_reorder.js
+++ b/jstests/core/timeseries/timeseries_groupby_reorder.js
@@ -5,7 +5,8 @@
  *   directly_against_shardsvrs_incompatible,
  *   does_not_support_stepdowns,
  *   does_not_support_transactions,
- *   requires_fcv_61,
+ *   requires_fcv_50,
+ *   requires_timeseries,
  * ]
  */
 (function() {
@@ -37,7 +38,18 @@ if (!isMongos(db)) {
     });
 }
 
-const res = coll.aggregate([{$group: {_id: '$meta', accmin: {$min: '$b'}, accmax: {$max: '$c'}}}])
-                .toArray();
-assert.docEq(res, [{"_id": null, "accmin": 1, "accmax": 3}]);
+let res = coll.aggregate([{$group: {_id: '$meta', accmin: {$min: '$b'}, accmax: {$max: '$c'}}}])
+              .toArray();
+assert.docEq([{"_id": null, "accmin": 1, "accmax": 3}], res);
+
+// 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}]);
 })();
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 79dafdb4618..0f5d37b2c27 100644
--- a/src/mongo/db/pipeline/document_source_internal_unpack_bucket.cpp
+++ b/src/mongo/db/pipeline/document_source_internal_unpack_bucket.cpp
@@ -939,10 +939,10 @@ DocumentSourceInternalUnpackBucket::rewriteGroupByMinMax(Pipeline::SourceContain
         return {};
     }
 
-    if (!_bucketUnpacker.bucketSpec().metaField()) {
+    if (!_bucketUnpacker.bucketSpec().metaField) {
         return {};
     }
-    const auto& metaField = *_bucketUnpacker.bucketSpec().metaField();
+    const auto& metaField = *_bucketUnpacker.bucketSpec().metaField;
 
     const auto& idFields = groupPtr->getIdFields();
     if (idFields.size() != 1) {
@@ -971,7 +971,7 @@ DocumentSourceInternalUnpackBucket::rewriteGroupByMinMax(Pipeline::SourceContain
             }
 
             const auto& rootFieldName = path.getFieldName(1);
-            if (rootFieldName == _bucketUnpacker.bucketSpec().timeField()) {
+            if (rootFieldName == _bucketUnpacker.bucketSpec().timeField) {
                 // Rewrite not valid for time field. We want to eliminate the bucket
                 // unpack stage here.
                 return {};
@@ -987,10 +987,10 @@ DocumentSourceInternalUnpackBucket::rewriteGroupByMinMax(Pipeline::SourceContain
                 }
             } else {
                 // Update aggregates to reference the control field.
-                const auto op = stmt.expr.name;
-                if (op == "$min") {
+                const auto op = stmt.makeAccumulator()->getOpName();
+                if (op == "$min"_sd) {
                     os << timeseries::kControlMinFieldNamePrefix;
-                } else if (op == "$max") {
+                } else if (op == "$max"_sd) {
                     os << timeseries::kControlMaxFieldNamePrefix;
                 } else {
                     // Rewrite is valid only for min and max aggregates.
@@ -1011,6 +1011,8 @@ DocumentSourceInternalUnpackBucket::rewriteGroupByMinMax(Pipeline::SourceContain
             AccumulationExpression accExpr = stmt.expr;
             accExpr.argument = newExpr;
             accumulationStatements.emplace_back(stmt.fieldName, std::move(accExpr));
+        } else {
+            return {};
         }
     }
 
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 6100897fd79..6dbc68d3616 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
@@ -94,6 +94,33 @@ TEST_F(InternalUnpackBucketGroupReorder, MinMaxGroupOnMetadata) {
     ASSERT_BSONOBJ_EQ(optimized, serialized[0]);
 }
 
+// Test SERVER-73822 fix: complex $min and $max (i.e. not just straight field refs) work correctly.
+TEST_F(InternalUnpackBucketGroupReorder, MinMaxComplexGroupOnMetadata) {
+    auto unpackSpecObj = fromjson(
+        "{$_internalUnpackBucket: { include: ['a', 'b', 'c'], metaField: 'meta1', timeField: 't', "
+        "bucketMaxSpanSeconds: 3600}}");
+    auto groupSpecObj = fromjson(
+        "{$group: {_id: '$meta1.a.b', accmin: {$min: {$add: ['$b', {$const: 0}]}}, accmax: {$max: "
+        "{$add: [{$const: 0}, '$c']}}}}");
+
+    auto pipeline = Pipeline::parse(makeVector(unpackSpecObj, groupSpecObj), getExpCtx());
+    pipeline->optimizePipeline();
+
+    auto serialized = pipeline->serializeToBson();
+    ASSERT_EQ(2, serialized.size());
+
+    // Order of fields may be different from the original stage.
+    auto unpackSpecResultObj = fromjson(
+        "{$_internalUnpackBucket: { include: ['a', 'b', 'c'], timeField: 't', metaField: 'meta1', "
+        "bucketMaxSpanSeconds: 3600}}");
+    ASSERT_BSONOBJ_EQ(unpackSpecResultObj, serialized[0]);
+
+    auto groupSpecOutputObj = fromjson(
+        "{$group: {_id: '$meta1.a.b', accmin: {$min: {$add: ['$b', {$const: 0}]}}, accmax: {$max: "
+        "{$add: ['$c', {$const: 0}]}}}}");
+    ASSERT_BSONOBJ_EQ(groupSpecOutputObj, serialized[1]);
+}
+
 TEST_F(InternalUnpackBucketGroupReorder, MinMaxGroupOnMetafield) {
     auto unpackSpecObj = fromjson(
         "{$_internalUnpackBucket: { include: ['a', 'b', 'c'], metaField: 'meta1', timeField: 't', "