diff options
author | James Wahlin <james@mongodb.com> | 2018-05-04 14:24:37 -0400 |
---|---|---|
committer | James Wahlin <james@mongodb.com> | 2018-05-22 16:31:27 -0400 |
commit | a3210c8e070ca82907415c111b0919a82210362c (patch) | |
tree | 036018dacdd5339971daf22086b80d80681882c0 | |
parent | e651e0fb73dd6e7c975c8b67fab9ecf72c0cc530 (diff) | |
download | mongo-a3210c8e070ca82907415c111b0919a82210362c.tar.gz |
SERVER-34851 Disallow index selection for identical min & max values on find
(cherry picked from commit 737596d7005ae061809cbd8483ba91b4a2a89d86)
(cherry picked from commit dc67135fd82a254faabd8e2a32ac6624d3135655)
(cherry picked from commit bf5eb1a3278ca3e23bac00e10077e97607cc0b67)
-rw-r--r-- | jstests/core/minmax.js | 239 | ||||
-rw-r--r-- | jstests/core/minmax_edge.js | 6 | ||||
-rw-r--r-- | src/mongo/db/query/query_planner.cpp | 2 | ||||
-rw-r--r-- | src/mongo/db/query/query_planner_common.cpp | 4 | ||||
-rw-r--r-- | src/mongo/db/query/query_planner_test.cpp | 7 |
5 files changed, 146 insertions, 112 deletions
diff --git a/jstests/core/minmax.js b/jstests/core/minmax.js index 670c7d2f8b2..7e4feb1f412 100644 --- a/jstests/core/minmax.js +++ b/jstests/core/minmax.js @@ -1,97 +1,142 @@ -// test min / max query parameters - -addData = function() { - t.save({a: 1, b: 1}); - t.save({a: 1, b: 2}); - t.save({a: 2, b: 1}); - t.save({a: 2, b: 2}); -}; - -t = db.jstests_minmax; -t.drop(); -t.ensureIndex({a: 1, b: 1}); -addData(); - -printjson(t.find().min({a: 1, b: 2}).max({a: 2, b: 1}).toArray()); -assert.eq(1, t.find().min({a: 1, b: 2}).max({a: 2, b: 1}).toArray().length); -assert.eq(2, t.find().min({a: 1, b: 2}).max({a: 2, b: 1.5}).toArray().length); -assert.eq(2, t.find().min({a: 1, b: 2}).max({a: 2, b: 2}).toArray().length); - -// just one bound -assert.eq(3, t.find().min({a: 1, b: 2}).toArray().length); -assert.eq(3, t.find().max({a: 2, b: 1.5}).toArray().length); -assert.eq(3, t.find().min({a: 1, b: 2}).hint({a: 1, b: 1}).toArray().length); -assert.eq(3, t.find().max({a: 2, b: 1.5}).hint({a: 1, b: 1}).toArray().length); - -t.drop(); -t.ensureIndex({a: 1, b: -1}); -addData(); -assert.eq(4, t.find().min({a: 1, b: 2}).toArray().length); -assert.eq(4, t.find().max({a: 2, b: 0.5}).toArray().length); -assert.eq(1, t.find().min({a: 2, b: 1}).toArray().length); -assert.eq(1, t.find().max({a: 1, b: 1.5}).toArray().length); -assert.eq(4, t.find().min({a: 1, b: 2}).hint({a: 1, b: -1}).toArray().length); -assert.eq(4, t.find().max({a: 2, b: 0.5}).hint({a: 1, b: -1}).toArray().length); -assert.eq(1, t.find().min({a: 2, b: 1}).hint({a: 1, b: -1}).toArray().length); -assert.eq(1, t.find().max({a: 1, b: 1.5}).hint({a: 1, b: -1}).toArray().length); - -// hint doesn't match -assert.throws(function() { - t.find().min({a: 1}).hint({a: 1, b: -1}).toArray(); -}); -assert.throws(function() { - t.find().min({a: 1, b: 1}).max({a: 1}).hint({a: 1, b: -1}).toArray(); -}); -assert.throws(function() { - t.find().min({b: 1}).max({a: 1, b: 2}).hint({a: 1, b: -1}).toArray(); -}); -assert.throws(function() { - t.find().min({a: 1}).hint({$natural: 1}).toArray(); -}); -assert.throws(function() { - t.find().max({a: 1}).hint({$natural: 1}).toArray(); -}); - -// Reverse direction scan of the a:1 index between a:6 (inclusive) and a:3 (exclusive). -t.drop(); -t.ensureIndex({a: 1}); -for (i = 0; i < 10; ++i) { - t.save({_id: i, a: i}); -} -if (0) { // SERVER-3766 - reverseResult = t.find().min({a: 6}).max({a: 3}).sort({a: -1}).hint({a: 1}).toArray(); - assert.eq([{_id: 6, a: 6}, {_id: 5, a: 5}, {_id: 4, a: 4}], reverseResult); -} - -// -// SERVER-15015. -// - -// Test ascending index. -t.drop(); -t.ensureIndex({a: 1}); -t.insert({a: 3}); -t.insert({a: 4}); -t.insert({a: 5}); - -var cursor = t.find().min({a: 4}); -assert.eq(4, cursor.next()["a"]); -assert.eq(5, cursor.next()["a"]); -assert(!cursor.hasNext()); - -cursor = t.find().max({a: 4}); -assert.eq(3, cursor.next()["a"]); -assert(!cursor.hasNext()); - -// Test descending index. -t.dropIndexes(); -t.ensureIndex({a: -1}); - -cursor = t.find().min({a: 4}); -assert.eq(4, cursor.next()["a"]); -assert.eq(3, cursor.next()["a"]); -assert(!cursor.hasNext()); - -cursor = t.find().max({a: 4}); -assert.eq(5, cursor.next()["a"]); -assert(!cursor.hasNext()); +// Test min / max query parameters. +(function() { + "use strict"; + + load("jstests/aggregation/extras/utils.js"); // For resultsEq. + + const coll = db.jstests_minmax; + coll.drop(); + + function addData() { + assert.writeOK(coll.save({a: 1, b: 1})); + assert.writeOK(coll.save({a: 1, b: 2})); + assert.writeOK(coll.save({a: 2, b: 1})); + assert.writeOK(coll.save({a: 2, b: 2})); + } + + assert.commandWorked(coll.ensureIndex({a: 1, b: 1})); + addData(); + + assert.eq(1, coll.find().min({a: 1, b: 2}).max({a: 2, b: 1}).toArray().length); + assert.eq(2, coll.find().min({a: 1, b: 2}).max({a: 2, b: 1.5}).toArray().length); + assert.eq(2, coll.find().min({a: 1, b: 2}).max({a: 2, b: 2}).toArray().length); + + // Single bound. + assert.eq(3, coll.find().min({a: 1, b: 2}).toArray().length); + assert.eq(3, coll.find().max({a: 2, b: 1.5}).toArray().length); + assert.eq(3, coll.find().min({a: 1, b: 2}).hint({a: 1, b: 1}).toArray().length); + assert.eq(3, coll.find().max({a: 2, b: 1.5}).hint({a: 1, b: 1}).toArray().length); + + coll.drop(); + assert.commandWorked(coll.ensureIndex({a: 1, b: -1})); + addData(); + assert.eq(4, coll.find().min({a: 1, b: 2}).toArray().length); + assert.eq(4, coll.find().max({a: 2, b: 0.5}).toArray().length); + assert.eq(1, coll.find().min({a: 2, b: 1}).toArray().length); + assert.eq(1, coll.find().max({a: 1, b: 1.5}).toArray().length); + assert.eq(4, coll.find().min({a: 1, b: 2}).hint({a: 1, b: -1}).toArray().length); + assert.eq(4, coll.find().max({a: 2, b: 0.5}).hint({a: 1, b: -1}).toArray().length); + assert.eq(1, coll.find().min({a: 2, b: 1}).hint({a: 1, b: -1}).toArray().length); + assert.eq(1, coll.find().max({a: 1, b: 1.5}).hint({a: 1, b: -1}).toArray().length); + + // Hint doesn't match. + assert.throws(function() { + coll.find().min({a: 1}).hint({a: 1, b: -1}).toArray(); + }); + assert.throws(function() { + coll.find().min({a: 1, b: 1}).max({a: 1}).hint({a: 1, b: -1}).toArray(); + }); + assert.throws(function() { + coll.find().min({b: 1}).max({a: 1, b: 2}).hint({a: 1, b: -1}).toArray(); + }); + assert.throws(function() { + coll.find().min({a: 1}).hint({$natural: 1}).toArray(); + }); + assert.throws(function() { + coll.find().max({a: 1}).hint({$natural: 1}).toArray(); + }); + + coll.drop(); + assert.commandWorked(coll.ensureIndex({a: 1})); + for (var i = 0; i < 10; ++i) { + assert.writeOK(coll.save({_id: i, a: i})); + } + + // Reverse direction scan of the a:1 index between a:6 (inclusive) and a:3 (exclusive) is + // expected to fail, as max must be > min. + var error = assert.throws(function() { + coll.find().min({a: 6}).max({a: 3}).sort({a: -1}).toArray(); + }); + assert.eq(error.code, ErrorCodes.BadValue); + + error = assert.throws(function() { + coll.find().min({a: 6}).max({a: 3}).sort({a: -1}).hint({a: 1}).toArray(); + }); + assert.eq(error.code, ErrorCodes.BadValue); + + // A find with identical min and max values is expected to fail, as max is exclusive. + error = assert.throws(function() { + coll.find().min({a: 2}).max({a: 2}).toArray(); + }); + assert.eq(error.code, ErrorCodes.BadValue); + + error = assert.throws(function() { + coll.find().min({a: 2}).max({a: 2}).hint({a: 1}).toArray(); + }); + assert.eq(error.code, ErrorCodes.BadValue); + + error = assert.throws(function() { + coll.find().min({a: 2}).max({a: 2}).sort({a: -1}).toArray(); + }); + assert.eq(error.code, ErrorCodes.BadValue); + + error = assert.throws(function() { + coll.find().min({a: 2}).max({a: 2}).sort({a: -1}).hint({a: 1}).toArray(); + }); + assert.eq(error.code, ErrorCodes.BadValue); + + coll.drop(); + addData(); + assert.commandWorked(coll.ensureIndex({a: 1, b: 1})); + + error = assert.throws(function() { + coll.find().min({a: 1, b: 2}).max({a: 1, b: 2}).toArray(); + }); + assert.eq(error.code, ErrorCodes.BadValue); + + error = assert.throws(function() { + coll.find().min({a: 1, b: 2}).max({a: 1, b: 2}).hint({a: 1, b: 1}).toArray(); + }); + assert.eq(error.code, ErrorCodes.BadValue); + + // Test ascending index. + coll.drop(); + assert.commandWorked(coll.ensureIndex({a: 1})); + assert.writeOK(coll.insert({a: 3})); + assert.writeOK(coll.insert({a: 4})); + assert.writeOK(coll.insert({a: 5})); + + var cursor = coll.find().min({a: 4}); + assert.eq(4, cursor.next().a); + assert.eq(5, cursor.next().a); + + assert(!cursor.hasNext()); + + cursor = coll.find().max({a: 4}); + assert.eq(3, cursor.next()["a"]); + assert(!cursor.hasNext()); + + // Test descending index. + assert.commandWorked(coll.dropIndexes()); + assert.commandWorked(coll.ensureIndex({a: -1})); + + cursor = coll.find().min({a: 4}); + assert.eq(4, cursor.next().a); + assert.eq(3, cursor.next().a); + + assert(!cursor.hasNext()); + + cursor = coll.find().max({a: 4}); + assert.eq(5, cursor.next()["a"]); + assert(!cursor.hasNext()); +}()); diff --git a/jstests/core/minmax_edge.js b/jstests/core/minmax_edge.js index abd39724a80..759d87dc21e 100644 --- a/jstests/core/minmax_edge.js +++ b/jstests/core/minmax_edge.js @@ -79,8 +79,6 @@ verifyMax({a: {a: 1}}, [0, 1, 2, 3, 4, 5, 6, 7, 8]); verifyMin({a: 'a'}, []); verifyMax({a: 'a'}, [0, 1, 2, 3, 4, 5, 6, 7, 8]); -verifyResultIds(t.find().min({a: 4}).max({a: 4}).toArray(), []); - // Now with a compound index. reset(t); assert.commandWorked(t.ensureIndex({a: 1, b: -1})); @@ -101,8 +99,6 @@ verifyMax({a: {a: 1}, b: 1}, [0, 1, 2, 3, 4, 5, 6, 7, 8]); verifyMin({a: 'a', b: 1}, []); verifyMax({a: 'a', b: 1}, [0, 1, 2, 3, 4, 5, 6, 7, 8]); -verifyResultIds(t.find().min({a: 4, b: 1}).max({a: 4, b: 1}).toArray(), []); - // Edge cases on b values verifyMin({a: 1, b: Infinity}, [0, 1, 2, 3, 4, 5, 6, 7, 8]); verifyMin({a: 2, b: Infinity}, [3, 4, 5, 6, 7, 8]); @@ -146,8 +142,6 @@ verifyMax({a: {a: 1}}, []); verifyMin({a: 'a'}, [0, 1, 2, 3, 4, 5, 6, 7, 8]); verifyMax({a: 'a'}, []); -verifyResultIds(t.find().min({a: 4}).max({a: 4}).toArray(), []); - // Now with a compound index. reset(t); t.ensureIndex({a: -1, b: -1}); diff --git a/src/mongo/db/query/query_planner.cpp b/src/mongo/db/query/query_planner.cpp index cb44bc12908..69177b9c0ce 100644 --- a/src/mongo/db/query/query_planner.cpp +++ b/src/mongo/db/query/query_planner.cpp @@ -621,7 +621,7 @@ Status QueryPlanner::plan(const CanonicalQuery& query, // 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, kp, false)) { + if (0 > finishedMinObj.woCompare(finishedMaxObj, kp, false)) { // Found a relevant index. idxNo = i; break; diff --git a/src/mongo/db/query/query_planner_common.cpp b/src/mongo/db/query/query_planner_common.cpp index 2ae977a825e..3560d734404 100644 --- a/src/mongo/db/query/query_planner_common.cpp +++ b/src/mongo/db/query/query_planner_common.cpp @@ -61,8 +61,8 @@ void QueryPlannerCommon::reverseScans(QuerySolutionNode* node) { } if (!isn->bounds.isValidFor(isn->indexKeyPattern, isn->direction)) { - LOG(5) << "Invalid bounds: " << isn->bounds.toString() << std::endl; - invariant(0); + severe() << "Invalid bounds: " << isn->bounds.toString(); + MONGO_UNREACHABLE; } // TODO: we can just negate every value in the already computed properties. diff --git a/src/mongo/db/query/query_planner_test.cpp b/src/mongo/db/query/query_planner_test.cpp index 30fa95eb58c..fcc66de5273 100644 --- a/src/mongo/db/query/query_planner_test.cpp +++ b/src/mongo/db/query/query_planner_test.cpp @@ -1053,12 +1053,7 @@ TEST_F(QueryPlannerTest, MaxValid) { TEST_F(QueryPlannerTest, MinMaxSameValue) { addIndex(BSON("a" << 1)); - runQueryHintMinMax(BSONObj(), BSONObj(), fromjson("{a: 1}"), fromjson("{a: 1}")); - - assertNumSolutions(1U); - assertSolutionExists( - "{fetch: {filter: null, " - "node: {ixscan: {filter: null, pattern: {a: 1}}}}}"); + runInvalidQueryHintMinMax(BSONObj(), BSONObj(), fromjson("{a: 1}"), fromjson("{a: 1}")); } TEST_F(QueryPlannerTest, MaxWithoutIndex) { |