SERVER-72843 Fallback to the classic engine for FilterExpression

5 files changed, 9 insertions, 119 deletions

diff --git a/jstests/noPassthroughWithMongod/group_pushdown.js b/jstests/noPassthroughWithMongod/group_pushdown.js
index cb5743bfd1e..fe11f9f8def 100644
--- a/jstests/noPassthroughWithMongod/group_pushdown.js
+++ b/jstests/noPassthroughWithMongod/group_pushdown.js
@@ -332,21 +332,6 @@ assertResultsMatchWithAndWithoutPushdown(coll,
                                          [{"_id": null, "lowp": 2, "highp": 1}],
                                          2);
 
-// The second $group stage refers to top-fields below a $filter
-assertResultsMatchWithAndWithoutPushdown(
-    coll,
-    [
-        {$group: {_id: "$item", prices: {$push: "$price"}}},
-        {
-            $group: {
-                _id: "$_id",
-                o: {$push: {$filter: {input: "$prices", as: "p", cond: {$gte: ["$$p", 5]}}}}
-            }
-        }
-    ],
-    [{"_id": "a", "o": [[10, 5]]}, {"_id": "b", "o": [[20, 10]]}, {"_id": "c", "o": [[5]]}],
-    2);
-
 // The second $group stage refers to top-fields below a $let
 assertResultsMatchWithAndWithoutPushdown(
     coll,
diff --git a/src/mongo/db/exec/sbe/expressions/expression.cpp b/src/mongo/db/exec/sbe/expressions/expression.cpp
index a96a559cd2d..1c44cdb8585 100644
--- a/src/mongo/db/exec/sbe/expressions/expression.cpp
+++ b/src/mongo/db/exec/sbe/expressions/expression.cpp
@@ -1253,18 +1253,10 @@ vm::CodeFragment ELocalLambda::compileBodyDirect(CompileCtx& ctx) const {
     // Lambda parameter is no longer accessible after this point so remove the frame.
     body.removeFrame(_frameId);
 
-    // TODO SERVER-72843 Remove the adjustment below when the issue if fixed.
-    // Adjust the stack offsets by 1 to account to maintain the bug compatibility that
-    // allows traverse lambdas to access clousre variables under special conditions.
-    // Lambda captures are not supported, and adjustment below is only meant to keep
-    // existing stage builder code from falling apart.
-    body.fixupStackOffsets(1);
-
-    // TODO SERVER-72843: Add assert that verifies that lambda uses no closure variables.
-    // Possibly by ensuring that there are no outstanding frames in the lambda code fragment, like:
-    // tassert(7284301,
-    //         !body.hasFrames(),
-    //         "Accessing closure variables from lambda body in not supported.");
+    // Verify that 'body' does not refer to local variables defined outside of 'body'.
+    tassert(7284301,
+            "Lambda referring to local variable defined outside of the lambda is not supported.",
+            !body.hasFrames());
 
     return body;
 }
diff --git a/src/mongo/db/exec/sbe/vm/vm.h b/src/mongo/db/exec/sbe/vm/vm.h
index b6ee6d2dc75..3fc5c173df4 100644
--- a/src/mongo/db/exec/sbe/vm/vm.h
+++ b/src/mongo/db/exec/sbe/vm/vm.h
@@ -912,13 +912,10 @@ public:
     // Returns whether the are any frames currently in scope.
     bool hasFrames() const;
 
+private:
     // Adjusts all the stack offsets in the outstanding fixups by the provided delta.
-    // TODO SERVER-72843: Make fixupStackOffsets private after fixing the issue.
-    // ELocalLambda needs access to fixupStackOffsets as the bug workaround, and should be
-    // treated as private otherwise.
     void fixupStackOffsets(int stackOffsetDelta);
 
-private:
     // Stores the fixup information for stack frames.
     // stackPosition - stack depth of where the frame was declared, or kPositionNotSet if not known
     // yet.
@@ -931,7 +928,6 @@ private:
         int64_t stackPosition{kPositionNotSet};
     };
 
-private:
     template <typename... Ts>
     void appendSimpleInstruction(Instruction::Tags tag, Ts&&... params);
     auto allocateSpace(size_t size) {
@@ -956,7 +952,6 @@ private:
     FrameInfo& getOrDefineFrame(FrameId frameId);
     void fixupFrame(FrameInfo& frame);
 
-private:
     absl::InlinedVector<uint8_t, 16> _instrs;
 
     // A collection of frame information for local variables.
diff --git a/src/mongo/db/pipeline/expression.cpp b/src/mongo/db/pipeline/expression.cpp
index ec733aad176..3a5e4107e27 100644
--- a/src/mongo/db/pipeline/expression.cpp
+++ b/src/mongo/db/pipeline/expression.cpp
@@ -2664,9 +2664,7 @@ ExpressionFilter::ExpressionFilter(ExpressionContext* const expCtx,
       _varName(std::move(varName)),
       _varId(varId),
       _limit(_children.size() == 3 ? 2 : boost::optional<size_t>(boost::none)) {
-    if (_limit) {
-        expCtx->sbeCompatible = false;
-    }
+    expCtx->sbeCompatible = false;
 }
 
 intrusive_ptr<Expression> ExpressionFilter::optimize() {
diff --git a/src/mongo/db/query/sbe_stage_builder_expression.cpp b/src/mongo/db/query/sbe_stage_builder_expression.cpp
index 4df0357c297..2c208e15ec8 100644
--- a/src/mongo/db/query/sbe_stage_builder_expression.cpp
+++ b/src/mongo/db/query/sbe_stage_builder_expression.cpp
@@ -129,12 +129,6 @@ struct ExpressionVisitorContext {
     std::stack<VarsFrame> varsFrameStack;
 
     const PlanStageSlots* slots = nullptr;
-
-    // This stack contains the FrameIds used by the $filter expressions that are currently
-    // being processed.
-    std::stack<sbe::FrameId> filterExprFrameStack;
-    // We use this counter to track which children of $filter we've already processed.
-    std::stack<int> filterExprChildrenCounter;
 };
 
 /**
@@ -304,12 +298,7 @@ public:
     void visit(const ExpressionDivide* expr) final {}
     void visit(const ExpressionExp* expr) final {}
     void visit(const ExpressionFieldPath* expr) final {}
-    void visit(const ExpressionFilter* expr) final {
-        tassert(
-            6987502, "SBE does not support $filter expression with 'limit' arg", !expr->hasLimit());
-
-        _context->filterExprChildrenCounter.push(1);
-    }
+    void visit(const ExpressionFilter* expr) final {}
     void visit(const ExpressionFloor* expr) final {}
     void visit(const ExpressionIfNull* expr) final {}
     void visit(const ExpressionIn* expr) final {}
@@ -467,32 +456,7 @@ public:
     void visit(const ExpressionDivide* expr) final {}
     void visit(const ExpressionExp* expr) final {}
     void visit(const ExpressionFieldPath* expr) final {}
-    void visit(const ExpressionFilter* expr) final {
-        // $filter has up to three children: cond, as, and limit (optional). SBE currently does not
-        // support $filter when the 'limit' arg is specified.
-        //
-        // Only the filter predicate (cond) needs access to the value of the "as" arg, here referred
-        // to as current element var. The filter predicate will be the second element in the
-        // _children vector the expression_walker walks.
-        const auto filterPredIndex = 2;
-        auto variableId = expr->getVariableId();
-
-        // We use this counter in the visit methods of ExpressionFilter to track which child we are
-        // processing and which children we've already processed.
-        auto& currentIndex = _context->filterExprChildrenCounter.top();
-        if (++currentIndex == filterPredIndex) {
-            tassert(3273901,
-                    "Current element variable already exists in _context",
-                    _context->environment.find(variableId) == _context->environment.end());
-            auto frameId = _context->state.frameId();
-            _context->environment.emplace(variableId, frameId);
-            // This stack maintains the current element variable for $filter so that we can erase it
-            // from our context in inVisitor when processing the optional limit arg, but then still
-            // have access to this var again in postVisitor when constructing the filter
-            // predicate/'cond' subtree.
-            _context->filterExprFrameStack.push(frameId);
-        }
-    }
+    void visit(const ExpressionFilter* expr) final {}
     void visit(const ExpressionFloor* expr) final {}
     void visit(const ExpressionIfNull* expr) final {}
     void visit(const ExpressionIn* expr) final {}
@@ -1943,51 +1907,7 @@ public:
         pushABT(std::move(resultExpr));
     }
     void visit(const ExpressionFilter* expr) final {
-        // Remove index tracking current child of $filter expression, since it is not used anymore.
-        _context->filterExprChildrenCounter.pop();
-
-        _context->ensureArity(2);
-        // Extract filter predicate expression and sub-tree.
-        auto filterExpr = _context->popABTExpr();
-        auto inputExpr = _context->popABTExpr();
-
-        // We no longer need this mapping because filter predicate which expects it was already
-        // compiled.
-        _context->environment.erase(expr->getVariableId());
-
-        tassert(7158304,
-                "Expected frame id for the current element variable of $filter expression",
-                !_context->filterExprFrameStack.empty());
-        auto lambdaFrameId = _context->filterExprFrameStack.top();
-        _context->filterExprFrameStack.pop();
-
-        auto lambdaParamName = makeLocalVariableName(lambdaFrameId, 0);
-
-        // If coerceToBool() returns true we return the lambda input, otherwise we return Nothing.
-        // This will effectively cause all elements in the array that coerce to False to get
-        // filtered out, and only the elements that coerce to True will remain.
-        auto lambdaBodyExpr = optimizer::make<optimizer::If>(
-            makeFillEmptyFalse(makeABTFunction("coerceToBool", std::move(filterExpr))),
-            makeVariable(lambdaParamName),
-            optimizer::Constant::nothing());
-
-        auto lambdaExpr = optimizer::make<optimizer::LambdaAbstraction>(std::move(lambdaParamName),
-                                                                        std::move(lambdaBodyExpr));
-
-        auto inputName = makeLocalVariableName(_context->state.frameId(), 0);
-        auto traversePExpr = makeABTFunction("traverseP",
-                                             makeVariable(inputName),
-                                             std::move(lambdaExpr),
-                                             optimizer::Constant::int32(1));
-
-        // If input is null or missing, we do not evaluate filter predicate and return Null.
-        auto resultExpr = buildABTMultiBranchConditional(
-            ABTCaseValuePair{generateABTNullOrMissing(inputName), optimizer::Constant::null()},
-            ABTCaseValuePair{makeABTFunction("isArray", makeVariable(inputName)),
-                             std::move(traversePExpr)},
-            makeABTFail(ErrorCodes::Error{7158305}, "input to $filter must be an array"));
-        pushABT(optimizer::make<optimizer::Let>(
-            std::move(inputName), std::move(inputExpr), std::move(resultExpr)));
+        unsupportedExpression("$filter");
     }
 
     void visit(const ExpressionFloor* expr) final {