diff options
author | Ian Boros <puppyofkosh@gmail.com> | 2019-04-04 19:20:35 -0400 |
---|---|---|
committer | Ian Boros <puppyofkosh@gmail.com> | 2019-04-11 18:34:10 -0400 |
commit | 502df279c7476c01758ab210728f4acc4a27a218 (patch) | |
tree | f350da86174a673c792ca99e16ae43a0319e71c8 /src | |
parent | d131d7861c73efe052c5909ae8f1452c100a461d (diff) | |
download | mongo-502df279c7476c01758ab210728f4acc4a27a218.tar.gz |
SERVER-39567 Change find min/max options to require hint
Diffstat (limited to 'src')
-rw-r--r-- | src/mongo/db/exec/cached_plan.cpp | 7 | ||||
-rw-r--r-- | src/mongo/db/exec/subplan.cpp | 6 | ||||
-rw-r--r-- | src/mongo/db/query/get_executor.cpp | 6 | ||||
-rw-r--r-- | src/mongo/db/query/index_entry.h | 8 | ||||
-rw-r--r-- | src/mongo/db/query/plan_cache_test.cpp | 10 | ||||
-rw-r--r-- | src/mongo/db/query/planner_ixselect.cpp | 5 | ||||
-rw-r--r-- | src/mongo/db/query/planner_ixselect.h | 2 | ||||
-rw-r--r-- | src/mongo/db/query/query_planner.cpp | 76 | ||||
-rw-r--r-- | src/mongo/db/query/query_planner_collation_test.cpp | 65 | ||||
-rw-r--r-- | src/mongo/db/query/query_planner_test.cpp | 52 | ||||
-rw-r--r-- | src/mongo/db/query/query_planner_wildcard_index_test.cpp | 15 | ||||
-rw-r--r-- | src/mongo/db/query/query_request.cpp | 2 | ||||
-rw-r--r-- | src/mongo/dbtests/querytests.cpp | 33 |
13 files changed, 134 insertions, 153 deletions
diff --git a/src/mongo/db/exec/cached_plan.cpp b/src/mongo/db/exec/cached_plan.cpp index b824b0a9e66..e426a0ad340 100644 --- a/src/mongo/db/exec/cached_plan.cpp +++ b/src/mongo/db/exec/cached_plan.cpp @@ -198,10 +198,9 @@ Status CachedPlanStage::replan(PlanYieldPolicy* yieldPolicy, bool shouldCache) { // Use the query planning module to plan the whole query. auto statusWithSolutions = QueryPlanner::plan(*_canonicalQuery, _plannerParams); if (!statusWithSolutions.isOK()) { - return Status(ErrorCodes::BadValue, - str::stream() << "error processing query: " << _canonicalQuery->toString() - << " planner returned error: " - << statusWithSolutions.getStatus().reason()); + return statusWithSolutions.getStatus().withContext( + str::stream() << "error processing query: " << _canonicalQuery->toString() + << " planner returned error"); } auto solutions = std::move(statusWithSolutions.getValue()); diff --git a/src/mongo/db/exec/subplan.cpp b/src/mongo/db/exec/subplan.cpp index dade23b7b5e..168fc342d3d 100644 --- a/src/mongo/db/exec/subplan.cpp +++ b/src/mongo/db/exec/subplan.cpp @@ -372,9 +372,9 @@ Status SubplanStage::choosePlanWholeQuery(PlanYieldPolicy* yieldPolicy) { // Use the query planning module to plan the whole query. auto statusWithSolutions = QueryPlanner::plan(*_query, _plannerParams); if (!statusWithSolutions.isOK()) { - return Status(ErrorCodes::BadValue, - "error processing query: " + _query->toString() + - " planner returned error: " + statusWithSolutions.getStatus().reason()); + return statusWithSolutions.getStatus().withContext( + str::stream() << "error processing query: " << _query->toString() + << " planner returned error"); } auto solutions = std::move(statusWithSolutions.getValue()); diff --git a/src/mongo/db/query/get_executor.cpp b/src/mongo/db/query/get_executor.cpp index 727f6d52449..339074d502e 100644 --- a/src/mongo/db/query/get_executor.cpp +++ b/src/mongo/db/query/get_executor.cpp @@ -469,9 +469,9 @@ StatusWith<PrepareExecutionResult> prepareExecution(OperationContext* opCtx, auto statusWithSolutions = QueryPlanner::plan(*canonicalQuery, plannerParams); if (!statusWithSolutions.isOK()) { - return Status(ErrorCodes::BadValue, - "error processing query: " + canonicalQuery->toString() + - " planner returned error: " + statusWithSolutions.getStatus().reason()); + return statusWithSolutions.getStatus().withContext( + str::stream() << "error processing query: " << canonicalQuery->toString() + << " planner returned error"); } auto solutions = std::move(statusWithSolutions.getValue()); diff --git a/src/mongo/db/query/index_entry.h b/src/mongo/db/query/index_entry.h index 89b14ffd609..ede83b8e700 100644 --- a/src/mongo/db/query/index_entry.h +++ b/src/mongo/db/query/index_entry.h @@ -71,6 +71,8 @@ struct CoreIndexInfo { invariant((type == IndexType::INDEX_WILDCARD) == (projExec != nullptr)); } + virtual ~CoreIndexInfo() = default; + /** * This struct is used to uniquely identify an index. The index "Identifier" has two * components: catalog name, and "disambiguator". The catalog name is just the name of the @@ -164,6 +166,12 @@ struct IndexEntry : CoreIndexInfo { invariant(multikeyPaths.empty() || multikeyPathSet.empty()); } + IndexEntry(const IndexEntry&) = default; + IndexEntry(IndexEntry&&) = default; + + IndexEntry& operator=(const IndexEntry&) = default; + IndexEntry& operator=(IndexEntry&&) = default; + ~IndexEntry() { // An IndexEntry should never have both formats of multikey metadata simultaneously. invariant(multikeyPaths.empty() || multikeyPathSet.empty()); diff --git a/src/mongo/db/query/plan_cache_test.cpp b/src/mongo/db/query/plan_cache_test.cpp index 6cf50b688a1..5614137b90a 100644 --- a/src/mongo/db/query/plan_cache_test.cpp +++ b/src/mongo/db/query/plan_cache_test.cpp @@ -394,7 +394,8 @@ TEST(PlanCacheTest, ShouldNotCacheQueryWithHint) { * Min queries are a specialized case of hinted queries */ TEST(PlanCacheTest, ShouldNotCacheQueryWithMin) { - unique_ptr<CanonicalQuery> cq(canonicalize("{a: 1}", "{}", "{}", 0, 0, "{}", "{a: 100}", "{}")); + unique_ptr<CanonicalQuery> cq( + canonicalize("{a: 1}", "{}", "{}", 0, 0, "{a: 1}", "{a: 100}", "{}")); assertShouldNotCacheQuery(*cq); } @@ -402,7 +403,8 @@ TEST(PlanCacheTest, ShouldNotCacheQueryWithMin) { * Max queries are non-cacheable for the same reasons as min queries. */ TEST(PlanCacheTest, ShouldNotCacheQueryWithMax) { - unique_ptr<CanonicalQuery> cq(canonicalize("{a: 1}", "{}", "{}", 0, 0, "{}", "{}", "{a: 100}")); + unique_ptr<CanonicalQuery> cq( + canonicalize("{a: 1}", "{}", "{}", 0, 0, "{a: 1}", "{}", "{a: 100}")); assertShouldNotCacheQuery(*cq); } @@ -1659,7 +1661,7 @@ TEST_F(CachePlanSelectionTest, GeoNear2DNotCached) { TEST_F(CachePlanSelectionTest, MinNotCached) { addIndex(BSON("a" << 1), "a_1"); - runQueryHintMinMax(BSONObj(), BSONObj(), fromjson("{a: 1}"), BSONObj()); + runQueryHintMinMax(BSONObj(), fromjson("{a: 1}"), fromjson("{a: 1}"), BSONObj()); assertNotCached( "{fetch: {filter: null, " "node: {ixscan: {filter: null, pattern: {a: 1}}}}}"); @@ -1667,7 +1669,7 @@ TEST_F(CachePlanSelectionTest, MinNotCached) { TEST_F(CachePlanSelectionTest, MaxNotCached) { addIndex(BSON("a" << 1), "a_1"); - runQueryHintMinMax(BSONObj(), BSONObj(), BSONObj(), fromjson("{a: 1}")); + runQueryHintMinMax(BSONObj(), fromjson("{a: 1}"), BSONObj(), fromjson("{a: 1}")); assertNotCached( "{fetch: {filter: null, " "node: {ixscan: {filter: null, pattern: {a: 1}}}}}"); diff --git a/src/mongo/db/query/planner_ixselect.cpp b/src/mongo/db/query/planner_ixselect.cpp index 1f314787fdb..77f662a4701 100644 --- a/src/mongo/db/query/planner_ixselect.cpp +++ b/src/mongo/db/query/planner_ixselect.cpp @@ -321,14 +321,13 @@ std::vector<IndexEntry> QueryPlannerIXSelect::findRelevantIndices( } std::vector<IndexEntry> QueryPlannerIXSelect::expandIndexes( - const stdx::unordered_set<std::string>& fields, - const std::vector<IndexEntry>& relevantIndices) { + const stdx::unordered_set<std::string>& fields, std::vector<IndexEntry> relevantIndices) { std::vector<IndexEntry> out; for (auto&& entry : relevantIndices) { if (entry.type == IndexType::INDEX_WILDCARD) { wcp::expandWildcardIndexEntry(entry, fields, &out); } else { - out.push_back(entry); + out.push_back(std::move(entry)); } } diff --git a/src/mongo/db/query/planner_ixselect.h b/src/mongo/db/query/planner_ixselect.h index b4269f9b64d..1b7fed5eed0 100644 --- a/src/mongo/db/query/planner_ixselect.h +++ b/src/mongo/db/query/planner_ixselect.h @@ -133,7 +133,7 @@ public: * "expanded" indexes (where the $** indexes in the given list have been expanded). */ static std::vector<IndexEntry> expandIndexes(const stdx::unordered_set<std::string>& fields, - const std::vector<IndexEntry>& relevantIndices); + std::vector<IndexEntry> relevantIndices); /** * Check if this match expression is a leaf and is supported by a wildcard index. diff --git a/src/mongo/db/query/query_planner.cpp b/src/mongo/db/query/query_planner.cpp index fb819ce9c36..655a6816194 100644 --- a/src/mongo/db/query/query_planner.cpp +++ b/src/mongo/db/query/query_planner.cpp @@ -151,6 +151,7 @@ static BSONObj getKeyFromQuery(const BSONObj& keyPattern, const BSONObj& query) static bool indexCompatibleMaxMin(const BSONObj& obj, const CollatorInterface* queryCollator, const IndexEntry& indexEntry) { + // Wildcard indexes should have been filtered out by the time this is called. if (indexEntry.type == IndexType::INDEX_WILDCARD) { return false; } @@ -596,6 +597,8 @@ StatusWith<std::vector<std::unique_ptr<QuerySolution>>> QueryPlanner::plan( // indexes. std::vector<IndexEntry> fullIndexList; + // Will hold a copy of the index entry chosen by the hint. + boost::optional<IndexEntry> hintedIndexEntry; if (hintedIndex.isEmpty()) { fullIndexList = params.indices; } else { @@ -613,6 +616,8 @@ StatusWith<std::vector<std::unique_ptr<QuerySolution>>> QueryPlanner::plan( << " and " << fullIndexList[1].toString()); } + + hintedIndexEntry.emplace(fullIndexList.front()); } // Figure out what fields we care about. @@ -625,7 +630,7 @@ StatusWith<std::vector<std::unique_ptr<QuerySolution>>> QueryPlanner::plan( fullIndexList = QueryPlannerIXSelect::expandIndexes(fields, std::move(fullIndexList)); std::vector<IndexEntry> relevantIndices; - if (hintedIndex.isEmpty()) { + if (!hintedIndexEntry) { relevantIndices = QueryPlannerIXSelect::findRelevantIndices(fields, fullIndexList); } else { relevantIndices = fullIndexList; @@ -639,60 +644,45 @@ StatusWith<std::vector<std::unique_ptr<QuerySolution>>> QueryPlanner::plan( } } - // TODO SERVER-35335 Ensure min/max can generate bounds over $** index. // Deal with the .min() and .max() query options. If either exist we can only use an index // that matches the object inside. if (!query.getQueryRequest().getMin().isEmpty() || !query.getQueryRequest().getMax().isEmpty()) { + + if (!hintedIndexEntry) { + return Status(ErrorCodes::Error(51173), + "When using min()/max() a hint of which index to use must be provided"); + } + BSONObj minObj = query.getQueryRequest().getMin(); BSONObj maxObj = query.getQueryRequest().getMax(); - // The unfinished siblings of these objects may not be proper index keys because they - // may be empty objects or have field names. When an index is picked to use for the - // min/max query, these "finished" objects will always be valid index keys for the - // index's key pattern. - BSONObj finishedMinObj; - BSONObj finishedMaxObj; - - // Index into the 'fulledIndexList' vector indicating the index that we will use to answer - // this min/max query. - size_t idxNo = numeric_limits<size_t>::max(); - - for (size_t i = 0; i < fullIndexList.size(); ++i) { - const auto& indexEntry = fullIndexList[i]; - - const BSONObj toUse = minObj.isEmpty() ? maxObj : minObj; - if (indexCompatibleMaxMin(toUse, query.getCollator(), indexEntry)) { - // In order to be fully compatible, the min has to be less than the max - // according to the index key pattern ordering. The first step in verifying - // this is "finish" the min and max by replacing empty objects and stripping - // field names. - finishedMinObj = finishMinObj(indexEntry, minObj, maxObj); - finishedMaxObj = finishMaxObj(indexEntry, minObj, maxObj); - - // Now we have the final min and max. This index is only relevant for - // the min/max query if min < max. - if (0 > finishedMinObj.woCompare(finishedMaxObj, indexEntry.keyPattern, false)) { - // Found a relevant index. - idxNo = i; - break; - } - - // This index is not relevant; move on to the next. - } + if (!indexCompatibleMaxMin( + minObj.isEmpty() ? maxObj : minObj, query.getCollator(), *hintedIndexEntry)) { + return Status(ErrorCodes::Error(51174), + "The index chosen is not compatible with min/max"); } + // Be sure that index expansion didn't do anything. As wildcard indexes are banned for + // min/max, we expect to find a single hinted index entry. + invariant(fullIndexList.size() == 1); + invariant(*hintedIndexEntry == fullIndexList.front()); - if (idxNo == numeric_limits<size_t>::max()) { - LOG(5) << "Can't find relevant index to use for max/min query"; - // Can't find an index to use, bail out. - return Status(ErrorCodes::BadValue, "unable to find relevant index for max/min query"); - } + // In order to be fully compatible, the min has to be less than the max according to the + // index key pattern ordering. The first step in verifying this is "finish" the min and max + // by replacing empty objects and stripping field names. + BSONObj finishedMinObj = finishMinObj(*hintedIndexEntry, minObj, maxObj); + BSONObj finishedMaxObj = finishMaxObj(*hintedIndexEntry, minObj, maxObj); - LOG(5) << "Max/min query using index " << fullIndexList[idxNo].toString(); + // Now we have the final min and max. This index is only relevant for the min/max query if + // min < max. + if (finishedMinObj.woCompare(finishedMaxObj, hintedIndexEntry->keyPattern, false) >= 0) { + return Status(ErrorCodes::Error(51175), + "The value provided for min() does not come before the value provided " + "for max() in the hinted index"); + } - // Make our scan and output. std::unique_ptr<QuerySolutionNode> solnRoot(QueryPlannerAccess::makeIndexScan( - fullIndexList[idxNo], query, params, finishedMinObj, finishedMaxObj)); + *hintedIndexEntry, query, params, finishedMinObj, finishedMaxObj)); invariant(solnRoot); auto soln = QueryPlannerAnalysis::analyzeDataAccess(query, params, std::move(solnRoot)); diff --git a/src/mongo/db/query/query_planner_collation_test.cpp b/src/mongo/db/query/query_planner_collation_test.cpp index 9606b62f4a0..5cefee957c1 100644 --- a/src/mongo/db/query/query_planner_collation_test.cpp +++ b/src/mongo/db/query/query_planner_collation_test.cpp @@ -211,9 +211,9 @@ TEST_F(QueryPlannerTest, MinMaxWithStringBoundsCannotBeCoveredWithCollator) { CollatorInterfaceMock collator(CollatorInterfaceMock::MockType::kReverseString); addIndex(fromjson("{a: 1, b: 1}"), &collator); - runQueryAsCommand( - fromjson("{find: 'testns', min: {a: 1, b: 2}, max: {a: 2, b: 1}, " - "projection: {_id: 0, a: 1, b: 1}, collation: {locale: 'reverse'}}")); + runQueryAsCommand(fromjson( + "{find: 'testns', min: {a: 1, b: 2}, max: {a: 2, b: 1}, " + "projection: {_id: 0, a: 1, b: 1}, hint: {a: 1, b: 1}, collation: {locale: 'reverse'}}")); assertNumSolutions(1U); assertSolutionExists( @@ -446,7 +446,8 @@ TEST_F(QueryPlannerTest, SuccessfullyPlanWhenMinMaxHaveNumberBoundariesAndCollat addIndex(fromjson("{a: 1, b: 1, c: 1}"), &indexCollator); runQueryAsCommand( - fromjson("{find: 'testns', min: {a: 1, b: 1, c: 1}, max: {a: 3, b: 3, c: 3}}")); + fromjson("{find: 'testns', min: {a: 1, b: 1, c: 1}, max: {a: 3, b: 3, c: 3}," + "hint: {a:1, b: 1, c: 1}}")); assertNumSolutions(1U); assertSolutionExists("{fetch: {node: {ixscan: {pattern: {a: 1, b: 1, c: 1}}}}}"); @@ -455,37 +456,48 @@ TEST_F(QueryPlannerTest, SuccessfullyPlanWhenMinMaxHaveNumberBoundariesAndCollat TEST_F(QueryPlannerTest, FailToPlanWhenMinHasStringBoundaryAndCollationsDontMatch) { CollatorInterfaceMock indexCollator(CollatorInterfaceMock::MockType::kToLowerString); addIndex(fromjson("{a: 1, b: 1, c: 1}"), &indexCollator); - runInvalidQueryAsCommand(fromjson("{find: 'testns', min: {a: 1, b: 'foo', c: 1}}")); + runInvalidQueryAsCommand( + fromjson("{find: 'testns', min: {a: 1, b: 'foo', c: 1}, " + "hint: {a: 1, b: 1, c: 1}}")); } TEST_F(QueryPlannerTest, FailToPlanWhenMaxHasStringBoundaryAndCollationsDontMatch) { CollatorInterfaceMock indexCollator(CollatorInterfaceMock::MockType::kToLowerString); addIndex(fromjson("{a: 1, b: 1, c: 1}"), &indexCollator); - runInvalidQueryAsCommand(fromjson("{find: 'testns', max: {a: 1, b: 'foo', c: 1}}")); + runInvalidQueryAsCommand( + fromjson("{find: 'testns', max: {a: 1, b: 'foo', c: 1}, hint: {a: 1, b: 1, c: 1}}")); } TEST_F(QueryPlannerTest, FailToPlanWhenMinHasObjectBoundaryAndCollationsDontMatch) { CollatorInterfaceMock indexCollator(CollatorInterfaceMock::MockType::kToLowerString); addIndex(fromjson("{a: 1, b: 1, c: 1}"), &indexCollator); - runInvalidQueryAsCommand(fromjson("{find: 'testns', min: {a: 1, b: {d: 'foo'}, c: 1}}")); + runInvalidQueryAsCommand( + fromjson("{find: 'testns', min: {a: 1, b: {d: 'foo'}, c: 1}, " + "hint: {a: 1, b: 1, c: 1}}")); } TEST_F(QueryPlannerTest, FailToPlanWhenMaxHasObjectBoundaryAndCollationsDontMatch) { CollatorInterfaceMock indexCollator(CollatorInterfaceMock::MockType::kToLowerString); addIndex(fromjson("{a: 1, b: 1, c: 1}"), &indexCollator); - runInvalidQueryAsCommand(fromjson("{find: 'testns', max: {a: 1, b: {d: 'foo'}, c: 1}}")); + runInvalidQueryAsCommand( + fromjson("{find: 'testns', max: {a: 1, b: {d: 'foo'}, c: 1}, " + "hint: {a: 1, b: 1, c: 1}}")); } TEST_F(QueryPlannerTest, FailToPlanWhenMinHasArrayBoundaryAndCollationsDontMatch) { CollatorInterfaceMock indexCollator(CollatorInterfaceMock::MockType::kToLowerString); addIndex(fromjson("{a: 1, b: 1, c: 1}"), &indexCollator); - runInvalidQueryAsCommand(fromjson("{find: 'testns', min: {a: 1, b: 1, c: [1, 'foo']}}")); + runInvalidQueryAsCommand( + fromjson("{find: 'testns', min: {a: 1, b: 1, c: [1, 'foo']}, " + "hint: {a: 1, b: 1, c: 1}}")); } TEST_F(QueryPlannerTest, FailToPlanWhenMaxHasArrayBoundaryAndCollationsDontMatch) { CollatorInterfaceMock indexCollator(CollatorInterfaceMock::MockType::kToLowerString); addIndex(fromjson("{a: 1, b: 1, c: 1}"), &indexCollator); - runInvalidQueryAsCommand(fromjson("{find: 'testns', max: {a: 1, b: 1, c: [1, 'foo']}}")); + runInvalidQueryAsCommand( + fromjson("{find: 'testns', max: {a: 1, b: 1, c: [1, 'foo']}, " + "hint: {a: 1, b: 1, c: 1}}")); } TEST_F(QueryPlannerTest, FailToPlanWhenHintingIndexIncompatibleWithMinDueToCollation) { @@ -502,35 +514,30 @@ TEST_F(QueryPlannerTest, FailToPlanWhenHintingIndexIncompatibleWithMaxDueToColla runInvalidQueryAsCommand(fromjson("{find: 'testns', max: {a: 'foo'}, hint: 'indexToHint'}")); } -TEST_F(QueryPlannerTest, SelectIndexWithMatchingSimpleCollationWhenMinHasStringBoundary) { - CollatorInterfaceMock indexCollator(CollatorInterfaceMock::MockType::kToLowerString); - addIndex(fromjson("{a: 1}"), &indexCollator, "withCollation"_sd); +TEST_F(QueryPlannerTest, SuccessWithIndexWithMatchingSimpleCollationWhenMinHasStringBoundary) { addIndex(fromjson("{a: 1}"), nullptr, "noCollation"_sd); - runQueryAsCommand(fromjson("{find: 'testns', min: {a: 'foo'}}")); + runQueryAsCommand(fromjson("{find: 'testns', min: {a: 'foo'}, hint: {a: 1}}")); assertNumSolutions(1U); assertSolutionExists("{fetch: {node: {ixscan: {pattern: {a: 1}, name: 'noCollation'}}}}"); } -TEST_F(QueryPlannerTest, SelectIndexWithMatchingNonSimpleCollationWhenMinHasStringBoundary) { +TEST_F(QueryPlannerTest, SuccessWithIndexWithMatchingNonSimpleCollationWhenMinHasStringBoundary) { CollatorInterfaceMock indexCollator(CollatorInterfaceMock::MockType::kReverseString); addIndex(fromjson("{a: 1}"), &indexCollator, "withCollation"_sd); - addIndex(fromjson("{a: 1}"), nullptr, "noCollation"_sd); - runQueryAsCommand( - fromjson("{find: 'testns', min: {a: 'foo'}, collation: {locale: 'reverse'}}")); + runQueryAsCommand(fromjson( + "{find: 'testns', min: {a: 'foo'}, hint: {a: 1}, collation: {locale: 'reverse'}}")); assertNumSolutions(1U); assertSolutionExists("{fetch: {node: {ixscan: {pattern: {a: 1}, name: 'withCollation'}}}}"); } -TEST_F(QueryPlannerTest, SelectIndexWithMatchingSimpleCollationWhenMaxHasStringBoundary) { - CollatorInterfaceMock indexCollator(CollatorInterfaceMock::MockType::kToLowerString); - addIndex(fromjson("{a: 1}"), &indexCollator, "withCollation"_sd); +TEST_F(QueryPlannerTest, SuccessWithIndexWithMatchingSimpleCollationWhenMaxHasStringBoundary) { addIndex(fromjson("{a: 1}"), nullptr, "noCollation"_sd); - runQueryAsCommand(fromjson("{find: 'testns', max: {a: 'foo'}}")); + runQueryAsCommand(fromjson("{find: 'testns', max: {a: 'foo'}, hint: {a: 1}}")); assertNumSolutions(1U); assertSolutionExists("{fetch: {node: {ixscan: {pattern: {a: 1}, name: 'noCollation'}}}}"); @@ -540,8 +547,8 @@ TEST_F(QueryPlannerTest, MustSortInMemoryWhenMinMaxQueryHasCollationAndIndexDoes addIndex(fromjson("{a: 1, b: 1}")); runQueryAsCommand( - fromjson("{find: 'testns', min: {a: 1, b: 1}, max: {a: 2, b: 1}, collation: {locale: " - "'reverse'}, sort: {a: 1, b: 1}}")); + fromjson("{find: 'testns', min: {a: 1, b: 1}, max: {a: 2, b: 1}, hint: {a:1, b:1}, " + "collation: {locale: 'reverse'}, sort: {a: 1, b: 1}}")); assertNumSolutions(1U); @@ -555,7 +562,8 @@ TEST_F(QueryPlannerTest, MustSortInMemoryWhenMinMaxIndexHasCollationAndQueryDoes addIndex(fromjson("{a: 1, b:1}"), &collator); runQueryAsCommand( - fromjson("{find: 'testns', min: {a: 1, b: 1}, max: {a: 2, b: 1}, sort: {a: 1, b: 1}}")); + fromjson("{find: 'testns', min: {a: 1, b: 1}, max: {a: 2, b: 1}, sort: {a: 1, b: 1}, " + "hint: {a: 1, b: 1}}")); assertNumSolutions(1U); @@ -569,7 +577,7 @@ TEST_F(QueryPlannerTest, CanProduceCoveredSortPlanWhenQueryHasCollationButIndexD runQueryAsCommand(fromjson( "{find: 'testns', projection: {a: 1, b: 1, _id: 0}, min: {a: 1, b: 1}, max: {a: 2, " - "b: 1}, collation: {locale: 'reverse'}, sort: {a: 1, b: 1}}")); + "b: 1}, hint: {a: 1, b: 1}, collation: {locale: 'reverse'}, sort: {a: 1, b: 1}}")); assertNumSolutions(1U); assertSolutionExists( @@ -631,8 +639,9 @@ TEST_F(QueryPlannerTest, NoSortStageWhenMinMaxIndexCollationDoesNotMatchButBound addIndex(fromjson("{a: 1, b: 1, c: 1}")); runQueryAsCommand( - fromjson("{find: 'testns', min: {a: 1, b: 8, c: 1}, max: {a: 1, b: 8, c: 100}, collation: " - "{locale: 'reverse'}, sort: {a: 1, b: 1, c: 1}}")); + fromjson("{find: 'testns', min: {a: 1, b: 8, c: 1}, max: {a: 1, b: 8, c: 100}, " + "hint: {a: 1, b: 1, c: 1}, collation: {locale: 'reverse'}, " + "sort: {a: 1, b: 1, c: 1}}")); assertNumSolutions(1U); assertSolutionExists("{fetch: {node: {ixscan: {pattern: {a: 1, b: 1, c: 1}}}}}"); diff --git a/src/mongo/db/query/query_planner_test.cpp b/src/mongo/db/query/query_planner_test.cpp index fe55792abfe..3c38b272fd1 100644 --- a/src/mongo/db/query/query_planner_test.cpp +++ b/src/mongo/db/query/query_planner_test.cpp @@ -1358,7 +1358,7 @@ TEST_F(QueryPlannerTest, OrWithExactAndInexact3) { TEST_F(QueryPlannerTest, MinValid) { addIndex(BSON("a" << 1)); - runQueryHintMinMax(BSONObj(), BSONObj(), fromjson("{a: 1}"), BSONObj()); + runQueryHintMinMax(BSONObj(), BSONObj(fromjson("{a: 1}")), fromjson("{a: 1}"), BSONObj()); assertNumSolutions(1U); assertSolutionExists( @@ -1367,7 +1367,8 @@ TEST_F(QueryPlannerTest, MinValid) { } TEST_F(QueryPlannerTest, MinWithoutIndex) { - runInvalidQueryHintMinMax(BSONObj(), BSONObj(), fromjson("{a: 1}"), BSONObj()); + runInvalidQueryHintMinMax( + BSONObj(), BSONObj(fromjson("{a: 1}")), fromjson("{a: 1}"), BSONObj()); } TEST_F(QueryPlannerTest, MinBadHint) { @@ -1377,7 +1378,7 @@ TEST_F(QueryPlannerTest, MinBadHint) { TEST_F(QueryPlannerTest, MaxValid) { addIndex(BSON("a" << 1)); - runQueryHintMinMax(BSONObj(), BSONObj(), BSONObj(), fromjson("{a: 1}")); + runQueryHintMinMax(BSONObj(), BSONObj(fromjson("{a: 1}")), BSONObj(), fromjson("{a: 1}")); assertNumSolutions(1U); assertSolutionExists( @@ -1387,11 +1388,13 @@ TEST_F(QueryPlannerTest, MaxValid) { TEST_F(QueryPlannerTest, MinMaxSameValue) { addIndex(BSON("a" << 1)); - runInvalidQueryHintMinMax(BSONObj(), BSONObj(), fromjson("{a: 1}"), fromjson("{a: 1}")); + runInvalidQueryHintMinMax( + BSONObj(), BSONObj(fromjson("{a: 1}")), fromjson("{a: 1}"), fromjson("{a: 1}")); } TEST_F(QueryPlannerTest, MaxWithoutIndex) { - runInvalidQueryHintMinMax(BSONObj(), BSONObj(), BSONObj(), fromjson("{a: 1}")); + runInvalidQueryHintMinMax( + BSONObj(), BSONObj(fromjson("{a: 1}")), BSONObj(), fromjson("{a: 1}")); } TEST_F(QueryPlannerTest, MaxBadHint) { @@ -1408,7 +1411,7 @@ TEST_F(QueryPlannerTest, MaxMinSort) { BSONObj(), 0, 0, - BSONObj(), + fromjson("{a: 1}"), fromjson("{a: 2}"), fromjson("{a: 8}")); @@ -1425,7 +1428,7 @@ TEST_F(QueryPlannerTest, MaxMinSortEqualityFirstSortSecond) { BSONObj(), 0, 0, - BSONObj(), + fromjson("{a: 1, b: 1}"), fromjson("{a: 1, b: 1}"), fromjson("{a: 1, b: 2}")); @@ -1442,7 +1445,7 @@ TEST_F(QueryPlannerTest, MaxMinSortInequalityFirstSortSecond) { BSONObj(), 0, 0, - BSONObj(), + fromjson("{a: 1, b: 1}"), fromjson("{a: 1, b: 1}"), fromjson("{a: 2, b: 2}")); @@ -1461,7 +1464,7 @@ TEST_F(QueryPlannerTest, MaxMinReverseSort) { BSONObj(), 0, 0, - BSONObj(), + fromjson("{a: 1}"), fromjson("{a: 2}"), fromjson("{a: 8}")); @@ -1478,7 +1481,7 @@ TEST_F(QueryPlannerTest, MaxMinReverseIndexDir) { BSONObj(), 0, 0, - BSONObj(), + fromjson("{a: -1}"), fromjson("{a: 8}"), fromjson("{a: 2}")); @@ -1496,7 +1499,7 @@ TEST_F(QueryPlannerTest, MaxMinReverseIndexDirSort) { BSONObj(), 0, 0, - BSONObj(), + fromjson("{a: -1}"), fromjson("{a: 8}"), fromjson("{a: 2}")); @@ -1508,34 +1511,13 @@ TEST_F(QueryPlannerTest, MaxMinReverseIndexDirSort) { TEST_F(QueryPlannerTest, MaxMinNoMatchingIndexDir) { addIndex(BSON("a" << -1)); - runInvalidQueryHintMinMax(BSONObj(), fromjson("{a: 2}"), BSONObj(), fromjson("{a: 8}")); + runInvalidQueryHintMinMax( + BSONObj(), fromjson("{a: 1}"), BSONObj(fromjson("{a: 2}")), fromjson("{a: 8}")); } -TEST_F(QueryPlannerTest, MaxMinSelectCorrectlyOrderedIndex) { +TEST_F(QueryPlannerTest, MaxMinBadHintIfIndexOrderDoesNotMatch) { // There are both ascending and descending indices on 'a'. addIndex(BSON("a" << 1)); - addIndex(BSON("a" << -1)); - - // The ordering of min and max means that we *must* use the descending index. - runQueryFull( - BSONObj(), BSONObj(), BSONObj(), 0, 0, BSONObj(), fromjson("{a: 8}"), fromjson("{a: 2}")); - - assertNumSolutions(1); - assertSolutionExists("{fetch: {node: {ixscan: {filter: null, dir: 1, pattern: {a: -1}}}}}"); - - // If we switch the ordering, then we use the ascending index. - // The ordering of min and max means that we *must* use the descending index. - runQueryFull( - BSONObj(), BSONObj(), BSONObj(), 0, 0, BSONObj(), fromjson("{a: 2}"), fromjson("{a: 8}")); - - assertNumSolutions(1); - assertSolutionExists("{fetch: {node: {ixscan: {filter: null, dir: 1, pattern: {a: 1}}}}}"); -} - -TEST_F(QueryPlannerTest, MaxMinBadHintSelectsReverseIndex) { - // There are both ascending and descending indices on 'a'. - addIndex(BSON("a" << 1)); - addIndex(BSON("a" << -1)); // A query hinting on {a: 1} is bad if min is {a: 8} and {a: 2} because this // min/max pairing requires a descending index. diff --git a/src/mongo/db/query/query_planner_wildcard_index_test.cpp b/src/mongo/db/query/query_planner_wildcard_index_test.cpp index a4841718b28..d0fd0def30e 100644 --- a/src/mongo/db/query/query_planner_wildcard_index_test.cpp +++ b/src/mongo/db/query/query_planner_wildcard_index_test.cpp @@ -1691,12 +1691,14 @@ TEST_F(QueryPlannerWildcardTest, PathSpecifiedWildcardIndexDoesNotSupportMinMax) fromjson("{x: {$eq: 5}}"), BSON("x.$**" << 1), fromjson("{x: 1}"), fromjson("{x: 10}")); } -TEST_F(QueryPlannerWildcardTest, WildcardIndexDoesNotEffectValidMinMax) { +TEST_F(QueryPlannerWildcardTest, WildcardIndexDoesNotAffectValidMinMax) { addWildcardIndex(BSON("$**" << 1)); addIndex(BSON("x" << 1)); - runQueryHintMinMax( - fromjson("{x: {$eq: 5}}"), BSONObj(), fromjson("{x: 1}"), fromjson("{x: 10}")); + runQueryHintMinMax(fromjson("{x: {$eq: 5}}"), + BSONObj(fromjson("{x: 1}")), + fromjson("{x: 1}"), + fromjson("{x: 10}")); assertNumSolutions(1U); assertSolutionExists( @@ -1704,13 +1706,6 @@ TEST_F(QueryPlannerWildcardTest, WildcardIndexDoesNotEffectValidMinMax) { "node: {ixscan: {filter: null, pattern: {x: 1}}}}}"); } -TEST_F(QueryPlannerWildcardTest, WildcardIndexDoesNotSupportMinMaxWithoutHint) { - addWildcardIndex(BSON("$**" << 1)); - - runInvalidQueryHintMinMax( - fromjson("{x: {$eq: 5}}"), BSONObj(), fromjson("{x: 1}"), fromjson("{x: 10}")); -} - TEST_F(QueryPlannerWildcardTest, CanAnswerEqualityToEmptyObject) { addWildcardIndex(BSON("$**" << 1)); runQuery(fromjson("{a: {}}")); diff --git a/src/mongo/db/query/query_request.cpp b/src/mongo/db/query/query_request.cpp index ca26724a64b..4dddbb9d361 100644 --- a/src/mongo/db/query/query_request.cpp +++ b/src/mongo/db/query/query_request.cpp @@ -583,7 +583,7 @@ Status QueryRequest::validate() const { // Min and Max objects must have the same fields. if (!_min.isEmpty() && !_max.isEmpty()) { if (!_min.isFieldNamePrefixOf(_max) || (_min.nFields() != _max.nFields())) { - return Status(ErrorCodes::BadValue, "min and max must have the same field names"); + return Status(ErrorCodes::Error(51176), "min and max must have the same field names"); } } diff --git a/src/mongo/dbtests/querytests.cpp b/src/mongo/dbtests/querytests.cpp index 6c178e4f11e..880d94913db 100644 --- a/src/mongo/dbtests/querytests.cpp +++ b/src/mongo/dbtests/querytests.cpp @@ -1058,30 +1058,27 @@ public: _client.insert(ns, BSON("a" << 2 << "b" << 2)); ASSERT_EQUALS(4, count(_client.query(NamespaceString(ns), BSONObj()))); - BSONObj hints[] = {BSONObj(), BSON("a" << 1 << "b" << 1)}; - for (int i = 0; i < 2; ++i) { - check(0, 0, 3, 3, 4, hints[i]); - check(1, 1, 2, 2, 3, hints[i]); - check(1, 2, 2, 2, 2, hints[i]); - check(1, 2, 2, 1, 1, hints[i]); - - unique_ptr<DBClientCursor> c = query(1, 2, 2, 2, hints[i]); - BSONObj obj = c->next(); - ASSERT_EQUALS(1, obj.getIntField("a")); - ASSERT_EQUALS(2, obj.getIntField("b")); - obj = c->next(); - ASSERT_EQUALS(2, obj.getIntField("a")); - ASSERT_EQUALS(1, obj.getIntField("b")); - ASSERT(!c->more()); - } + BSONObj hint = BSON("a" << 1 << "b" << 1); + check(0, 0, 3, 3, 4, hint); + check(1, 1, 2, 2, 3, hint); + check(1, 2, 2, 2, 2, hint); + check(1, 2, 2, 1, 1, hint); + + unique_ptr<DBClientCursor> c = query(1, 2, 2, 2, hint); + BSONObj obj = c->next(); + ASSERT_EQUALS(1, obj.getIntField("a")); + ASSERT_EQUALS(2, obj.getIntField("b")); + obj = c->next(); + ASSERT_EQUALS(2, obj.getIntField("a")); + ASSERT_EQUALS(1, obj.getIntField("b")); + ASSERT(!c->more()); } private: unique_ptr<DBClientCursor> query(int minA, int minB, int maxA, int maxB, const BSONObj& hint) { Query q; q = q.minKey(BSON("a" << minA << "b" << minB)).maxKey(BSON("a" << maxA << "b" << maxB)); - if (!hint.isEmpty()) - q.hint(hint); + q.hint(hint); return _client.query(NamespaceString(ns), q); } void check( |