diff options
-rw-r--r-- | jstests/noPassthroughWithMongod/group_pushdown.js | 15 | ||||
-rw-r--r-- | src/mongo/db/exec/sbe/expressions/expression.cpp | 16 | ||||
-rw-r--r-- | src/mongo/db/exec/sbe/vm/vm.h | 7 | ||||
-rw-r--r-- | src/mongo/db/pipeline/expression.cpp | 4 | ||||
-rw-r--r-- | src/mongo/db/query/sbe_stage_builder_expression.cpp | 86 |
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 { |