SERVER-34851 Disallow index selection for identical min & max values on find

(cherry picked from commit 737596d7005ae061809cbd8483ba91b4a2a89d86)
(cherry picked from commit dc67135fd82a254faabd8e2a32ac6624d3135655)

6 files changed, 146 insertions, 125 deletions

diff --git a/jstests/core/minmax.js b/jstests/core/minmax.js
index 670c7d2f8b2..68dcdb38992 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 (let 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.
+    let 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}));
+
+    let 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 850276f35e7..2388154276c 100644
--- a/src/mongo/db/query/query_planner.cpp
+++ b/src/mongo/db/query/query_planner.cpp
@@ -659,7 +659,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 >=
+                    if (0 >
                         finishedMinObj.woCompare(finishedMaxObj, indexEntry.keyPattern, false)) {
                         // Found a relevant index.
                         idxNo = i;
diff --git a/src/mongo/db/query/query_planner_collation_test.cpp b/src/mongo/db/query/query_planner_collation_test.cpp
index 54a549723c8..97860b36add 100644
--- a/src/mongo/db/query/query_planner_collation_test.cpp
+++ b/src/mongo/db/query/query_planner_collation_test.cpp
@@ -220,19 +220,6 @@ TEST_F(QueryPlannerTest, MinMaxWithStringBoundsCannotBeCoveredWithCollator) {
         "{locale: 'reverse'}, node: {ixscan: {pattern: {a: 1, b: 1}}}}}}}");
 }
 
-TEST_F(QueryPlannerTest, MinMaxWithoutStringBoundsBoundsCanBeCoveredWithCollator) {
-    CollatorInterfaceMock collator(CollatorInterfaceMock::MockType::kReverseString);
-    addIndex(fromjson("{a: 1, b: 1}"), &collator);
-
-    runQueryAsCommand(
-        fromjson("{find: 'testns', min: {a: 1, b: 2}, max: {a: 1, b: 2}, "
-                 "projection: {_id: 0, a: 1, b: 1}, collation: {locale: 'reverse'}}"));
-
-    assertNumSolutions(1U);
-    assertSolutionExists(
-        "{proj: {spec: {_id: 0, a: 1, b: 1}, node: {ixscan: {pattern: {a: 1, b: 1}}}}}");
-}
-
 TEST_F(QueryPlannerTest, SimpleRegexCanUseAnIndexWithACollatorWithLooseBounds) {
     CollatorInterfaceMock collator(CollatorInterfaceMock::MockType::kReverseString);
     addIndex(fromjson("{a: 1}"), &collator);
diff --git a/src/mongo/db/query/query_planner_common.cpp b/src/mongo/db/query/query_planner_common.cpp
index 33eb72100cc..337f3a045fc 100644
--- a/src/mongo/db/query/query_planner_common.cpp
+++ b/src/mongo/db/query/query_planner_common.cpp
@@ -71,8 +71,8 @@ void QueryPlannerCommon::reverseScans(QuerySolutionNode* node) {
         }
 
         if (!isn->bounds.isValidFor(isn->index.keyPattern, isn->direction)) {
-            LOG(5) << "Invalid bounds: " << redact(isn->bounds.toString());
-            invariant(0);
+            severe() << "Invalid bounds: " << redact(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 7fc29a4e42f..f4f608e0b58 100644
--- a/src/mongo/db/query/query_planner_test.cpp
+++ b/src/mongo/db/query/query_planner_test.cpp
@@ -1055,12 +1055,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) {