From d6e3fe52025157200e88407c66f73de889df9bf1 Mon Sep 17 00:00:00 2001 From: KevinCybura Date: Fri, 20 Apr 2018 11:11:48 -0400 Subject: [PATCH 1/8] optimizeFilter 04/20/2018: test complete ready for pull request --- src/mongo/db/pipeline/expression.cpp | 46 +++++++++++++ src/mongo/db/pipeline/expression.h | 4 ++ src/mongo/db/pipeline/expression_test.cpp | 80 ++++++++++++++++++++++- 3 files changed, 129 insertions(+), 1 deletion(-) diff --git a/src/mongo/db/pipeline/expression.cpp b/src/mongo/db/pipeline/expression.cpp index 211f2ab7f1d97..f21bd28dd9ce7 100644 --- a/src/mongo/db/pipeline/expression.cpp +++ b/src/mongo/db/pipeline/expression.cpp @@ -523,6 +523,17 @@ Value ExpressionArrayElemAt::evaluate(const Document& root) const { const size_t index = static_cast(i); return array[index]; } +intrusive_ptr ExpressionArrayElemAt::optimize() { + if (dynamic_cast(vpOperand[0].get())) { + if (auto expConstant = dynamic_cast(vpOperand[1].get())) { + int index = expConstant->getValue().getInt() + 1; + if (index >= 0) { + dynamic_cast(vpOperand[0].get())->setLimit(index); + } + } + } + return this; +}; REGISTER_EXPRESSION(arrayElemAt, ExpressionArrayElemAt::parse); const char* ExpressionArrayElemAt::getOpName() const { @@ -2206,12 +2217,19 @@ Value ExpressionFilter::evaluate(const Document& root) const { if (_filter->evaluate(root).coerceToBool()) { output.push_back(std::move(elem)); + if(_limit && static_cast(output.size()) == _limit.get()){ + return Value(std::move(output)); + } } } return Value(std::move(output)); } +void ExpressionFilter::setLimit(int limit) { + _limit = boost::optional(limit); +} + void ExpressionFilter::_doAddDependencies(DepsTracker* deps) const { _input->addDependencies(deps); _filter->addDependencies(deps); @@ -3900,6 +3918,34 @@ Value ExpressionSlice::evaluate(const Document& root) const { return Value(vector(array.begin() + start, array.begin() + end)); } +intrusive_ptr ExpressionSlice::optimize() { + // If ExpressionSlice is passed an ExpressionFilter we can stop filtering once the size of + // the array returned by the filter is equal to the last arguement passed to ExpressionSlice. + if (dynamic_cast(vpOperand[0].get())) { + if (auto arg1 = dynamic_cast(vpOperand[1].get())) { + int firstArg = arg1->getValue().getInt(); + if (vpOperand.size() > 2) { + if (auto arg2 = dynamic_cast(vpOperand[2].get())) { + int secondArg = arg2->getValue().getInt(); + // Need both arguements to be postive. + if (firstArg >= 0 && secondArg >= 0) { + // If ExpressionSlice is given three arguments set limit to 'firstArg' + + // 'secondArg' + 1. + dynamic_cast(vpOperand[0].get()) + ->setLimit(firstArg + secondArg + 1); + } + } + } + // Can't set a limit if it is negative. + else { + if (firstArg >= 0) + dynamic_cast(vpOperand[0].get())->setLimit(firstArg); + } + } + } + return this; +} + REGISTER_EXPRESSION(slice, ExpressionSlice::parse); const char* ExpressionSlice::getOpName() const { return "$slice"; diff --git a/src/mongo/db/pipeline/expression.h b/src/mongo/db/pipeline/expression.h index 5b4a0286b7c03..3a95b5bdc4f80 100644 --- a/src/mongo/db/pipeline/expression.h +++ b/src/mongo/db/pipeline/expression.h @@ -736,6 +736,7 @@ class ExpressionArrayElemAt final : public ExpressionFixedArity& expCtx) : ExpressionFixedArity(expCtx) {} + boost::intrusive_ptr optimize() final; Value evaluate(const Document& root) const final; const char* getOpName() const final; @@ -1155,6 +1156,7 @@ class ExpressionFilter final : public Expression { const boost::intrusive_ptr& expCtx, BSONElement expr, const VariablesParseState& vps); + void setLimit(int limit); protected: void _doAddDependencies(DepsTracker* deps) const final; @@ -1174,6 +1176,7 @@ class ExpressionFilter final : public Expression { boost::intrusive_ptr _input; // The expression determining whether each element should be present in the result array. boost::intrusive_ptr _filter; + boost::optional _limit; }; @@ -1666,6 +1669,7 @@ class ExpressionSlice final : public ExpressionRangedArity(expCtx) {} Value evaluate(const Document& root) const final; + boost::intrusive_ptr optimize() final; const char* getOpName() const final; }; diff --git a/src/mongo/db/pipeline/expression_test.cpp b/src/mongo/db/pipeline/expression_test.cpp index 79a30eb56e2f4..132cc2bb6c676 100644 --- a/src/mongo/db/pipeline/expression_test.cpp +++ b/src/mongo/db/pipeline/expression_test.cpp @@ -25,7 +25,6 @@ * delete this exception statement from all source files in the program, * then also delete it in the license file. */ - #include "mongo/platform/basic.h" #include "mongo/bson/bsonmisc.h" @@ -2894,6 +2893,85 @@ TEST(ExpressionObjectOptimizations, } // namespace Object +TEST(ExpressionFilter, ExpressionFilterWithASetLimitShouldReturnAArrayNoGreaterThanTheLimit) { + intrusive_ptr expCtx(new ExpressionContextForTest()); + VariablesParseState vps = expCtx->variablesParseState; + auto filterSpec = BSON( + "$filter" << BSON("input" << BSON_ARRAY(1 << 2 << 3 << 4 << 5 << 6 << 7 << 8 << 9) << "as" + << "arr" + << "cond" << BSON("$gt" << BSON_ARRAY("$$arr" << 3)))); + + + auto expFilter = ExpressionFilter::parse(expCtx, filterSpec.firstElement(), vps); + dynamic_cast(expFilter.get())->setLimit(1); + auto oneElemArray = expFilter->evaluate(Document()); + ASSERT_TRUE(oneElemArray.getArray().size() == 1); + ASSERT_VALUE_EQ(oneElemArray, Value(BSON_ARRAY(4))); + + dynamic_cast(expFilter.get())->setLimit(2); + auto twoElemArray = expFilter->evaluate(Document()); + ASSERT_TRUE(twoElemArray.getArray().size() == 2); + ASSERT_VALUE_EQ(twoElemArray, Value(BSON_ARRAY(4 << 5))); + + dynamic_cast(expFilter.get())->setLimit(5); + auto fiveElemArray = expFilter->evaluate(Document()); + ASSERT_TRUE(fiveElemArray.getArray().size() == 5); + ASSERT_VALUE_EQ(fiveElemArray, Value(BSON_ARRAY(4 << 5 << 6 << 7 << 8))); + // Filter runs out of elements before limit is reached + dynamic_cast(expFilter.get())->setLimit(10); + auto sixElemArray = expFilter->evaluate(Document()); + ASSERT_TRUE(sixElemArray.getArray().size() == 6); + ASSERT_VALUE_EQ(sixElemArray, Value(BSON_ARRAY(4 << 5 << 6 << 7 << 8 << 9))); +} +TEST(ExpressionArrayElemAt, ShouldEvaluateProperly) { + intrusive_ptr expCtx(new ExpressionContextForTest()); + VariablesParseState vps = expCtx->variablesParseState; + auto filterSpec = BSON( + "$filter" << BSON("input" << BSON_ARRAY(1 << 2 << 3 << 4 << 5 << 6 << 7 << 8 << 9) << "as" + << "arr" + << "cond" << BSON("$gt" << BSON_ARRAY("$$arr" << 3)))); + + auto arrayElemAtSpec = BSON("$arrayElemAt" << BSON_ARRAY(filterSpec << 2)); + + auto expArrayElemAt = ExpressionArrayElemAt::parse(expCtx, arrayElemAtSpec.firstElement(), vps); + auto xpArrayElemAt = dynamic_cast(expArrayElemAt.get())->optimize(); + auto val = xpArrayElemAt->evaluate(Document()); + ASSERT_VALUE_EQ(val, Value(6)); + + auto ElemAtNegativeIndex = BSON("$arrayElemAt" << BSON_ARRAY(filterSpec << -2)); + auto expElemAtNegativeIndex = + ExpressionArrayElemAt::parse(expCtx, ElemAtNegativeIndex.firstElement(), vps); + auto elemAtNegativeIndexOptimized = + dynamic_cast(expElemAtNegativeIndex.get())->optimize(); + ASSERT_VALUE_EQ(elemAtNegativeIndexOptimized->evaluate(Document()), Value(8)); +} + +TEST(ExpressionSlice, ShouldEvaluateProperly) { + intrusive_ptr expCtx(new ExpressionContextForTest()); + VariablesParseState vps = expCtx->variablesParseState; + auto filterSpec = BSON( + "$filter" << BSON("input" << BSON_ARRAY(1 << 2 << 3 << 4 << 5 << 6 << 7 << 8 << 9) << "as" + << "arr" + << "cond" << BSON("$gt" << BSON_ARRAY("$$arr" << 1)))); + auto sliceSpec = BSON("$slice" << BSON_ARRAY(filterSpec << 2 << 2)); + auto expSlice = ExpressionSlice::parse(expCtx, sliceSpec.firstElement(), vps); + auto optimizedSlice = dynamic_cast(expSlice.get())->optimize(); + ASSERT_VALUE_EQ(optimizedSlice->evaluate(Document()), Value(BSON_ARRAY(4 << 5))); + + auto sliceNegative = BSON("$slice" << BSON_ARRAY(filterSpec << -4 << 4)); + auto expSliceNegative = ExpressionSlice::parse(expCtx, sliceNegative.firstElement(), vps); + auto optimizedSliceNegative = + dynamic_cast(expSliceNegative.get())->optimize(); + ASSERT_VALUE_EQ(optimizedSliceNegative->evaluate(Document()), + Value(BSON_ARRAY(6 << 7 << 8 << 9))); + auto sliceNegative2ndArg = BSON("$slice" << BSON_ARRAY(filterSpec << -2)); + auto expSliceNegative2ndArg = + ExpressionSlice::parse(expCtx, sliceNegative2ndArg.firstElement(), vps); + auto optimizedSliceNegative2ndArg = + dynamic_cast(expSliceNegative2ndArg.get())->optimize(); + ASSERT_VALUE_EQ(optimizedSliceNegative2ndArg->evaluate(Document()), Value(BSON_ARRAY(8 << 9))); +} + namespace Or { class ExpectedResultBase { From 5c95848bb7a2b27cca6ad57d457f9038712c55fb Mon Sep 17 00:00:00 2001 From: KevinCybura Date: Fri, 20 Apr 2018 14:40:23 -0400 Subject: [PATCH 2/8] optimizeFilter 04/20/2018: ready for pull request --- src/mongo/db/pipeline/expression.cpp | 78 ++++++++++++++++++++++------ 1 file changed, 63 insertions(+), 15 deletions(-) diff --git a/src/mongo/db/pipeline/expression.cpp b/src/mongo/db/pipeline/expression.cpp index f21bd28dd9ce7..420562a09c31a 100644 --- a/src/mongo/db/pipeline/expression.cpp +++ b/src/mongo/db/pipeline/expression.cpp @@ -523,12 +523,29 @@ Value ExpressionArrayElemAt::evaluate(const Document& root) const { const size_t index = static_cast(i); return array[index]; } + intrusive_ptr ExpressionArrayElemAt::optimize() { + // If ExpressionArrayElemAt is passed an ExpressionFilter as its first arugment + // set a limit on the filter so filter returns an array with the last element being the value we + // want. if (dynamic_cast(vpOperand[0].get())) { if (auto expConstant = dynamic_cast(vpOperand[1].get())) { - int index = expConstant->getValue().getInt() + 1; - if (index >= 0) { - dynamic_cast(vpOperand[0].get())->setLimit(index); + auto indexArg = expConstant->getValue(); + + uassert(50802, + str::stream() << getOpName() << "'s second argument must be a numeric value," + << " but is " + << typeName(indexArg.getType()), + indexArg.numeric()); + uassert(50803, + str::stream() << getOpName() << "'s second argument must be representable as" + << " a 32-bit integer: " + << indexArg.coerceToDouble(), + indexArg.integral()); + + // Can't optimize of the index is less that 0 + if (indexArg.getInt() >= 0) { + dynamic_cast(vpOperand[0].get())->setLimit(indexArg.getInt() + 1); } } } @@ -3922,25 +3939,56 @@ intrusive_ptr ExpressionSlice::optimize() { // If ExpressionSlice is passed an ExpressionFilter we can stop filtering once the size of // the array returned by the filter is equal to the last arguement passed to ExpressionSlice. if (dynamic_cast(vpOperand[0].get())) { - if (auto arg1 = dynamic_cast(vpOperand[1].get())) { - int firstArg = arg1->getValue().getInt(); - if (vpOperand.size() > 2) { - if (auto arg2 = dynamic_cast(vpOperand[2].get())) { - int secondArg = arg2->getValue().getInt(); + if (auto secondArg = dynamic_cast(vpOperand[1].get())) { + auto arg2 = secondArg->getValue(); + + uassert(50797, + str::stream() << "Second argument to $slice must be a numeric value," + << " but is of type: " + << typeName(arg2.getType()), + arg2.numeric()); + + uassert(50798, + str::stream() << "Second argument to $slice can't be represented as" + << " a 32-bit integer: " + << arg2.coerceToDouble(), + arg2.integral()); + + if (vpOperand.size() == 2) { + // Can't set a limit if it is negative. + if (arg2.getInt() >= 0) { + dynamic_cast(vpOperand[0].get()) + ->setLimit(arg2.getInt() + 1); + } + } else if (vpOperand.size() > 2) { + if (auto thirdArg = dynamic_cast(vpOperand[2].get())) { + auto arg3 = thirdArg->getValue(); + + uassert(50799, + str::stream() << "Third argument to $slice must be numeric, but " + << "is of type: " << typeName(arg3.getType()), + arg3.numeric()); + + uassert(50800, + str::stream() << "Third argument to $slice can't be represented" + << " as a 32-bit integer: " << arg3.coerceToDouble(), + arg3.integral()); + + uassert(50801, + str::stream() << "Third argument to $slice must be positive: " + << arg3.coerceToInt(), + arg3.coerceToInt() > 0); + + // Need both arguements to be postive. - if (firstArg >= 0 && secondArg >= 0) { + if (arg2.getInt() >= 0 && arg3.getInt() >= 0) { // If ExpressionSlice is given three arguments set limit to 'firstArg' + // 'secondArg' + 1. dynamic_cast(vpOperand[0].get()) - ->setLimit(firstArg + secondArg + 1); + ->setLimit(arg2.getInt() + arg3.getInt() + 1); } } } - // Can't set a limit if it is negative. - else { - if (firstArg >= 0) - dynamic_cast(vpOperand[0].get())->setLimit(firstArg); - } } } return this; From 9971faf0048527d3da98a878301d343446762f10 Mon Sep 17 00:00:00 2001 From: KevinCybura Date: Wed, 25 Apr 2018 11:34:26 -0400 Subject: [PATCH 3/8] optimizeFilter 04/25/2018: ready for pull request --- src/mongo/db/pipeline/expression.cpp | 47 ++++++++++++----------- src/mongo/db/pipeline/expression_test.cpp | 11 +++++- 2 files changed, 33 insertions(+), 25 deletions(-) diff --git a/src/mongo/db/pipeline/expression.cpp b/src/mongo/db/pipeline/expression.cpp index 420562a09c31a..643db79338c92 100644 --- a/src/mongo/db/pipeline/expression.cpp +++ b/src/mongo/db/pipeline/expression.cpp @@ -542,10 +542,10 @@ intrusive_ptr ExpressionArrayElemAt::optimize() { << " a 32-bit integer: " << indexArg.coerceToDouble(), indexArg.integral()); - + auto ind = indexArg.coerceToInt(); // Can't optimize of the index is less that 0 - if (indexArg.getInt() >= 0) { - dynamic_cast(vpOperand[0].get())->setLimit(indexArg.getInt() + 1); + if (ind >= 0) { + dynamic_cast(vpOperand[0].get())->setLimit(ind + 1); } } } @@ -3940,52 +3940,53 @@ intrusive_ptr ExpressionSlice::optimize() { // the array returned by the filter is equal to the last arguement passed to ExpressionSlice. if (dynamic_cast(vpOperand[0].get())) { if (auto secondArg = dynamic_cast(vpOperand[1].get())) { - auto arg2 = secondArg->getValue(); + auto secondVal = secondArg->getValue(); uassert(50797, str::stream() << "Second argument to $slice must be a numeric value," - << " but is of type: " - << typeName(arg2.getType()), - arg2.numeric()); + << " but is of type: " + << typeName(secondVal.getType()), + secondVal.numeric()); uassert(50798, str::stream() << "Second argument to $slice can't be represented as" << " a 32-bit integer: " - << arg2.coerceToDouble(), - arg2.integral()); + << secondVal.coerceToDouble(), + secondVal.integral()); + int arg2 = secondVal.coerceToInt(); if (vpOperand.size() == 2) { // Can't set a limit if it is negative. - if (arg2.getInt() >= 0) { - dynamic_cast(vpOperand[0].get()) - ->setLimit(arg2.getInt() + 1); + if (arg2 >= 0) { + // If slice is given two arguments set limit to the position we want to slice. + dynamic_cast(vpOperand[0].get())->setLimit(arg2); } } else if (vpOperand.size() > 2) { if (auto thirdArg = dynamic_cast(vpOperand[2].get())) { - auto arg3 = thirdArg->getValue(); + auto thirdVal = thirdArg->getValue(); uassert(50799, str::stream() << "Third argument to $slice must be numeric, but " - << "is of type: " << typeName(arg3.getType()), - arg3.numeric()); + << "is of type: " << typeName(thirdVal.getType()), + thirdVal.numeric()); uassert(50800, str::stream() << "Third argument to $slice can't be represented" - << " as a 32-bit integer: " << arg3.coerceToDouble(), - arg3.integral()); + << " as a 32-bit integer: " + << thirdVal.coerceToDouble(), + thirdVal.integral()); uassert(50801, str::stream() << "Third argument to $slice must be positive: " - << arg3.coerceToInt(), - arg3.coerceToInt() > 0); - + << thirdVal.coerceToInt(), + thirdVal.coerceToInt() > 0); - // Need both arguements to be postive. - if (arg2.getInt() >= 0 && arg3.getInt() >= 0) { + int arg3 = thirdVal.coerceToInt(); + if (arg2 >= 0) { // If ExpressionSlice is given three arguments set limit to 'firstArg' + // 'secondArg' + 1. dynamic_cast(vpOperand[0].get()) - ->setLimit(arg2.getInt() + arg3.getInt() + 1); + ->setLimit(arg2 + arg3 + 1); } } } diff --git a/src/mongo/db/pipeline/expression_test.cpp b/src/mongo/db/pipeline/expression_test.cpp index 132cc2bb6c676..1096260b44a1b 100644 --- a/src/mongo/db/pipeline/expression_test.cpp +++ b/src/mongo/db/pipeline/expression_test.cpp @@ -2923,9 +2923,10 @@ TEST(ExpressionFilter, ExpressionFilterWithASetLimitShouldReturnAArrayNoGreaterT ASSERT_TRUE(sixElemArray.getArray().size() == 6); ASSERT_VALUE_EQ(sixElemArray, Value(BSON_ARRAY(4 << 5 << 6 << 7 << 8 << 9))); } -TEST(ExpressionArrayElemAt, ShouldEvaluateProperly) { +TEST(ExpressionArrayElemAt, ArrayElemAtWithFilterShouldEvaluateCorrectly) { intrusive_ptr expCtx(new ExpressionContextForTest()); VariablesParseState vps = expCtx->variablesParseState; + // Returns an array with all values greater than 3. auto filterSpec = BSON( "$filter" << BSON("input" << BSON_ARRAY(1 << 2 << 3 << 4 << 5 << 6 << 7 << 8 << 9) << "as" << "arr" @@ -2938,6 +2939,11 @@ TEST(ExpressionArrayElemAt, ShouldEvaluateProperly) { auto val = xpArrayElemAt->evaluate(Document()); ASSERT_VALUE_EQ(val, Value(6)); + auto eA = ExpressionArrayElemAt::parse(expCtx, BSON("$arrayElemAt" << BSON_ARRAY(BSON_ARRAY(1 << 2 <<3 << 4 <<5) << 1)).firstElement(), vps); + auto eAO = dynamic_cast(eA.get())->optimize(); + auto va = eAO->evaluate(Document()); + ASSERT_VALUE_EQ(va, Value(2)); + auto ElemAtNegativeIndex = BSON("$arrayElemAt" << BSON_ARRAY(filterSpec << -2)); auto expElemAtNegativeIndex = ExpressionArrayElemAt::parse(expCtx, ElemAtNegativeIndex.firstElement(), vps); @@ -2946,9 +2952,10 @@ TEST(ExpressionArrayElemAt, ShouldEvaluateProperly) { ASSERT_VALUE_EQ(elemAtNegativeIndexOptimized->evaluate(Document()), Value(8)); } -TEST(ExpressionSlice, ShouldEvaluateProperly) { +TEST(ExpressionSlice, SliceWithFilterShouldEvaluateCorrectly) { intrusive_ptr expCtx(new ExpressionContextForTest()); VariablesParseState vps = expCtx->variablesParseState; + // Returns an array with values greater than 1. auto filterSpec = BSON( "$filter" << BSON("input" << BSON_ARRAY(1 << 2 << 3 << 4 << 5 << 6 << 7 << 8 << 9) << "as" << "arr" From ed427183814a9b4dd215b39b96fdebca341f950d Mon Sep 17 00:00:00 2001 From: KevinCybura Date: Wed, 25 Apr 2018 11:44:56 -0400 Subject: [PATCH 4/8] optimizeFilter 04/25/2018: ready for pull request --- src/mongo/db/pipeline/expression.cpp | 1 - src/mongo/db/pipeline/expression.h | 2 ++ src/mongo/db/pipeline/expression_test.cpp | 1 + 3 files changed, 3 insertions(+), 1 deletion(-) diff --git a/src/mongo/db/pipeline/expression.cpp b/src/mongo/db/pipeline/expression.cpp index 643db79338c92..ca31ec6730426 100644 --- a/src/mongo/db/pipeline/expression.cpp +++ b/src/mongo/db/pipeline/expression.cpp @@ -3994,7 +3994,6 @@ intrusive_ptr ExpressionSlice::optimize() { } return this; } - REGISTER_EXPRESSION(slice, ExpressionSlice::parse); const char* ExpressionSlice::getOpName() const { return "$slice"; diff --git a/src/mongo/db/pipeline/expression.h b/src/mongo/db/pipeline/expression.h index 3a95b5bdc4f80..940fe266ddc29 100644 --- a/src/mongo/db/pipeline/expression.h +++ b/src/mongo/db/pipeline/expression.h @@ -1176,6 +1176,8 @@ class ExpressionFilter final : public Expression { boost::intrusive_ptr _input; // The expression determining whether each element should be present in the result array. boost::intrusive_ptr _filter; + // For expression ArrayElemAt and Slice we can optimize filter by setting a limit to end the + // filter once all values needed are filtered. boost::optional _limit; }; diff --git a/src/mongo/db/pipeline/expression_test.cpp b/src/mongo/db/pipeline/expression_test.cpp index 1096260b44a1b..c6593996dcf6f 100644 --- a/src/mongo/db/pipeline/expression_test.cpp +++ b/src/mongo/db/pipeline/expression_test.cpp @@ -25,6 +25,7 @@ * delete this exception statement from all source files in the program, * then also delete it in the license file. */ + #include "mongo/platform/basic.h" #include "mongo/bson/bsonmisc.h" From f2df3ca7c29f2d73820f9e19a11754e80ebc2d4b Mon Sep 17 00:00:00 2001 From: KevinCybura Date: Thu, 26 Apr 2018 14:05:03 -0400 Subject: [PATCH 5/8] optimizeFilter 04/26/2018: ready for pull request --- src/mongo/db/pipeline/expression.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/mongo/db/pipeline/expression.cpp b/src/mongo/db/pipeline/expression.cpp index ca31ec6730426..0ecfc8338a870 100644 --- a/src/mongo/db/pipeline/expression.cpp +++ b/src/mongo/db/pipeline/expression.cpp @@ -543,7 +543,7 @@ intrusive_ptr ExpressionArrayElemAt::optimize() { << indexArg.coerceToDouble(), indexArg.integral()); auto ind = indexArg.coerceToInt(); - // Can't optimize of the index is less that 0 + // Can't optimize of the index is less that 0. if (ind >= 0) { dynamic_cast(vpOperand[0].get())->setLimit(ind + 1); } From e856658a1c8c4649e6ed535c3ed36d362af3a5bd Mon Sep 17 00:00:00 2001 From: KevinCybura Date: Fri, 27 Apr 2018 15:22:34 -0400 Subject: [PATCH 6/8] optimizeFilter 04/27/2018: requested changes made --- src/mongo/db/pipeline/expression.cpp | 72 ++++++----------------- src/mongo/db/pipeline/expression.h | 4 +- src/mongo/db/pipeline/expression_test.cpp | 58 +++++++++--------- 3 files changed, 50 insertions(+), 84 deletions(-) diff --git a/src/mongo/db/pipeline/expression.cpp b/src/mongo/db/pipeline/expression.cpp index 0ecfc8338a870..c822f47a0dcd6 100644 --- a/src/mongo/db/pipeline/expression.cpp +++ b/src/mongo/db/pipeline/expression.cpp @@ -525,27 +525,27 @@ Value ExpressionArrayElemAt::evaluate(const Document& root) const { } intrusive_ptr ExpressionArrayElemAt::optimize() { - // If ExpressionArrayElemAt is passed an ExpressionFilter as its first arugment - // set a limit on the filter so filter returns an array with the last element being the value we - // want. + // This will optimize all arguments to this expression. + auto optimized = ExpressionNary::optimize(); + if (optimized.get() != this) { + return optimized; + } + + // If ExpressionArrayElemAt is passed an ExpressionFilter as its first arugment set a limit on + // the filter so filter returns an array with the last element being the value we want. if (dynamic_cast(vpOperand[0].get())) { if (auto expConstant = dynamic_cast(vpOperand[1].get())) { auto indexArg = expConstant->getValue(); - uassert(50802, - str::stream() << getOpName() << "'s second argument must be a numeric value," - << " but is " - << typeName(indexArg.getType()), - indexArg.numeric()); uassert(50803, str::stream() << getOpName() << "'s second argument must be representable as" - << " a 32-bit integer: " + << " a 32-bit integer: " << indexArg.coerceToDouble(), indexArg.integral()); - auto ind = indexArg.coerceToInt(); + auto index = indexArg.coerceToInt(); // Can't optimize of the index is less that 0. - if (ind >= 0) { - dynamic_cast(vpOperand[0].get())->setLimit(ind + 1); + if (index >= 0) { + dynamic_cast(vpOperand[0].get())->setLimit(index + 1); } } } @@ -2234,7 +2234,7 @@ Value ExpressionFilter::evaluate(const Document& root) const { if (_filter->evaluate(root).coerceToBool()) { output.push_back(std::move(elem)); - if(_limit && static_cast(output.size()) == _limit.get()){ + if (_limit && static_cast(output.size()) == _limit.get()) { return Value(std::move(output)); } } @@ -3913,20 +3913,7 @@ Value ExpressionSlice::evaluate(const Document& root) const { return Value(BSONNULL); } - uassert(28727, - str::stream() << "Third argument to $slice must be numeric, but " - << "is of type: " - << typeName(countVal.getType()), - countVal.numeric()); - uassert(28728, - str::stream() << "Third argument to $slice can't be represented" - << " as a 32-bit integer: " - << countVal.coerceToDouble(), - countVal.integral()); - uassert(28729, - str::stream() << "Third argument to $slice must be positive: " - << countVal.coerceToInt(), - countVal.coerceToInt() > 0); + uassertIfNotIntegralAndNonNegative(countVal, "$slice", "third argument"); size_t count = size_t(countVal.coerceToInt()); end = std::min(start + count, array.size()); @@ -3942,15 +3929,9 @@ intrusive_ptr ExpressionSlice::optimize() { if (auto secondArg = dynamic_cast(vpOperand[1].get())) { auto secondVal = secondArg->getValue(); - uassert(50797, - str::stream() << "Second argument to $slice must be a numeric value," - << " but is of type: " - << typeName(secondVal.getType()), - secondVal.numeric()); - uassert(50798, str::stream() << "Second argument to $slice can't be represented as" - << " a 32-bit integer: " + << " a 32-bit integer: " << secondVal.coerceToDouble(), secondVal.integral()); @@ -3965,28 +3946,13 @@ intrusive_ptr ExpressionSlice::optimize() { if (auto thirdArg = dynamic_cast(vpOperand[2].get())) { auto thirdVal = thirdArg->getValue(); - uassert(50799, - str::stream() << "Third argument to $slice must be numeric, but " - << "is of type: " << typeName(thirdVal.getType()), - thirdVal.numeric()); - - uassert(50800, - str::stream() << "Third argument to $slice can't be represented" - << " as a 32-bit integer: " - << thirdVal.coerceToDouble(), - thirdVal.integral()); - - uassert(50801, - str::stream() << "Third argument to $slice must be positive: " - << thirdVal.coerceToInt(), - thirdVal.coerceToInt() > 0); + uassertIfNotIntegralAndNonNegative(thirdVal, "$slice", "third argument"); int arg3 = thirdVal.coerceToInt(); if (arg2 >= 0) { - // If ExpressionSlice is given three arguments set limit to 'firstArg' + - // 'secondArg' + 1. - dynamic_cast(vpOperand[0].get()) - ->setLimit(arg2 + arg3 + 1); + // The limit needs to set as the last element we want in this case its + // the position argument + the first n elements argument. + dynamic_cast(vpOperand[0].get())->setLimit(arg2 + arg3); } } } diff --git a/src/mongo/db/pipeline/expression.h b/src/mongo/db/pipeline/expression.h index 940fe266ddc29..563cc1d2879e7 100644 --- a/src/mongo/db/pipeline/expression.h +++ b/src/mongo/db/pipeline/expression.h @@ -1176,8 +1176,8 @@ class ExpressionFilter final : public Expression { boost::intrusive_ptr _input; // The expression determining whether each element should be present in the result array. boost::intrusive_ptr _filter; - // For expression ArrayElemAt and Slice we can optimize filter by setting a limit to end the - // filter once all values needed are filtered. + // When $filter is passed as an argument to $arrayElemAt or $slice we can set a limit on $filter + // to stop filtering once all the values needed are in the result array. boost::optional _limit; }; diff --git a/src/mongo/db/pipeline/expression_test.cpp b/src/mongo/db/pipeline/expression_test.cpp index c6593996dcf6f..67deebc52c20b 100644 --- a/src/mongo/db/pipeline/expression_test.cpp +++ b/src/mongo/db/pipeline/expression_test.cpp @@ -2894,13 +2894,14 @@ TEST(ExpressionObjectOptimizations, } // namespace Object -TEST(ExpressionFilter, ExpressionFilterWithASetLimitShouldReturnAArrayNoGreaterThanTheLimit) { +TEST(ExpressionFilter, ExpressionFilterWithASetLimitShouldReturnAnArrayNoGreaterThanTheLimit) { intrusive_ptr expCtx(new ExpressionContextForTest()); VariablesParseState vps = expCtx->variablesParseState; auto filterSpec = BSON( "$filter" << BSON("input" << BSON_ARRAY(1 << 2 << 3 << 4 << 5 << 6 << 7 << 8 << 9) << "as" << "arr" - << "cond" << BSON("$gt" << BSON_ARRAY("$$arr" << 3)))); + << "cond" + << BSON("$gt" << BSON_ARRAY("$$arr" << 3)))); auto expFilter = ExpressionFilter::parse(expCtx, filterSpec.firstElement(), vps); @@ -2931,28 +2932,30 @@ TEST(ExpressionArrayElemAt, ArrayElemAtWithFilterShouldEvaluateCorrectly) { auto filterSpec = BSON( "$filter" << BSON("input" << BSON_ARRAY(1 << 2 << 3 << 4 << 5 << 6 << 7 << 8 << 9) << "as" << "arr" - << "cond" << BSON("$gt" << BSON_ARRAY("$$arr" << 3)))); + << "cond" + << BSON("$gt" << BSON_ARRAY("$$arr" << 3)))); auto arrayElemAtSpec = BSON("$arrayElemAt" << BSON_ARRAY(filterSpec << 2)); auto expArrayElemAt = ExpressionArrayElemAt::parse(expCtx, arrayElemAtSpec.firstElement(), vps); - auto xpArrayElemAt = dynamic_cast(expArrayElemAt.get())->optimize(); - auto val = xpArrayElemAt->evaluate(Document()); + auto optimized = dynamic_cast(expArrayElemAt.get())->optimize(); + auto val = optimized->evaluate(Document()); ASSERT_VALUE_EQ(val, Value(6)); - auto eA = ExpressionArrayElemAt::parse(expCtx, BSON("$arrayElemAt" << BSON_ARRAY(BSON_ARRAY(1 << 2 <<3 << 4 <<5) << 1)).firstElement(), vps); - auto eAO = dynamic_cast(eA.get())->optimize(); - auto va = eAO->evaluate(Document()); - ASSERT_VALUE_EQ(va, Value(2)); + expArrayElemAt = ExpressionArrayElemAt::parse( + expCtx, + BSON("$arrayElemAt" << BSON_ARRAY(BSON_ARRAY(1 << 2 << 3 << 4 << 5) << 1)).firstElement(), + vps); - auto ElemAtNegativeIndex = BSON("$arrayElemAt" << BSON_ARRAY(filterSpec << -2)); - auto expElemAtNegativeIndex = - ExpressionArrayElemAt::parse(expCtx, ElemAtNegativeIndex.firstElement(), vps); - auto elemAtNegativeIndexOptimized = - dynamic_cast(expElemAtNegativeIndex.get())->optimize(); - ASSERT_VALUE_EQ(elemAtNegativeIndexOptimized->evaluate(Document()), Value(8)); -} + optimized = dynamic_cast(expArrayElemAt.get())->optimize(); + val = optimized->evaluate(Document()); + ASSERT_VALUE_EQ(val, Value(2)); + expArrayElemAt = ExpressionArrayElemAt::parse( + expCtx, BSON("$arrayElemAt" << BSON_ARRAY(filterSpec << -2)).firstElement(), vps); + optimized = dynamic_cast(expArrayElemAt.get())->optimize(); + ASSERT_VALUE_EQ(optimized->evaluate(Document()), Value(8)); +} TEST(ExpressionSlice, SliceWithFilterShouldEvaluateCorrectly) { intrusive_ptr expCtx(new ExpressionContextForTest()); VariablesParseState vps = expCtx->variablesParseState; @@ -2960,24 +2963,21 @@ TEST(ExpressionSlice, SliceWithFilterShouldEvaluateCorrectly) { auto filterSpec = BSON( "$filter" << BSON("input" << BSON_ARRAY(1 << 2 << 3 << 4 << 5 << 6 << 7 << 8 << 9) << "as" << "arr" - << "cond" << BSON("$gt" << BSON_ARRAY("$$arr" << 1)))); + << "cond" + << BSON("$gt" << BSON_ARRAY("$$arr" << 1)))); auto sliceSpec = BSON("$slice" << BSON_ARRAY(filterSpec << 2 << 2)); auto expSlice = ExpressionSlice::parse(expCtx, sliceSpec.firstElement(), vps); auto optimizedSlice = dynamic_cast(expSlice.get())->optimize(); ASSERT_VALUE_EQ(optimizedSlice->evaluate(Document()), Value(BSON_ARRAY(4 << 5))); - auto sliceNegative = BSON("$slice" << BSON_ARRAY(filterSpec << -4 << 4)); - auto expSliceNegative = ExpressionSlice::parse(expCtx, sliceNegative.firstElement(), vps); - auto optimizedSliceNegative = - dynamic_cast(expSliceNegative.get())->optimize(); - ASSERT_VALUE_EQ(optimizedSliceNegative->evaluate(Document()), - Value(BSON_ARRAY(6 << 7 << 8 << 9))); - auto sliceNegative2ndArg = BSON("$slice" << BSON_ARRAY(filterSpec << -2)); - auto expSliceNegative2ndArg = - ExpressionSlice::parse(expCtx, sliceNegative2ndArg.firstElement(), vps); - auto optimizedSliceNegative2ndArg = - dynamic_cast(expSliceNegative2ndArg.get())->optimize(); - ASSERT_VALUE_EQ(optimizedSliceNegative2ndArg->evaluate(Document()), Value(BSON_ARRAY(8 << 9))); + sliceSpec = BSON("$slice" << BSON_ARRAY(filterSpec << -4 << 4)); + expSlice = ExpressionSlice::parse(expCtx, sliceSpec.firstElement(), vps); + optimizedSlice = dynamic_cast(expSlice.get())->optimize(); + ASSERT_VALUE_EQ(optimizedSlice->evaluate(Document()), Value(BSON_ARRAY(6 << 7 << 8 << 9))); + sliceSpec = BSON("$slice" << BSON_ARRAY(filterSpec << -2)); + expSlice = ExpressionSlice::parse(expCtx, sliceSpec.firstElement(), vps); + optimizedSlice = dynamic_cast(expSlice.get())->optimize(); + ASSERT_VALUE_EQ(optimizedSlice->evaluate(Document()), Value(BSON_ARRAY(8 << 9))); } namespace Or { From 385438b02600fc04676c963000403c525c059772 Mon Sep 17 00:00:00 2001 From: KevinCybura Date: Tue, 1 May 2018 13:11:31 -0400 Subject: [PATCH 7/8] optimizeFilter 05/1/2018: added ExpressionNary optimize and unittests for it --- src/mongo/db/pipeline/expression.cpp | 9 +++++-- src/mongo/db/pipeline/expression_test.cpp | 32 +++++++++++++++++++++-- 2 files changed, 37 insertions(+), 4 deletions(-) diff --git a/src/mongo/db/pipeline/expression.cpp b/src/mongo/db/pipeline/expression.cpp index c822f47a0dcd6..c4ac880cd8920 100644 --- a/src/mongo/db/pipeline/expression.cpp +++ b/src/mongo/db/pipeline/expression.cpp @@ -527,9 +527,9 @@ Value ExpressionArrayElemAt::evaluate(const Document& root) const { intrusive_ptr ExpressionArrayElemAt::optimize() { // This will optimize all arguments to this expression. auto optimized = ExpressionNary::optimize(); - if (optimized.get() != this) { + if (optimized.get() != this) return optimized; - } + // If ExpressionArrayElemAt is passed an ExpressionFilter as its first arugment set a limit on // the filter so filter returns an array with the last element being the value we want. @@ -3923,6 +3923,11 @@ Value ExpressionSlice::evaluate(const Document& root) const { } intrusive_ptr ExpressionSlice::optimize() { + // This will optimize all arguments to this expression. + auto optimized = ExpressionNary::optimize(); + if(optimized.get() != this) + return optimized; + // If ExpressionSlice is passed an ExpressionFilter we can stop filtering once the size of // the array returned by the filter is equal to the last arguement passed to ExpressionSlice. if (dynamic_cast(vpOperand[0].get())) { diff --git a/src/mongo/db/pipeline/expression_test.cpp b/src/mongo/db/pipeline/expression_test.cpp index 67deebc52c20b..449cbf8e41862 100644 --- a/src/mongo/db/pipeline/expression_test.cpp +++ b/src/mongo/db/pipeline/expression_test.cpp @@ -2925,6 +2925,19 @@ TEST(ExpressionFilter, ExpressionFilterWithASetLimitShouldReturnAnArrayNoGreater ASSERT_TRUE(sixElemArray.getArray().size() == 6); ASSERT_VALUE_EQ(sixElemArray, Value(BSON_ARRAY(4 << 5 << 6 << 7 << 8 << 9))); } + +TEST(ExpressionArrayElemAt, ArrayElemAtWithAllConstantValueShouldOptimizeToAnExpressionConstant) { + intrusive_ptr expCtx(new ExpressionContextForTest()); + VariablesParseState vps = expCtx->variablesParseState; + + auto expArrayElemAt = ExpressionArrayElemAt::parse( + expCtx, + BSON("$arrayElemAt" << BSON_ARRAY(BSON_ARRAY(1 << 2 << 3 << 4 << 5) << 1)).firstElement(), + vps); + expArrayElemAt = expArrayElemAt.get()->optimize(); + ASSERT_TRUE(dynamic_cast(expArrayElemAt.get())); +} + TEST(ExpressionArrayElemAt, ArrayElemAtWithFilterShouldEvaluateCorrectly) { intrusive_ptr expCtx(new ExpressionContextForTest()); VariablesParseState vps = expCtx->variablesParseState; @@ -2932,9 +2945,10 @@ TEST(ExpressionArrayElemAt, ArrayElemAtWithFilterShouldEvaluateCorrectly) { auto filterSpec = BSON( "$filter" << BSON("input" << BSON_ARRAY(1 << 2 << 3 << 4 << 5 << 6 << 7 << 8 << 9) << "as" << "arr" - << "cond" + << "cond" << BSON("$gt" << BSON_ARRAY("$$arr" << 3)))); + auto arrayElemAtSpec = BSON("$arrayElemAt" << BSON_ARRAY(filterSpec << 2)); auto expArrayElemAt = ExpressionArrayElemAt::parse(expCtx, arrayElemAtSpec.firstElement(), vps); @@ -2956,6 +2970,20 @@ TEST(ExpressionArrayElemAt, ArrayElemAtWithFilterShouldEvaluateCorrectly) { optimized = dynamic_cast(expArrayElemAt.get())->optimize(); ASSERT_VALUE_EQ(optimized->evaluate(Document()), Value(8)); } + +TEST(ExpressionSlice, ExpressionSliceWithAllConstantValueShouldOptimizeToAnExpressionConstant) { + intrusive_ptr expCtx(new ExpressionContextForTest()); + VariablesParseState vps = expCtx->variablesParseState; + + auto expSlice = ExpressionSlice::parse( + expCtx, + BSON("$arrayElemAt" << BSON_ARRAY(BSON_ARRAY(1 << 2 << 3 << 4 << 5) << 1 << 1)) + .firstElement(), + vps); + expSlice = expSlice.get()->optimize(); + ASSERT_TRUE(dynamic_cast(expSlice.get())); +} + TEST(ExpressionSlice, SliceWithFilterShouldEvaluateCorrectly) { intrusive_ptr expCtx(new ExpressionContextForTest()); VariablesParseState vps = expCtx->variablesParseState; @@ -2964,7 +2992,7 @@ TEST(ExpressionSlice, SliceWithFilterShouldEvaluateCorrectly) { "$filter" << BSON("input" << BSON_ARRAY(1 << 2 << 3 << 4 << 5 << 6 << 7 << 8 << 9) << "as" << "arr" << "cond" - << BSON("$gt" << BSON_ARRAY("$$arr" << 1)))); + << BSON("$gt" << BSON_ARRAY("$$arr" << 1)))); auto sliceSpec = BSON("$slice" << BSON_ARRAY(filterSpec << 2 << 2)); auto expSlice = ExpressionSlice::parse(expCtx, sliceSpec.firstElement(), vps); auto optimizedSlice = dynamic_cast(expSlice.get())->optimize(); From 11eabed23b4cc580ed685356439567ab1ffd55e2 Mon Sep 17 00:00:00 2001 From: KevinCybura Date: Fri, 4 May 2018 11:12:16 -0400 Subject: [PATCH 8/8] optimizeFilter: final changes --- src/mongo/db/pipeline/expression.h | 2 +- src/mongo/db/pipeline/expression_test.cpp | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/mongo/db/pipeline/expression.h b/src/mongo/db/pipeline/expression.h index 563cc1d2879e7..53e684713c5d7 100644 --- a/src/mongo/db/pipeline/expression.h +++ b/src/mongo/db/pipeline/expression.h @@ -736,8 +736,8 @@ class ExpressionArrayElemAt final : public ExpressionFixedArity& expCtx) : ExpressionFixedArity(expCtx) {} - boost::intrusive_ptr optimize() final; + boost::intrusive_ptr optimize() final; Value evaluate(const Document& root) const final; const char* getOpName() const final; }; diff --git a/src/mongo/db/pipeline/expression_test.cpp b/src/mongo/db/pipeline/expression_test.cpp index 449cbf8e41862..8d8e774f85103 100644 --- a/src/mongo/db/pipeline/expression_test.cpp +++ b/src/mongo/db/pipeline/expression_test.cpp @@ -2926,7 +2926,7 @@ TEST(ExpressionFilter, ExpressionFilterWithASetLimitShouldReturnAnArrayNoGreater ASSERT_VALUE_EQ(sixElemArray, Value(BSON_ARRAY(4 << 5 << 6 << 7 << 8 << 9))); } -TEST(ExpressionArrayElemAt, ArrayElemAtWithAllConstantValueShouldOptimizeToAnExpressionConstant) { +TEST(ExpressionArrayElemAt, ArrayElemAtWithAllConstantValuesShouldOptimizeToAnExpressionConstant) { intrusive_ptr expCtx(new ExpressionContextForTest()); VariablesParseState vps = expCtx->variablesParseState; @@ -2971,7 +2971,7 @@ TEST(ExpressionArrayElemAt, ArrayElemAtWithFilterShouldEvaluateCorrectly) { ASSERT_VALUE_EQ(optimized->evaluate(Document()), Value(8)); } -TEST(ExpressionSlice, ExpressionSliceWithAllConstantValueShouldOptimizeToAnExpressionConstant) { +TEST(ExpressionSlice, ExpressionSliceWithAllConstantValuesShouldOptimizeToAnExpressionConstant) { intrusive_ptr expCtx(new ExpressionContextForTest()); VariablesParseState vps = expCtx->variablesParseState;