SERVER-27213 Recompute dependencies when joining matches

6 files changed, 138 insertions, 15 deletions

diff --git a/src/mongo/db/matcher/expression_algo.h b/src/mongo/db/matcher/expression_algo.h
index c4316213827..1144715c8cf 100644
--- a/src/mongo/db/matcher/expression_algo.h
+++ b/src/mongo/db/matcher/expression_algo.h
@@ -99,7 +99,8 @@ bool isPathPrefixOf(StringData first, StringData second);
 
 /**
  * Applies 'func' to each node of 'expr', where the first argument is a pointer to that actual node
- * (not a copy), and the second argument is the path to that node.
+ * (not a copy), and the second argument is the path to that node. Callers should not depend on the
+ * order of the traversal of the nodes.
  */
 void mapOver(MatchExpression* expr, NodeTraversalFunc func, std::string path = "");
 
diff --git a/src/mongo/db/pipeline/document_source_match.cpp b/src/mongo/db/pipeline/document_source_match.cpp
index 196f106619d..18cb22db71d 100644
--- a/src/mongo/db/pipeline/document_source_match.cpp
+++ b/src/mongo/db/pipeline/document_source_match.cpp
@@ -360,6 +360,8 @@ void DocumentSourceMatch::joinMatchWith(intrusive_ptr<DocumentSourceMatch> other
     StatusWithMatchExpression status = uassertStatusOK(
         MatchExpressionParser::parse(_predicate, ExtensionsCallbackNoop(), pExpCtx->getCollator()));
     _expression = std::move(status.getValue());
+    _dependencies = DepsTracker(_dependencies.getMetadataAvailable());
+    getDependencies(&_dependencies);
 }
 
 pair<intrusive_ptr<DocumentSourceMatch>, intrusive_ptr<DocumentSourceMatch>>
@@ -370,7 +372,7 @@ DocumentSourceMatch::splitSourceBy(const std::set<std::string>& fields) {
     invariant(newExpr.first || newExpr.second);
 
     if (!newExpr.first) {
-        // The entire $match dependends on 'fields'.
+        // The entire $match depends on 'fields'.
         _expression = std::move(newExpr.second);
         return {nullptr, this};
     } else if (!newExpr.second) {
diff --git a/src/mongo/db/pipeline/document_source_match.h b/src/mongo/db/pipeline/document_source_match.h
index 4e2354c6d83..e7f3d4227d9 100644
--- a/src/mongo/db/pipeline/document_source_match.h
+++ b/src/mongo/db/pipeline/document_source_match.h
@@ -126,16 +126,16 @@ public:
     static BSONObj getObjectForMatch(const Document& input, const std::set<std::string>& fields);
 
     /**
-     * Should be called _only_ on a MatchExpression  that is a predicate on 'path', or subfields  of
-     * 'path'. It is also invalid to call this method on a $match including a $elemMatch on 'path',
-     * for example: {$match: {'path': {$elemMatch: {'subfield': 3}}}}
-     *
-     * Returns a new DocumentSourceMatch that, if executed on the subdocument at 'path', is
-     * equivalent to 'expression'.
+     * Returns a new DocumentSourceMatch with a MatchExpression that, if executed on the
+     * sub-document at 'path', is equivalent to 'expression'.
      *
      * For example, if the original expression is {$and: [{'a.b': {$gt: 0}}, {'a.d': {$eq: 3}}]},
      * the new $match will have the expression {$and: [{b: {$gt: 0}}, {d: {$eq: 3}}]} after
      * descending on the path 'a'.
+     *
+     * Should be called _only_ on a MatchExpression that is a predicate on 'path', or subfields of
+     * 'path'. It is also invalid to call this method on an expression including a $elemMatch on
+     * 'path', for example: {'path': {$elemMatch: {'subfield': 3}}}
      */
     static boost::intrusive_ptr<DocumentSourceMatch> descendMatchOnPath(
         MatchExpression* matchExpr,
diff --git a/src/mongo/db/pipeline/document_source_match_test.cpp b/src/mongo/db/pipeline/document_source_match_test.cpp
index 61cd89143cd..f471d6aad31 100644
--- a/src/mongo/db/pipeline/document_source_match_test.cpp
+++ b/src/mongo/db/pipeline/document_source_match_test.cpp
@@ -33,11 +33,15 @@
 #include "mongo/bson/bsonmisc.h"
 #include "mongo/bson/bsonobj.h"
 #include "mongo/bson/json.h"
+#include "mongo/db/matcher/extensions_callback_noop.h"
+#include "mongo/db/matcher/matcher.h"
 #include "mongo/db/pipeline/aggregation_context_fixture.h"
 #include "mongo/db/pipeline/document.h"
 #include "mongo/db/pipeline/document_source_match.h"
 #include "mongo/db/pipeline/document_source_mock.h"
+#include "mongo/db/pipeline/document_value_test_util.h"
 #include "mongo/db/pipeline/pipeline.h"
+#include "mongo/unittest/death_test.h"
 #include "mongo/unittest/unittest.h"
 
 namespace mongo {
@@ -326,6 +330,112 @@ TEST_F(DocumentSourceMatchTest, ShouldPropagatePauses) {
     ASSERT_TRUE(match->getNext().isEOF());
 }
 
+TEST_F(DocumentSourceMatchTest, ShouldCorrectlyJoinWithSubsequentMatch) {
+    const auto match = DocumentSourceMatch::create(BSON("a" << 1), getExpCtx());
+    const auto secondMatch = DocumentSourceMatch::create(BSON("b" << 1), getExpCtx());
+
+    match->joinMatchWith(secondMatch);
+
+    const auto mock = DocumentSourceMock::create({Document{{"a", 1}, {"b", 1}},
+                                                  Document{{"a", 2}, {"b", 1}},
+                                                  Document{{"a", 1}, {"b", 2}},
+                                                  Document{{"a", 2}, {"b", 2}}});
+
+    match->setSource(mock.get());
+
+    // The first result should match.
+    auto next = match->getNext();
+    ASSERT_TRUE(next.isAdvanced());
+    ASSERT_DOCUMENT_EQ(next.releaseDocument(), (Document{{"a", 1}, {"b", 1}}));
+
+    // The rest should not match.
+    ASSERT_TRUE(match->getNext().isEOF());
+    ASSERT_TRUE(match->getNext().isEOF());
+    ASSERT_TRUE(match->getNext().isEOF());
+}
+
+DEATH_TEST_F(DocumentSourceMatchTest,
+             ShouldFailToDescendExpressionOnPathThatIsNotACommonPrefix,
+             "Invariant failure expression::isPathPrefixOf") {
+    const auto expCtx = getExpCtx();
+    const auto matchSpec = BSON("a.b" << 1 << "b.c" << 1);
+    const auto matchExpression = unittest::assertGet(
+        MatchExpressionParser::parse(matchSpec, ExtensionsCallbackNoop(), expCtx->getCollator()));
+    DocumentSourceMatch::descendMatchOnPath(matchExpression.get(), "a", expCtx);
+}
+
+DEATH_TEST_F(DocumentSourceMatchTest,
+             ShouldFailToDescendExpressionOnPathThatContainsElemMatchWithObject,
+             "Invariant failure node->matchType()") {
+    const auto expCtx = getExpCtx();
+    const auto matchSpec = BSON("a" << BSON("$elemMatch" << BSON("a.b" << 1)));
+    const auto matchExpression = unittest::assertGet(
+        MatchExpressionParser::parse(matchSpec, ExtensionsCallbackNoop(), expCtx->getCollator()));
+    BSONObjBuilder out;
+    matchExpression->serialize(&out);
+    DocumentSourceMatch::descendMatchOnPath(matchExpression.get(), "a", expCtx);
+}
+
+// Due to the order of traversal of the MatchExpression tree, this test may actually trigger the
+// invariant failure that the path being descended is not a prefix of the path of the
+// MatchExpression node corresponding to the '$gt' expression, which will report an empty path.
+DEATH_TEST_F(DocumentSourceMatchTest,
+             ShouldFailToDescendExpressionOnPathThatContainsElemMatchWithValue,
+             "Invariant failure") {
+    const auto expCtx = getExpCtx();
+    const auto matchSpec = BSON("a" << BSON("$elemMatch" << BSON("$gt" << 0)));
+    const auto matchExpression = unittest::assertGet(
+        MatchExpressionParser::parse(matchSpec, ExtensionsCallbackNoop(), expCtx->getCollator()));
+    DocumentSourceMatch::descendMatchOnPath(matchExpression.get(), "a", expCtx);
+}
+
+TEST_F(DocumentSourceMatchTest, ShouldMatchCorrectlyAfterDescendingMatch) {
+    const auto expCtx = getExpCtx();
+    const auto matchSpec = BSON("a.b" << 1 << "a.c" << 1 << "a.d" << 1);
+    const auto matchExpression = unittest::assertGet(
+        MatchExpressionParser::parse(matchSpec, ExtensionsCallbackNoop(), expCtx->getCollator()));
+
+    const auto descendedMatch =
+        DocumentSourceMatch::descendMatchOnPath(matchExpression.get(), "a", expCtx);
+    const auto mock = DocumentSourceMock::create(
+        {Document{{"b", 1}, {"c", 1}, {"d", 1}},
+         Document{{"b", 1}, {"a", Document{{"c", 1}}}, {"d", 1}},
+         Document{{"a", Document{{"b", 1}}}, {"a", Document{{"c", 1}}}, {"d", 1}},
+         Document{
+             {"a", Document{{"b", 1}}}, {"a", Document{{"c", 1}}}, {"a", Document{{"d", 1}}}}});
+    descendedMatch->setSource(mock.get());
+
+    auto next = descendedMatch->getNext();
+    ASSERT_TRUE(next.isAdvanced());
+    ASSERT_DOCUMENT_EQ(next.releaseDocument(), (Document{{"b", 1}, {"c", 1}, {"d", 1}}));
+
+    ASSERT_TRUE(descendedMatch->getNext().isEOF());
+    ASSERT_TRUE(descendedMatch->getNext().isEOF());
+    ASSERT_TRUE(descendedMatch->getNext().isEOF());
+}
+
+TEST_F(DocumentSourceMatchTest, ShouldCorrectlyEvaluateElemMatchPredicate) {
+    const auto match =
+        DocumentSourceMatch::create(BSON("a" << BSON("$elemMatch" << BSON("b" << 1))), getExpCtx());
+
+    const std::vector<Document> matchingVector = {Document{{"b", 0}}, Document{{"b", 1}}};
+    const std::vector<Document> nonMatchingVector = {Document{{"b", 0}}, Document{{"b", 2}}};
+    const auto mock = DocumentSourceMock::create(
+        {Document{{"a", matchingVector}}, Document{{"a", nonMatchingVector}}, Document{{"a", 1}}});
+
+    match->setSource(mock.get());
+
+    // The first result should match.
+    auto next = match->getNext();
+    ASSERT_TRUE(next.isAdvanced());
+    ASSERT_DOCUMENT_EQ(next.releaseDocument(), (Document{{"a", matchingVector}}));
+
+    // The rest should not match.
+    ASSERT_TRUE(match->getNext().isEOF());
+    ASSERT_TRUE(match->getNext().isEOF());
+    ASSERT_TRUE(match->getNext().isEOF());
+}
+
 TEST(ObjectForMatch, ShouldExtractTopLevelFieldIfDottedFieldNeeded) {
     Document input(fromjson("{a: 1, b: {c: 1, d: 1}}"));
     BSONObj expected = fromjson("{b: {c: 1, d: 1}}");
diff --git a/src/mongo/db/pipeline/value.cpp b/src/mongo/db/pipeline/value.cpp
index f6cd179f1a6..db1c3047bd9 100644
--- a/src/mongo/db/pipeline/value.cpp
+++ b/src/mongo/db/pipeline/value.cpp
@@ -242,13 +242,22 @@ Value::Value(const BSONArray& arr) : _storage(Array) {
     _storage.putVector(vec.get());
 }
 
-Value::Value(const vector<BSONObj>& arr) : _storage(Array) {
-    intrusive_ptr<RCVector> vec(new RCVector);
-    vec->vec.reserve(arr.size());
-    for (auto&& obj : arr) {
-        vec->vec.push_back(Value(obj));
+Value::Value(const vector<BSONObj>& vec) : _storage(Array) {
+    intrusive_ptr<RCVector> storageVec(new RCVector);
+    storageVec->vec.reserve(vec.size());
+    for (auto&& obj : vec) {
+        storageVec->vec.push_back(Value(obj));
     }
-    _storage.putVector(vec.get());
+    _storage.putVector(storageVec.get());
+}
+
+Value::Value(const vector<Document>& vec) : _storage(Array) {
+    intrusive_ptr<RCVector> storageVec(new RCVector);
+    storageVec->vec.reserve(vec.size());
+    for (auto&& obj : vec) {
+        storageVec->vec.push_back(Value(obj));
+    }
+    _storage.putVector(storageVec.get());
 }
 
 Value Value::createIntOrLong(long long longValue) {
diff --git a/src/mongo/db/pipeline/value.h b/src/mongo/db/pipeline/value.h
index 544cce0d604..7997188bfde 100644
--- a/src/mongo/db/pipeline/value.h
+++ b/src/mongo/db/pipeline/value.h
@@ -103,7 +103,8 @@ public:
     explicit Value(const Document& doc) : _storage(Object, doc) {}
     explicit Value(const BSONObj& obj);
     explicit Value(const BSONArray& arr);
-    explicit Value(const std::vector<BSONObj>& arr);
+    explicit Value(const std::vector<BSONObj>& vec);
+    explicit Value(const std::vector<Document>& vec);
     explicit Value(std::vector<Value> vec) : _storage(Array, new RCVector(std::move(vec))) {}
     explicit Value(const BSONBinData& bd) : _storage(BinData, bd) {}
     explicit Value(const BSONRegEx& re) : _storage(RegEx, re) {}