SERVER-43844: Remove redundant collation BSONObj on ExpressionContext

12 files changed, 45 insertions, 83 deletions

diff --git a/jstests/noPassthrough/currentop_query.js b/jstests/noPassthrough/currentop_query.js
index 82dcc421de3..56f39449cce 100644
--- a/jstests/noPassthrough/currentop_query.js
+++ b/jstests/noPassthrough/currentop_query.js
@@ -206,7 +206,7 @@ function runTests({conn, readMode, currentOp, truncatedOps, localOps}) {
                     "aggregate": {$exists: true},
                     "pipeline.0.$match.$comment": "currentop_query",
                     "comment": "currentop_query_2",
-                    "collation": {locale: "fr"},
+                    "collation.locale": "fr",
                     "hint": {_id: 1}
                 },
                                                              isRemoteShardCurOp)
@@ -220,10 +220,8 @@ function runTests({conn, readMode, currentOp, truncatedOps, localOps}) {
                 },
                 command: "count",
                 planSummary: "COLLSCAN",
-                currentOpFilter: {
-                    "command.query.$comment": "currentop_query",
-                    "command.collation": {locale: "fr"}
-                }
+                currentOpFilter:
+                    {"command.query.$comment": "currentop_query", "command.collation.locale": "fr"}
             },
             {
                 test: function(db) {
@@ -234,10 +232,8 @@ function runTests({conn, readMode, currentOp, truncatedOps, localOps}) {
                 },
                 command: "distinct",
                 planSummary: "COLLSCAN",
-                currentOpFilter: {
-                    "command.query.$comment": "currentop_query",
-                    "command.collation": {locale: "fr"}
-                }
+                currentOpFilter:
+                    {"command.query.$comment": "currentop_query", "command.collation.locale": "fr"}
             },
             {
                 test: function(db) {
@@ -259,10 +255,8 @@ function runTests({conn, readMode, currentOp, truncatedOps, localOps}) {
                 },
                 command: "findandmodify",
                 planSummary: "IXSCAN { _id: 1 }",
-                currentOpFilter: {
-                    "command.query.$comment": "currentop_query",
-                    "command.collation": {locale: "fr"}
-                }
+                currentOpFilter:
+                    {"command.query.$comment": "currentop_query", "command.collation.locale": "fr"}
             },
             {
                 test: function(db) {
@@ -289,7 +283,7 @@ function runTests({conn, readMode, currentOp, truncatedOps, localOps}) {
                                       ? {"command.delete": coll.getName(), "command.ordered": true}
                                       : {
                                             "command.q.$comment": "currentop_query",
-                                            "command.collation": {locale: "fr"}
+                                            "command.collation.locale": "fr"
                                         })
             },
             {
@@ -305,7 +299,7 @@ function runTests({conn, readMode, currentOp, truncatedOps, localOps}) {
                                       ? {"command.update": coll.getName(), "command.ordered": true}
                                       : {
                                             "command.q.$comment": "currentop_query",
-                                            "command.collation": {locale: "fr"}
+                                            "command.collation.locale": "fr"
                                         })
             }
         ];
@@ -327,7 +321,7 @@ function runTests({conn, readMode, currentOp, truncatedOps, localOps}) {
                 command: "find",
                 planSummary: "COLLSCAN",
                 currentOpFilter:
-                    {"command.comment": "currentop_query", "command.collation": {locale: "fr"}}
+                    {"command.comment": "currentop_query", "command.collation.locale": "fr"}
             });
         }
 
@@ -360,7 +354,7 @@ function runTests({conn, readMode, currentOp, truncatedOps, localOps}) {
             currentOpFilter: commandOrOriginatingCommand({
                 "aggregate": {$exists: true},
                 "pipeline.0.$geoNear.query.$comment": "currentop_query",
-                "collation": {locale: "fr"},
+                "collation.locale": "fr",
                 "comment": "currentop_query",
             },
                                                          isRemoteShardCurOp)
diff --git a/src/mongo/db/commands/find_cmd.cpp b/src/mongo/db/commands/find_cmd.cpp
index 5b91aa22420..82f9dd07c96 100644
--- a/src/mongo/db/commands/find_cmd.cpp
+++ b/src/mongo/db/commands/find_cmd.cpp
@@ -106,7 +106,6 @@ boost::intrusive_ptr<ExpressionContext> makeExpressionContext(OperationContext*
                                           queryRequest.allowDiskUse(),
                                           false,  // bypassDocumentValidation
                                           queryRequest.nss(),
-                                          queryRequest.getCollation(),
                                           queryRequest.getRuntimeConstants(),
                                           std::move(collator),
                                           nullptr,  // mongoProcessInterface
diff --git a/src/mongo/db/commands/map_reduce_agg.cpp b/src/mongo/db/commands/map_reduce_agg.cpp
index 73fdcf897ca..68ad9cb031d 100644
--- a/src/mongo/db/commands/map_reduce_agg.cpp
+++ b/src/mongo/db/commands/map_reduce_agg.cpp
@@ -86,7 +86,6 @@ auto makeExpressionContext(OperationContext* opCtx, const MapReduce& parsedMr) {
         true,         // allowDiskUse
         parsedMr.getBypassDocumentValidation().get_value_or(false),
         parsedMr.getNamespace(),
-        parsedMr.getCollation().get_value_or(BSONObj()),
         runtimeConstants,
         std::move(resolvedCollator),
         MongoProcessInterface::create(opCtx),
diff --git a/src/mongo/db/pipeline/document_source_graph_lookup.cpp b/src/mongo/db/pipeline/document_source_graph_lookup.cpp
index 830ba3b9be0..3c95d5cf3a6 100644
--- a/src/mongo/db/pipeline/document_source_graph_lookup.cpp
+++ b/src/mongo/db/pipeline/document_source_graph_lookup.cpp
@@ -457,16 +457,7 @@ DocumentSourceGraphLookUp::DocumentSourceGraphLookUp(
       _cache(pExpCtx->getValueComparator()),
       _unwind(unwindSrc) {
     const auto& resolvedNamespace = pExpCtx->getResolvedNamespace(_from);
-
-    // We always set an explicit collator on the copied expression context, even if the collator is
-    // null (i.e. the simple collation). Otherwise, in the situation where the user did not specify
-    // a collation and the simple collation was inherited from the local collection, the execution
-    // machinery will incorrectly interpret the null collator and empty user collation as an
-    // indication that it should inherit the foreign collection's collation.
-    _fromExpCtx =
-        expCtx->copyWith(resolvedNamespace.ns,
-                         boost::none,
-                         expCtx->getCollator() ? expCtx->getCollator()->clone() : nullptr);
+    _fromExpCtx = pExpCtx->copyWith(resolvedNamespace.ns);
 
     // We append an additional BSONObj to '_fromPipeline' as a placeholder for the $match stage
     // we'll eventually construct from the input document.
diff --git a/src/mongo/db/pipeline/document_source_lookup.cpp b/src/mongo/db/pipeline/document_source_lookup.cpp
index e11a44fcf32..f8d88637824 100644
--- a/src/mongo/db/pipeline/document_source_lookup.cpp
+++ b/src/mongo/db/pipeline/document_source_lookup.cpp
@@ -61,14 +61,7 @@ DocumentSourceLookUp::DocumentSourceLookUp(NamespaceString fromNs,
     const auto& resolvedNamespace = expCtx->getResolvedNamespace(_fromNs);
     _resolvedNs = resolvedNamespace.ns;
     _resolvedPipeline = resolvedNamespace.pipeline;
-
-    // We always set an explicit collator on the copied expression context, even if the collator is
-    // null (i.e. the simple collation). Otherwise, in the situation where the user did not specify
-    // a collation and the simple collation was inherited from the local collection, the execution
-    // machinery will incorrectly interpret the null collator and empty user collation as an
-    // indication that it should inherit the foreign collection's collation.
-    _fromExpCtx = expCtx->copyWith(
-        _resolvedNs, boost::none, expCtx->getCollator() ? expCtx->getCollator()->clone() : nullptr);
+    _fromExpCtx = expCtx->copyWith(_resolvedNs);
 
     _fromExpCtx->subPipelineDepth += 1;
     uassert(ErrorCodes::MaxSubPipelineDepthExceeded,
diff --git a/src/mongo/db/pipeline/expression_context.cpp b/src/mongo/db/pipeline/expression_context.cpp
index e281178ab3e..1ecd751eb69 100644
--- a/src/mongo/db/pipeline/expression_context.cpp
+++ b/src/mongo/db/pipeline/expression_context.cpp
@@ -58,7 +58,6 @@ ExpressionContext::ExpressionContext(OperationContext* opCtx,
                         request.shouldAllowDiskUse(),
                         request.shouldBypassDocumentValidation(),
                         request.getNamespaceString(),
-                        request.getCollation(),
                         request.getRuntimeConstants(),
                         std::move(collator),
                         std::move(processInterface),
@@ -73,7 +72,6 @@ ExpressionContext::ExpressionContext(
     bool allowDiskUse,
     bool bypassDocumentValidation,
     const NamespaceString& ns,
-    const BSONObj& collation,
     const boost::optional<RuntimeConstants>& runtimeConstants,
     std::unique_ptr<CollatorInterface> collator,
     const std::shared_ptr<MongoProcessInterface>& mongoProcessInterface,
@@ -91,13 +89,13 @@ ExpressionContext::ExpressionContext(
       timeZoneDatabase(opCtx && opCtx->getServiceContext()
                            ? TimeZoneDatabase::get(opCtx->getServiceContext())
                            : nullptr),
-      collation(collation),
       variablesParseState(variables.useIdGenerator()),
       _ownedCollator(std::move(collator)),
       _unownedCollator(_ownedCollator.get()),
       _documentComparator(_unownedCollator),
       _valueComparator(_unownedCollator),
       _resolvedNamespaces(std::move(resolvedNamespaces)) {
+
     if (runtimeConstants)
         variables.setRuntimeConstants(*runtimeConstants);
     else
@@ -134,12 +132,9 @@ ExpressionContext::CollatorStash::CollatorStash(
     const boost::intrusive_ptr<ExpressionContext>& expCtx,
     std::unique_ptr<CollatorInterface> newCollator)
     : _expCtx(expCtx),
-      _originalCollation(_expCtx->collation),
       _originalCollatorOwned(std::move(_expCtx->_ownedCollator)),
       _originalCollatorUnowned(_expCtx->_unownedCollator) {
     _expCtx->setCollator(std::move(newCollator));
-    _expCtx->collation =
-        _expCtx->getCollator() ? _expCtx->getCollator()->getSpec().toBSON().getOwned() : BSONObj();
 }
 
 ExpressionContext::CollatorStash::~CollatorStash() {
@@ -154,7 +149,6 @@ ExpressionContext::CollatorStash::~CollatorStash() {
             _expCtx->_ownedCollator = nullptr;
         }
     }
-    _expCtx->collation = _originalCollation;
 }
 
 std::unique_ptr<ExpressionContext::CollatorStash> ExpressionContext::temporarilyChangeCollator(
@@ -176,16 +170,9 @@ intrusive_ptr<ExpressionContext> ExpressionContext::copyWith(
     boost::optional<UUID> uuid,
     boost::optional<std::unique_ptr<CollatorInterface>> updatedCollator) const {
 
-    auto [collationParam, collatorParam] = [&]() {
-        if (updatedCollator)
-            return std::pair(*updatedCollator ? (*updatedCollator)->getSpec().toBSON()
-                                              : CollationSpec::kSimpleSpec,
-                             std::move(*updatedCollator));
-        else
-            return std::pair(collation,
-                             _ownedCollator ? _ownedCollator->clone()
-                                            : std::unique_ptr<CollatorInterface>{});
-    }();
+    auto collator = updatedCollator
+        ? std::move(*updatedCollator)
+        : (_ownedCollator ? _ownedCollator->clone() : std::unique_ptr<CollatorInterface>{});
 
     auto expCtx = make_intrusive<ExpressionContext>(opCtx,
                                                     explain,
@@ -194,9 +181,8 @@ intrusive_ptr<ExpressionContext> ExpressionContext::copyWith(
                                                     allowDiskUse,
                                                     bypassDocumentValidation,
                                                     ns,
-                                                    std::move(collationParam),
                                                     boost::none,  // runtimeConstants
-                                                    std::move(collatorParam),
+                                                    std::move(collator),
                                                     mongoProcessInterface,
                                                     _resolvedNamespaces,
                                                     uuid);
@@ -213,7 +199,7 @@ intrusive_ptr<ExpressionContext> ExpressionContext::copyWith(
     // effort to divorce the ExpressionContext from general Agg resources by creating an
     // AggregationContext. If that effort comes to fruition, this special-case collator handling
     // will be made unnecessary.
-    if (!updatedCollator && !collatorParam && _unownedCollator)
+    if (!updatedCollator && !collator && _unownedCollator)
         expCtx->setCollator(_unownedCollator);
 
     expCtx->variables = variables;
diff --git a/src/mongo/db/pipeline/expression_context.h b/src/mongo/db/pipeline/expression_context.h
index c48455935c0..e82d7bef9c5 100644
--- a/src/mongo/db/pipeline/expression_context.h
+++ b/src/mongo/db/pipeline/expression_context.h
@@ -94,7 +94,6 @@ public:
 
         boost::intrusive_ptr<ExpressionContext> _expCtx;
 
-        BSONObj _originalCollation;
         std::unique_ptr<CollatorInterface> _originalCollatorOwned;
         const CollatorInterface* _originalCollatorUnowned{nullptr};
     };
@@ -122,7 +121,6 @@ public:
                       bool allowDiskUse,
                       bool bypassDocumentValidation,
                       const NamespaceString& ns,
-                      const BSONObj& collation,
                       const boost::optional<RuntimeConstants>& runtimeConstants,
                       std::unique_ptr<CollatorInterface> collator,
                       const std::shared_ptr<MongoProcessInterface>& mongoProcessInterface,
@@ -132,6 +130,8 @@ public:
     /**
      * Constructs an ExpressionContext suitable for use outside of the aggregation system, including
      * for MatchExpression parsing and executing pipeline-style operations in the Update system.
+     *
+     * If 'collator' is null, the simple collator will be used.
      */
     ExpressionContext(OperationContext* opCtx,
                       const CollatorInterface* collator,
@@ -168,6 +168,19 @@ public:
         return _unownedCollator;
     }
 
+    /**
+     * Returns the BSON spec for the ExpressionContext's collator, or the simple collator spec if
+     * the collator is null.
+     *
+     * The ExpressionContext is always set up with the fully-resolved collation. So even though
+     * SERVER-24433 describes an ambiguity between a null collator, here we can say confidently that
+     * null must mean simple since we have already handled "absence of a collator" before creating
+     * the ExpressionContext.
+     */
+    BSONObj getCollatorBSON() const {
+        return _unownedCollator ? _unownedCollator->getSpec().toBSON() : CollationSpec::kSimpleSpec;
+    }
+
     void setCollator(const CollatorInterface* collator);
 
     const DocumentComparator& getDocumentComparator() const {
@@ -259,10 +272,6 @@ public:
 
     const TimeZoneDatabase* timeZoneDatabase;
 
-    // Collation requested by the user for this pipeline. Empty if the user did not request a
-    // collation.
-    BSONObj collation;
-
     Variables variables;
     VariablesParseState variablesParseState;
 
diff --git a/src/mongo/db/pipeline/expression_context_for_test.h b/src/mongo/db/pipeline/expression_context_for_test.h
index b9f891b1603..d716469f6e7 100644
--- a/src/mongo/db/pipeline/expression_context_for_test.h
+++ b/src/mongo/db/pipeline/expression_context_for_test.h
@@ -59,7 +59,6 @@ public:
                             false,        // allowDiskUse,
                             false,        // bypassDocumentValidation,
                             nss,
-                            {},  // collation,
                             RuntimeConstants(Date_t::now(), Timestamp(1, 0)),
                             {},  // collator
                             std::make_shared<StubMongoProcessInterface>(),
diff --git a/src/mongo/db/pipeline/pipeline_d.cpp b/src/mongo/db/pipeline/pipeline_d.cpp
index 59bf5c596a0..a0e6996829e 100644
--- a/src/mongo/db/pipeline/pipeline_d.cpp
+++ b/src/mongo/db/pipeline/pipeline_d.cpp
@@ -211,15 +211,10 @@ StatusWith<std::unique_ptr<PlanExecutor, PlanExecutor::Deleter>> attemptToGetExe
         qr->setHint(aggRequest->getHint());
     }
 
-    // If the pipeline has a non-null collator, set the collation option to the result of
-    // serializing the collator's spec back into BSON. We do this in order to fill in all options
-    // that the user omitted.
-    //
-    // If pipeline has a null collator (representing the "simple" collation), we simply set the
-    // collation option to the original user BSON, which is either the empty object (unspecified),
-    // or the specification for the "simple" collation.
-    qr->setCollation(pExpCtx->getCollator() ? pExpCtx->getCollator()->getSpec().toBSON()
-                                            : pExpCtx->collation);
+    // The collation on the ExpressionContext has been resolved to either the user-specified
+    // collation or the collection default. This BSON should never be empty even if the resolved
+    // collator is simple.
+    qr->setCollation(pExpCtx->getCollatorBSON());
 
     const ExtensionsCallbackReal extensionsCallback(pExpCtx->opCtx, &nss);
 
diff --git a/src/mongo/db/pipeline/sharded_agg_helpers.cpp b/src/mongo/db/pipeline/sharded_agg_helpers.cpp
index f451d139d21..08f970cd34b 100644
--- a/src/mongo/db/pipeline/sharded_agg_helpers.cpp
+++ b/src/mongo/db/pipeline/sharded_agg_helpers.cpp
@@ -393,7 +393,7 @@ BSONObj createCommandForTargetedShards(
                                      expCtx->opCtx,
                                      expCtx->explain,
                                      expCtx->getRuntimeConstants(),
-                                     expCtx->collation);
+                                     expCtx->getCollatorBSON());
 }
 
 sharded_agg_helpers::DispatchShardPipelineResults dispatchExchangeConsumerPipeline(
@@ -569,9 +569,10 @@ DispatchShardPipelineResults dispatchShardPipeline(
         : boost::optional<CachedCollectionRoutingInfo>{};
 
     // Determine whether we can run the entire aggregation on a single shard.
+    const auto collationObj = expCtx->getCollatorBSON();
     const bool mustRunOnAll = mustRunOnAllShards(expCtx->ns, hasChangeStream);
-    std::set<ShardId> shardIds = getTargetedShards(
-        opCtx, mustRunOnAll, executionNsRoutingInfo, shardQuery, expCtx->collation);
+    std::set<ShardId> shardIds =
+        getTargetedShards(opCtx, mustRunOnAll, executionNsRoutingInfo, shardQuery, collationObj);
 
     // Don't need to split the pipeline if we are only targeting a single shard, unless:
     // - There is a stage that needs to be run on the primary shard and the single target shard
@@ -604,7 +605,7 @@ DispatchShardPipelineResults dispatchShardPipeline(
                                            expCtx->explain,
                                            expCtx->getRuntimeConstants(),
                                            pipeline.get(),
-                                           expCtx->collation);
+                                           collationObj);
 
     // A $changeStream pipeline must run on all shards, and will also open an extra cursor on the
     // config server in order to monitor for new shards. To guarantee that we do not miss any
@@ -624,7 +625,7 @@ DispatchShardPipelineResults dispatchShardPipeline(
         }
         // Rebuild the set of shards as the shard registry might have changed.
         shardIds = getTargetedShards(
-            opCtx, mustRunOnAll, executionNsRoutingInfo, shardQuery, expCtx->collation);
+            opCtx, mustRunOnAll, executionNsRoutingInfo, shardQuery, collationObj);
     }
 
     // If there were no shards when we began execution, we wouldn't have run this aggregation in the
@@ -658,7 +659,7 @@ DispatchShardPipelineResults dispatchShardPipeline(
                                                            ReadPreferenceSetting::get(opCtx),
                                                            Shard::RetryPolicy::kIdempotent,
                                                            shardQuery,
-                                                           expCtx->collation);
+                                                           collationObj);
         }
     } else {
         cursors = establishShardCursors(opCtx,
diff --git a/src/mongo/s/commands/cluster_map_reduce_agg.cpp b/src/mongo/s/commands/cluster_map_reduce_agg.cpp
index 6df1e77250c..3492b3a5e78 100644
--- a/src/mongo/s/commands/cluster_map_reduce_agg.cpp
+++ b/src/mongo/s/commands/cluster_map_reduce_agg.cpp
@@ -89,7 +89,6 @@ auto makeExpressionContext(OperationContext* opCtx,
         true,         // allowDiskUse
         parsedMr.getBypassDocumentValidation().get_value_or(false),
         nss,
-        collationObj,
         runtimeConstants,
         std::move(resolvedCollator),
         std::make_shared<MongoSInterface>(),
diff --git a/src/mongo/s/query/cluster_aggregate.cpp b/src/mongo/s/query/cluster_aggregate.cpp
index 9de374b9202..4696181c003 100644
--- a/src/mongo/s/query/cluster_aggregate.cpp
+++ b/src/mongo/s/query/cluster_aggregate.cpp
@@ -122,9 +122,6 @@ boost::intrusive_ptr<ExpressionContext> makeExpressionContext(
                                           std::move(resolvedNamespaces),
                                           uuid);
 
-    // Keep the backing collation object on the context up to date with the resolved collator.
-    mergeCtx->collation = collationObj;
-
     mergeCtx->inMongos = true;
     return mergeCtx;
 }