diff options
author | Nicholas Zolnierz <nicholas.zolnierz@mongodb.com> | 2019-10-21 21:10:54 +0000 |
---|---|---|
committer | evergreen <evergreen@mongodb.com> | 2019-10-21 21:10:54 +0000 |
commit | 9365527cb8dd3d9803b31f08db0119302460cb0d (patch) | |
tree | f03ab8631c00b760070a85bb8d56b1b416062e38 | |
parent | 822a350683d0d657b9d4237954cbb66590420f0e (diff) | |
download | mongo-9365527cb8dd3d9803b31f08db0119302460cb0d.tar.gz |
SERVER-43844: Remove redundant collation BSONObj on ExpressionContext
-rw-r--r-- | jstests/noPassthrough/currentop_query.js | 28 | ||||
-rw-r--r-- | src/mongo/db/commands/find_cmd.cpp | 1 | ||||
-rw-r--r-- | src/mongo/db/commands/map_reduce_agg.cpp | 1 | ||||
-rw-r--r-- | src/mongo/db/pipeline/document_source_graph_lookup.cpp | 11 | ||||
-rw-r--r-- | src/mongo/db/pipeline/document_source_lookup.cpp | 9 | ||||
-rw-r--r-- | src/mongo/db/pipeline/expression_context.cpp | 26 | ||||
-rw-r--r-- | src/mongo/db/pipeline/expression_context.h | 21 | ||||
-rw-r--r-- | src/mongo/db/pipeline/expression_context_for_test.h | 1 | ||||
-rw-r--r-- | src/mongo/db/pipeline/pipeline_d.cpp | 13 | ||||
-rw-r--r-- | src/mongo/db/pipeline/sharded_agg_helpers.cpp | 13 | ||||
-rw-r--r-- | src/mongo/s/commands/cluster_map_reduce_agg.cpp | 1 | ||||
-rw-r--r-- | src/mongo/s/query/cluster_aggregate.cpp | 3 |
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; } |