From a5f85e1d87d6920e5f40eeb940518123d31054b2 Mon Sep 17 00:00:00 2001 From: KevinCybura Date: Tue, 20 Mar 2018 14:17:28 -0400 Subject: [PATCH 01/12] optimization 03/19/18: finished class --- src/mongo/db/pipeline/expression.cpp | 100 +++++++++++++++++++++++---- src/mongo/db/pipeline/expression.h | 11 ++- 2 files changed, 96 insertions(+), 15 deletions(-) diff --git a/src/mongo/db/pipeline/expression.cpp b/src/mongo/db/pipeline/expression.cpp index c35499f78af5b..b447786b4f8fa 100644 --- a/src/mongo/db/pipeline/expression.cpp +++ b/src/mongo/db/pipeline/expression.cpp @@ -482,6 +482,21 @@ Value ExpressionArray::serialize(bool explain) const { return Value(std::move(expressions)); } +intrusive_ptr ExpressionArray::optimize() { + bool allValuesConstant = true; + for (auto& expr : vpOperand) { + expr = expr->optimize(); + if (!dynamic_cast(expr.get())) { + allValuesConstant = false; + } + } + // If all values in ExpressionObject are constant evaluate to ExpressionConstant. + if (allValuesConstant) { + return ExpressionConstant::create(getExpressionContext(), evaluate(Document())); + } + return this; +} + const char* ExpressionArray::getOpName() const { // This should never be called, but is needed to inherit from ExpressionNary. return "$array"; @@ -2717,30 +2732,89 @@ Value ExpressionIndexOfArray::evaluate(const Document& root) const { std::vector array = arrayArg.getArray(); - Value searchItem = vpOperand[1]->evaluate(root); + std::vector operands = parseDeps(root, vpOperand, array.size()); + for (int i = operands[1].getInt(); i < operands[2].getInt(); i++) { + if (getExpressionContext()->getValueComparator().evaluate(array[i] == operands[0])) { + return Value(static_cast(i)); + } + } - size_t startIndex = 0; - if (vpOperand.size() > 2) { - Value startIndexArg = vpOperand[2]->evaluate(root); + return Value(-1); +} + +vector ExpressionIndexOfArray::parseDeps(const Document& root, + const ExpressionVector& operands, + size_t arrayLength) const { + std::vector deps; + deps.push_back(operands[1]->evaluate(root)); + + int startIndex = 0; + if (operands.size() > 2) { + Value startIndexArg = operands[2]->evaluate(root); uassertIfNotIntegralAndNonNegative(startIndexArg, getOpName(), "starting index"); - startIndex = static_cast(startIndexArg.coerceToInt()); + startIndex = static_cast(startIndexArg.coerceToInt()); } + deps.push_back(Value(startIndex)); - size_t endIndex = array.size(); - if (vpOperand.size() > 3) { - Value endIndexArg = vpOperand[3]->evaluate(root); + int endIndex = arrayLength; + if (operands.size() > 3) { + Value endIndexArg = operands[3]->evaluate(root); uassertIfNotIntegralAndNonNegative(endIndexArg, getOpName(), "ending index"); // Don't let 'endIndex' exceed the length of the array. - endIndex = std::min(array.size(), static_cast(endIndexArg.coerceToInt())); + endIndex = std::min(arrayLength, static_cast(endIndexArg.coerceToInt())); } + deps.push_back(Value(endIndex)); + return deps; +} - for (size_t i = startIndex; i < endIndex; i++) { - if (getExpressionContext()->getValueComparator().evaluate(array[i] == searchItem)) { - return Value(static_cast(i)); +class ExpressionIndexOfArray::Optimized : public ExpressionIndexOfArray { +public: + Optimized(const boost::intrusive_ptr& expCtx, + const ValueUnorderedMap& indexMap, + const ExpressionVector& operands) + : ExpressionIndexOfArray(expCtx), _indexMap(std::move(indexMap)) { + vpOperand = operands; + } + + virtual Value evaluate(const Document& root) const { + std::vector operands = parseDeps(root, vpOperand, _indexMap.size()); + auto index = _indexMap.find(operands[0]); + if (index != _indexMap.end() && index->second > operands[1].getInt() && + index->second < operands[2].getInt()) { + return Value(index->second); + } else { + return Value(-1); } } - return Value(-1); +private: + const ValueUnorderedMap _indexMap; +}; + +intrusive_ptr ExpressionIndexOfArray::optimize() { + vpOperand[0]->optimize(); + if (ExpressionConstant* ec = dynamic_cast(vpOperand[0].get())) { + const Value valueArray = ec->getValue(); + uassert(50749, + str::stream() << "First operand of $indexOfArray must be an array. First " + << "argument is of type: " + << typeName(valueArray.getType()), + valueArray.isArray()); + + auto arr = valueArray.getArray(); + + ValueUnorderedMap indexMap = + getExpressionContext()->getValueComparator().makeUnorderedValueMap(); + + for (int i = 0; i < int(arr.size()); i++) { + auto pair = std::make_pair(arr[i], i); + indexMap.insert(pair); + } + intrusive_ptr optimizedWithConstant( + new Optimized(getExpressionContext(), indexMap, vpOperand)); + return optimizedWithConstant; + } + return this; } REGISTER_EXPRESSION(indexOfArray, ExpressionIndexOfArray::parse); diff --git a/src/mongo/db/pipeline/expression.h b/src/mongo/db/pipeline/expression.h index 5b4a0286b7c03..e131d8d5c5c86 100644 --- a/src/mongo/db/pipeline/expression.h +++ b/src/mongo/db/pipeline/expression.h @@ -728,6 +728,7 @@ class ExpressionArray final : public ExpressionVariadic { Value evaluate(const Document& root) const final; Value serialize(bool explain) const final; + boost::intrusive_ptr optimize() final; const char* getOpName() const final; }; @@ -1218,13 +1219,19 @@ class ExpressionIn final : public ExpressionFixedArity { }; -class ExpressionIndexOfArray final : public ExpressionRangedArity { +class ExpressionIndexOfArray : public ExpressionRangedArity { public: explicit ExpressionIndexOfArray(const boost::intrusive_ptr& expCtx) : ExpressionRangedArity(expCtx) {} - Value evaluate(const Document& root) const final; + Value evaluate(const Document& root) const ; + std::vector parseDeps(const Document& root, + const ExpressionVector& operands, + size_t arrayLength) const; + boost::intrusive_ptr optimize() ; const char* getOpName() const final; +private: + class Optimized; }; From 83dbf349d04a7d07f1ffc0c978cb85062ab87df5 Mon Sep 17 00:00:00 2001 From: KevinCybura Date: Wed, 21 Mar 2018 09:42:13 -0400 Subject: [PATCH 02/12] optimize 03/20/18: added comments --- src/mongo/db/pipeline/expression.cpp | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/src/mongo/db/pipeline/expression.cpp b/src/mongo/db/pipeline/expression.cpp index b447786b4f8fa..f634bc2aef2e1 100644 --- a/src/mongo/db/pipeline/expression.cpp +++ b/src/mongo/db/pipeline/expression.cpp @@ -2766,7 +2766,11 @@ vector ExpressionIndexOfArray::parseDeps(const Document& root, deps.push_back(Value(endIndex)); return deps; } - +/** + * This class handles the case where IndexOfArray is given an ExpressionConstant + * instead of using a vector and searching through it we can use a unordered_map + * for O(1) lookup time. + **/ class ExpressionIndexOfArray::Optimized : public ExpressionIndexOfArray { public: Optimized(const boost::intrusive_ptr& expCtx, @@ -2793,6 +2797,7 @@ class ExpressionIndexOfArray::Optimized : public ExpressionIndexOfArray { intrusive_ptr ExpressionIndexOfArray::optimize() { vpOperand[0]->optimize(); + // If the input arr is an ExpressionConstant we can optimize using a unordered_map instead of a vector. if (ExpressionConstant* ec = dynamic_cast(vpOperand[0].get())) { const Value valueArray = ec->getValue(); uassert(50749, From c24332562899b0c7f0fbd45e354d2f6bf844b734 Mon Sep 17 00:00:00 2001 From: KevinCybura Date: Thu, 22 Mar 2018 19:00:32 -0400 Subject: [PATCH 03/12] optimize: completed with test case added --- src/mongo/db/pipeline/expression.cpp | 19 ++++++++--------- src/mongo/db/pipeline/expression.h | 2 +- src/mongo/db/pipeline/expression_test.cpp | 26 +++++++++++++++++++++++ 3 files changed, 36 insertions(+), 11 deletions(-) diff --git a/src/mongo/db/pipeline/expression.cpp b/src/mongo/db/pipeline/expression.cpp index f634bc2aef2e1..e0172ad740efc 100644 --- a/src/mongo/db/pipeline/expression.cpp +++ b/src/mongo/db/pipeline/expression.cpp @@ -26,7 +26,6 @@ * it in the license file. */ - #include "mongo/platform/basic.h" #include "mongo/db/pipeline/expression.h" @@ -484,7 +483,7 @@ Value ExpressionArray::serialize(bool explain) const { intrusive_ptr ExpressionArray::optimize() { bool allValuesConstant = true; - for (auto& expr : vpOperand) { + for (auto&& expr : vpOperand) { expr = expr->optimize(); if (!dynamic_cast(expr.get())) { allValuesConstant = false; @@ -2731,7 +2730,6 @@ Value ExpressionIndexOfArray::evaluate(const Document& root) const { arrayArg.isArray()); std::vector array = arrayArg.getArray(); - std::vector operands = parseDeps(root, vpOperand, array.size()); for (int i = operands[1].getInt(); i < operands[2].getInt(); i++) { if (getExpressionContext()->getValueComparator().evaluate(array[i] == operands[0])) { @@ -2796,14 +2794,16 @@ class ExpressionIndexOfArray::Optimized : public ExpressionIndexOfArray { }; intrusive_ptr ExpressionIndexOfArray::optimize() { - vpOperand[0]->optimize(); - // If the input arr is an ExpressionConstant we can optimize using a unordered_map instead of a vector. - if (ExpressionConstant* ec = dynamic_cast(vpOperand[0].get())) { + ExpressionNary::optimize(); + + // If the input arr is an ExpressionConstant we can optimize using a unordered_map instead of a + if (dynamic_cast(vpOperand[0].get())) { + ExpressionConstant* ec = dynamic_cast(vpOperand[0].get()); const Value valueArray = ec->getValue(); uassert(50749, str::stream() << "First operand of $indexOfArray must be an array. First " - << "argument is of type: " - << typeName(valueArray.getType()), + << "argument is of type: " + << typeName(valueArray.getType()), valueArray.isArray()); auto arr = valueArray.getArray(); @@ -2815,9 +2815,8 @@ intrusive_ptr ExpressionIndexOfArray::optimize() { auto pair = std::make_pair(arr[i], i); indexMap.insert(pair); } - intrusive_ptr optimizedWithConstant( + return intrusive_ptr( new Optimized(getExpressionContext(), indexMap, vpOperand)); - return optimizedWithConstant; } return this; } diff --git a/src/mongo/db/pipeline/expression.h b/src/mongo/db/pipeline/expression.h index e131d8d5c5c86..ad9166a3c1187 100644 --- a/src/mongo/db/pipeline/expression.h +++ b/src/mongo/db/pipeline/expression.h @@ -1228,7 +1228,7 @@ class ExpressionIndexOfArray : public ExpressionRangedArity parseDeps(const Document& root, const ExpressionVector& operands, size_t arrayLength) const; - boost::intrusive_ptr optimize() ; + boost::intrusive_ptr optimize() override; const char* getOpName() const final; private: class Optimized; diff --git a/src/mongo/db/pipeline/expression_test.cpp b/src/mongo/db/pipeline/expression_test.cpp index 6e0ab87fcc327..4677c1c814e55 100644 --- a/src/mongo/db/pipeline/expression_test.cpp +++ b/src/mongo/db/pipeline/expression_test.cpp @@ -2208,6 +2208,32 @@ TEST(ExpressionPowTest, NegativeOneRaisedToNegativeOddExponentShouldOutPutNegati }); } +TEST(ExpressionArrayTest, ExpressionArrayWithALlConstantValuesShouldOptimizeToExpressionConstant) { + intrusive_ptr expCtx(new ExpressionContextForTest()); + VariablesParseState vps = expCtx->variablesParseState; + BSONObj bsonarrayOfConstants = BSON("" << BSON_ARRAY(1 << 2 << 3 << 4)); + BSONElement elementArray = bsonarrayOfConstants.firstElement(); + auto expressionArr = ExpressionArray::parse(expCtx, elementArray, vps); + auto optimizedToConstant = expressionArr->optimize(); + auto exprConstant = dynamic_cast(optimizedToConstant.get()); + ASSERT_TRUE(exprConstant); + + BSONObj bsonarray = BSON("" << BSON_ARRAY(1 << "$x" << 3 << 4)); + BSONElement elementArrayNotConstant = bsonarray.firstElement(); + auto expressionArrNotConstant = ExpressionArray::parse(expCtx, elementArrayNotConstant, vps); + auto notOptimized = expressionArrNotConstant->optimize(); + auto notExprConstant = dynamic_cast(notOptimized.get()); + ASSERT_FALSE(notExprConstant); + + BSONObj bsonarrayWithAdd = + BSON("" << BSON_ARRAY(1 << BSON("$add" << BSON_ARRAY(1 << 1)) << 3 << 4)); + BSONElement elementArrayWithAdd = bsonarrayWithAdd.firstElement(); + auto expressionArrWithAdd = ExpressionArray::parse(expCtx, elementArrayWithAdd, vps); + auto optimizedToConstantWithAdd = expressionArrWithAdd->optimize(); + auto constantWithAdd = dynamic_cast(optimizedToConstantWithAdd.get()); + ASSERT_TRUE(constantWithAdd); +} + namespace FieldPath { /** The provided field path does not pass validation. */ From 4559172f2ba75acb64bb2d6bdebfa866650c2296 Mon Sep 17 00:00:00 2001 From: KevinCybura Date: Thu, 22 Mar 2018 19:36:55 -0400 Subject: [PATCH 04/12] 03/22/18: added more comments --- src/mongo/db/pipeline/expression.cpp | 8 ++++++-- src/mongo/db/pipeline/expression.h | 4 ++-- src/mongo/db/pipeline/expression_test.cpp | 19 +++++++++++++------ 3 files changed, 21 insertions(+), 10 deletions(-) diff --git a/src/mongo/db/pipeline/expression.cpp b/src/mongo/db/pipeline/expression.cpp index e0172ad740efc..4b0df093a905f 100644 --- a/src/mongo/db/pipeline/expression.cpp +++ b/src/mongo/db/pipeline/expression.cpp @@ -489,7 +489,7 @@ intrusive_ptr ExpressionArray::optimize() { allValuesConstant = false; } } - // If all values in ExpressionObject are constant evaluate to ExpressionConstant. + // If all values in ExpressionArray are constant evaluate to ExpressionConstant. if (allValuesConstant) { return ExpressionConstant::create(getExpressionContext(), evaluate(Document())); } @@ -2785,6 +2785,7 @@ class ExpressionIndexOfArray::Optimized : public ExpressionIndexOfArray { index->second < operands[2].getInt()) { return Value(index->second); } else { + // If the item we are searching is not found or if not found in given range return -1. return Value(-1); } } @@ -2796,10 +2797,13 @@ class ExpressionIndexOfArray::Optimized : public ExpressionIndexOfArray { intrusive_ptr ExpressionIndexOfArray::optimize() { ExpressionNary::optimize(); - // If the input arr is an ExpressionConstant we can optimize using a unordered_map instead of a + // If the input arr is an ExpressionConstant we can optimize using a unordered_map instead of an Array if (dynamic_cast(vpOperand[0].get())) { ExpressionConstant* ec = dynamic_cast(vpOperand[0].get()); const Value valueArray = ec->getValue(); + if (valueArray.nullish()) { + return this; + } uassert(50749, str::stream() << "First operand of $indexOfArray must be an array. First " << "argument is of type: " diff --git a/src/mongo/db/pipeline/expression.h b/src/mongo/db/pipeline/expression.h index ad9166a3c1187..3134921e66205 100644 --- a/src/mongo/db/pipeline/expression.h +++ b/src/mongo/db/pipeline/expression.h @@ -1228,7 +1228,7 @@ class ExpressionIndexOfArray : public ExpressionRangedArity parseDeps(const Document& root, const ExpressionVector& operands, size_t arrayLength) const; - boost::intrusive_ptr optimize() override; + boost::intrusive_ptr optimize() final; const char* getOpName() const final; private: class Optimized; @@ -2020,4 +2020,4 @@ class ExpressionConvert final : public Expression { boost::intrusive_ptr _onError; boost::intrusive_ptr _onNull; }; -} +} \ No newline at end of file diff --git a/src/mongo/db/pipeline/expression_test.cpp b/src/mongo/db/pipeline/expression_test.cpp index 4677c1c814e55..12ad41bc4a1dc 100644 --- a/src/mongo/db/pipeline/expression_test.cpp +++ b/src/mongo/db/pipeline/expression_test.cpp @@ -2211,6 +2211,8 @@ TEST(ExpressionPowTest, NegativeOneRaisedToNegativeOddExponentShouldOutPutNegati TEST(ExpressionArrayTest, ExpressionArrayWithALlConstantValuesShouldOptimizeToExpressionConstant) { intrusive_ptr expCtx(new ExpressionContextForTest()); VariablesParseState vps = expCtx->variablesParseState; + + // ExpressionArray of constant values should optimize to ExpressionConsant. BSONObj bsonarrayOfConstants = BSON("" << BSON_ARRAY(1 << 2 << 3 << 4)); BSONElement elementArray = bsonarrayOfConstants.firstElement(); auto expressionArr = ExpressionArray::parse(expCtx, elementArray, vps); @@ -2218,20 +2220,25 @@ TEST(ExpressionArrayTest, ExpressionArrayWithALlConstantValuesShouldOptimizeToEx auto exprConstant = dynamic_cast(optimizedToConstant.get()); ASSERT_TRUE(exprConstant); + // ExpressionArray with not all constant values should not optimize to ExpressionConstant. BSONObj bsonarray = BSON("" << BSON_ARRAY(1 << "$x" << 3 << 4)); BSONElement elementArrayNotConstant = bsonarray.firstElement(); auto expressionArrNotConstant = ExpressionArray::parse(expCtx, elementArrayNotConstant, vps); auto notOptimized = expressionArrNotConstant->optimize(); auto notExprConstant = dynamic_cast(notOptimized.get()); ASSERT_FALSE(notExprConstant); +} - BSONObj bsonarrayWithAdd = +TEST(ExpressionArrayTest, ExpressionArrayShouldOptimizeSubExpressionToExpressionConstant) { + // ExpressionArray with constant values and sub expression that evaluates to constant should + // optimize to Expression constant. + BSONObj bsonarrayWithSubExpression = BSON("" << BSON_ARRAY(1 << BSON("$add" << BSON_ARRAY(1 << 1)) << 3 << 4)); - BSONElement elementArrayWithAdd = bsonarrayWithAdd.firstElement(); - auto expressionArrWithAdd = ExpressionArray::parse(expCtx, elementArrayWithAdd, vps); - auto optimizedToConstantWithAdd = expressionArrWithAdd->optimize(); - auto constantWithAdd = dynamic_cast(optimizedToConstantWithAdd.get()); - ASSERT_TRUE(constantWithAdd); + BSONElement elementArrayWithSubExpression = bsonarrayWithSubExpression.firstElement(); + auto expressionArrWithSubExpression = ExpressionArray::parse(expCtx, elementArrayWithSubExpression, vps); + auto optimizedToConstantWithSubExpression = expressionArrWithSubExpression->optimize(); + auto constantExpression = dynamic_cast(optimizedToConstantWithSubExpression.get()); + ASSERT_TRUE(constantExpression); } namespace FieldPath { From 3eb410df55cab98146dd6723b312f890c48825b4 Mon Sep 17 00:00:00 2001 From: KevinCybura Date: Fri, 23 Mar 2018 15:28:50 -0400 Subject: [PATCH 05/12] optimize 03/23/18: final changes --- src/mongo/db/pipeline/expression.cpp | 9 ++++++--- src/mongo/db/pipeline/expression_test.cpp | 10 ++++++++-- 2 files changed, 14 insertions(+), 5 deletions(-) diff --git a/src/mongo/db/pipeline/expression.cpp b/src/mongo/db/pipeline/expression.cpp index 4b0df093a905f..a6c975de868f4 100644 --- a/src/mongo/db/pipeline/expression.cpp +++ b/src/mongo/db/pipeline/expression.cpp @@ -2781,8 +2781,10 @@ class ExpressionIndexOfArray::Optimized : public ExpressionIndexOfArray { virtual Value evaluate(const Document& root) const { std::vector operands = parseDeps(root, vpOperand, _indexMap.size()); auto index = _indexMap.find(operands[0]); - if (index != _indexMap.end() && index->second > operands[1].getInt() && - index->second < operands[2].getInt()) { + auto comparator = getExpressionContext()->getValueComparator(); + Value vIndex = Value(index->second); + if (index != _indexMap.end() && comparator.evaluate(Value(index->second) >= operands[1]) && + comparator.evaluate(Value(index->second) < operands[2])) { return Value(index->second); } else { // If the item we are searching is not found or if not found in given range return -1. @@ -2797,7 +2799,8 @@ class ExpressionIndexOfArray::Optimized : public ExpressionIndexOfArray { intrusive_ptr ExpressionIndexOfArray::optimize() { ExpressionNary::optimize(); - // If the input arr is an ExpressionConstant we can optimize using a unordered_map instead of an Array + // If the input arr is an ExpressionConstant we can optimize using a unordered_map instead of an + // Array if (dynamic_cast(vpOperand[0].get())) { ExpressionConstant* ec = dynamic_cast(vpOperand[0].get()); const Value valueArray = ec->getValue(); diff --git a/src/mongo/db/pipeline/expression_test.cpp b/src/mongo/db/pipeline/expression_test.cpp index 12ad41bc4a1dc..5b3a878b4c133 100644 --- a/src/mongo/db/pipeline/expression_test.cpp +++ b/src/mongo/db/pipeline/expression_test.cpp @@ -2230,14 +2230,20 @@ TEST(ExpressionArrayTest, ExpressionArrayWithALlConstantValuesShouldOptimizeToEx } TEST(ExpressionArrayTest, ExpressionArrayShouldOptimizeSubExpressionToExpressionConstant) { + intrusive_ptr expCtx(new ExpressionContextForTest()); + VariablesParseState vps = expCtx->variablesParseState; + + // ExpressionArray with constant values and sub expression that evaluates to constant should // optimize to Expression constant. BSONObj bsonarrayWithSubExpression = BSON("" << BSON_ARRAY(1 << BSON("$add" << BSON_ARRAY(1 << 1)) << 3 << 4)); BSONElement elementArrayWithSubExpression = bsonarrayWithSubExpression.firstElement(); - auto expressionArrWithSubExpression = ExpressionArray::parse(expCtx, elementArrayWithSubExpression, vps); + auto expressionArrWithSubExpression = + ExpressionArray::parse(expCtx, elementArrayWithSubExpression, vps); auto optimizedToConstantWithSubExpression = expressionArrWithSubExpression->optimize(); - auto constantExpression = dynamic_cast(optimizedToConstantWithSubExpression.get()); + auto constantExpression = + dynamic_cast(optimizedToConstantWithSubExpression.get()); ASSERT_TRUE(constantExpression); } From ee42c4b02fc88e5062199f4fd83e512ad4a78da4 Mon Sep 17 00:00:00 2001 From: KevinCybura Date: Thu, 5 Apr 2018 12:45:47 -0400 Subject: [PATCH 06/12] optimize 04/05/2018: made requested changes --- src/mongo/db/pipeline/expression.cpp | 30 ++++++++--------- src/mongo/db/pipeline/expression.h | 16 ++++++--- src/mongo/db/pipeline/expression_test.cpp | 41 ++++++++++++++++++++++- 3 files changed, 66 insertions(+), 21 deletions(-) diff --git a/src/mongo/db/pipeline/expression.cpp b/src/mongo/db/pipeline/expression.cpp index a6c975de868f4..29afe754f1306 100644 --- a/src/mongo/db/pipeline/expression.cpp +++ b/src/mongo/db/pipeline/expression.cpp @@ -2730,7 +2730,7 @@ Value ExpressionIndexOfArray::evaluate(const Document& root) const { arrayArg.isArray()); std::vector array = arrayArg.getArray(); - std::vector operands = parseDeps(root, vpOperand, array.size()); + std::vector operands = evaluateAndValidateArguments(root, vpOperand, array.size()); for (int i = operands[1].getInt(); i < operands[2].getInt(); i++) { if (getExpressionContext()->getValueComparator().evaluate(array[i] == operands[0])) { return Value(static_cast(i)); @@ -2740,7 +2740,7 @@ Value ExpressionIndexOfArray::evaluate(const Document& root) const { return Value(-1); } -vector ExpressionIndexOfArray::parseDeps(const Document& root, +vector ExpressionIndexOfArray::evaluateAndValidateArguments(const Document& root, const ExpressionVector& operands, size_t arrayLength) const { std::vector deps; @@ -2765,10 +2765,10 @@ vector ExpressionIndexOfArray::parseDeps(const Document& root, return deps; } /** - * This class handles the case where IndexOfArray is given an ExpressionConstant - * instead of using a vector and searching through it we can use a unordered_map - * for O(1) lookup time. - **/ + * This class handles the case where IndexOfArray is given an ExpressionConstant + * instead of using a vector and searching through it we can use a unordered_map + * for O(1) lookup time. + */ class ExpressionIndexOfArray::Optimized : public ExpressionIndexOfArray { public: Optimized(const boost::intrusive_ptr& expCtx, @@ -2779,7 +2779,7 @@ class ExpressionIndexOfArray::Optimized : public ExpressionIndexOfArray { } virtual Value evaluate(const Document& root) const { - std::vector operands = parseDeps(root, vpOperand, _indexMap.size()); + std::vector operands = evaluateAndValidateArguments(root, vpOperand, _indexMap.size()); auto index = _indexMap.find(operands[0]); auto comparator = getExpressionContext()->getValueComparator(); Value vIndex = Value(index->second); @@ -2797,15 +2797,15 @@ class ExpressionIndexOfArray::Optimized : public ExpressionIndexOfArray { }; intrusive_ptr ExpressionIndexOfArray::optimize() { + // This is optimize all arguments to this expression ExpressionNary::optimize(); - // If the input arr is an ExpressionConstant we can optimize using a unordered_map instead of an - // Array - if (dynamic_cast(vpOperand[0].get())) { - ExpressionConstant* ec = dynamic_cast(vpOperand[0].get()); - const Value valueArray = ec->getValue(); + // If the input array is an ExpressionConstant we can optimize using a unordered_map instead of an + // array + if (auto constantArray = dynamic_cast(vpOperand[0].get())) { + const Value valueArray = constantArray->getValue(); if (valueArray.nullish()) { - return this; + return ExpressionConstant::create(getExpressionContext(), Value(BSONNULL)); } uassert(50749, str::stream() << "First operand of $indexOfArray must be an array. First " @@ -2819,8 +2819,8 @@ intrusive_ptr ExpressionIndexOfArray::optimize() { getExpressionContext()->getValueComparator().makeUnorderedValueMap(); for (int i = 0; i < int(arr.size()); i++) { - auto pair = std::make_pair(arr[i], i); - indexMap.insert(pair); + // auto pair = std::make_pair(arr[i], i); + indexMap.emplace(arr[i], i); } return intrusive_ptr( new Optimized(getExpressionContext(), indexMap, vpOperand)); diff --git a/src/mongo/db/pipeline/expression.h b/src/mongo/db/pipeline/expression.h index 3134921e66205..6343b976bf801 100644 --- a/src/mongo/db/pipeline/expression.h +++ b/src/mongo/db/pipeline/expression.h @@ -1219,17 +1219,23 @@ class ExpressionIn final : public ExpressionFixedArity { }; -class ExpressionIndexOfArray : public ExpressionRangedArity { +class ExpressionIndexOfArray : public ExpressionRangedArity { public: explicit ExpressionIndexOfArray(const boost::intrusive_ptr& expCtx) : ExpressionRangedArity(expCtx) {} - Value evaluate(const Document& root) const ; - std::vector parseDeps(const Document& root, - const ExpressionVector& operands, - size_t arrayLength) const; + Value evaluate(const Document& root) const; boost::intrusive_ptr optimize() final; const char* getOpName() const final; + +protected: + // Evaluates and validates the value that is being searched for and if given the start index and + // the end index passed to ExpressionIndexOfArray returns arguments in the form of a + // vector + std::vector evaluateAndValidateArguments(const Document& root, + const ExpressionVector& operands, + size_t arrayLength) const; + private: class Optimized; }; diff --git a/src/mongo/db/pipeline/expression_test.cpp b/src/mongo/db/pipeline/expression_test.cpp index 5b3a878b4c133..4f70e65a44e08 100644 --- a/src/mongo/db/pipeline/expression_test.cpp +++ b/src/mongo/db/pipeline/expression_test.cpp @@ -2208,7 +2208,7 @@ TEST(ExpressionPowTest, NegativeOneRaisedToNegativeOddExponentShouldOutPutNegati }); } -TEST(ExpressionArrayTest, ExpressionArrayWithALlConstantValuesShouldOptimizeToExpressionConstant) { +TEST(ExpressionArrayTest, ExpressionArrayWithAllConstantValuesShouldOptimizeToExpressionConstant) { intrusive_ptr expCtx(new ExpressionContextForTest()); VariablesParseState vps = expCtx->variablesParseState; @@ -2247,6 +2247,45 @@ TEST(ExpressionArrayTest, ExpressionArrayShouldOptimizeSubExpressionToExpression ASSERT_TRUE(constantExpression); } +TEST(ExpressionIndexOfArray, ExpressionIndexOfArrayShouldOptimizeArguments) { + intrusive_ptr expCtx(new ExpressionContextForTest()); + + auto expIndexOfArray = Expression::parseExpression( + expCtx, + BSON("$indexOfArray" << BSON_ARRAY(BSON_ARRAY(BSON("$add" << BSON_ARRAY(1 << 1)) << 1 << 1) + // Start index + << BSON("$add" << BSON_ARRAY(1 << 1)) + // End index + << BSON("$add" << BSON_ARRAY(1 << 1)))), + expCtx->variablesParseState); + auto argsOptimizedToConstants = expIndexOfArray->optimize(); + auto shouldBeNary = dynamic_cast(argsOptimizedToConstants.get()); + ASSERT_TRUE(shouldBeNary); + + auto optimizedArgs = shouldBeNary->getOperandList(); + // All arguments should be ExpressionConstants. + ASSERT_TRUE(dynamic_cast(optimizedArgs[0].get())); + ASSERT_TRUE(dynamic_cast(optimizedArgs[1].get())); + ASSERT_TRUE(dynamic_cast(optimizedArgs[2].get())); +} + +TEST(ExpressionIndexOfArray, ExpressionIndexOfArrayShouldOptimizeNullishInputArrayToExpressionConstant) { + intrusive_ptr expCtx(new ExpressionContextForTest()); + VariablesParseState vps = expCtx->variablesParseState; + + auto expIndex = Expression::parseExpression( + expCtx, fromjson("{ $indexOfArray : [ undefined , 1, 1]}"), expCtx->variablesParseState); + + auto isExpIndexOfArray = dynamic_cast(expIndex.get()); + ASSERT_TRUE(isExpIndexOfArray); + + auto nullishValueOptimizedToExpConstant = isExpIndexOfArray->optimize(); + auto shouldBeExpressionConstant = dynamic_cast(nullishValueOptimizedToExpConstant.get()); + ASSERT_TRUE(shouldBeExpressionConstant); + // Nullish input array should become a Value(BSONNULL). + ASSERT_VALUE_EQ(Value(BSONNULL), shouldBeExpressionConstant->getValue()); +} + namespace FieldPath { /** The provided field path does not pass validation. */ From 9ae96cc79051a80e05df358a9b75184627a43f2f Mon Sep 17 00:00:00 2001 From: Kevin Cybura Date: Thu, 5 Apr 2018 12:48:26 -0400 Subject: [PATCH 07/12] Update expression.cpp --- src/mongo/db/pipeline/expression.cpp | 1 + 1 file changed, 1 insertion(+) diff --git a/src/mongo/db/pipeline/expression.cpp b/src/mongo/db/pipeline/expression.cpp index 29afe754f1306..9e09c7f35ce9d 100644 --- a/src/mongo/db/pipeline/expression.cpp +++ b/src/mongo/db/pipeline/expression.cpp @@ -26,6 +26,7 @@ * it in the license file. */ + #include "mongo/platform/basic.h" #include "mongo/db/pipeline/expression.h" From c7f9765f2d556983a48c1d9e28e97f57e815b048 Mon Sep 17 00:00:00 2001 From: Kevin Cybura Date: Thu, 5 Apr 2018 12:49:33 -0400 Subject: [PATCH 08/12] Update expression.cpp --- 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 9e09c7f35ce9d..50da7f17c150c 100644 --- a/src/mongo/db/pipeline/expression.cpp +++ b/src/mongo/db/pipeline/expression.cpp @@ -2798,7 +2798,7 @@ class ExpressionIndexOfArray::Optimized : public ExpressionIndexOfArray { }; intrusive_ptr ExpressionIndexOfArray::optimize() { - // This is optimize all arguments to this expression + // This is optimizes all arguments to this expression ExpressionNary::optimize(); // If the input array is an ExpressionConstant we can optimize using a unordered_map instead of an From 93fa412cbbc993096c04a958c0425f9b235e6456 Mon Sep 17 00:00:00 2001 From: KevinCybura Date: Fri, 6 Apr 2018 11:49:06 -0400 Subject: [PATCH 09/12] optimize 04/06/2018: Further Changes made --- src/mongo/db/pipeline/expression.cpp | 5 +- src/mongo/db/pipeline/expression.h | 10 ++-- src/mongo/db/pipeline/expression_test.cpp | 58 +++++++++++++++++++++-- 3 files changed, 62 insertions(+), 11 deletions(-) diff --git a/src/mongo/db/pipeline/expression.cpp b/src/mongo/db/pipeline/expression.cpp index 29afe754f1306..064c5999d1326 100644 --- a/src/mongo/db/pipeline/expression.cpp +++ b/src/mongo/db/pipeline/expression.cpp @@ -2797,11 +2797,11 @@ class ExpressionIndexOfArray::Optimized : public ExpressionIndexOfArray { }; intrusive_ptr ExpressionIndexOfArray::optimize() { - // This is optimize all arguments to this expression + // This is optimize all arguments to this expression. ExpressionNary::optimize(); // If the input array is an ExpressionConstant we can optimize using a unordered_map instead of an - // array + // array. if (auto constantArray = dynamic_cast(vpOperand[0].get())) { const Value valueArray = constantArray->getValue(); if (valueArray.nullish()) { @@ -2819,7 +2819,6 @@ intrusive_ptr ExpressionIndexOfArray::optimize() { getExpressionContext()->getValueComparator().makeUnorderedValueMap(); for (int i = 0; i < int(arr.size()); i++) { - // auto pair = std::make_pair(arr[i], i); indexMap.emplace(arr[i], i); } return intrusive_ptr( diff --git a/src/mongo/db/pipeline/expression.h b/src/mongo/db/pipeline/expression.h index 6343b976bf801..80f6a015969e9 100644 --- a/src/mongo/db/pipeline/expression.h +++ b/src/mongo/db/pipeline/expression.h @@ -1229,9 +1229,13 @@ class ExpressionIndexOfArray : public ExpressionRangedArity + /** + * When given 'operands' which correspond to the arguments to $indexOfArray, evaluates and + * validates the target value, starting index, and ending index arguments and returns their + * values in that order. The starting index and ending index are optional, so the returned + * vector will have a length between 1 and 3. Throws a UserException if the values are found to + * be invalid in some way, e.g. if the indexes are not numbers. + */ std::vector evaluateAndValidateArguments(const Document& root, const ExpressionVector& operands, size_t arrayLength) const; diff --git a/src/mongo/db/pipeline/expression_test.cpp b/src/mongo/db/pipeline/expression_test.cpp index 4f70e65a44e08..c785c2ec17891 100644 --- a/src/mongo/db/pipeline/expression_test.cpp +++ b/src/mongo/db/pipeline/expression_test.cpp @@ -2253,20 +2253,23 @@ TEST(ExpressionIndexOfArray, ExpressionIndexOfArrayShouldOptimizeArguments) { auto expIndexOfArray = Expression::parseExpression( expCtx, BSON("$indexOfArray" << BSON_ARRAY(BSON_ARRAY(BSON("$add" << BSON_ARRAY(1 << 1)) << 1 << 1) - // Start index + // Value we are searching for. << BSON("$add" << BSON_ARRAY(1 << 1)) - // End index + // Start index. + << BSON("$add" << BSON_ARRAY(1 << 1)) + // End index. << BSON("$add" << BSON_ARRAY(1 << 1)))), expCtx->variablesParseState); auto argsOptimizedToConstants = expIndexOfArray->optimize(); - auto shouldBeNary = dynamic_cast(argsOptimizedToConstants.get()); - ASSERT_TRUE(shouldBeNary); + auto shouldBeIndexOfArray = dynamic_cast(argsOptimizedToConstants.get()); + ASSERT_TRUE(shouldBeIndexOfArray); - auto optimizedArgs = shouldBeNary->getOperandList(); + auto optimizedArgs = shouldBeIndexOfArray->getOperandList(); // All arguments should be ExpressionConstants. ASSERT_TRUE(dynamic_cast(optimizedArgs[0].get())); ASSERT_TRUE(dynamic_cast(optimizedArgs[1].get())); ASSERT_TRUE(dynamic_cast(optimizedArgs[2].get())); + ASSERT_TRUE(dynamic_cast(optimizedArgs[3].get())); } TEST(ExpressionIndexOfArray, ExpressionIndexOfArrayShouldOptimizeNullishInputArrayToExpressionConstant) { @@ -2286,6 +2289,51 @@ TEST(ExpressionIndexOfArray, ExpressionIndexOfArrayShouldOptimizeNullishInputArr ASSERT_VALUE_EQ(Value(BSONNULL), shouldBeExpressionConstant->getValue()); } +TEST(ExpressionIndexOfArray, + OptimizedExpressionIndexOfArrayWithConstantArgumentsShouldEvaluateProperly) { + + intrusive_ptr expCtx(new ExpressionContextForTest()); + + auto expIndexOfArray = + Expression::parseExpression(expCtx, + // Search for 2. + fromjson("{ $indexOfArray : [ [0, 1, 2, 3, 4, 5] , 2] }"), + expCtx->variablesParseState); + auto optimizedIndexOfArray = expIndexOfArray->optimize(); + ASSERT_VALUE_EQ(Value(2), optimizedIndexOfArray->evaluate(Document())); + + auto IndexNotFound = + Expression::parseExpression(expCtx, + // Search for 10. + fromjson("{ $indexOfArray : [ [0, 1, 2, 3, 4, 5] , 10] }"), + expCtx->variablesParseState); + auto optimizedIndexNotFound = IndexNotFound->optimize(); + // Should evaluate to -1 if not found. + ASSERT_VALUE_EQ(Value(-1), optimizedIndexNotFound->evaluate(Document())); +} + +TEST(ExpressionIndexOfArray, + OptimizedExpressionIndexOfArrayWithConstantArgumentsShouldEvaluateProperlyWithRange) { + intrusive_ptr expCtx(new ExpressionContextForTest()); + + auto expIndexOfArray = + Expression::parseExpression(expCtx, + // Search for 4 between 3(inclusive) and 5 (exclusive). + fromjson("{ $indexOfArray : [ [0, 1, 2, 3, 4, 5] , 4, 3, 5] }"), + expCtx->variablesParseState); + auto optimizedIndexOfArray = expIndexOfArray->optimize(); + ASSERT_VALUE_EQ(Value(4), optimizedIndexOfArray->evaluate(Document())); + + auto IndexNotFoundInRange = + Expression::parseExpression(expCtx, + // Search for 0 between 3(inclusive) and 5 (exclusive). + fromjson("{ $indexOfArray : [ [0, 1, 2, 3, 4, 5] , 0, 3, 5] }"), + expCtx->variablesParseState); + auto optimizedIndexNotFoundInRange = IndexNotFoundInRange->optimize(); + // Should evaluate to -1 if not found in range. + ASSERT_VALUE_EQ(Value(-1), optimizedIndexNotFoundInRange->evaluate(Document())); +} + namespace FieldPath { /** The provided field path does not pass validation. */ From 2bcf423b3d9931f65f1d22c81189ed2dba19f482 Mon Sep 17 00:00:00 2001 From: KevinCybura Date: Mon, 16 Apr 2018 11:11:37 -0400 Subject: [PATCH 10/12] optimize 04/16/2018: addtional changes requested --- src/mongo/db/pipeline/expression.cpp | 39 +++++---- src/mongo/db/pipeline/expression.h | 14 +++- src/mongo/db/pipeline/expression_test.cpp | 97 ++++++++++++++--------- 3 files changed, 89 insertions(+), 61 deletions(-) diff --git a/src/mongo/db/pipeline/expression.cpp b/src/mongo/db/pipeline/expression.cpp index 14b91fa226521..e64b6c4b2e29e 100644 --- a/src/mongo/db/pipeline/expression.cpp +++ b/src/mongo/db/pipeline/expression.cpp @@ -2731,9 +2731,9 @@ Value ExpressionIndexOfArray::evaluate(const Document& root) const { arrayArg.isArray()); std::vector array = arrayArg.getArray(); - std::vector operands = evaluateAndValidateArguments(root, vpOperand, array.size()); - for (int i = operands[1].getInt(); i < operands[2].getInt(); i++) { - if (getExpressionContext()->getValueComparator().evaluate(array[i] == operands[0])) { + auto args = evaluateAndValidateArguments(root, vpOperand, array.size()); + for (int i = args.startIndex; i < args.endIndex; i++) { + if (getExpressionContext()->getValueComparator().evaluate(array[i] == args.targetOfSearch)) { return Value(static_cast(i)); } } @@ -2741,19 +2741,17 @@ Value ExpressionIndexOfArray::evaluate(const Document& root) const { return Value(-1); } -vector ExpressionIndexOfArray::evaluateAndValidateArguments(const Document& root, +ExpressionIndexOfArray::Arguments ExpressionIndexOfArray::evaluateAndValidateArguments(const Document& root, const ExpressionVector& operands, size_t arrayLength) const { - std::vector deps; - deps.push_back(operands[1]->evaluate(root)); - + int startIndex = 0; if (operands.size() > 2) { Value startIndexArg = operands[2]->evaluate(root); uassertIfNotIntegralAndNonNegative(startIndexArg, getOpName(), "starting index"); startIndex = static_cast(startIndexArg.coerceToInt()); } - deps.push_back(Value(startIndex)); + int endIndex = arrayLength; if (operands.size() > 3) { @@ -2762,8 +2760,8 @@ vector ExpressionIndexOfArray::evaluateAndValidateArguments(const Documen // Don't let 'endIndex' exceed the length of the array. endIndex = std::min(arrayLength, static_cast(endIndexArg.coerceToInt())); } - deps.push_back(Value(endIndex)); - return deps; + + return ExpressionIndexOfArray::Arguments(vpOperand[1]->evaluate(root), startIndex, endIndex); } /** * This class handles the case where IndexOfArray is given an ExpressionConstant @@ -2780,12 +2778,10 @@ class ExpressionIndexOfArray::Optimized : public ExpressionIndexOfArray { } virtual Value evaluate(const Document& root) const { - std::vector operands = evaluateAndValidateArguments(root, vpOperand, _indexMap.size()); - auto index = _indexMap.find(operands[0]); - auto comparator = getExpressionContext()->getValueComparator(); - Value vIndex = Value(index->second); - if (index != _indexMap.end() && comparator.evaluate(Value(index->second) >= operands[1]) && - comparator.evaluate(Value(index->second) < operands[2])) { + auto args = evaluateAndValidateArguments(root, vpOperand, _indexMap.size()); + auto index = _indexMap.find(args.targetOfSearch); + if (index != _indexMap.end() && index->second >= args.startIndex && + index->second < args.endIndex) { return Value(index->second); } else { // If the item we are searching is not found or if not found in given range return -1. @@ -2798,9 +2794,11 @@ class ExpressionIndexOfArray::Optimized : public ExpressionIndexOfArray { }; intrusive_ptr ExpressionIndexOfArray::optimize() { - // This is optimize all arguments to this expression. - ExpressionNary::optimize(); - + // This will optimize all arguments to this expression. + auto optimized = ExpressionNary::optimize(); + if(optimized.get() != this){ + return optimized; + } // If the input array is an ExpressionConstant we can optimize using a unordered_map instead of an // array. if (auto constantArray = dynamic_cast(vpOperand[0].get())) { @@ -2820,7 +2818,8 @@ intrusive_ptr ExpressionIndexOfArray::optimize() { getExpressionContext()->getValueComparator().makeUnorderedValueMap(); for (int i = 0; i < int(arr.size()); i++) { - indexMap.emplace(arr[i], i); + if(indexMap.find(arr[i]) == indexMap.end()) + indexMap.emplace(arr[i], i); } return intrusive_ptr( new Optimized(getExpressionContext(), indexMap, vpOperand)); diff --git a/src/mongo/db/pipeline/expression.h b/src/mongo/db/pipeline/expression.h index 80f6a015969e9..70b81a03f7b13 100644 --- a/src/mongo/db/pipeline/expression.h +++ b/src/mongo/db/pipeline/expression.h @@ -1236,9 +1236,17 @@ class ExpressionIndexOfArray : public ExpressionRangedArity evaluateAndValidateArguments(const Document& root, - const ExpressionVector& operands, - size_t arrayLength) const; + struct Arguments { + Arguments(Value targetOfSearch, int startIndex, int endIndex) + : targetOfSearch(targetOfSearch), startIndex(startIndex), endIndex(endIndex) {} + + Value targetOfSearch; + int startIndex; + int endIndex; + }; + Arguments evaluateAndValidateArguments(const Document& root, + const ExpressionVector& operands, + size_t arrayLength) const; private: class Optimized; diff --git a/src/mongo/db/pipeline/expression_test.cpp b/src/mongo/db/pipeline/expression_test.cpp index c785c2ec17891..297c58cbe7336 100644 --- a/src/mongo/db/pipeline/expression_test.cpp +++ b/src/mongo/db/pipeline/expression_test.cpp @@ -2208,7 +2208,7 @@ TEST(ExpressionPowTest, NegativeOneRaisedToNegativeOddExponentShouldOutPutNegati }); } -TEST(ExpressionArrayTest, ExpressionArrayWithAllConstantValuesShouldOptimizeToExpressionConstant) { +TEST(ExpressionArray, ExpressionArrayWithAllConstantValuesShouldOptimizeToExpressionConstant) { intrusive_ptr expCtx(new ExpressionContextForTest()); VariablesParseState vps = expCtx->variablesParseState; @@ -2229,7 +2229,7 @@ TEST(ExpressionArrayTest, ExpressionArrayWithAllConstantValuesShouldOptimizeToEx ASSERT_FALSE(notExprConstant); } -TEST(ExpressionArrayTest, ExpressionArrayShouldOptimizeSubExpressionToExpressionConstant) { +TEST(ExpressionArray, ExpressionArrayShouldOptimizeSubExpressionToExpressionConstant) { intrusive_ptr expCtx(new ExpressionContextForTest()); VariablesParseState vps = expCtx->variablesParseState; @@ -2251,28 +2251,24 @@ TEST(ExpressionIndexOfArray, ExpressionIndexOfArrayShouldOptimizeArguments) { intrusive_ptr expCtx(new ExpressionContextForTest()); auto expIndexOfArray = Expression::parseExpression( - expCtx, - BSON("$indexOfArray" << BSON_ARRAY(BSON_ARRAY(BSON("$add" << BSON_ARRAY(1 << 1)) << 1 << 1) - // Value we are searching for. - << BSON("$add" << BSON_ARRAY(1 << 1)) - // Start index. - << BSON("$add" << BSON_ARRAY(1 << 1)) - // End index. - << BSON("$add" << BSON_ARRAY(1 << 1)))), + expCtx, // 2, 1, 1 + BSON("$indexOfArray" << BSON_ARRAY( + BSON_ARRAY(BSON("$add" << BSON_ARRAY(1 << 1)) << 1 << 1 << 2) + // Value we are searching for = 2. + << BSON("$add" << BSON_ARRAY(1 << 1)) + // Start index = 1. + << BSON("$add" << BSON_ARRAY(0 << 1)) + // End index = 4. + << BSON("$add" << BSON_ARRAY(1 << 3)))), expCtx->variablesParseState); auto argsOptimizedToConstants = expIndexOfArray->optimize(); - auto shouldBeIndexOfArray = dynamic_cast(argsOptimizedToConstants.get()); + auto shouldBeIndexOfArray = dynamic_cast(argsOptimizedToConstants.get()); ASSERT_TRUE(shouldBeIndexOfArray); - - auto optimizedArgs = shouldBeIndexOfArray->getOperandList(); - // All arguments should be ExpressionConstants. - ASSERT_TRUE(dynamic_cast(optimizedArgs[0].get())); - ASSERT_TRUE(dynamic_cast(optimizedArgs[1].get())); - ASSERT_TRUE(dynamic_cast(optimizedArgs[2].get())); - ASSERT_TRUE(dynamic_cast(optimizedArgs[3].get())); + ASSERT_VALUE_EQ(Value(3), shouldBeIndexOfArray->getValue()); } -TEST(ExpressionIndexOfArray, ExpressionIndexOfArrayShouldOptimizeNullishInputArrayToExpressionConstant) { +TEST(ExpressionIndexOfArray, + ExpressionIndexOfArrayShouldOptimizeNullishInputArrayToExpressionConstant) { intrusive_ptr expCtx(new ExpressionContextForTest()); VariablesParseState vps = expCtx->variablesParseState; @@ -2283,7 +2279,8 @@ TEST(ExpressionIndexOfArray, ExpressionIndexOfArrayShouldOptimizeNullishInputArr ASSERT_TRUE(isExpIndexOfArray); auto nullishValueOptimizedToExpConstant = isExpIndexOfArray->optimize(); - auto shouldBeExpressionConstant = dynamic_cast(nullishValueOptimizedToExpConstant.get()); + auto shouldBeExpressionConstant = + dynamic_cast(nullishValueOptimizedToExpConstant.get()); ASSERT_TRUE(shouldBeExpressionConstant); // Nullish input array should become a Value(BSONNULL). ASSERT_VALUE_EQ(Value(BSONNULL), shouldBeExpressionConstant->getValue()); @@ -2297,43 +2294,67 @@ TEST(ExpressionIndexOfArray, auto expIndexOfArray = Expression::parseExpression(expCtx, // Search for 2. - fromjson("{ $indexOfArray : [ [0, 1, 2, 3, 4, 5] , 2] }"), + fromjson("{ $indexOfArray : [ [0, 1, 2, 3, 4, 5] , '$x'] }"), expCtx->variablesParseState); auto optimizedIndexOfArray = expIndexOfArray->optimize(); - ASSERT_VALUE_EQ(Value(2), optimizedIndexOfArray->evaluate(Document())); - + ASSERT_VALUE_EQ(Value(1), optimizedIndexOfArray->evaluate(Document{{"x", 1}})); auto IndexNotFound = Expression::parseExpression(expCtx, - // Search for 10. - fromjson("{ $indexOfArray : [ [0, 1, 2, 3, 4, 5] , 10] }"), + // Search for 'x'. + fromjson("{ $indexOfArray : [ [0, 1, 2, 3, 4, 5] , '$x'] }"), expCtx->variablesParseState); auto optimizedIndexNotFound = IndexNotFound->optimize(); // Should evaluate to -1 if not found. - ASSERT_VALUE_EQ(Value(-1), optimizedIndexNotFound->evaluate(Document())); + ASSERT_VALUE_EQ(Value(-1), optimizedIndexNotFound->evaluate(Document{{"x", 10}})); } TEST(ExpressionIndexOfArray, OptimizedExpressionIndexOfArrayWithConstantArgumentsShouldEvaluateProperlyWithRange) { intrusive_ptr expCtx(new ExpressionContextForTest()); - auto expIndexOfArray = - Expression::parseExpression(expCtx, - // Search for 4 between 3(inclusive) and 5 (exclusive). - fromjson("{ $indexOfArray : [ [0, 1, 2, 3, 4, 5] , 4, 3, 5] }"), - expCtx->variablesParseState); + auto expIndexOfArray = Expression::parseExpression( + expCtx, + // Search for 4 between 3 and 5. + fromjson("{ $indexOfArray : [ [0, 1, 2, 3, 4, 5] , '$x', 3, 5] }"), + expCtx->variablesParseState); auto optimizedIndexOfArray = expIndexOfArray->optimize(); - ASSERT_VALUE_EQ(Value(4), optimizedIndexOfArray->evaluate(Document())); + ASSERT_VALUE_EQ(Value(4), optimizedIndexOfArray->evaluate(Document{{"x", 4}})); - auto IndexNotFoundInRange = - Expression::parseExpression(expCtx, - // Search for 0 between 3(inclusive) and 5 (exclusive). - fromjson("{ $indexOfArray : [ [0, 1, 2, 3, 4, 5] , 0, 3, 5] }"), - expCtx->variablesParseState); + auto IndexNotFoundInRange = Expression::parseExpression( + expCtx, + // Search for 0 between 3 and 5. + fromjson("{ $indexOfArray : [ [0, 1, 2, 3, 4, 5] , '$x', 3, 5] }"), + expCtx->variablesParseState); auto optimizedIndexNotFoundInRange = IndexNotFoundInRange->optimize(); // Should evaluate to -1 if not found in range. - ASSERT_VALUE_EQ(Value(-1), optimizedIndexNotFoundInRange->evaluate(Document())); + ASSERT_VALUE_EQ(Value(-1), optimizedIndexNotFoundInRange->evaluate(Document{{"x", 0}})); +} + +TEST(ExpressionIndexOfArray, + OptimizedExpressionIndexOfArrayWithConstantArrayShouldEvaluateProperlyWithDuplicateValues) { + intrusive_ptr expCtx(new ExpressionContextForTest()); + + auto expIndexOfArrayWithDuplicateValues = + Expression::parseExpression(expCtx, + // Search for 4 between 3 and 5. + fromjson("{ $indexOfArray : [ [0, 1, 2, 2, 3, 4, 5] , '$x'] }"), + expCtx->variablesParseState); + auto optimizedIndexOfArrayWithDuplicateValues = expIndexOfArrayWithDuplicateValues->optimize(); + ASSERT_VALUE_EQ(Value(2), + optimizedIndexOfArrayWithDuplicateValues->evaluate(Document{{"x", 2}})); + + auto expIndexInRangeWithhDuplicateValues = Expression::parseExpression( + expCtx, + // Search for 2 between 2 and 4 + fromjson("{ $indexOfArray : [ [0, 1, 2, 2, 4, 5] , '$x', 2, 4] }"), + expCtx->variablesParseState); + auto optimizedIndexInRangeWithDuplcateValues = expIndexInRangeWithhDuplicateValues->optimize(); + // Should evaluate to 2 + ASSERT_VALUE_EQ(Value(2), + optimizedIndexInRangeWithDuplcateValues->evaluate(Document{{"x", 2}})); } + namespace FieldPath { /** The provided field path does not pass validation. */ From a2e5a1d83003ebc628b789dabfd3383161315e52 Mon Sep 17 00:00:00 2001 From: KevinCybura Date: Tue, 17 Apr 2018 12:53:02 -0400 Subject: [PATCH 11/12] optimize indexOfArray: 04/17/2018 bugfix changed unorderedmap to unorderedmap and added additional comments --- src/mongo/db/pipeline/expression.cpp | 36 +++++++++++++++-------- src/mongo/db/pipeline/expression.h | 15 +++++----- src/mongo/db/pipeline/expression_test.cpp | 8 ++--- 3 files changed, 35 insertions(+), 24 deletions(-) diff --git a/src/mongo/db/pipeline/expression.cpp b/src/mongo/db/pipeline/expression.cpp index e64b6c4b2e29e..231a05ecfa603 100644 --- a/src/mongo/db/pipeline/expression.cpp +++ b/src/mongo/db/pipeline/expression.cpp @@ -2760,7 +2760,7 @@ ExpressionIndexOfArray::Arguments ExpressionIndexOfArray::evaluateAndValidateArg // Don't let 'endIndex' exceed the length of the array. endIndex = std::min(arrayLength, static_cast(endIndexArg.coerceToInt())); } - + return ExpressionIndexOfArray::Arguments(vpOperand[1]->evaluate(root), startIndex, endIndex); } /** @@ -2771,7 +2771,7 @@ ExpressionIndexOfArray::Arguments ExpressionIndexOfArray::evaluateAndValidateArg class ExpressionIndexOfArray::Optimized : public ExpressionIndexOfArray { public: Optimized(const boost::intrusive_ptr& expCtx, - const ValueUnorderedMap& indexMap, + const ValueUnorderedMap>& indexMap, const ExpressionVector& operands) : ExpressionIndexOfArray(expCtx), _indexMap(std::move(indexMap)) { vpOperand = operands; @@ -2779,18 +2779,23 @@ class ExpressionIndexOfArray::Optimized : public ExpressionIndexOfArray { virtual Value evaluate(const Document& root) const { auto args = evaluateAndValidateArguments(root, vpOperand, _indexMap.size()); - auto index = _indexMap.find(args.targetOfSearch); - if (index != _indexMap.end() && index->second >= args.startIndex && - index->second < args.endIndex) { - return Value(index->second); + auto indexVec = _indexMap.find(args.targetOfSearch); + if (indexVec != _indexMap.end()) { + // Search through the vector of indecies for first index in our range. + for (auto index : indexVec->second) { + if (index >= args.startIndex && index < args.endIndex) { + return Value(index); + } + } + // the value we are searching for exists but is not in our range. + return Value(-1); } else { - // If the item we are searching is not found or if not found in given range return -1. return Value(-1); } } private: - const ValueUnorderedMap _indexMap; + const ValueUnorderedMap> _indexMap; }; intrusive_ptr ExpressionIndexOfArray::optimize() { @@ -2813,13 +2818,18 @@ intrusive_ptr ExpressionIndexOfArray::optimize() { valueArray.isArray()); auto arr = valueArray.getArray(); - - ValueUnorderedMap indexMap = - getExpressionContext()->getValueComparator().makeUnorderedValueMap(); + // To handle the case of duplicate values the values need to map to a vector of indecies. + ValueUnorderedMap> indexMap = + getExpressionContext()->getValueComparator().makeUnorderedValueMap>(); for (int i = 0; i < int(arr.size()); i++) { - if(indexMap.find(arr[i]) == indexMap.end()) - indexMap.emplace(arr[i], i); + if(indexMap.find(arr[i]) == indexMap.end()){ + indexMap.emplace(arr[i], vector()); + indexMap[arr[i]].push_back(i); + } + else { + indexMap[arr[i]].push_back(i); + } } return intrusive_ptr( new Optimized(getExpressionContext(), indexMap, vpOperand)); diff --git a/src/mongo/db/pipeline/expression.h b/src/mongo/db/pipeline/expression.h index 70b81a03f7b13..7fb281cc758e9 100644 --- a/src/mongo/db/pipeline/expression.h +++ b/src/mongo/db/pipeline/expression.h @@ -1229,13 +1229,6 @@ class ExpressionIndexOfArray : public ExpressionRangedArityvariablesParseState); auto optimizedIndexInRangeWithDuplcateValues = expIndexInRangeWithhDuplicateValues->optimize(); - // Should evaluate to 2 - ASSERT_VALUE_EQ(Value(2), + // Should evaluate to 4. + ASSERT_VALUE_EQ(Value(4), optimizedIndexInRangeWithDuplcateValues->evaluate(Document{{"x", 2}})); } From 2b419adcb1b6a312b07331411b139f94e3f954ab Mon Sep 17 00:00:00 2001 From: KevinCybura Date: Fri, 20 Apr 2018 13:32:43 -0400 Subject: [PATCH 12/12] optimize 04/20/2018: stylistic changes made --- src/mongo/db/pipeline/expression.cpp | 58 +++++++++++------------ src/mongo/db/pipeline/expression_test.cpp | 39 ++++++++------- 2 files changed, 46 insertions(+), 51 deletions(-) diff --git a/src/mongo/db/pipeline/expression.cpp b/src/mongo/db/pipeline/expression.cpp index 231a05ecfa603..24314e929e77b 100644 --- a/src/mongo/db/pipeline/expression.cpp +++ b/src/mongo/db/pipeline/expression.cpp @@ -2741,28 +2741,26 @@ Value ExpressionIndexOfArray::evaluate(const Document& root) const { return Value(-1); } -ExpressionIndexOfArray::Arguments ExpressionIndexOfArray::evaluateAndValidateArguments(const Document& root, - const ExpressionVector& operands, - size_t arrayLength) const { - +ExpressionIndexOfArray::Arguments ExpressionIndexOfArray::evaluateAndValidateArguments( + const Document& root, const ExpressionVector& operands, size_t arrayLength) const { + int startIndex = 0; if (operands.size() > 2) { Value startIndexArg = operands[2]->evaluate(root); uassertIfNotIntegralAndNonNegative(startIndexArg, getOpName(), "starting index"); - startIndex = static_cast(startIndexArg.coerceToInt()); + startIndex = startIndexArg.coerceToInt(); } - int endIndex = arrayLength; if (operands.size() > 3) { Value endIndexArg = operands[3]->evaluate(root); uassertIfNotIntegralAndNonNegative(endIndexArg, getOpName(), "ending index"); // Don't let 'endIndex' exceed the length of the array. - endIndex = std::min(arrayLength, static_cast(endIndexArg.coerceToInt())); + endIndex = std::min(static_cast(arrayLength), endIndexArg.coerceToInt()); } - - return ExpressionIndexOfArray::Arguments(vpOperand[1]->evaluate(root), startIndex, endIndex); + return {vpOperand[1]->evaluate(root), startIndex, endIndex}; } + /** * This class handles the case where IndexOfArray is given an ExpressionConstant * instead of using a vector and searching through it we can use a unordered_map @@ -2780,21 +2778,23 @@ class ExpressionIndexOfArray::Optimized : public ExpressionIndexOfArray { virtual Value evaluate(const Document& root) const { auto args = evaluateAndValidateArguments(root, vpOperand, _indexMap.size()); auto indexVec = _indexMap.find(args.targetOfSearch); - if (indexVec != _indexMap.end()) { - // Search through the vector of indecies for first index in our range. - for (auto index : indexVec->second) { - if (index >= args.startIndex && index < args.endIndex) { - return Value(index); - } - } - // the value we are searching for exists but is not in our range. - return Value(-1); - } else { + + if (indexVec == _indexMap.end()) return Value(-1); + + // Search through the vector of indecies for first index in our range. + for (auto index : indexVec->second) { + if (index >= args.startIndex && index < args.endIndex) { + return Value(index); + } } + // The value we are searching for exists but is not in our range. + return Value(-1); } private: + // Maps the values in the array to the positions at which they occur. We need to remember the + // positions so that we can verify they are in the appropriate range. const ValueUnorderedMap> _indexMap; }; @@ -2806,33 +2806,29 @@ intrusive_ptr ExpressionIndexOfArray::optimize() { } // If the input array is an ExpressionConstant we can optimize using a unordered_map instead of an // array. - if (auto constantArray = dynamic_cast(vpOperand[0].get())) { + if (auto constantArray = dynamic_cast(vpOperand[0].get())) { const Value valueArray = constantArray->getValue(); if (valueArray.nullish()) { return ExpressionConstant::create(getExpressionContext(), Value(BSONNULL)); } uassert(50749, str::stream() << "First operand of $indexOfArray must be an array. First " - << "argument is of type: " - << typeName(valueArray.getType()), + << "argument is of type: " + << typeName(valueArray.getType()), valueArray.isArray()); auto arr = valueArray.getArray(); // To handle the case of duplicate values the values need to map to a vector of indecies. - ValueUnorderedMap> indexMap = + auto indexMap = getExpressionContext()->getValueComparator().makeUnorderedValueMap>(); for (int i = 0; i < int(arr.size()); i++) { - if(indexMap.find(arr[i]) == indexMap.end()){ - indexMap.emplace(arr[i], vector()); - indexMap[arr[i]].push_back(i); - } - else { - indexMap[arr[i]].push_back(i); + if (indexMap.find(arr[i]) == indexMap.end()) { + indexMap.emplace(arr[i], vector()); } + indexMap[arr[i]].push_back(i); } - return intrusive_ptr( - new Optimized(getExpressionContext(), indexMap, vpOperand)); + return new Optimized(getExpressionContext(), indexMap, vpOperand); } return this; } diff --git a/src/mongo/db/pipeline/expression_test.cpp b/src/mongo/db/pipeline/expression_test.cpp index c44239eaf586f..c629963de2c38 100644 --- a/src/mongo/db/pipeline/expression_test.cpp +++ b/src/mongo/db/pipeline/expression_test.cpp @@ -2291,21 +2291,27 @@ TEST(ExpressionIndexOfArray, intrusive_ptr expCtx(new ExpressionContextForTest()); - auto expIndexOfArray = - Expression::parseExpression(expCtx, - // Search for 2. - fromjson("{ $indexOfArray : [ [0, 1, 2, 3, 4, 5] , '$x'] }"), - expCtx->variablesParseState); + auto expIndexOfArray = Expression::parseExpression( + expCtx, + // Search for $x. + fromjson("{ $indexOfArray : [ [0, 1, 2, 3, 4, 5, 'val'] , '$x'] }"), + expCtx->variablesParseState); auto optimizedIndexOfArray = expIndexOfArray->optimize(); + ASSERT_VALUE_EQ(Value(0), optimizedIndexOfArray->evaluate(Document{{"x", 0}})); ASSERT_VALUE_EQ(Value(1), optimizedIndexOfArray->evaluate(Document{{"x", 1}})); - auto IndexNotFound = - Expression::parseExpression(expCtx, - // Search for 'x'. - fromjson("{ $indexOfArray : [ [0, 1, 2, 3, 4, 5] , '$x'] }"), - expCtx->variablesParseState); - auto optimizedIndexNotFound = IndexNotFound->optimize(); + ASSERT_VALUE_EQ(Value(2), optimizedIndexOfArray->evaluate(Document{{"x", 2}})); + ASSERT_VALUE_EQ(Value(3), optimizedIndexOfArray->evaluate(Document{{"x", 3}})); + ASSERT_VALUE_EQ(Value(4), optimizedIndexOfArray->evaluate(Document{{"x", 4}})); + ASSERT_VALUE_EQ(Value(5), optimizedIndexOfArray->evaluate(Document{{"x", 5}})); + ASSERT_VALUE_EQ(Value(6), optimizedIndexOfArray->evaluate(Document{{"x", string("val")}})); + + auto optimizedIndexNotFound = optimizedIndexOfArray->optimize(); // Should evaluate to -1 if not found. ASSERT_VALUE_EQ(Value(-1), optimizedIndexNotFound->evaluate(Document{{"x", 10}})); + ASSERT_VALUE_EQ(Value(-1), optimizedIndexNotFound->evaluate(Document{{"x", 100}})); + ASSERT_VALUE_EQ(Value(-1), optimizedIndexNotFound->evaluate(Document{{"x", 1000}})); + ASSERT_VALUE_EQ(Value(-1), optimizedIndexNotFound->evaluate(Document{{"x", string("string")}})); + ASSERT_VALUE_EQ(Value(-1), optimizedIndexNotFound->evaluate(Document{{"x", -1}})); } TEST(ExpressionIndexOfArray, @@ -2320,14 +2326,8 @@ TEST(ExpressionIndexOfArray, auto optimizedIndexOfArray = expIndexOfArray->optimize(); ASSERT_VALUE_EQ(Value(4), optimizedIndexOfArray->evaluate(Document{{"x", 4}})); - auto IndexNotFoundInRange = Expression::parseExpression( - expCtx, - // Search for 0 between 3 and 5. - fromjson("{ $indexOfArray : [ [0, 1, 2, 3, 4, 5] , '$x', 3, 5] }"), - expCtx->variablesParseState); - auto optimizedIndexNotFoundInRange = IndexNotFoundInRange->optimize(); // Should evaluate to -1 if not found in range. - ASSERT_VALUE_EQ(Value(-1), optimizedIndexNotFoundInRange->evaluate(Document{{"x", 0}})); + ASSERT_VALUE_EQ(Value(-1), optimizedIndexOfArray->evaluate(Document{{"x", 0}})); } TEST(ExpressionIndexOfArray, @@ -2342,7 +2342,7 @@ TEST(ExpressionIndexOfArray, auto optimizedIndexOfArrayWithDuplicateValues = expIndexOfArrayWithDuplicateValues->optimize(); ASSERT_VALUE_EQ(Value(2), optimizedIndexOfArrayWithDuplicateValues->evaluate(Document{{"x", 2}})); - + // Duplicate Values in a range. auto expIndexInRangeWithhDuplicateValues = Expression::parseExpression( expCtx, // Search for 2 between 4 and 6. @@ -2354,7 +2354,6 @@ TEST(ExpressionIndexOfArray, optimizedIndexInRangeWithDuplcateValues->evaluate(Document{{"x", 2}})); } - namespace FieldPath { /** The provided field path does not pass validation. */