diff options
5 files changed, 156 insertions, 7 deletions
diff --git a/jstests/aggregation/sources/graphLookup/collation_graphlookup.js b/jstests/aggregation/sources/graphLookup/collation_graphlookup.js index 459a7e70704..1d9bd3e7384 100644 --- a/jstests/aggregation/sources/graphLookup/collation_graphlookup.js +++ b/jstests/aggregation/sources/graphLookup/collation_graphlookup.js @@ -46,7 +46,8 @@ assert.eq(1, res.length); assert.eq("erica", res[0].username); assert.eq(2, res[0].friendUsers.length); -// Negative test: ensure that we don't find any friends when the collation is simple. +// Negative test: The local collation does not have a default collation, and so we use the simple +// collation. Ensure that we don't find any friends when the collation is simple. res = coll.aggregate([ {$match: {username: "erica"}}, { @@ -64,6 +65,38 @@ assert.eq(1, res.length); assert.eq("erica", res[0].username); assert.eq(0, res[0].friendUsers.length); +// Create a foreign collection with a case-insensitive default collation. +foreignColl.drop(); +assert.commandWorked(db.createCollection(foreignColl.getName(), caseInsensitiveUS)); +assert.commandWorked(foreignColl.insert([{username: "JEREMY"}, {username: "JIMMY"}])); + +// Insert some more documents into the local collection. +assert.commandWorked(coll.insert({username: "fiona", friends: ["jeremy"]})); +assert.commandWorked(coll.insert({username: "gretchen", friends: ["jimmy"]})); + +// Test that $graphLookup uses the simple collation in the case where the collection on which it is +// run does not have a default collation, and that this collation is used instead of the default +// collation of the foreign collection. Exercises the fix for SERVER-43350. +res = coll.aggregate([{$match: {username: {$in: ["erica", "fiona", "gretchen"]}}}, + { + $graphLookup: { + from: foreignColl.getName(), + startWith: "$friends", + connectFromField: "friends", + connectToField: "username", + as: "friendUsers" + } + } + ]) + .toArray(); +assert.eq(3, res.length, tojson(res)); +for (let i = 0; i < res.length; ++i) { + assert(["erica", "fiona", "gretchen"].includes(res[i].username)); + assert.eq(0, res[i].friendUsers.length); +} + +// Recreate the local collection with a case-insensitive default collation, and the foreign +// collection with a case-sensitive default collation. coll.drop(); assert.commandWorked(db.createCollection(coll.getName(), caseInsensitiveUS)); assert.commandWorked(coll.insert({username: "erica", friends: ["jeremy", "jimmy"]})); diff --git a/jstests/aggregation/sources/lookup/lookup_collation.js b/jstests/aggregation/sources/lookup/lookup_collation.js new file mode 100644 index 00000000000..57c00f2550e --- /dev/null +++ b/jstests/aggregation/sources/lookup/lookup_collation.js @@ -0,0 +1,99 @@ +/** + * Tests that $lookup respects the user-specified collation or the inherited local collation + * when performing comparisons on a foreign collection with a different default collation. Exercises + * the fix for SERVER-43350. + * @tags: [assumes_unsharded_collection] + */ +(function() { + +"use strict"; + +const testDB = db.getSiblingDB(jsTestName()); +const noCollationColl = testDB.no_collation; +const caseInsensitiveColl = testDB.case_insensitive; + +caseInsensitiveColl.drop(); +noCollationColl.drop(); + +const caseInsensitiveCollation = { + locale: "en_US", + strength: 1 +}; + +const simpleCollation = { + locale: "simple" +}; + +assert.commandWorked(testDB.createCollection(noCollationColl.getName())); +assert.commandWorked( + testDB.createCollection(caseInsensitiveColl.getName(), {collation: caseInsensitiveCollation})); + +assert.commandWorked( + noCollationColl.insert([{_id: "a"}, {_id: "b"}, {_id: "c"}, {_id: "d"}, {_id: "e"}])); +assert.commandWorked( + caseInsensitiveColl.insert([{_id: "a"}, {_id: "B"}, {_id: "c"}, {_id: "D"}, {_id: "e"}])); + +const lookupWithPipeline = (foreignColl) => { + return { + $lookup: {from: foreignColl.getName(), as: "foreignMatch", let: {l_id: "$_id"}, pipeline: [{$match: {$expr: {$eq: ["$_id", "$$l_id"]}}}]} + }; +}; +const lookupNoPipeline = (foreignColl) => { + return { + $lookup: {from: foreignColl.getName(), localField: "_id", foreignField: "_id", as: "foreignMatch"} + }; +}; + +for (let lookupInto of [lookupWithPipeline, lookupNoPipeline]) { + // Verify that a $lookup whose local collection has no default collation uses the simple + // collation for comparisons on a foreign collection with a non-simple default collation. + let results = noCollationColl.aggregate([lookupInto(caseInsensitiveColl)]).toArray(); + assert.docEq(results, [ + {_id: "a", foreignMatch: [{_id: "a"}]}, + {_id: "b", foreignMatch: []}, + {_id: "c", foreignMatch: [{_id: "c"}]}, + {_id: "d", foreignMatch: []}, + {_id: "e", foreignMatch: [{_id: "e"}]} + ]); + + // Verify that a $lookup whose local collection has no default collation but which is running in + // a pipeline with a non-simple user-specified collation uses the latter for comparisons on the + // foreign collection. + results = + noCollationColl + .aggregate([lookupInto(caseInsensitiveColl)], {collation: caseInsensitiveCollation}) + .toArray(); + assert.docEq(results, [ + {_id: "a", foreignMatch: [{_id: "a"}]}, + {_id: "b", foreignMatch: [{_id: "B"}]}, + {_id: "c", foreignMatch: [{_id: "c"}]}, + {_id: "d", foreignMatch: [{_id: "D"}]}, + {_id: "e", foreignMatch: [{_id: "e"}]} + ]); + + // Verify that a $lookup whose local collection has a non-simple collation uses the latter for + // comparisons on a foreign collection with no default collation. + results = caseInsensitiveColl.aggregate([lookupInto(noCollationColl)]).toArray(); + assert.docEq(results, [ + {_id: "a", foreignMatch: [{_id: "a"}]}, + {_id: "B", foreignMatch: [{_id: "b"}]}, + {_id: "c", foreignMatch: [{_id: "c"}]}, + {_id: "D", foreignMatch: [{_id: "d"}]}, + {_id: "e", foreignMatch: [{_id: "e"}]} + ]); + + // Verify that a $lookup whose local collection has a non-simple collation but which is running + // in a pipeline with a user-specified simple collation uses the latter for comparisons on the + // foreign collection. + results = + caseInsensitiveColl.aggregate([lookupInto(noCollationColl)], {collation: simpleCollation}) + .toArray(); + assert.docEq(results, [ + {_id: "a", foreignMatch: [{_id: "a"}]}, + {_id: "B", foreignMatch: []}, + {_id: "c", foreignMatch: [{_id: "c"}]}, + {_id: "D", foreignMatch: []}, + {_id: "e", foreignMatch: [{_id: "e"}]} + ]); +} +})();
\ No newline at end of file diff --git a/jstests/sharding/collation_lookup.js b/jstests/sharding/collation_lookup.js index f5e928e8374..1b8a98b5529 100644 --- a/jstests/sharding/collation_lookup.js +++ b/jstests/sharding/collation_lookup.js @@ -216,8 +216,7 @@ function runTests(withDefaultCollationColl, withoutDefaultCollationColl, collati " up to ordering"); // Test that the $lookup stage respects an explicit collation on the aggregation operation - // when - // it optimizes with an $unwind stage. + // when it optimizes with an $unwind stage. res = withoutDefaultCollationColl .aggregate( [ @@ -302,7 +301,9 @@ function runTests(withDefaultCollationColl, withoutDefaultCollationColl, collati "Expected " + tojson(expected) + " to equal " + tojson(res) + " up to ordering"); // Test that the $lookup stage uses the "simple" collation if a collation isn't set on the - // collection or the aggregation operation. + // collection or the aggregation operation, even if the foreign collection has a collation. + // TODO SERVER-38830: when a pipeline $lookup is capable of serializing its 'let' variables to + // remote shards, add a test-case to exercise SERVER-43350. res = withoutDefaultCollationColl .aggregate([ {$match: {_id: "lowercase"}}, diff --git a/src/mongo/db/pipeline/document_source_graph_lookup.cpp b/src/mongo/db/pipeline/document_source_graph_lookup.cpp index f071e756850..830ba3b9be0 100644 --- a/src/mongo/db/pipeline/document_source_graph_lookup.cpp +++ b/src/mongo/db/pipeline/document_source_graph_lookup.cpp @@ -457,11 +457,20 @@ DocumentSourceGraphLookUp::DocumentSourceGraphLookUp( _cache(pExpCtx->getValueComparator()), _unwind(unwindSrc) { const auto& resolvedNamespace = pExpCtx->getResolvedNamespace(_from); - _fromExpCtx = pExpCtx->copyWith(resolvedNamespace.ns); - _fromPipeline = 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(resolvedNamespace.ns, + boost::none, + expCtx->getCollator() ? expCtx->getCollator()->clone() : nullptr); // We append an additional BSONObj to '_fromPipeline' as a placeholder for the $match stage // we'll eventually construct from the input document. + _fromPipeline = resolvedNamespace.pipeline; _fromPipeline.reserve(_fromPipeline.size() + 1); _fromPipeline.push_back(BSON("$match" << BSONObj())); } diff --git a/src/mongo/db/pipeline/document_source_lookup.cpp b/src/mongo/db/pipeline/document_source_lookup.cpp index a4b1bf4221a..575d3335ba4 100644 --- a/src/mongo/db/pipeline/document_source_lookup.cpp +++ b/src/mongo/db/pipeline/document_source_lookup.cpp @@ -61,7 +61,14 @@ DocumentSourceLookUp::DocumentSourceLookUp(NamespaceString fromNs, const auto& resolvedNamespace = expCtx->getResolvedNamespace(_fromNs); _resolvedNs = resolvedNamespace.ns; _resolvedPipeline = resolvedNamespace.pipeline; - _fromExpCtx = expCtx->copyWith(_resolvedNs); + + // 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->subPipelineDepth += 1; uassert(ErrorCodes::MaxSubPipelineDepthExceeded, |