From b4d1ffd5c9474f6b3665b2fb0f886005c6cd91cc Mon Sep 17 00:00:00 2001 From: Ruoxin Xu Date: Tue, 15 Sep 2020 02:59:44 +0100 Subject: SERVER-50504 $facet+$match on positional path does not seem to pass results to certain subsequent stages --- jstests/aggregation/sources/facet/use_cases.js | 17 +++++++++++++++++ .../aggregation/sources/match/dotted_numeric_path.js | 20 ++++++++++++++++++++ src/mongo/db/matcher/expression_path.h | 15 +++++++++++++++ src/mongo/db/pipeline/document_source_match_test.cpp | 14 ++++++++++++++ 4 files changed, 66 insertions(+) create mode 100644 jstests/aggregation/sources/match/dotted_numeric_path.js diff --git a/jstests/aggregation/sources/facet/use_cases.js b/jstests/aggregation/sources/facet/use_cases.js index c3ced6a289d..1a2d662d437 100644 --- a/jstests/aggregation/sources/facet/use_cases.js +++ b/jstests/aggregation/sources/facet/use_cases.js @@ -106,4 +106,21 @@ const facetAutoBucketedPrices = facetResult[0].autoBucketedPrices; assert.sameMembers(facetManufacturers, mostCommonManufacturers); assert.sameMembers(facetBucketedPrices, numTVsBucketedByPriceRange); assert.sameMembers(facetAutoBucketedPrices, numTVsAutomaticallyBucketedByPriceRange); + +/** + * A simple case using $facet + $match. This also tests the bug found in SERVER-50504 is fixed. + */ + +coll.drop(); +assert.commandWorked(coll.insert({"_id": 1, "quizzes": [{"score": 100}]})); +assert.commandWorked(coll.insert({"_id": 2, "quizzes": [{"score": 200}]})); + +const facetPipeline = + [{$facet: {scoreRank: [{$match: {'quizzes.0.score': {$gt: 0}}}, {$count: 'count'}]}}]; + +const facetRes = coll.aggregate(facetPipeline).toArray(); +assert.eq(facetRes.length, 1); +const scoreRank = facetRes[0]['scoreRank']; +assert.eq(scoreRank.length, 1); +assert.eq(scoreRank[0]['count'], 2); }()); diff --git a/jstests/aggregation/sources/match/dotted_numeric_path.js b/jstests/aggregation/sources/match/dotted_numeric_path.js new file mode 100644 index 00000000000..e2e77147979 --- /dev/null +++ b/jstests/aggregation/sources/match/dotted_numeric_path.js @@ -0,0 +1,20 @@ +/** + * Tests that $match works correctly with dotted numeric path. + * + * @tags: [sbe_incompatible] + */ +(function() { +"use strict"; + +const collName = "dotted_numeric_path"; +const coll = db.getCollection(collName); +coll.drop(); + +assert.commandWorked(coll.insert({"_id": 1, "quizzes": [{"score": 100}]})); +assert.commandWorked(coll.insert({"_id": 2, "quizzes": [{"score": 200}]})); + +const res = coll.aggregate([{$match: {'quizzes.0.score': {$gt: 0}}}, {$count: 'count'}]).toArray(); + +assert.eq(res.length, 1); +assert.eq(res[0]['count'], 2); +}()); diff --git a/src/mongo/db/matcher/expression_path.h b/src/mongo/db/matcher/expression_path.h index a9b6ec6ca80..191e0356121 100644 --- a/src/mongo/db/matcher/expression_path.h +++ b/src/mongo/db/matcher/expression_path.h @@ -29,6 +29,7 @@ #pragma once +#include "mongo/db/field_ref.h" #include "mongo/db/matcher/expression.h" namespace mongo { @@ -138,6 +139,20 @@ public: protected: void _doAddDependencies(DepsTracker* deps) const final { if (!path().empty()) { + // If a path contains a numeric component then it should not be naively added to the + // projection, since we do not support projecting specific array indices. Instead we add + // the prefix of the path up to the numeric path component. Note that we start at path + // component 1 rather than 0, because a numeric path component at the root of the + // document can only ever be a field name, never an array index. + FieldRef fieldRef(path()); + for (size_t i = 1; i < fieldRef.numParts(); ++i) { + if (fieldRef.isNumericPathComponentStrict(i)) { + auto prefix = fieldRef.dottedSubstring(0, i); + deps->fields.insert(prefix.toString()); + return; + } + } + deps->fields.insert(path().toString()); } } diff --git a/src/mongo/db/pipeline/document_source_match_test.cpp b/src/mongo/db/pipeline/document_source_match_test.cpp index 634129596c9..703512557d6 100644 --- a/src/mongo/db/pipeline/document_source_match_test.cpp +++ b/src/mongo/db/pipeline/document_source_match_test.cpp @@ -226,6 +226,20 @@ TEST_F(DocumentSourceMatchTest, ShouldAddDependenciesOfAllBranchesOfOrClause) { ASSERT_EQUALS(false, dependencies.getNeedsMetadata(DocumentMetadataFields::kTextScore)); } +TEST_F(DocumentSourceMatchTest, ShouldNotAddPotentialArrayIndexToDependencies) { + auto match = DocumentSourceMatch::create( + fromjson("{$or: [{'a.0': 1, '3': 1, 'd.01': 1}, {'b.c.d.1': {$gt: 1}}]}"), getExpCtx()); + DepsTracker dependencies; + ASSERT_EQUALS(DepsTracker::State::SEE_NEXT, match->getDependencies(&dependencies)); + // Here we add "a" instead of "a.0". Since we do not support projecting specific array indices, + // we add the prefix of the path up to the numeric path component instead. + ASSERT_EQUALS(1U, dependencies.fields.count("a")); + ASSERT_EQUALS(1U, dependencies.fields.count("b.c.d")); + ASSERT_EQUALS(1U, dependencies.fields.count("3")); + ASSERT_EQUALS(1U, dependencies.fields.count("d.01")); + ASSERT_EQUALS(4U, dependencies.fields.size()); +} + TEST_F(DocumentSourceMatchTest, TextSearchShouldRequireWholeDocumentAndTextScore) { auto match = DocumentSourceMatch::create(fromjson("{$text: {$search: 'hello'} }"), getExpCtx()); DepsTracker dependencies(DepsTracker::kAllMetadata & ~DepsTracker::kOnlyTextScore); -- cgit v1.2.1