From 15290f5fe583d4d77e865046c1d8b8d415c2f457 Mon Sep 17 00:00:00 2001 From: Charlie Swanson Date: Tue, 29 Nov 2016 15:39:47 -0500 Subject: SERVER-27213 Recompute dependencies when joining matches --- src/mongo/db/matcher/expression_algo.h | 3 +- src/mongo/db/pipeline/document_source_match.cpp | 4 +- src/mongo/db/pipeline/document_source_match.h | 12 +-- .../db/pipeline/document_source_match_test.cpp | 110 +++++++++++++++++++++ src/mongo/db/pipeline/value.cpp | 21 ++-- src/mongo/db/pipeline/value.h | 3 +- 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 other StatusWithMatchExpression status = uassertStatusOK( MatchExpressionParser::parse(_predicate, ExtensionsCallbackNoop(), pExpCtx->getCollator())); _expression = std::move(status.getValue()); + _dependencies = DepsTracker(_dependencies.getMetadataAvailable()); + getDependencies(&_dependencies); } pair, intrusive_ptr> @@ -370,7 +372,7 @@ DocumentSourceMatch::splitSourceBy(const std::set& 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& 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 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 matchingVector = {Document{{"b", 0}}, Document{{"b", 1}}}; + const std::vector 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& arr) : _storage(Array) { - intrusive_ptr vec(new RCVector); - vec->vec.reserve(arr.size()); - for (auto&& obj : arr) { - vec->vec.push_back(Value(obj)); +Value::Value(const vector& vec) : _storage(Array) { + intrusive_ptr 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& vec) : _storage(Array) { + intrusive_ptr 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& arr); + explicit Value(const std::vector& vec); + explicit Value(const std::vector& vec); explicit Value(std::vector vec) : _storage(Array, new RCVector(std::move(vec))) {} explicit Value(const BSONBinData& bd) : _storage(BinData, bd) {} explicit Value(const BSONRegEx& re) : _storage(RegEx, re) {} -- cgit v1.2.1