SERVER-63902 Fix $natural hint on time-series collections

8 files changed, 192 insertions, 33 deletions

diff --git a/jstests/core/timeseries/timeseries_delete_hint.js b/jstests/core/timeseries/timeseries_delete_hint.js
index ebcca3e3560..0facca816e1 100644
--- a/jstests/core/timeseries/timeseries_delete_hint.js
+++ b/jstests/core/timeseries/timeseries_delete_hint.js
@@ -131,6 +131,20 @@ const objC = {
     [metaFieldName]: {c: "C"}
 };
 
+// Query using a $natural hint.
+validateDeleteIndex([objA, objB, objC],
+                    [objB, objC],
+                    1,
+                    [{q: {[metaFieldName]: {a: "A"}}, limit: 0, hint: {$natural: 1}}],
+                    [{[metaFieldName]: 1}],
+                    "COLLSCAN");
+validateDeleteIndex([objA, objB, objC],
+                    [objB, objC],
+                    1,
+                    [{q: {[metaFieldName]: {a: "A"}}, limit: 0, hint: {$natural: -1}}],
+                    [{[metaFieldName]: 1}],
+                    "COLLSCAN");
+
 // Query using the metaField index as a hint.
 validateDeleteIndex([objA, objB, objC],
                     [objB, objC],
diff --git a/jstests/core/timeseries/timeseries_hint.js b/jstests/core/timeseries/timeseries_hint.js
new file mode 100644
index 00000000000..037de311829
--- /dev/null
+++ b/jstests/core/timeseries/timeseries_hint.js
@@ -0,0 +1,90 @@
+/**
+ * Test $natural hint on a time-series collection, for find and aggregate.
+ *
+ * @tags: [
+ *   does_not_support_stepdowns,
+ *   does_not_support_transactions,
+ *   requires_fcv_51,
+ *   requires_getmore,
+ *   requires_pipeline_optimization,
+ *   # Explain of a resolved view must be executed by mongos.
+ *   directly_against_shardsvrs_incompatible,
+ * ]
+ */
+(function() {
+"use strict";
+
+load("jstests/core/timeseries/libs/timeseries.js");
+load("jstests/libs/analyze_plan.js");
+
+const coll = db.timeseries_hint;
+coll.drop();
+assert.commandWorked(db.createCollection(coll.getName(), {
+    timeseries: {timeField: 't', metaField: 'm'},
+}));
+
+// With only one event per bucket, the order of results will be predictable.
+// The min time of each bucket is encoded in its _id, and the clustered collection
+// ensures that $natural order is also _id order.
+const docsAsc = [
+    {_id: 1, m: 1, t: ISODate('1970-01-01')},
+    {_id: 2, m: 2, t: ISODate('1970-01-02')},
+    {_id: 3, m: 3, t: ISODate('1970-01-03')},
+];
+const docsDesc = docsAsc.slice().reverse();
+assert.commandWorked(coll.insert(docsAsc));
+
+function runTest({command, expectedResult, expectedDirection}) {
+    const result = assert.commandWorked(db.runCommand(command));
+    assert.docEq(result.cursor.firstBatch, expectedResult);
+
+    const plan = db.runCommand({explain: command});
+    const scan = getAggPlanStage(plan, 'COLLSCAN');
+    assert(scan, 'Expected a COLLSCAN stage' + tojson(plan));
+    assert.eq(scan.direction,
+              expectedDirection,
+              'Expected a ' + expectedDirection + ' COLLSCAN ' + tojson(scan));
+}
+
+// Test find: ascending and descending.
+runTest({
+    command: {
+        find: coll.getName(),
+        filter: {},
+        hint: {$natural: 1},
+    },
+    expectedResult: docsAsc,
+    expectedDirection: 'forward',
+});
+runTest({
+    command: {
+        find: coll.getName(),
+        filter: {},
+        hint: {$natural: -1},
+    },
+    expectedResult: docsDesc,
+    expectedDirection: 'backward',
+});
+
+// Test aggregate: ascending and descending.
+runTest({
+    command: {
+        aggregate: coll.getName(),
+        pipeline: [],
+        cursor: {},
+        hint: {$natural: 1},
+    },
+    expectedResult: docsAsc,
+    expectedDirection: 'forward',
+});
+runTest({
+    command: {
+        aggregate: coll.getName(),
+        pipeline: [],
+        cursor: {},
+        hint: {$natural: -1},
+    },
+    expectedResult: docsDesc,
+    expectedDirection: 'backward',
+});
+})();
diff --git a/jstests/core/timeseries/timeseries_update_hint.js b/jstests/core/timeseries/timeseries_update_hint.js
index 9dbc145a66f..e51e021c356 100644
--- a/jstests/core/timeseries/timeseries_update_hint.js
+++ b/jstests/core/timeseries/timeseries_update_hint.js
@@ -39,6 +39,9 @@ const dbName = jsTestName();
  */
 const testUpdateHintSucceeded =
     ({initialDocList, indexes, updateList, resultDocList, nModifiedBuckets, expectedPlan}) => {
+        const testDB = db.getSiblingDB(dbName);
+        const coll = testDB.getCollection(collName);
+
         const awaitTestUpdate = startParallelShell(funWithArgs(
             function(dbName,
                      collName,
@@ -88,20 +91,18 @@ const testUpdateHintSucceeded =
             updateList,
             resultDocList,
             nModifiedBuckets));
-
-        const testDB = db.getSiblingDB(dbName);
-        const coll = testDB.getCollection(collName);
-
-        const childCurOp =
-            waitForCurOpByFailPoint(testDB, coll.getFullName(), "hangAfterBatchUpdate")[0];
-
-        // Verify that the query plan uses the expected index.
-        assert.eq(childCurOp.planSummary, expectedPlan);
-
-        assert.commandWorked(
-            testDB.adminCommand({configureFailPoint: "hangAfterBatchUpdate", mode: "off"}));
-
-        awaitTestUpdate();
+        try {
+            const childCurOp =
+                waitForCurOpByFailPoint(testDB, coll.getFullName(), "hangAfterBatchUpdate")[0];
+
+            // Verify that the query plan uses the expected index.
+            assert.eq(childCurOp.planSummary, expectedPlan);
+        } finally {
+            assert.commandWorked(
+                testDB.adminCommand({configureFailPoint: "hangAfterBatchUpdate", mode: "off"}));
+
+            awaitTestUpdate();
+        }
     };
 
 /**
@@ -153,6 +154,44 @@ const hintDoc3 = {
 };
 
 /************* Tests passing a hint to an update on a collection with a single index. *************/
+// Query on and update the metaField using a forward collection scan: hint: {$natural 1}.
+testUpdateHintSucceeded({
+    initialDocList: [hintDoc1, hintDoc2, hintDoc3],
+    indexes: [{[metaFieldName]: 1}],
+    updateList: [{
+        q: {[metaFieldName + ".a"]: {$lte: 2}},
+        u: {$inc: {[metaFieldName + ".a"]: 10}},
+        multi: true,
+        hint: {$natural: 1}
+    }],
+    resultDocList: [
+        {_id: 1, [timeFieldName]: dateTime, [metaFieldName]: {"a": 11}},
+        {_id: 2, [timeFieldName]: dateTime, [metaFieldName]: {"a": 12}},
+        hintDoc3
+    ],
+    nModifiedBuckets: 2,
+    expectedPlan: "COLLSCAN",
+});
+
+// Query on and update the metaField using a backward collection scan: hint: {$natural -1}.
+testUpdateHintSucceeded({
+    initialDocList: [hintDoc1, hintDoc2, hintDoc3],
+    indexes: [{[metaFieldName]: 1}],
+    updateList: [{
+        q: {[metaFieldName + ".a"]: {$lte: 2}},
+        u: {$inc: {[metaFieldName + ".a"]: 10}},
+        multi: true,
+        hint: {$natural: -1}
+    }],
+    resultDocList: [
+        {_id: 1, [timeFieldName]: dateTime, [metaFieldName]: {"a": 11}},
+        {_id: 2, [timeFieldName]: dateTime, [metaFieldName]: {"a": 12}},
+        hintDoc3
+    ],
+    nModifiedBuckets: 2,
+    expectedPlan: "COLLSCAN",
+});
+
 // Query on and update the metaField using the metaField index as a hint, specifying the hint with
 // an index specification document.
 testUpdateHintSucceeded({
diff --git a/src/mongo/db/ops/write_ops_exec.cpp b/src/mongo/db/ops/write_ops_exec.cpp
index b0b23e3e42b..d1afdc8dde9 100644
--- a/src/mongo/db/ops/write_ops_exec.cpp
+++ b/src/mongo/db/ops/write_ops_exec.cpp
@@ -809,10 +809,8 @@ static SingleWriteResult performSingleUpdateOp(OperationContext* opCtx,
                 "Cannot perform an upsert on a time-series collection",
                 !updateRequest->isUpsert());
 
-        // Only translate the hint (if there is one) if it is specified with an index specification
-        // document.
-        if (!updateRequest->getHint().isEmpty() &&
-            updateRequest->getHint().firstElement().fieldNameStringData() != "$hint"_sd) {
+        // Only translate the hint if it is specified with an index key.
+        if (timeseries::isHintIndexKey(updateRequest->getHint())) {
             updateRequest->setHint(
                 uassertStatusOK(timeseries::createBucketsIndexSpecFromTimeseriesIndexSpec(
                     *timeseriesOptions, updateRequest->getHint())));
@@ -1144,10 +1142,8 @@ static SingleWriteResult performSingleDeleteOp(OperationContext* opCtx,
                 "Time-series buckets collection is missing time-series options",
                 timeseriesOptions);
 
-        // Only translate the hint if it is specified by index spec.
-        if (!request.getHint().isEmpty() &&
-            (request.getHint().firstElement().fieldNameStringData() != "$hint"_sd ||
-             request.getHint().firstElement().type() != BSONType::String)) {
+        // Only translate the hint if it is specified by index key.
+        if (timeseries::isHintIndexKey(request.getHint())) {
             request.setHint(
                 uassertStatusOK(timeseries::createBucketsIndexSpecFromTimeseriesIndexSpec(
                     *timeseriesOptions, request.getHint())));
diff --git a/src/mongo/db/timeseries/timeseries_index_schema_conversion_functions.cpp b/src/mongo/db/timeseries/timeseries_index_schema_conversion_functions.cpp
index 1dff8c06341..9338f2c237d 100644
--- a/src/mongo/db/timeseries/timeseries_index_schema_conversion_functions.cpp
+++ b/src/mongo/db/timeseries/timeseries_index_schema_conversion_functions.cpp
@@ -71,6 +71,14 @@ std::pair<std::string, std::string> extractControlPrefixAndKey(const StringData&
 StatusWith<BSONObj> createBucketsSpecFromTimeseriesSpec(const TimeseriesOptions& timeseriesOptions,
                                                         const BSONObj& timeseriesIndexSpecBSON,
                                                         bool isShardKeySpec) {
+    tassert(6390200, "Empty object is not a valid index spec", !timeseriesIndexSpecBSON.isEmpty());
+    tassert(6390201,
+            str::stream() << "Invalid index spec (perhaps it's a valid hint, that was incorrectly "
+                          << "passed to createBucketsSpecFromTimeseriesSpec): "
+                          << timeseriesIndexSpecBSON,
+            timeseriesIndexSpecBSON.firstElement().fieldNameStringData() != "$hint"_sd &&
+                timeseriesIndexSpecBSON.firstElement().fieldNameStringData() != "$natural"_sd);
+
     auto timeField = timeseriesOptions.getTimeField();
     auto metaField = timeseriesOptions.getMetaField();
 
@@ -474,4 +482,16 @@ bool doesBucketsIndexIncludeMeasurement(OperationContext* opCtx,
     return false;
 }
 
+bool isHintIndexKey(const BSONObj& obj) {
+    if (obj.isEmpty())
+        return false;
+    StringData fieldName = obj.firstElement().fieldNameStringData();
+    if (fieldName == "$hint"_sd)
+        return false;
+    if (fieldName == "$natural"_sd)
+        return false;
+
+    return true;
+}
+
 }  // namespace mongo::timeseries
diff --git a/src/mongo/db/timeseries/timeseries_index_schema_conversion_functions.h b/src/mongo/db/timeseries/timeseries_index_schema_conversion_functions.h
index ca9fc2e1a77..ad1bb795fd2 100644
--- a/src/mongo/db/timeseries/timeseries_index_schema_conversion_functions.h
+++ b/src/mongo/db/timeseries/timeseries_index_schema_conversion_functions.h
@@ -88,4 +88,13 @@ bool doesBucketsIndexIncludeMeasurement(OperationContext* opCtx,
                                         const TimeseriesOptions& timeseriesOptions,
                                         const BSONObj& bucketsIndex);
 
+/**
+ * Takes a 'hint' object, in the same format used by FindCommandRequest, and returns
+ * true if the hint is an index key.
+ *
+ * Besides an index key, a hint can be {$hint: <index name>} or {$natural: <direction>},
+ * or it can be {} which means no hint is given.
+ */
+bool isHintIndexKey(const BSONObj& obj);
+
 }  // namespace mongo::timeseries
diff --git a/src/mongo/db/timeseries/timeseries_index_schema_conversion_functions_test.cpp b/src/mongo/db/timeseries/timeseries_index_schema_conversion_functions_test.cpp
index 88a2fd76b7d..db6eadda021 100644
--- a/src/mongo/db/timeseries/timeseries_index_schema_conversion_functions_test.cpp
+++ b/src/mongo/db/timeseries/timeseries_index_schema_conversion_functions_test.cpp
@@ -128,14 +128,6 @@ TEST(TimeseriesIndexSchemaConversionTest, OriginalSpecFieldName) {
     }
 }
 
-// {} <=> {}
-TEST(TimeseriesIndexSchemaConversionTest, EmptyTimeseriesIndexSpecDoesNothing) {
-    TimeseriesOptions timeseriesOptions = makeTimeseriesOptions();
-    BSONObj emptyIndexSpec = {};
-
-    testBothWaysIndexSpecConversion(timeseriesOptions, emptyIndexSpec, emptyIndexSpec);
-}
-
 // {tm: 1} <=> {control.min.tm: 1, control.max.tm: 1}
 TEST(TimeseriesIndexSchemaConversionTest, AscendingTimeIndexSpecConversion) {
     TimeseriesOptions timeseriesOptions = makeTimeseriesOptions();
diff --git a/src/mongo/db/views/resolved_view.cpp b/src/mongo/db/views/resolved_view.cpp
index ec60c6f0826..244d38a0b55 100644
--- a/src/mongo/db/views/resolved_view.cpp
+++ b/src/mongo/db/views/resolved_view.cpp
@@ -183,9 +183,8 @@ AggregateCommandRequest ResolvedView::asExpandedViewAggregation(
     if (request.getHint() && _timeseriesOptions) {
         BSONObj original = *request.getHint();
         BSONObj rewritten = original;
-        // Only convert if we are given an index spec, not an index name. An index name is provided
-        // in the form of {"$hint": <name>}.
-        if (!original.isEmpty() && original.firstElementFieldNameStringData() != "$hint"_sd) {
+        // Only convert if we are given an index spec, not an index name or a $natural hint.
+        if (timeseries::isHintIndexKey(original)) {
             auto converted = timeseries::createBucketsIndexSpecFromTimeseriesIndexSpec(
                 *_timeseriesOptions, original);
             if (converted.isOK()) {