SERVER-54421 [SBE] Fix tests failing to match a document using {x: null}

predicate when 'x' is missing

5 files changed, 65 insertions, 30 deletions

diff --git a/jstests/core/arrayfind5.js b/jstests/core/arrayfind5.js
index 171fe49e1f8..6f63ee89bfb 100644
--- a/jstests/core/arrayfind5.js
+++ b/jstests/core/arrayfind5.js
@@ -1,7 +1,4 @@
 // Test indexed elemmatch of missing field.
-// @tags: [
-//   sbe_incompatible,
-// ]
 
 t = db.jstests_arrayfind5;
 t.drop();
diff --git a/jstests/core/dotted_path_in_null.js b/jstests/core/dotted_path_in_null.js
index ac4832482a1..7c8baf42709 100644
--- a/jstests/core/dotted_path_in_null.js
+++ b/jstests/core/dotted_path_in_null.js
@@ -1,8 +1,3 @@
-/**
- * @tags: [
- *   sbe_incompatible,
- * ]
- */
 (function() {
 "use strict";
 
diff --git a/jstests/core/indexf.js b/jstests/core/indexf.js
index 4dc8fb0b060..37c27967229 100644
--- a/jstests/core/indexf.js
+++ b/jstests/core/indexf.js
@@ -1,9 +1,4 @@
 
-/**
- * @tags: [
- *   sbe_incompatible,
- * ]
- */
 t = db.indexf;
 t.drop();
 
diff --git a/jstests/core/null_query_semantics.js b/jstests/core/null_query_semantics.js
index 28c2e8e1e90..2a5437ada9d 100644
--- a/jstests/core/null_query_semantics.js
+++ b/jstests/core/null_query_semantics.js
@@ -1,7 +1,4 @@
 // Tests the behavior of queries with a {$eq: null} or {$ne: null} predicate.
-// @tags: [
-//   sbe_incompatible,
-// ]
 (function() {
 "use strict";
 
@@ -102,14 +99,19 @@ function testNotEqualsNullSemantics() {
             {_id: "a_subobject_b_not_null", a: {b: "hi"}},
             {_id: "a_subobject_b_null", a: {b: null}},
             {_id: "a_subobject_b_undefined", a: {b: undefined}},
-
-            // TODO: SERVER-21929: $in may (or may not) miss fields with value "undefined".
-            {_id: "a_undefined", a: undefined},
         ];
-        assert(resultsEq(noProjectResults, expected), tojson(noProjectResults));
+        // TODO: SERVER-21929: $in may (or may not) miss fields with value "undefined".
+        const expectedWithUndefined = expected.concat([
+            {_id: "a_undefined", a: undefined},
+        ]);
+        assert(resultsEq(noProjectResults, expected) ||
+                   resultsEq(noProjectResults, expectedWithUndefined),
+               tojson(noProjectResults));
 
         const projectResults = coll.find(query, projectToOnlyA).toArray();
-        assert(resultsEq(projectResults, extractAValues(expected)), tojson(projectResults));
+        assert(resultsEq(projectResults, extractAValues(expected)) ||
+                   resultsEq(projectResults, extractAValues(expectedWithUndefined)),
+               projectResults);
     }());
 
     (function testExistsFalse() {
diff --git a/src/mongo/db/query/sbe_stage_builder_filter.cpp b/src/mongo/db/query/sbe_stage_builder_filter.cpp
index 1f475d75344..ec290338a93 100644
--- a/src/mongo/db/query/sbe_stage_builder_filter.cpp
+++ b/src/mongo/db/query/sbe_stage_builder_filter.cpp
@@ -365,13 +365,21 @@ EvalExprStagePair generatePathTraversal(EvalStage inputStage,
                                 mode,
                                 stateHelper);
 
-    if (stateHelper.stateContainsValue()) {
-        auto isInputArray = slotIdGenerator->generate();
-        fromBranch = makeProject(std::move(fromBranch),
-                                 planNodeId,
-                                 isInputArray,
-                                 makeFunction("isArray"sv, sbe::makeE<sbe::EVariable>(fieldSlot)));
+    auto isInputArray = [&]() -> boost::optional<sbe::value::SlotId> {
+        if (stateHelper.stateContainsValue() || !isLeafField) {
+            auto slot = slotIdGenerator->generate();
+            fromBranch =
+                makeProject(std::move(fromBranch),
+                            planNodeId,
+                            slot,
+                            makeFillEmptyFalse(makeFunction("isArray", makeVariable(fieldSlot))));
+            return slot;
+        }
+        return {};
+    }();
 
+    if (stateHelper.stateContainsValue()) {
+        tassert(5442101, "isInputArray must be set", isInputArray.has_value());
         // The expression below checks if input is an array. In this case it returns initial state.
         // This value will be the first one to be stored in 'traverseOutputSlot'. On the subsequent
         // iterations 'traverseOutputSlot' is updated according to fold expression.
@@ -383,7 +391,7 @@ EvalExprStagePair generatePathTraversal(EvalStage inputStage,
             makeLocalBind(frameIdGenerator,
                           [&](sbe::EVariable state) {
                               return sbe::makeE<sbe::EIf>(
-                                  sbe::makeE<sbe::EVariable>(isInputArray),
+                                  makeVariable(*isInputArray),
                                   stateHelper.makeInitialState(stateHelper.getBool(state.clone())),
                                   state.clone());
                           },
@@ -394,6 +402,25 @@ EvalExprStagePair generatePathTraversal(EvalStage inputStage,
     innerBranch =
         makeProject(std::move(innerBranch), planNodeId, innerResultSlot, innerExpr.extractExpr());
 
+    // For the non leaf nodes we insert a filter that allows the nested getField only for objects.
+    // But only if the outer value is an array. This is relevant in this example: given 2 documents
+    // {a:10} and {a:[10]} the filer {'a.b':null} returns the first document but not the second.
+    // Without the filter we'd try to traverse 'a', and in both cases the inner side of the
+    // 'traverse' would get the value '10'. However, in the first case we'd try to apply getField()
+    // to a standalone scalar, which would return a missing field, which is equal to null, whilst in
+    // a second case to a scalar which is an array element. According to the legacy implementation,
+    // this is not allowed and we shouldn't try to do a nesting path traversal of the array
+    // elements, unless an element is an object.
+    if (!isLeafField) {
+        tassert(5442102, "isInputArray must be set", isInputArray.has_value());
+        innerBranch = makeFilter<true>(
+            std::move(innerBranch),
+            sbe::makeE<sbe::EIf>(makeVariable(*isInputArray),
+                                 makeFunction("isObject", makeVariable(fieldSlot)),
+                                 makeConstant(sbe::value::TypeTags::Boolean, true)),
+            planNodeId);
+    }
+
     // Generate the traverse stage for the current nested level. There are several cases covered
     // during this phase:
     //  1. If input is not an array, value from 'in' branch is returned (see comment for the 'in'
@@ -1299,12 +1326,16 @@ public:
 
         auto arrSet = sbe::value::getArraySetView(arrSetVal);
 
+        auto hasNull = false;
         for (auto&& equality : equalities) {
             auto [tagView, valView] = sbe::bson::convertFrom(true,
                                                              equality.rawdata(),
                                                              equality.rawdata() + equality.size(),
                                                              equality.fieldNameSize() - 1);
 
+            if (tagView == sbe::value::TypeTags::Null) {
+                hasNull = true;
+            }
             // An ArraySet assumes ownership of it's values so we have to make a copy here.
             auto [tag, val] = sbe::value::copyValue(tagView, valView);
             arrSet->push_back(tag, val);
@@ -1320,7 +1351,14 @@ public:
                 // makePredicate function can be invoked multiple times in 'generateTraverse'.
                 auto [equalitiesTag, equalitiesVal] = sbe::value::copyValue(arrSetTag, arrSetVal);
 
-                return {makeIsMember(sbe::makeE<sbe::EVariable>(inputSlot),
+                // We have to match nulls and undefined if a 'null' is present in equalities.
+                auto inputExpr = !hasNull
+                    ? makeVariable(inputSlot)
+                    : sbe::makeE<sbe::EIf>(generateNullOrMissing(sbe::EVariable(inputSlot)),
+                                           makeConstant(sbe::value::TypeTags::Null, 0),
+                                           makeVariable(inputSlot));
+
+                return {makeIsMember(std::move(inputExpr),
                                      sbe::makeE<sbe::EConstant>(equalitiesTag, equalitiesVal),
                                      _context->env),
                         std::move(inputStage)};
@@ -1398,8 +1436,16 @@ public:
                     auto [equalitiesTag, equalitiesVal] =
                         sbe::value::copyValue(arrSetTag, arrSetVal);
                     std::vector<EvalExprStagePair> branches;
+
+                    // We have to match nulls and undefined if a 'null' is present in equalities.
+                    auto inputExpr = !hasNull
+                        ? makeVariable(inputSlot)
+                        : sbe::makeE<sbe::EIf>(generateNullOrMissing(sbe::EVariable(inputSlot)),
+                                               makeConstant(sbe::value::TypeTags::Null, 0),
+                                               makeVariable(inputSlot));
+
                     branches.emplace_back(
-                        makeIsMember(sbe::makeE<sbe::EVariable>(inputSlot),
+                        makeIsMember(std::move(inputExpr),
                                      sbe::makeE<sbe::EConstant>(equalitiesTag, equalitiesVal),
                                      _context->env),
                         EvalStage{});