SERVER-26751 $indexOfArray can optimize for constant array #1229

KevinCybura · 2018-03-23T19:33:15Z

No description provided.

cswanson310

Looking good! Mostly stylistic comments and a couple suggestions that might help simplify the code a bit.

cswanson310 · 2018-04-03T03:15:31Z

src/mongo/db/pipeline/expression.cpp

 * it in the license file.
 */

-


Please avoid stray whitespace changes. It makes it harder to review and complicates the git blame unnecessarily. Not generally a big deal, but please avoid it going forward.

cswanson310 · 2018-04-03T03:17:56Z

src/mongo/db/pipeline/expression.h

        : ExpressionRangedArity<ExpressionIndexOfArray, 2, 4>(expCtx) {}

-    Value evaluate(const Document& root) const final;
+    Value evaluate(const Document& root) const ;


extra space after 'const'.

cswanson310 · 2018-04-03T03:19:08Z

src/mongo/db/pipeline/expression.h


-    Value evaluate(const Document& root) const final;
+    Value evaluate(const Document& root) const ;
+    std::vector<Value> parseDeps(const Document& root,


Please add a comment here describing what this does. Also - parseDeps doesn't really give me a clue what this method does, so a more descriptive name like 'evaluateAndValidateArguments' might be more appropriate. Finally, if this is only used within IndexOfArray and IndexOfArray::Optimized, can you move it to the private or protected section?

cswanson310 · 2018-04-03T03:20:25Z

src/mongo/db/pipeline/expression_test.cpp

                          });
 }

+TEST(ExpressionArrayTest, ExpressionArrayWithALlConstantValuesShouldOptimizeToExpressionConstant) {


Extra capital in WithALlConstant

cswanson310 · 2018-04-03T03:21:13Z

src/mongo/db/pipeline/expression.cpp

+ *  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.
+ **/


extra * in close, just one here.

cswanson310 · 2018-04-03T03:22:12Z

src/mongo/db/pipeline/expression.cpp

+};
+
+intrusive_ptr<Expression> ExpressionIndexOfArray::optimize() {
+    ExpressionNary::optimize();


Add a comment here indicating that this will optimize the arguments to this expression. In particular, if all arguments are constant, this should immediately evaluate to a constant. Can you add tests for that like you did for ExpressionArray?

cswanson310 · 2018-04-03T03:23:18Z

src/mongo/db/pipeline/expression.cpp

+intrusive_ptr<Expression> ExpressionIndexOfArray::optimize() {
+    ExpressionNary::optimize();
+
+    // If the input arr is an ExpressionConstant we can optimize using a unordered_map instead of an


Don't abbreviate - it just makes it harder to understand, which is counter to the point of the comment. arr -> array.

cswanson310 · 2018-04-03T03:24:24Z

src/mongo/db/pipeline/expression.cpp

+    // If the input arr is an ExpressionConstant we can optimize using a unordered_map instead of an
+    // Array
+    if (dynamic_cast<ExpressionConstant*>(vpOperand[0].get())) {
+        ExpressionConstant* ec = dynamic_cast<ExpressionConstant*>(vpOperand[0].get());


Avoid short variable names like ec. Also, you can combine this with the if statement above:
if (auto constantArray = dynamic_cast<ExpressionConstant*>(vpOperand[0].get())) {

cswanson310 · 2018-04-03T03:26:32Z

src/mongo/db/pipeline/expression.cpp

+        ExpressionConstant* ec = dynamic_cast<ExpressionConstant*>(vpOperand[0].get());
+        const Value valueArray = ec->getValue();
+        if (valueArray.nullish()) {
+            return this;


We know what the answer is in this case, just return ExpressionConstant::create(getExpressionContext(), Value(BSONNULL)) to save time. Also add a test case for this please :)

cswanson310 · 2018-04-03T03:28:28Z

src/mongo/db/pipeline/expression.cpp

+            getExpressionContext()->getValueComparator().makeUnorderedValueMap<int>();
+
+        for (int i = 0; i < int(arr.size()); i++) {
+            auto pair = std::make_pair(arr[i], i);


optional, re-write this as
indexMap.emplace(arr[i], i); or if that doesn't work, indexMap[arr[i]] = i;? I'm curious why you made a pair here.

cswanson310

A couple more points of feedback.

cswanson310 · 2018-04-06T01:36:37Z

src/mongo/db/pipeline/expression.cpp

        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);


Remove dead code.

cswanson310 · 2018-04-06T01:37:19Z

src/mongo/db/pipeline/expression.cpp

-        ExpressionConstant* ec = dynamic_cast<ExpressionConstant*>(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


End comments in periods - also lines 2801 and in the header.

cswanson310 · 2018-04-06T01:37:49Z

src/mongo/db/pipeline/expression.h

+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<Value>


/** * Use this syntax for comments documenting the behavior of a function or method. */

Also - can we re-word this comment to make it more clear what the vector of values will be? How about something like
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.

cswanson310 · 2018-04-06T01:42:23Z

src/mongo/db/pipeline/expression.cpp

-vector<Value> ExpressionIndexOfArray::parseDeps(const Document& root,
+vector<Value> ExpressionIndexOfArray::evaluateAndValidateArguments(const Document& root,
                                                const ExpressionVector& operands,
                                                size_t arrayLength) const {


arrayLength is unused?

cswanson310 · 2018-04-06T01:42:56Z

src/mongo/db/pipeline/expression.cpp

+vector<Value> ExpressionIndexOfArray::evaluateAndValidateArguments(const Document& root,
                                                const ExpressionVector& operands,
                                                size_t arrayLength) const {
    std::vector<Value> deps;


deps is a strange name for this - how about validatedArguments?

cswanson310 · 2018-04-06T01:46:12Z

src/mongo/db/pipeline/expression_test.cpp

+    auto expIndexOfArray = Expression::parseExpression(
+        expCtx,
+        BSON("$indexOfArray" << BSON_ARRAY(BSON_ARRAY(BSON("$add" << BSON_ARRAY(1 << 1)) << 1 << 1)
+                                           // Start index


I don't think this is the start index, I think this is the value you're searching for?

cswanson310 · 2018-04-06T01:47:37Z

src/mongo/db/pipeline/expression_test.cpp

+    ASSERT_TRUE(shouldBeNary);
+
+    auto optimizedArgs = shouldBeNary->getOperandList();
+    // All arguments should be ExpressionConstants.


Oh, interesting - I would have expected it to fully optimize to a constant in this case... I wonder why it didn't? Can you make sure it does?

I think this is because we are optimizing an ExpresssionIndexOfArray not evaluating so it should still be an ExpressionIndexOfArray.

Im casting it to an ExpressionNary because I need to be able to call getOperandList() to determine if the arguments optimized. I think this is where the confusion is to clearify I will cast it to an ExpressionIndexOfArray

cswanson310 · 2018-04-06T01:49:31Z

src/mongo/db/pipeline/expression_test.cpp

+    // Nullish input array should become a Value(BSONNULL).
+    ASSERT_VALUE_EQ(Value(BSONNULL), shouldBeExpressionConstant->getValue());
+}
+


Since we aren't adding any coverage of actually evaluating $indexOfArray when it can be optimized, can you either add some tests here which exercise the new code (create an IndexOfArray with a constant array and various other arguments, making sure to call optimize() before evaluate()), or find some that are already written to prove that it will be exercised?

cswanson310 · 2018-04-12T22:34:42Z

src/mongo/db/pipeline/expression.cpp

+};
+
+intrusive_ptr<Expression> ExpressionIndexOfArray::optimize() {
+    // This is optimize all arguments to this expression.


This will optimize ...

cswanson310 · 2018-04-12T22:36:15Z

src/mongo/db/pipeline/expression.cpp

+            getExpressionContext()->getValueComparator().makeUnorderedValueMap<int>();
+
+        for (int i = 0; i < int(arr.size()); i++) {
+            indexMap.emplace(arr[i], i);


As discussed, we should not add a value if it already occurred so that we only ever return the first occurrence.

cswanson310 · 2018-04-12T22:41:22Z

src/mongo/db/pipeline/expression.cpp

+        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])) {


Oh this is pretty awkward. You have to keep using comparator.evaluate(...) because operands[1] and operands[2] are of type Value. In reality, we know they will be of type int, right? Or even size_t?

Can we change the signature of evaluateAndValidateArguments to return a struct, something like

struct Arguments { Arguments(Value targetOfSearch, int startIndex, boost::optional<int> endIndex) : targetOfSearch(targetOfSearch), startIndex(startIndex), endIndex(endIndex) {} Value targetOfSearch; int startIndex; boost::optional<int> endIndex; };

It might be more complicated in the parsing code, but I think it would make this much easier to read.

Also, how do you know that operands[2] is valid? Couldn't the user have left off the end index?

cswanson310 · 2018-04-12T22:44:25Z

src/mongo/db/pipeline/expression_test.cpp

+    intrusive_ptr<ExpressionContextForTest> expCtx(new ExpressionContextForTest());
+    VariablesParseState vps = expCtx->variablesParseState;
+
+


delete extra newline.

cswanson310 · 2018-04-12T22:47:00Z

src/mongo/db/pipeline/expression_test.cpp

+    ASSERT_TRUE(shouldBeIndexOfArray);
+
+    auto optimizedArgs = shouldBeIndexOfArray->getOperandList();
+    // All arguments should be ExpressionConstants.


I think if you change the optimize implementation to return what ExpressionNary::optimize() returns then the whole expression should evaluate to a constant.

cswanson310 · 2018-04-12T22:48:33Z

src/mongo/db/pipeline/expression_test.cpp

+}
+
+TEST(ExpressionIndexOfArray,
+     OptimizedExpressionIndexOfArrayWithConstantArgumentsShouldEvaluateProperly) {


This is a good test, but because you're using all constant values, I think you will hit the case where all arguments are constant instead of when just the array is constant. If you change the search values (e.g. 2 and 10) to be something like $x, then use Document{{"x", 2}} as the argument to evaluate(), you'll be testing what you want to test.

Same thing for the test below.

cswanson310 · 2018-04-12T23:01:55Z

src/mongo/db/pipeline/expression_test.cpp

+    // Should evaluate to -1 if not found in range.
+    ASSERT_VALUE_EQ(Value(-1), optimizedIndexNotFoundInRange->evaluate(Document()));
+}
+


Can you add a test where there are duplicate values in the array? Such a test should have caught the bug you mentioned with inserting into the unordered_map twice.

cswanson310 · 2018-04-16T21:46:08Z

src/mongo/db/pipeline/expression.cpp

+            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.


Hmm I actually think this won't work. Consider the following:

{$indexOfArray: [[0, 1, "a", 0, 1, 1, "a"], "a", 3, 8]}

I think that the map here will be populated with 0-> 0, 1-> 1, "a" -> 2, but then it has lost the information about where the second or third occurrences of those values are. I think we need to map a value to a vector<int> of all the positions so that we can rule out those which are out of range.

… unorderedmap<vector<int> and added additional comments

cswanson310

This is looking really good! I think all the changes I proposed are more cosmetic, they should do the same thing, but with less code :)

cswanson310 · 2018-04-19T22:20:14Z

src/mongo/db/pipeline/expression.cpp

+        Value startIndexArg = operands[2]->evaluate(root);
        uassertIfNotIntegralAndNonNegative(startIndexArg, getOpName(), "starting index");
-        startIndex = static_cast<size_t>(startIndexArg.coerceToInt());
+        startIndex = static_cast<int>(startIndexArg.coerceToInt());


I don't think you need a static_cast here.

cswanson310 · 2018-04-19T22:20:26Z

src/mongo/db/pipeline/expression.cpp

-        startIndex = static_cast<size_t>(startIndexArg.coerceToInt());
+        startIndex = static_cast<int>(startIndexArg.coerceToInt());
    }
+


extra newline.

cswanson310 · 2018-04-19T22:20:49Z

src/mongo/db/pipeline/expression.cpp

        uassertIfNotIntegralAndNonNegative(endIndexArg, getOpName(), "ending index");
        // Don't let 'endIndex' exceed the length of the array.
-        endIndex = std::min(array.size(), static_cast<size_t>(endIndexArg.coerceToInt()));
+        endIndex = std::min(arrayLength, static_cast<size_t>(endIndexArg.coerceToInt()));


I don't think you need a static_cast here.

cswanson310 · 2018-04-19T22:22:08Z

src/mongo/db/pipeline/expression.cpp

-    for (size_t i = startIndex; i < endIndex; i++) {
-        if (getExpressionContext()->getValueComparator().evaluate(array[i] == searchItem)) {
-            return Value(static_cast<int>(i));
+    return ExpressionIndexOfArray::Arguments(vpOperand[1]->evaluate(root), startIndex, endIndex);


I think the complier knows the return types, so you could just say
return {vpOperand[1]->evaluate(root), startIndex, endIndex}

Or, if you prefer to keep the name, this method is defined on ExpressionIndexOfArray, so you don't need to include the ExpressionIndexOfArray:: prefix here.

cswanson310 · 2018-04-19T22:23:40Z

src/mongo/db/pipeline/expression.cpp

+    virtual Value evaluate(const Document& root) const {
+        auto args = evaluateAndValidateArguments(root, vpOperand, _indexMap.size());
+        auto indexVec = _indexMap.find(args.targetOfSearch);
+        if (indexVec != _indexMap.end()) {


I think this would be easier to read as

if (indexVec == _indexMap.end()) { return Value(-1); } // Search through the vector of indices for the first index in our range. ... // Un-indented code.

Alternatively, you could just leave off the else, and move the return Value(-1); to always be the last thing that happens, whether you get into the if statement or not.

cswanson310 · 2018-04-19T22:34:43Z

src/mongo/db/pipeline/expression_test.cpp

+                                    fromjson("{ $indexOfArray : [ [0, 1, 2, 3, 4, 5] , '$x'] }"),
+                                    expCtx->variablesParseState);
+    auto optimizedIndexOfArray = expIndexOfArray->optimize();
+    ASSERT_VALUE_EQ(Value(1), optimizedIndexOfArray->evaluate(Document{{"x", 1}}));


Might as well add a bunch of these:

ASSERT_VALUE_EQ(Value(0), optimizedIndexOfArray->evaluate(Document{{"x", 0}})); ASSERT_VALUE_EQ(Value(1), optimizedIndexOfArray->evaluate(Document{{"x", 1}})); 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}}));

cswanson310 · 2018-04-19T22:35:05Z

src/mongo/db/pipeline/expression_test.cpp

+                                    expCtx->variablesParseState);
+    auto optimizedIndexOfArray = expIndexOfArray->optimize();
+    ASSERT_VALUE_EQ(Value(1), optimizedIndexOfArray->evaluate(Document{{"x", 1}}));
+    auto IndexNotFound =


I don't think you need to re-parse or optimize, you can use the same one.

cswanson310 · 2018-04-19T22:35:26Z

src/mongo/db/pipeline/expression_test.cpp

+    auto optimizedIndexOfArray = expIndexOfArray->optimize();
+    ASSERT_VALUE_EQ(Value(4), optimizedIndexOfArray->evaluate(Document{{"x", 4}}));
+
+    auto IndexNotFoundInRange = Expression::parseExpression(


I think you can re-use optimizedIndexOfArray

cswanson310 · 2018-04-19T22:35:53Z

src/mongo/db/pipeline/expression_test.cpp

+    ASSERT_VALUE_EQ(Value(2),
+                    optimizedIndexOfArrayWithDuplicateValues->evaluate(Document{{"x", 2}}));
+
+    auto expIndexInRangeWithhDuplicateValues = Expression::parseExpression(


Again, I think you can re-use the expression from above.

cswanson310 · 2018-04-19T22:36:34Z

src/mongo/db/pipeline/expression_test.cpp

+                    optimizedIndexInRangeWithDuplcateValues->evaluate(Document{{"x", 2}}));
+}
+
+


Extra newline.

KevinCybura added 5 commits March 20, 2018 14:17

optimization 03/19/18: finished class

a5f85e1

optimize 03/20/18: added comments

83dbf34

optimize: completed with test case added

c243325

03/22/18: added more comments

4559172

optimize 03/23/18: final changes

3eb410d

cswanson310 self-requested a review March 23, 2018 19:42

cswanson310 self-assigned this Mar 23, 2018

cswanson310 requested changes Apr 3, 2018

View reviewed changes

KevinCybura and others added 3 commits April 5, 2018 12:45

optimize 04/05/2018: made requested changes

ee42c4b

Update expression.cpp

9ae96cc

Update expression.cpp

c7f9765

cswanson310 reviewed Apr 6, 2018

View reviewed changes

KevinCybura added 2 commits April 6, 2018 11:49

optimize 04/06/2018: Further Changes made

93fa412

$indexOfArray optimize 04/06/2018: further changes made

ac11799

cswanson310 requested changes Apr 12, 2018

View reviewed changes

optimize 04/16/2018: addtional changes requested

2bcf423

cswanson310 reviewed Apr 16, 2018

View reviewed changes

optimize indexOfArray: 04/17/2018 bugfix changed unorderedmap<int> to…

a2e5a1d

… unorderedmap<vector<int> and added additional comments

cswanson310 requested changes Apr 19, 2018

View reviewed changes

optimize 04/20/2018: stylistic changes made

2b419ad

cswanson310 closed this in ae01d46 Apr 26, 2018

		intrusive_ptr<ExpressionContextForTest> expCtx(new ExpressionContextForTest());
		VariablesParseState vps = expCtx->variablesParseState;

		optimizedIndexInRangeWithDuplcateValues->evaluate(Document{{"x", 2}}));
		}

SERVER-26751 $indexOfArray can optimize for constant array #1229

SERVER-26751 $indexOfArray can optimize for constant array #1229

Uh oh!

Conversation

KevinCybura commented Mar 23, 2018

Uh oh!

cswanson310 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cswanson310 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cswanson310 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment