From 47810e76e1fa2c654fcc5ea90ec643de5b4e7a91 Mon Sep 17 00:00:00 2001 From: James Date: Thu, 6 Nov 2014 10:10:40 +0000 Subject: [PATCH 01/36] quick hack --- arangod/Aql/ExecutionBlock.cpp | 52 ++++++++++++++++++---------------- arangod/Aql/ExecutionBlock.h | 6 ++-- arangod/Aql/ExecutionNode.h | 4 ++- 3 files changed, 34 insertions(+), 28 deletions(-) diff --git a/arangod/Aql/ExecutionBlock.cpp b/arangod/Aql/ExecutionBlock.cpp index 7b205b703c..2df7e4805f 100644 --- a/arangod/Aql/ExecutionBlock.cpp +++ b/arangod/Aql/ExecutionBlock.cpp @@ -931,13 +931,13 @@ int IndexRangeBlock::initialize () { throw; } } - else { // _allBoundsConstant - readIndex(); - } + //else { // _allBoundsConstant + // readIndex(); + //} return res; } -bool IndexRangeBlock::readIndex () { +bool IndexRangeBlock::readIndex (size_t atMost) { // This is either called from initialize if all bounds are constant, // in this case it is never called again. If there is at least one // variable bound, then readIndex is called once for every item coming @@ -948,11 +948,11 @@ bool IndexRangeBlock::readIndex () { // _pos to evaluate the variable bounds. if (_documents.empty()) { - _documents.reserve(DefaultBatchSize); - } - else { - _documents.clear(); + _documents.reserve(atMost); } + //else { + // _documents.clear(); + //} auto en = static_cast(getPlanNode()); IndexOrCondition const* condition = &en->_ranges; @@ -1055,15 +1055,17 @@ bool IndexRangeBlock::readIndex () { } if (en->_index->type == TRI_IDX_TYPE_PRIMARY_INDEX) { + // atMost not passed since only equality is supported readPrimaryIndex(*condition); } else if (en->_index->type == TRI_IDX_TYPE_HASH_INDEX) { - readHashIndex(*condition); + readHashIndex(*condition, atMost); } else if (en->_index->type == TRI_IDX_TYPE_SKIPLIST_INDEX) { - readSkiplistIndex(*condition); + readSkiplistIndex(*condition, atMost); } else if (en->_index->type == TRI_IDX_TYPE_EDGE_INDEX) { + // atMost not passed since only equality is supported readEdgeIndex(*condition); } else { @@ -1081,9 +1083,9 @@ int IndexRangeBlock::initializeCursor (AqlItemBlock* items, size_t pos) { _pos = 0; _posInDocs = 0; - if (_allBoundsConstant && _documents.size() == 0) { + /*if (_allBoundsConstant && _documents.size() == 0) { _done = true; - } + }*/ return TRI_ERROR_NO_ERROR; } @@ -1114,9 +1116,9 @@ AqlItemBlock* IndexRangeBlock::getSome (size_t, // atLeast _pos = 0; // this is in the first block // This is a new item, so let's read the index if bounds are variable: - if (! _allBoundsConstant) { - readIndex(); - } + //if (! _allBoundsConstant) { + readIndex(atMost); + //} _posInDocs = 0; // position in _documents . . . } @@ -1175,7 +1177,7 @@ AqlItemBlock* IndexRangeBlock::getSome (size_t, // atLeast // let's read the index if bounds are variable: if (! _buffer.empty() && ! _allBoundsConstant) { - readIndex(); + readIndex(atMost); // TODO: add atMost? } // If _buffer is empty, then we will fetch a new block in the next call // and then read the index. @@ -1211,9 +1213,9 @@ size_t IndexRangeBlock::skipSome (size_t atLeast, _pos = 0; // this is in the first block // This is a new item, so let's read the index if bounds are variable: - if (! _allBoundsConstant) { - readIndex(); - } + //if (! _allBoundsConstant) { + readIndex(atMost); // TODO: add atMost? + //} _posInDocs = 0; // position in _documents . . . } @@ -1239,7 +1241,7 @@ size_t IndexRangeBlock::skipSome (size_t atLeast, // let's read the index if bounds are variable: if (! _buffer.empty() && ! _allBoundsConstant) { - readIndex(); + readIndex(atMost); // TODO: add atMost? } _posInDocs = 0; @@ -1319,7 +1321,7 @@ void IndexRangeBlock::readPrimaryIndex (IndexOrCondition const& ranges) { /// @brief read documents using a hash index //////////////////////////////////////////////////////////////////////////////// -void IndexRangeBlock::readHashIndex (IndexOrCondition const& ranges) { +void IndexRangeBlock::readHashIndex (IndexOrCondition const& ranges, size_t atMost) { auto en = static_cast(getPlanNode()); TRI_index_t* idx = en->_index->data; TRI_ASSERT(idx != nullptr); @@ -1341,7 +1343,7 @@ void IndexRangeBlock::readHashIndex (IndexOrCondition const& ranges) { }; auto setupSearchValue = [&]() { - size_t const n = hashIndex->_paths._length; + size_t const n = std::min(hashIndex->_paths._length, atMost); searchValue._length = 0; searchValue._values = static_cast(TRI_Allocate(TRI_CORE_MEM_ZONE, n * sizeof(TRI_shaped_json_t), true)); @@ -1479,7 +1481,7 @@ void IndexRangeBlock::readEdgeIndex (IndexOrCondition const& ranges) { // (i.e. the 1 in x.c >= 1) cannot be lists or arrays. // -void IndexRangeBlock::readSkiplistIndex (IndexOrCondition const& ranges) { +void IndexRangeBlock::readSkiplistIndex (IndexOrCondition const& ranges, size_t atMost) { auto en = static_cast(getPlanNode()); TRI_index_t* idx = en->_index->data; TRI_ASSERT(idx != nullptr); @@ -1557,13 +1559,15 @@ void IndexRangeBlock::readSkiplistIndex (IndexOrCondition const& ranges) { } try { + size_t nrSent = 0; while (true) { TRI_skiplist_index_element_t* indexElement = skiplistIterator->next(skiplistIterator); - if (indexElement == nullptr) { + if (indexElement == nullptr || nrSent == atMost) { break; } _documents.push_back(*(indexElement->_document)); + ++nrSent; ++_engine->_stats.scannedIndex; } TRI_FreeSkiplistIterator(skiplistIterator); diff --git a/arangod/Aql/ExecutionBlock.h b/arangod/Aql/ExecutionBlock.h index 9b83a62bea..1452e9d43d 100644 --- a/arangod/Aql/ExecutionBlock.h +++ b/arangod/Aql/ExecutionBlock.h @@ -566,7 +566,7 @@ namespace triagens { /// @brief continue fetching of documents //////////////////////////////////////////////////////////////////////////////// - bool readIndex (); + bool readIndex (size_t atMost); //////////////////////////////////////////////////////////////////////////////// /// @brief read using the primary index @@ -584,13 +584,13 @@ namespace triagens { /// @brief read using a skiplist index //////////////////////////////////////////////////////////////////////////////// - void readSkiplistIndex (IndexOrCondition const&); + void readSkiplistIndex (IndexOrCondition const&, size_t atMost); //////////////////////////////////////////////////////////////////////////////// /// @brief read using a hash index //////////////////////////////////////////////////////////////////////////////// - void readHashIndex (IndexOrCondition const&); + void readHashIndex (IndexOrCondition const&, size_t atMost); // ----------------------------------------------------------------------------- // --SECTION-- private variables diff --git a/arangod/Aql/ExecutionNode.h b/arangod/Aql/ExecutionNode.h index caae52f4aa..a46072ff63 100644 --- a/arangod/Aql/ExecutionNode.h +++ b/arangod/Aql/ExecutionNode.h @@ -1298,7 +1298,9 @@ namespace triagens { //////////////////////////////////////////////////////////////////////////////// double estimateCost () const override final { - return 1.005 * (std::min)(static_cast(_limit), _dependencies.at(0)->getCost()); +// return 1.005 * (std::min)(static_cast(_limit), _dependencies.at(0)->getCost()); + return 1.005 * static_cast(_limit) + _dependencies.at(0)->getCost(); + //FIXME improve this estimate . . . } From c063b85d29b26cde470610cc83757529fc7a1f84 Mon Sep 17 00:00:00 2001 From: James Date: Thu, 6 Nov 2014 11:42:31 +0000 Subject: [PATCH 02/36] working except for 1 test. --- arangod/Aql/ExecutionBlock.cpp | 30 ++++++++++++++++-------------- 1 file changed, 16 insertions(+), 14 deletions(-) diff --git a/arangod/Aql/ExecutionBlock.cpp b/arangod/Aql/ExecutionBlock.cpp index 2df7e4805f..8001be4441 100644 --- a/arangod/Aql/ExecutionBlock.cpp +++ b/arangod/Aql/ExecutionBlock.cpp @@ -473,8 +473,7 @@ int ExecutionBlock::getOrSkipSome (size_t atLeast, return TRI_ERROR_NO_ERROR; } else { - if (! getBlock(atLeast - skipped, - (std::max)(atMost - skipped, DefaultBatchSize))) { + if (! getBlock(atLeast - skipped, atMost - skipped)) { _done = true; break; // must still put things in the result from the collector . . . } @@ -950,9 +949,9 @@ bool IndexRangeBlock::readIndex (size_t atMost) { if (_documents.empty()) { _documents.reserve(atMost); } - //else { - // _documents.clear(); - //} + else { //FIXME does this trash some stuff unnecessarily? + _documents.clear(); + } auto en = static_cast(getPlanNode()); IndexOrCondition const* condition = &en->_ranges; @@ -1094,12 +1093,13 @@ int IndexRangeBlock::initializeCursor (AqlItemBlock* items, size_t pos) { /// @brief getSome //////////////////////////////////////////////////////////////////////////////// -AqlItemBlock* IndexRangeBlock::getSome (size_t, // atLeast +AqlItemBlock* IndexRangeBlock::getSome (size_t atLeast, size_t atMost) { if (_done) { return nullptr; } + //std::cout << "atMost = " << atMost << "\n"; unique_ptr res(nullptr); do { @@ -1109,7 +1109,7 @@ AqlItemBlock* IndexRangeBlock::getSome (size_t, // atLeast // try again! if (_buffer.empty()) { - if (! ExecutionBlock::getBlock(DefaultBatchSize, DefaultBatchSize)) { + if (! ExecutionBlock::getBlock(atLeast, atMost)) { _done = true; return nullptr; } @@ -1132,7 +1132,8 @@ AqlItemBlock* IndexRangeBlock::getSome (size_t, // atLeast if (toSend > 0) { - res.reset(new AqlItemBlock(toSend, getPlanNode()->getRegisterPlan()->nrRegs[getPlanNode()->getDepth()])); + res.reset(new AqlItemBlock(toSend, + getPlanNode()->getRegisterPlan()->nrRegs[getPlanNode()->getDepth()])); // automatically freed should we throw TRI_ASSERT(curRegs <= res->getNrRegs()); @@ -1141,7 +1142,8 @@ AqlItemBlock* IndexRangeBlock::getSome (size_t, // atLeast inheritRegisters(cur, res.get(), _pos); // set our collection for our output register - res->setDocumentCollection(static_cast(curRegs), _trx->documentCollection(_collection->cid())); + res->setDocumentCollection(static_cast(curRegs), + _trx->documentCollection(_collection->cid())); for (size_t j = 0; j < toSend; j++) { if (j > 0) { @@ -1176,8 +1178,8 @@ AqlItemBlock* IndexRangeBlock::getSome (size_t, // atLeast } // let's read the index if bounds are variable: - if (! _buffer.empty() && ! _allBoundsConstant) { - readIndex(atMost); // TODO: add atMost? + if (! _buffer.empty()) { + readIndex(atMost); } // If _buffer is empty, then we will fetch a new block in the next call // and then read the index. @@ -1206,7 +1208,7 @@ size_t IndexRangeBlock::skipSome (size_t atLeast, while (skipped < atLeast ){ if (_buffer.empty()) { - if (! ExecutionBlock::getBlock(DefaultBatchSize, DefaultBatchSize)) { + if (! ExecutionBlock::getBlock(atLeast, atMost)) { _done = true; return skipped; } @@ -1214,7 +1216,7 @@ size_t IndexRangeBlock::skipSome (size_t atLeast, // This is a new item, so let's read the index if bounds are variable: //if (! _allBoundsConstant) { - readIndex(atMost); // TODO: add atMost? + readIndex(atMost); //} _posInDocs = 0; // position in _documents . . . @@ -1240,7 +1242,7 @@ size_t IndexRangeBlock::skipSome (size_t atLeast, } // let's read the index if bounds are variable: - if (! _buffer.empty() && ! _allBoundsConstant) { + if (! _buffer.empty()) { readIndex(atMost); // TODO: add atMost? } _posInDocs = 0; From c54b92c6e7ec7f90c0a08ef63abbde1646768a4f Mon Sep 17 00:00:00 2001 From: James Date: Thu, 6 Nov 2014 12:24:28 +0000 Subject: [PATCH 03/36] getting tests to work --- arangod/Aql/ExecutionBlock.cpp | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/arangod/Aql/ExecutionBlock.cpp b/arangod/Aql/ExecutionBlock.cpp index 8001be4441..f8d611f5e1 100644 --- a/arangod/Aql/ExecutionBlock.cpp +++ b/arangod/Aql/ExecutionBlock.cpp @@ -473,7 +473,8 @@ int ExecutionBlock::getOrSkipSome (size_t atLeast, return TRI_ERROR_NO_ERROR; } else { - if (! getBlock(atLeast - skipped, atMost - skipped)) { + if (! getBlock(atLeast - skipped, + (std::max)(atMost - skipped, DefaultBatchSize))) { _done = true; break; // must still put things in the result from the collector . . . } @@ -1109,7 +1110,7 @@ AqlItemBlock* IndexRangeBlock::getSome (size_t atLeast, // try again! if (_buffer.empty()) { - if (! ExecutionBlock::getBlock(atLeast, atMost)) { + if (! ExecutionBlock::getBlock(DefaultBatchSize, DefaultBatchSize)) { _done = true; return nullptr; } @@ -1208,7 +1209,7 @@ size_t IndexRangeBlock::skipSome (size_t atLeast, while (skipped < atLeast ){ if (_buffer.empty()) { - if (! ExecutionBlock::getBlock(atLeast, atMost)) { + if (! ExecutionBlock::getBlock(DefaultBatchSize, DefaultBatchSize)) { _done = true; return skipped; } From 6822594bff9b431c3773a0e219c4de94fb295486 Mon Sep 17 00:00:00 2001 From: James Date: Thu, 6 Nov 2014 12:51:32 +0000 Subject: [PATCH 04/36] changed to number of blocks pulled by default by CalculationBlock and ExecutionBlock --- arangod/Aql/ExecutionBlock.cpp | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/arangod/Aql/ExecutionBlock.cpp b/arangod/Aql/ExecutionBlock.cpp index f8d611f5e1..528347e05b 100644 --- a/arangod/Aql/ExecutionBlock.cpp +++ b/arangod/Aql/ExecutionBlock.cpp @@ -473,8 +473,7 @@ int ExecutionBlock::getOrSkipSome (size_t atLeast, return TRI_ERROR_NO_ERROR; } else { - if (! getBlock(atLeast - skipped, - (std::max)(atMost - skipped, DefaultBatchSize))) { + if (! getBlock(atLeast - skipped, atMost - skipped)) { _done = true; break; // must still put things in the result from the collector . . . } @@ -1977,7 +1976,7 @@ AqlItemBlock* CalculationBlock::getSome (size_t atLeast, size_t atMost) { unique_ptr res(ExecutionBlock::getSomeWithoutRegisterClearout( - atLeast, atMost)); + DefaultBatchSize, DefaultBatchSize)); if (res.get() == nullptr) { return nullptr; From b849a6da722dcf75b7cee47d473ee4061e4a291b Mon Sep 17 00:00:00 2001 From: James Date: Sat, 8 Nov 2014 11:36:56 +0000 Subject: [PATCH 05/36] some more cleaning up of replace-OR-with-IN --- arangod/Aql/AstNode.cpp | 6 + arangod/Aql/AstNode.h | 2 + arangod/Aql/OptimizerRules.cpp | 207 ++++++++++++++++----------------- arangod/Aql/OptimizerRules.h | 1 + 4 files changed, 112 insertions(+), 104 deletions(-) diff --git a/arangod/Aql/AstNode.cpp b/arangod/Aql/AstNode.cpp index 2516cc8646..c42231642b 100644 --- a/arangod/Aql/AstNode.cpp +++ b/arangod/Aql/AstNode.cpp @@ -1431,6 +1431,12 @@ void AstNode::stringify (triagens::basics::StringBuffer* buffer, THROW_ARANGO_EXCEPTION_MESSAGE(TRI_ERROR_INTERNAL, message); } +std::string AstNode::toString () const { + triagens::basics::StringBuffer buffer(TRI_UNKNOWN_MEM_ZONE); + stringify(&buffer, false); + return std::string(buffer.c_str(), buffer.length()); +} + // ----------------------------------------------------------------------------- // --SECTION-- private methods // ----------------------------------------------------------------------------- diff --git a/arangod/Aql/AstNode.h b/arangod/Aql/AstNode.h index fc7bf5dce1..26fd13cd36 100644 --- a/arangod/Aql/AstNode.h +++ b/arangod/Aql/AstNode.h @@ -648,6 +648,8 @@ namespace triagens { void stringify (triagens::basics::StringBuffer*, bool) const; + + std::string toString () const; // ----------------------------------------------------------------------------- // --SECTION-- private methods diff --git a/arangod/Aql/OptimizerRules.cpp b/arangod/Aql/OptimizerRules.cpp index 9d6fa5f007..a6eb59f095 100644 --- a/arangod/Aql/OptimizerRules.cpp +++ b/arangod/Aql/OptimizerRules.cpp @@ -2519,18 +2519,96 @@ int triagens::aql::undistributeRemoveAfterEnumColl (Optimizer* opt, /// @brief auxilliary struct for the OR-to-IN conversion //////////////////////////////////////////////////////////////////////////////// -struct OrToInConverter { - AstNode const* variableNode; - std::string variableName; - std::vector valueNodes; +struct CommonNodeFinder { std::vector possibleNodes; - std::vector orConditions; + + bool find (AstNode const* node, + AstNodeType condition, + AstNode const*& commonNode, + std::string& commonName ) { + + if (node->type == NODE_TYPE_OPERATOR_BINARY_OR) { + return (find(node->getMember(0), condition, commonNode, commonName) + && find(node->getMember(1), condition, commonNode, commonName)); + } - std::string getString (AstNode const* node) { - triagens::basics::StringBuffer buffer(TRI_UNKNOWN_MEM_ZONE); - node->stringify(&buffer, false); - return std::string(buffer.c_str(), buffer.length()); + if (node->type == NODE_TYPE_VALUE) { + return true; + } + + if (node->type == condition) { + + auto lhs = node->getMember(0); + auto rhs = node->getMember(1); + + if (lhs->isConstant()) { + commonNode = rhs; + return true; + } + + if (rhs->isConstant()) { + commonNode = lhs; + return true; + } + + if (rhs->type == NODE_TYPE_FCALL || + rhs->type == NODE_TYPE_FCALL_USER || + rhs->type == NODE_TYPE_REFERENCE) { + commonNode = lhs; + return true; + } + + if (lhs->type == NODE_TYPE_FCALL || + lhs->type == NODE_TYPE_FCALL_USER || + lhs->type == NODE_TYPE_REFERENCE) { + commonNode = rhs; + return true; + } + + if (lhs->type == NODE_TYPE_ATTRIBUTE_ACCESS || + lhs->type == NODE_TYPE_INDEXED_ACCESS) { + if (possibleNodes.size() == 2) { + for (size_t i = 0; i < 2; i++) { + if (lhs->toString() == possibleNodes[i]->toString()) { + commonNode = possibleNodes[i]; + commonName = commonNode->toString(); + return true; + } + } + // don't return must consider the other side of the condition + } + else { + possibleNodes.push_back(lhs); + } + } + if (rhs->type == NODE_TYPE_ATTRIBUTE_ACCESS || + rhs->type == NODE_TYPE_INDEXED_ACCESS) { + if (possibleNodes.size() == 2) { + for (size_t i = 0; i < 2; i++) { + if (rhs->toString() == possibleNodes[i]->toString()) { + commonNode = possibleNodes[i]; + commonName = commonNode->toString(); + return true; + } + } + return false; + } + else { + possibleNodes.push_back(rhs); + return true; + } + } + } + return (! commonName.empty()); } +}; + +struct OrToInConverter { + + std::vector valueNodes; + CommonNodeFinder finder; + AstNode const* commonNode = nullptr; + std::string commonName; AstNode* buildInExpression (Ast* ast) { // the list of comparison values @@ -2541,115 +2619,37 @@ struct OrToInConverter { // return a new IN operator node return ast->createNodeBinaryOperator(NODE_TYPE_OPERATOR_BINARY_IN, - variableNode->clone(ast), + commonNode->clone(ast), list); } - bool flattenOr (AstNode const* node) { - if (node->type == NODE_TYPE_OPERATOR_BINARY_OR) { - return (flattenOr(node->getMember(0)) && flattenOr(node->getMember(1))); - } - if (node->type == NODE_TYPE_OPERATOR_BINARY_EQ) { - orConditions.push_back(node); - return true; - } - if (node->type == NODE_TYPE_VALUE) { - return true; - } - return false; - } - - bool findCommonNode (AstNode const* node) { - - if (! flattenOr(node)) { - return false; - } - TRI_ASSERT(orConditions.size() > 1); - - for (AstNode const* n: orConditions) { - - auto lhs = n->getMember(0); - auto rhs = n->getMember(1); - - if (lhs->isConstant()) { - variableNode = rhs; - return true; - } - - if (rhs->isConstant()) { - variableNode = lhs; - return true; - } - - if (rhs->type == NODE_TYPE_FCALL || - rhs->type == NODE_TYPE_FCALL_USER || - rhs->type == NODE_TYPE_REFERENCE) { - variableNode = lhs; - return true; - } - - if (lhs->type == NODE_TYPE_FCALL || - lhs->type == NODE_TYPE_FCALL_USER || - lhs->type == NODE_TYPE_REFERENCE) { - variableNode = rhs; - return true; - } - - if (lhs->type == NODE_TYPE_ATTRIBUTE_ACCESS || - lhs->type == NODE_TYPE_INDEXED_ACCESS) { - if (possibleNodes.size() == 2) { - for (size_t i = 0; i < 2; i++) { - if (getString(lhs) == getString(possibleNodes[i])) { - variableNode = possibleNodes[i]; - variableName = getString(variableNode); - return true; - } - } - } - else { - possibleNodes.push_back(lhs); - } - } - if (rhs->type == NODE_TYPE_ATTRIBUTE_ACCESS || - rhs->type == NODE_TYPE_INDEXED_ACCESS) { - if (possibleNodes.size() == 2) { - for (size_t i = 0; i < 2; i++) { - if (getString(rhs) == getString(possibleNodes[i])) { - variableNode = possibleNodes[i]; - variableName = getString(variableNode); - return true; - } - } - return false; - } - else { - possibleNodes.push_back(rhs); - } - } - } - return false; - } - bool canConvertExpression (AstNode const* node) { + if(finder.find(node, NODE_TYPE_OPERATOR_BINARY_EQ, commonNode, commonName)){ + return canConvertExpressionWalker(node); + } + return false; + } + + bool canConvertExpressionWalker (AstNode const* node) { if (node->type == NODE_TYPE_OPERATOR_BINARY_OR) { - return (canConvertExpression(node->getMember(0)) && - canConvertExpression(node->getMember(1))); + return (canConvertExpressionWalker(node->getMember(0)) && + canConvertExpressionWalker(node->getMember(1))); } if (node->type == NODE_TYPE_OPERATOR_BINARY_EQ) { auto lhs = node->getMember(0); auto rhs = node->getMember(1); - if (canConvertExpression(rhs) && ! canConvertExpression(lhs)) { + if (canConvertExpressionWalker(rhs) && ! canConvertExpressionWalker(lhs)) { valueNodes.push_back(lhs); return true; } - if (canConvertExpression(lhs) && ! canConvertExpression(rhs)) { + if (canConvertExpressionWalker(lhs) && ! canConvertExpressionWalker(rhs)) { valueNodes.push_back(rhs); return true; } - // if canConvertExpression(lhs) and canConvertExpression(rhs), then one of + // if canConvertExpressionWalker(lhs) and canConvertExpressionWalker(rhs), then one of // the equalities in the OR statement is of the form x == x // fall-through intentional } @@ -2657,8 +2657,8 @@ struct OrToInConverter { node->type == NODE_TYPE_ATTRIBUTE_ACCESS || node->type == NODE_TYPE_INDEXED_ACCESS) { // get a string representation of the node for comparisons - std::string nodeString = getString(node); - return nodeString == getString(variableNode); + std::string nodeString = node->toString(); + return nodeString == commonName; } else if (node->isBoolValue()) { return true; } @@ -2705,8 +2705,7 @@ int triagens::aql::replaceOrWithIn (Optimizer* opt, } OrToInConverter converter; - if (converter.findCommonNode(cn->expression()->node()) - && converter.canConvertExpression(cn->expression()->node())) { + if (converter.canConvertExpression(cn->expression()->node())) { Expression* expr = nullptr; ExecutionNode* newNode = nullptr; auto inNode = converter.buildInExpression(plan->getAst()); diff --git a/arangod/Aql/OptimizerRules.h b/arangod/Aql/OptimizerRules.h index 6b56260496..feaadf0f8a 100644 --- a/arangod/Aql/OptimizerRules.h +++ b/arangod/Aql/OptimizerRules.h @@ -180,6 +180,7 @@ namespace triagens { // same (single) attribute. //////////////////////////////////////////////////////////////////////////////// + int replaceOrWithIn (Optimizer*, ExecutionPlan*, Optimizer::Rule const*); } // namespace aql From 53782f536406a61212869a04ef48eb146d16fe37 Mon Sep 17 00:00:00 2001 From: James Date: Sat, 8 Nov 2014 11:39:09 +0000 Subject: [PATCH 06/36] bug fix --- arangod/Aql/OptimizerRules.cpp | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/arangod/Aql/OptimizerRules.cpp b/arangod/Aql/OptimizerRules.cpp index a6eb59f095..13808e98d0 100644 --- a/arangod/Aql/OptimizerRules.cpp +++ b/arangod/Aql/OptimizerRules.cpp @@ -2543,11 +2543,13 @@ struct CommonNodeFinder { if (lhs->isConstant()) { commonNode = rhs; + commonName = commonNode->toString(); return true; } if (rhs->isConstant()) { commonNode = lhs; + commonName = commonNode->toString(); return true; } @@ -2555,6 +2557,7 @@ struct CommonNodeFinder { rhs->type == NODE_TYPE_FCALL_USER || rhs->type == NODE_TYPE_REFERENCE) { commonNode = lhs; + commonName = commonNode->toString(); return true; } @@ -2562,6 +2565,7 @@ struct CommonNodeFinder { lhs->type == NODE_TYPE_FCALL_USER || lhs->type == NODE_TYPE_REFERENCE) { commonNode = rhs; + commonName = commonNode->toString(); return true; } From e9b761f1ca1c11a2aaaf864a179f6fa473d80db3 Mon Sep 17 00:00:00 2001 From: James Date: Sat, 8 Nov 2014 12:17:13 +0000 Subject: [PATCH 07/36] another test for replace-or-with-in --- js/server/tests/aql-optimizer-rule-replace-or-with-in.js | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/js/server/tests/aql-optimizer-rule-replace-or-with-in.js b/js/server/tests/aql-optimizer-rule-replace-or-with-in.js index 769c00c988..c7a18f22ea 100644 --- a/js/server/tests/aql-optimizer-rule-replace-or-with-in.js +++ b/js/server/tests/aql-optimizer-rule-replace-or-with-in.js @@ -389,7 +389,7 @@ function NewAqlReplaceORWithINTestSuite () { }, - testFiresCommonConstant: function () { + testFiresCommonConstant1: function () { var query = "LET x = {a:@a} FOR v IN " + replace.name() + " FILTER x.a == v.value || x.a == v._key RETURN v._key"; @@ -400,6 +400,13 @@ function NewAqlReplaceORWithINTestSuite () { assertEqual(key, actual.toString()); }, + + testFiresCommonConstant2: function () { + var query = "LET x = {a:1} FOR v IN " + replace.name() + + " FILTER x.a == v.value || x.a == v._key RETURN v._key"; + + isRuleUsed(query, {}); + }, testDudAlwaysTrue: function () { var query = From 586cacc2ee3d866a2ef23ee854395e01a2a9127c Mon Sep 17 00:00:00 2001 From: James Date: Sat, 8 Nov 2014 13:45:14 +0000 Subject: [PATCH 08/36] replace-redundant-OR optimizer rule first version. --- arangod/Aql/AstNode.cpp | 6 +- arangod/Aql/AstNode.h | 2 + arangod/Aql/Optimizer.cpp | 6 + arangod/Aql/Optimizer.h | 9 +- arangod/Aql/OptimizerRules.cpp | 196 ++++++++++++++++++++++++++++++++- arangod/Aql/OptimizerRules.h | 3 +- 6 files changed, 212 insertions(+), 10 deletions(-) diff --git a/arangod/Aql/AstNode.cpp b/arangod/Aql/AstNode.cpp index c42231642b..cb2f56c949 100644 --- a/arangod/Aql/AstNode.cpp +++ b/arangod/Aql/AstNode.cpp @@ -166,7 +166,7 @@ static TRI_json_type_e GetNodeCompareType (AstNode const* node) { /// @brief compare two nodes //////////////////////////////////////////////////////////////////////////////// -static int CompareNodes (AstNode const* lhs, AstNode const* rhs) { +int triagens::aql::CompareAstNodes (AstNode const* lhs, AstNode const* rhs) { auto lType = GetNodeCompareType(lhs); auto rType = GetNodeCompareType(rhs); @@ -195,7 +195,7 @@ static int CompareNodes (AstNode const* lhs, AstNode const* rhs) { size_t const n = ((numLhs > numRhs) ? numRhs : numLhs); for (size_t i = 0; i < n; ++i) { - int res = CompareNodes(lhs->getMember(i), rhs->getMember(i)); + int res = triagens::aql::CompareAstNodes(lhs->getMember(i), rhs->getMember(i)); if (res != 0) { return res; } @@ -477,7 +477,7 @@ void AstNode::sort () { auto const l = static_cast(lhs); auto const r = static_cast(rhs); - return (CompareNodes(l, r) < 0); + return (triagens::aql::CompareAstNodes(l, r) < 0); }); setFlag(FLAG_SORTED); diff --git a/arangod/Aql/AstNode.h b/arangod/Aql/AstNode.h index 26fd13cd36..ba40786382 100644 --- a/arangod/Aql/AstNode.h +++ b/arangod/Aql/AstNode.h @@ -651,6 +651,7 @@ namespace triagens { std::string toString () const; + // ----------------------------------------------------------------------------- // --SECTION-- private methods // ----------------------------------------------------------------------------- @@ -707,6 +708,7 @@ namespace triagens { }; + int CompareAstNodes (AstNode const*, AstNode const*); } } diff --git a/arangod/Aql/Optimizer.cpp b/arangod/Aql/Optimizer.cpp index b039a0ca07..496d477f79 100644 --- a/arangod/Aql/Optimizer.cpp +++ b/arangod/Aql/Optimizer.cpp @@ -457,6 +457,12 @@ void Optimizer::setupRules () { replaceOrWithIn_pass6, true); + // try to remove redundant OR + registerRule("remove-redundant-OR", + removeRedundantOR, + removeRedundantOR_pass6, + true); + // try to find a filter after an enumerate collection and find an index . . . registerRule("use-index-range", useIndexRange, diff --git a/arangod/Aql/Optimizer.h b/arangod/Aql/Optimizer.h index 35aeec51a6..04296aa811 100644 --- a/arangod/Aql/Optimizer.h +++ b/arangod/Aql/Optimizer.h @@ -130,14 +130,17 @@ namespace triagens { // replace simple OR conditions with IN replaceOrWithIn_pass6 = 810, + // remove redundant OR conditions + removeRedundantOR_pass6 = 820, + // try to find a filter after an enumerate collection and find an index . . . - useIndexRange_pass6 = 820, + useIndexRange_pass6 = 830, // try to find sort blocks which are superseeded by indexes - useIndexForSort_pass6 = 830, + useIndexForSort_pass6 = 840, // try to remove filters covered by index ranges - removeFiltersCoveredByIndex_pass6 = 840, + removeFiltersCoveredByIndex_pass6 = 850, ////////////////////////////////////////////////////////////////////////////// /// "Pass 10": final transformations for the cluster diff --git a/arangod/Aql/OptimizerRules.cpp b/arangod/Aql/OptimizerRules.cpp index 13808e98d0..673b5a917c 100644 --- a/arangod/Aql/OptimizerRules.cpp +++ b/arangod/Aql/OptimizerRules.cpp @@ -2516,7 +2516,7 @@ int triagens::aql::undistributeRemoveAfterEnumColl (Optimizer* opt, } //////////////////////////////////////////////////////////////////////////////// -/// @brief auxilliary struct for the OR-to-IN conversion +/// @brief auxilliary struct for finding common nodes in OR conditions //////////////////////////////////////////////////////////////////////////////// struct CommonNodeFinder { @@ -2533,6 +2533,7 @@ struct CommonNodeFinder { } if (node->type == NODE_TYPE_VALUE) { + possibleNodes.clear(); return true; } @@ -2544,12 +2545,14 @@ struct CommonNodeFinder { if (lhs->isConstant()) { commonNode = rhs; commonName = commonNode->toString(); + possibleNodes.clear(); return true; } if (rhs->isConstant()) { commonNode = lhs; commonName = commonNode->toString(); + possibleNodes.clear(); return true; } @@ -2558,6 +2561,7 @@ struct CommonNodeFinder { rhs->type == NODE_TYPE_REFERENCE) { commonNode = lhs; commonName = commonNode->toString(); + possibleNodes.clear(); return true; } @@ -2566,6 +2570,7 @@ struct CommonNodeFinder { lhs->type == NODE_TYPE_REFERENCE) { commonNode = rhs; commonName = commonNode->toString(); + possibleNodes.clear(); return true; } @@ -2576,6 +2581,7 @@ struct CommonNodeFinder { if (lhs->toString() == possibleNodes[i]->toString()) { commonNode = possibleNodes[i]; commonName = commonNode->toString(); + possibleNodes.clear(); return true; } } @@ -2592,6 +2598,7 @@ struct CommonNodeFinder { if (rhs->toString() == possibleNodes[i]->toString()) { commonNode = possibleNodes[i]; commonName = commonNode->toString(); + possibleNodes.clear(); return true; } } @@ -2603,10 +2610,15 @@ struct CommonNodeFinder { } } } + possibleNodes.clear(); return (! commonName.empty()); } }; +//////////////////////////////////////////////////////////////////////////////// +/// @brief auxilliary struct for the OR-to-IN conversion +//////////////////////////////////////////////////////////////////////////////// + struct OrToInConverter { std::vector valueNodes; @@ -2661,8 +2673,7 @@ struct OrToInConverter { node->type == NODE_TYPE_ATTRIBUTE_ACCESS || node->type == NODE_TYPE_INDEXED_ACCESS) { // get a string representation of the node for comparisons - std::string nodeString = node->toString(); - return nodeString == commonName; + return (node->toString() == commonName); } else if (node->isBoolValue()) { return true; } @@ -2745,6 +2756,185 @@ int triagens::aql::replaceOrWithIn (Optimizer* opt, LEAVE_BLOCK; } +struct RemoveRedundantOR { + + AstNode const* bestValue = nullptr; + AstNodeType comparison; + CommonNodeFinder finder; + AstNode const* commonNode = nullptr; + std::string commonName; + int side = 0; + + AstNode* createReplacementNode (Ast* ast) { + TRI_ASSERT(commonNode != nullptr); + TRI_ASSERT(bestValue != nullptr); + TRI_ASSERT(side != 0); + if (side == 1) { + return ast->createNodeBinaryOperator(comparison, bestValue, + commonNode->clone(ast)); + } + return ast->createNodeBinaryOperator(comparison, commonNode->clone(ast), + bestValue); + } + + bool hasRedundantCondition (AstNode const* node) { + if(finder.find(node, NODE_TYPE_OPERATOR_BINARY_LT, commonNode, commonName)){ + comparison = NODE_TYPE_OPERATOR_BINARY_LT; + return hasRedundantConditionWalker(node, NODE_TYPE_OPERATOR_BINARY_LT); + } + if(finder.find(node, NODE_TYPE_OPERATOR_BINARY_LE, commonNode, commonName)){ + comparison = NODE_TYPE_OPERATOR_BINARY_LE; + return hasRedundantConditionWalker(node, NODE_TYPE_OPERATOR_BINARY_LE); + } + if(finder.find(node, NODE_TYPE_OPERATOR_BINARY_GT, commonNode, commonName)){ + comparison = NODE_TYPE_OPERATOR_BINARY_GT; + return hasRedundantConditionWalker(node, NODE_TYPE_OPERATOR_BINARY_GT); + } + if(finder.find(node, NODE_TYPE_OPERATOR_BINARY_GE, commonNode, commonName)){ + comparison = NODE_TYPE_OPERATOR_BINARY_GE; + return hasRedundantConditionWalker(node, NODE_TYPE_OPERATOR_BINARY_GE); + } + return false; + } + + bool hasRedundantConditionWalker (AstNode const* node, AstNodeType condition) { + if (node->type == NODE_TYPE_OPERATOR_BINARY_OR) { + return (hasRedundantConditionWalker(node->getMember(0), condition) && + hasRedundantConditionWalker(node->getMember(1), condition)); + } + + if (node->type == condition) { + auto lhs = node->getMember(0); + auto rhs = node->getMember(1); + + if (hasRedundantConditionWalker(rhs, condition) + && ! hasRedundantConditionWalker(lhs, condition) + && lhs->isConstant()) { + // lhs = value (<, >, <=, >=) commonNode = rhs + TRI_ASSERT(side == 0 || side == 1); + side = 1; // 1 = on the right hand side + + if (bestValue == nullptr) { + bestValue = lhs; + } + else { + int cmp = CompareAstNodes(lhs, bestValue); + if ((condition == NODE_TYPE_OPERATOR_BINARY_LT || condition == + NODE_TYPE_OPERATOR_BINARY_LE) && cmp == -1) { + bestValue = lhs; + } + else if ((condition == NODE_TYPE_OPERATOR_BINARY_GT || condition == + NODE_TYPE_OPERATOR_BINARY_GE) && cmp == 1) { + bestValue = lhs; + } + } + return true; + } + if (hasRedundantConditionWalker(lhs, condition) + && ! hasRedundantConditionWalker(rhs, condition) + && rhs->isConstant()) { + TRI_ASSERT(side == 0 || side == -1); + side = -1; // 1 = on the right hand side + // lhs = commonNode (<, >, <=, >=) value = rhs + if (bestValue == nullptr) { + bestValue = rhs; + } + else { + int cmp = CompareAstNodes(bestValue, rhs); + if ((condition == NODE_TYPE_OPERATOR_BINARY_LT || condition == + NODE_TYPE_OPERATOR_BINARY_LE) && cmp == 1) { + bestValue = rhs; + } + else if ((condition == NODE_TYPE_OPERATOR_BINARY_GT || condition == + NODE_TYPE_OPERATOR_BINARY_GE) && cmp == -1) { + bestValue = rhs; + } + } + return true; + } + + // if hasRedundantConditionWalker(lhs) and + // hasRedundantConditionWalker(rhs), then one of the conditions in the OR + // statement is of the form x == x fall-through intentional + } + else if (node->type == NODE_TYPE_REFERENCE || + node->type == NODE_TYPE_ATTRIBUTE_ACCESS || + node->type == NODE_TYPE_INDEXED_ACCESS) { + // get a string representation of the node for comparisons + return (node->toString() == commonName); + } else if (node->isBoolValue()) { + return true; + } + + return false; + } +}; + +int triagens::aql::removeRedundantOR (Optimizer* opt, + ExecutionPlan* plan, + Optimizer::Rule const* rule) { + ENTER_BLOCK; + std::vector nodes + = plan->findNodesOfType(EN::FILTER, true); + + bool modified = false; + for (auto n : nodes) { + auto deps = n->getDependencies(); + TRI_ASSERT(deps.size() == 1); + if (deps[0]->getType() != EN::CALCULATION) { + continue; + } + + auto fn = static_cast(n); + auto cn = static_cast(deps[0]); + + auto inVar = fn->getVariablesUsedHere(); + auto outVar = cn->getVariablesSetHere(); + + if (outVar.size() != 1 || outVar[0]->id != inVar[0]->id) { + continue; + } + if (cn->expression()->node()->type != NODE_TYPE_OPERATOR_BINARY_OR) { + continue; + } + + RemoveRedundantOR remover; + if(remover.hasRedundantCondition(cn->expression()->node())){ + Expression* expr = nullptr; + ExecutionNode* newNode = nullptr; + auto astNode = remover.createReplacementNode(plan->getAst()); + + try { + expr = new Expression(plan->getAst(), astNode); + } + catch (...) { + delete astNode; + throw; + } + + try { + newNode = new CalculationNode(plan, plan->nextId(), expr, outVar[0]); + } + catch (...) { + delete expr; + throw; + } + + plan->registerNode(newNode); + plan->replaceNode(cn, newNode); + modified = true; + } + } + + if (modified) { + plan->findVarUsage(); + } + opt->addPlan(plan, rule->level, modified); + + return TRI_ERROR_NO_ERROR; + LEAVE_BLOCK; +} + // Local Variables: // mode: outline-minor // outline-regexp: "^\\(/// @brief\\|/// {@inheritDoc}\\|/// @addtogroup\\|// --SECTION--\\|/// @\\}\\)" diff --git a/arangod/Aql/OptimizerRules.h b/arangod/Aql/OptimizerRules.h index feaadf0f8a..538442068b 100644 --- a/arangod/Aql/OptimizerRules.h +++ b/arangod/Aql/OptimizerRules.h @@ -180,9 +180,10 @@ namespace triagens { // same (single) attribute. //////////////////////////////////////////////////////////////////////////////// - int replaceOrWithIn (Optimizer*, ExecutionPlan*, Optimizer::Rule const*); + int removeRedundantOR (Optimizer*, ExecutionPlan*, Optimizer::Rule const*); + } // namespace aql } // namespace triagens From bc80940018732e34890b93264e95b94de8b3824b Mon Sep 17 00:00:00 2001 From: James Date: Sat, 8 Nov 2014 13:52:24 +0000 Subject: [PATCH 09/36] correct logic. --- arangod/Aql/OptimizerRules.cpp | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/arangod/Aql/OptimizerRules.cpp b/arangod/Aql/OptimizerRules.cpp index 673b5a917c..da2d399be2 100644 --- a/arangod/Aql/OptimizerRules.cpp +++ b/arangod/Aql/OptimizerRules.cpp @@ -2820,11 +2820,11 @@ struct RemoveRedundantOR { else { int cmp = CompareAstNodes(lhs, bestValue); if ((condition == NODE_TYPE_OPERATOR_BINARY_LT || condition == - NODE_TYPE_OPERATOR_BINARY_LE) && cmp == -1) { + NODE_TYPE_OPERATOR_BINARY_LE) && cmp == 1) { bestValue = lhs; } else if ((condition == NODE_TYPE_OPERATOR_BINARY_GT || condition == - NODE_TYPE_OPERATOR_BINARY_GE) && cmp == 1) { + NODE_TYPE_OPERATOR_BINARY_GE) && cmp == -1) { bestValue = lhs; } } @@ -2842,11 +2842,11 @@ struct RemoveRedundantOR { else { int cmp = CompareAstNodes(bestValue, rhs); if ((condition == NODE_TYPE_OPERATOR_BINARY_LT || condition == - NODE_TYPE_OPERATOR_BINARY_LE) && cmp == 1) { + NODE_TYPE_OPERATOR_BINARY_LE) && cmp == -1) { bestValue = rhs; } else if ((condition == NODE_TYPE_OPERATOR_BINARY_GT || condition == - NODE_TYPE_OPERATOR_BINARY_GE) && cmp == -1) { + NODE_TYPE_OPERATOR_BINARY_GE) && cmp == 1) { bestValue = rhs; } } From 103db8c8b5a603c5e22ee6700f554248c5a17ce8 Mon Sep 17 00:00:00 2001 From: James Date: Sat, 8 Nov 2014 16:42:39 +0000 Subject: [PATCH 10/36] more flexible remove-redundant-OR rule --- arangod/Aql/OptimizerRules.cpp | 197 +++++++++++++++++++-------------- 1 file changed, 114 insertions(+), 83 deletions(-) diff --git a/arangod/Aql/OptimizerRules.cpp b/arangod/Aql/OptimizerRules.cpp index da2d399be2..36b1d0db92 100644 --- a/arangod/Aql/OptimizerRules.cpp +++ b/arangod/Aql/OptimizerRules.cpp @@ -2521,9 +2521,10 @@ int triagens::aql::undistributeRemoveAfterEnumColl (Optimizer* opt, struct CommonNodeFinder { std::vector possibleNodes; + int isEqual = 0; // -1 equals, 0 not set, 1 one of >, <, >=, <= bool find (AstNode const* node, - AstNodeType condition, + AstNodeType condition, AstNode const*& commonNode, std::string& commonName ) { @@ -2536,27 +2537,23 @@ struct CommonNodeFinder { possibleNodes.clear(); return true; } + + if (isEqual == 0) {// not set + isEqual = (node->type == NODE_TYPE_OPERATOR_BINARY_EQ?-1:1); + } - if (node->type == condition) { + if (node->type == condition + || (condition != NODE_TYPE_OPERATOR_BINARY_EQ + && ( node->type == NODE_TYPE_OPERATOR_BINARY_LE + || node->type == NODE_TYPE_OPERATOR_BINARY_LT + || node->type == NODE_TYPE_OPERATOR_BINARY_GE + || node->type == NODE_TYPE_OPERATOR_BINARY_GT ))) { auto lhs = node->getMember(0); auto rhs = node->getMember(1); - if (lhs->isConstant()) { - commonNode = rhs; - commonName = commonNode->toString(); - possibleNodes.clear(); - return true; - } - - if (rhs->isConstant()) { - commonNode = lhs; - commonName = commonNode->toString(); - possibleNodes.clear(); - return true; - } - - if (rhs->type == NODE_TYPE_FCALL || + if (rhs->isConstant() || + rhs->type == NODE_TYPE_FCALL || rhs->type == NODE_TYPE_FCALL_USER || rhs->type == NODE_TYPE_REFERENCE) { commonNode = lhs; @@ -2565,7 +2562,8 @@ struct CommonNodeFinder { return true; } - if (lhs->type == NODE_TYPE_FCALL || + if (lhs->isConstant() || + lhs->type == NODE_TYPE_FCALL || lhs->type == NODE_TYPE_FCALL_USER || lhs->type == NODE_TYPE_REFERENCE) { commonNode = rhs; @@ -2585,7 +2583,7 @@ struct CommonNodeFinder { return true; } } - // don't return must consider the other side of the condition + // don't return, must consider the other side of the condition } else { possibleNodes.push_back(lhs); @@ -2760,106 +2758,139 @@ struct RemoveRedundantOR { AstNode const* bestValue = nullptr; AstNodeType comparison; + bool inclusive; + bool isComparisonSet = false; CommonNodeFinder finder; AstNode const* commonNode = nullptr; std::string commonName; - int side = 0; AstNode* createReplacementNode (Ast* ast) { TRI_ASSERT(commonNode != nullptr); TRI_ASSERT(bestValue != nullptr); - TRI_ASSERT(side != 0); - if (side == 1) { - return ast->createNodeBinaryOperator(comparison, bestValue, - commonNode->clone(ast)); - } + TRI_ASSERT(isComparisonSet == true); return ast->createNodeBinaryOperator(comparison, commonNode->clone(ast), bestValue); } + bool isInclusiveBound (AstNodeType type) { + return (type == NODE_TYPE_OPERATOR_BINARY_GE || type == NODE_TYPE_OPERATOR_BINARY_LE); + } + + int isCompatibleBound (AstNodeType type, AstNode const* value, bool reverse) { + + if (reverse) { + if ((comparison == NODE_TYPE_OPERATOR_BINARY_LE + || comparison == NODE_TYPE_OPERATOR_BINARY_LT) && + (type == NODE_TYPE_OPERATOR_BINARY_GE + || type == NODE_TYPE_OPERATOR_BINARY_GT)) { + return -1; //high bound + } else if ((comparison == NODE_TYPE_OPERATOR_BINARY_GE + || comparison == NODE_TYPE_OPERATOR_BINARY_GT) && + (type == NODE_TYPE_OPERATOR_BINARY_LE + || type == NODE_TYPE_OPERATOR_BINARY_LT)) { + return 1; //low bound + } + } else { + if ((comparison == NODE_TYPE_OPERATOR_BINARY_LE + || comparison == NODE_TYPE_OPERATOR_BINARY_LT) && + (type == NODE_TYPE_OPERATOR_BINARY_LE + || type == NODE_TYPE_OPERATOR_BINARY_LT)) { + return -1; //high bound + } else if ((comparison == NODE_TYPE_OPERATOR_BINARY_GE + || comparison == NODE_TYPE_OPERATOR_BINARY_GT) && + (type == NODE_TYPE_OPERATOR_BINARY_GE + || type == NODE_TYPE_OPERATOR_BINARY_GT)) { + return 1; //low bound + } + } + return 0; //incompatible bounds + } + + // returns false if the existing value is better and true if the input value is + // better + bool compareBounds(AstNodeType type, AstNode const* value, int lowhigh) { + + int cmp = CompareAstNodes(bestValue, value); + + if (cmp == 0 && (isInclusiveBound(comparison) != isInclusiveBound(type))) { + return (isInclusiveBound(type) ? true : false); + } + return (cmp * lowhigh == 1); + }; + bool hasRedundantCondition (AstNode const* node) { if(finder.find(node, NODE_TYPE_OPERATOR_BINARY_LT, commonNode, commonName)){ - comparison = NODE_TYPE_OPERATOR_BINARY_LT; - return hasRedundantConditionWalker(node, NODE_TYPE_OPERATOR_BINARY_LT); - } - if(finder.find(node, NODE_TYPE_OPERATOR_BINARY_LE, commonNode, commonName)){ - comparison = NODE_TYPE_OPERATOR_BINARY_LE; - return hasRedundantConditionWalker(node, NODE_TYPE_OPERATOR_BINARY_LE); - } - if(finder.find(node, NODE_TYPE_OPERATOR_BINARY_GT, commonNode, commonName)){ - comparison = NODE_TYPE_OPERATOR_BINARY_GT; - return hasRedundantConditionWalker(node, NODE_TYPE_OPERATOR_BINARY_GT); - } - if(finder.find(node, NODE_TYPE_OPERATOR_BINARY_GE, commonNode, commonName)){ - comparison = NODE_TYPE_OPERATOR_BINARY_GE; - return hasRedundantConditionWalker(node, NODE_TYPE_OPERATOR_BINARY_GE); + return hasRedundantConditionWalker(node); } return false; } - bool hasRedundantConditionWalker (AstNode const* node, AstNodeType condition) { - if (node->type == NODE_TYPE_OPERATOR_BINARY_OR) { - return (hasRedundantConditionWalker(node->getMember(0), condition) && - hasRedundantConditionWalker(node->getMember(1), condition)); - } + bool hasRedundantConditionWalker (AstNode const* node) { + AstNodeType type = node->type; + + if (type == NODE_TYPE_OPERATOR_BINARY_OR) { + return (hasRedundantConditionWalker(node->getMember(0)) && + hasRedundantConditionWalker(node->getMember(1))); + } + + if( type == NODE_TYPE_OPERATOR_BINARY_LE + || type == NODE_TYPE_OPERATOR_BINARY_LT + || type == NODE_TYPE_OPERATOR_BINARY_GE + || type == NODE_TYPE_OPERATOR_BINARY_GT ) { - if (node->type == condition) { auto lhs = node->getMember(0); auto rhs = node->getMember(1); - if (hasRedundantConditionWalker(rhs, condition) - && ! hasRedundantConditionWalker(lhs, condition) + if (hasRedundantConditionWalker(rhs) + && ! hasRedundantConditionWalker(lhs) && lhs->isConstant()) { - // lhs = value (<, >, <=, >=) commonNode = rhs - TRI_ASSERT(side == 0 || side == 1); - side = 1; // 1 = on the right hand side + + if (!isComparisonSet) { + comparison = type; + bestValue = lhs; + isComparisonSet = true; + return true; + } - if (bestValue == nullptr) { + int lowhigh = isCompatibleBound(type, lhs, true); + if (lowhigh == 0) { + return false; + } + + if (compareBounds(type, lhs, lowhigh)) { + comparison = type; bestValue = lhs; } - else { - int cmp = CompareAstNodes(lhs, bestValue); - if ((condition == NODE_TYPE_OPERATOR_BINARY_LT || condition == - NODE_TYPE_OPERATOR_BINARY_LE) && cmp == 1) { - bestValue = lhs; - } - else if ((condition == NODE_TYPE_OPERATOR_BINARY_GT || condition == - NODE_TYPE_OPERATOR_BINARY_GE) && cmp == -1) { - bestValue = lhs; - } - } return true; - } - if (hasRedundantConditionWalker(lhs, condition) - && ! hasRedundantConditionWalker(rhs, condition) + } + if (hasRedundantConditionWalker(lhs) + && ! hasRedundantConditionWalker(rhs) && rhs->isConstant()) { - TRI_ASSERT(side == 0 || side == -1); - side = -1; // 1 = on the right hand side - // lhs = commonNode (<, >, <=, >=) value = rhs - if (bestValue == nullptr) { + if (!isComparisonSet) { + comparison = type; bestValue = rhs; + isComparisonSet = true; + return true; } - else { - int cmp = CompareAstNodes(bestValue, rhs); - if ((condition == NODE_TYPE_OPERATOR_BINARY_LT || condition == - NODE_TYPE_OPERATOR_BINARY_LE) && cmp == -1) { + + int lowhigh = isCompatibleBound(type, rhs, false); + if (lowhigh == 0) { + return false; + } + + if (compareBounds(type, rhs, lowhigh)) { + comparison = type; bestValue = rhs; - } - else if ((condition == NODE_TYPE_OPERATOR_BINARY_GT || condition == - NODE_TYPE_OPERATOR_BINARY_GE) && cmp == 1) { - bestValue = rhs; - } } return true; - } - + } // if hasRedundantConditionWalker(lhs) and // hasRedundantConditionWalker(rhs), then one of the conditions in the OR // statement is of the form x == x fall-through intentional } - else if (node->type == NODE_TYPE_REFERENCE || - node->type == NODE_TYPE_ATTRIBUTE_ACCESS || - node->type == NODE_TYPE_INDEXED_ACCESS) { + else if (type == NODE_TYPE_REFERENCE || + type == NODE_TYPE_ATTRIBUTE_ACCESS || + type == NODE_TYPE_INDEXED_ACCESS) { // get a string representation of the node for comparisons return (node->toString() == commonName); } else if (node->isBoolValue()) { From e2b8456395eac64aaf98c0d6aa53ac3534bdb71c Mon Sep 17 00:00:00 2001 From: James Date: Sat, 8 Nov 2014 17:31:30 +0000 Subject: [PATCH 11/36] tests and bugfixes. --- arangod/Aql/OptimizerRules.cpp | 52 +- .../aql-optimizer-rule-remove-redundant-OR.js | 692 ++++++++++++++++++ 2 files changed, 733 insertions(+), 11 deletions(-) create mode 100644 js/server/tests/aql-optimizer-rule-remove-redundant-OR.js diff --git a/arangod/Aql/OptimizerRules.cpp b/arangod/Aql/OptimizerRules.cpp index 36b1d0db92..aca9c8d429 100644 --- a/arangod/Aql/OptimizerRules.cpp +++ b/arangod/Aql/OptimizerRules.cpp @@ -2521,7 +2521,6 @@ int triagens::aql::undistributeRemoveAfterEnumColl (Optimizer* opt, struct CommonNodeFinder { std::vector possibleNodes; - int isEqual = 0; // -1 equals, 0 not set, 1 one of >, <, >=, <= bool find (AstNode const* node, AstNodeType condition, @@ -2538,10 +2537,6 @@ struct CommonNodeFinder { return true; } - if (isEqual == 0) {// not set - isEqual = (node->type == NODE_TYPE_OPERATOR_BINARY_EQ?-1:1); - } - if (node->type == condition || (condition != NODE_TYPE_OPERATOR_BINARY_EQ && ( node->type == NODE_TYPE_OPERATOR_BINARY_LE @@ -2552,8 +2547,21 @@ struct CommonNodeFinder { auto lhs = node->getMember(0); auto rhs = node->getMember(1); - if (rhs->isConstant() || - rhs->type == NODE_TYPE_FCALL || + if (lhs->isConstant()) { + commonNode = rhs; + commonName = commonNode->toString(); + possibleNodes.clear(); + return true; + } + + if (rhs->isConstant()) { + commonNode = lhs; + commonName = commonNode->toString(); + possibleNodes.clear(); + return true; + } + + if (rhs->type == NODE_TYPE_FCALL || rhs->type == NODE_TYPE_FCALL_USER || rhs->type == NODE_TYPE_REFERENCE) { commonNode = lhs; @@ -2562,8 +2570,7 @@ struct CommonNodeFinder { return true; } - if (lhs->isConstant() || - lhs->type == NODE_TYPE_FCALL || + if (lhs->type == NODE_TYPE_FCALL || lhs->type == NODE_TYPE_FCALL_USER || lhs->type == NODE_TYPE_REFERENCE) { commonNode = rhs; @@ -2846,7 +2853,19 @@ struct RemoveRedundantOR { && lhs->isConstant()) { if (!isComparisonSet) { - comparison = type; + if (type == NODE_TYPE_OPERATOR_BINARY_LE) { + comparison = NODE_TYPE_OPERATOR_BINARY_GE; + } + else if (type == NODE_TYPE_OPERATOR_BINARY_LT) { + comparison = NODE_TYPE_OPERATOR_BINARY_GT; + } + else if (type == NODE_TYPE_OPERATOR_BINARY_GT) { + comparison = NODE_TYPE_OPERATOR_BINARY_LT; + } + else if (type == NODE_TYPE_OPERATOR_BINARY_GE) { + comparison = NODE_TYPE_OPERATOR_BINARY_LE; + } + bestValue = lhs; isComparisonSet = true; return true; @@ -2858,7 +2877,18 @@ struct RemoveRedundantOR { } if (compareBounds(type, lhs, lowhigh)) { - comparison = type; + if (type == NODE_TYPE_OPERATOR_BINARY_LE) { + comparison = NODE_TYPE_OPERATOR_BINARY_GE; + } + else if (type == NODE_TYPE_OPERATOR_BINARY_LT) { + comparison = NODE_TYPE_OPERATOR_BINARY_GT; + } + else if (type == NODE_TYPE_OPERATOR_BINARY_GT) { + comparison = NODE_TYPE_OPERATOR_BINARY_LT; + } + else if (type == NODE_TYPE_OPERATOR_BINARY_GE) { + comparison = NODE_TYPE_OPERATOR_BINARY_LE; + } bestValue = lhs; } return true; diff --git a/js/server/tests/aql-optimizer-rule-remove-redundant-OR.js b/js/server/tests/aql-optimizer-rule-remove-redundant-OR.js new file mode 100644 index 0000000000..06371796b7 --- /dev/null +++ b/js/server/tests/aql-optimizer-rule-remove-redundant-OR.js @@ -0,0 +1,692 @@ +/*jshint strict: false, maxlen: 500 */ +/*global require, assertEqual, assertTrue, AQL_EXPLAIN, AQL_EXECUTE */ + +//////////////////////////////////////////////////////////////////////////////// +/// @brief tests for Ahuacatl, skiplist index queries +/// +/// @file +/// +/// DISCLAIMER +/// +/// Copyright 2010-2012 triagens GmbH, Cologne, Germany +/// +/// Licensed under the Apache License, Version 2.0 (the "License"); +/// you may not use this file except in compliance with the License. +/// You may obtain a copy of the License at +/// +/// http://www.apache.org/licenses/LICENSE-2.0 +/// +/// Unless required by applicable law or agreed to in writing, software +/// distributed under the License is distributed on an "AS IS" BASIS, +/// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +/// See the License for the specific language governing permissions and +/// limitations under the License. +/// +/// Copyright holder is triAGENS GmbH, Cologne, Germany +/// +/// @author +/// @author Copyright 2014, triAGENS GmbH, Cologne, Germany +//////////////////////////////////////////////////////////////////////////////// + +var internal = require("internal"); +var jsunity = require("jsunity"); +var helper = require("org/arangodb/aql-helper"); +var getQueryResults = helper.getQueryResults; + +//////////////////////////////////////////////////////////////////////////////// +/// @brief test suite +//////////////////////////////////////////////////////////////////////////////// + +function NewAqlRemoveRedundantORTestSuite () { + var ruleName = "remove-redundant-OR"; + + var isRuleUsed = function (query, params) { + var result = AQL_EXPLAIN(query, params, { optimizer: { rules: [ "-all", "+" + ruleName ] } }); + assertTrue(result.plan.rules.indexOf(ruleName) !== -1, query); + result = AQL_EXPLAIN(query, params, { optimizer: { rules: [ "-all" ] } }); + assertTrue(result.plan.rules.indexOf(ruleName) === -1, query); + }; + + var ruleIsNotUsed = function (query, params) { + var result = AQL_EXPLAIN(query, params, { optimizer: { rules: [ "-all", "+" + ruleName ] } }); + assertTrue(result.plan.rules.indexOf(ruleName) === -1, query); + }; + + var executeWithRule = function (query, params) { + return AQL_EXECUTE(query, params, { optimizer: { rules: [ "-all", "+" + ruleName ] } }).json; + }; + + var executeWithoutRule = function (query, params) { + return AQL_EXECUTE(query, params, { optimizer: { rules: [ "-all" ] } }).json; + }; + + return { + +//////////////////////////////////////////////////////////////////////////////// +/// @brief set up +//////////////////////////////////////////////////////////////////////////////// + + setUp : function () { + }, + +//////////////////////////////////////////////////////////////////////////////// +/// @brief tear down +//////////////////////////////////////////////////////////////////////////////// + + tearDown : function () { + }, + +//////////////////////////////////////////////////////////////////////////////// +/// @brief test the rule fires for actual values +//////////////////////////////////////////////////////////////////////////////// + + testFiresGtGt1 : function () { + var query = "FOR i IN [ 1, 2, 3, 4, 5, 6, 7, 8, 9, 10 ] " + + " FILTER i > 1 || i > 2 RETURN i"; + + isRuleUsed(query, {}); + + var expected = [ 2, 3, 4, 5, 6, 7, 8, 9, 10 ]; + var actual = getQueryResults(query); + assertEqual(expected, actual); + assertEqual(executeWithRule(query, {}), executeWithoutRule(query, {})); + }, + + testFiresGtLt1 : function () { + var query = "FOR i IN [ 1, 2, 3, 4, 5, 6, 7, 8, 9, 10 ] FILTER i > 1 || 2 < i RETURN i"; + + isRuleUsed(query, {}); + + var expected = [ 2, 3, 4, 5, 6, 7, 8, 9, 10 ]; + var actual = getQueryResults(query); + assertEqual(expected, actual); + assertEqual(executeWithRule(query, {}), executeWithoutRule(query, {})); + }, + + testFiresLtLt1 : function () { + var query = "FOR i IN [ 1, 2, 3, 4, 5, 6, 7, 8, 9, 10 ] FILTER 1 < i || 2 < i RETURN i"; + + isRuleUsed(query, {}); + + var expected = [ 2, 3, 4, 5, 6, 7, 8, 9, 10 ]; + var actual = getQueryResults(query); + assertEqual(expected, actual); + assertEqual(executeWithRule(query, {}), executeWithoutRule(query, {})); + }, + + testFiresLtGt1 : function () { + var query = "FOR i IN [ 1, 2, 3, 4, 5, 6, 7, 8, 9, 10 ] FILTER 1 < i || i > 2 RETURN i"; + + isRuleUsed(query, {}); + + var expected = [ 2, 3, 4, 5, 6, 7, 8, 9, 10 ]; + var actual = getQueryResults(query); + assertEqual(expected, actual); + assertEqual(executeWithRule(query, {}), executeWithoutRule(query, {})); + }, + +//////////////////////////////////////////////////////////////////////////////// + + testFiresGtGe1 : function () { + var query = "FOR i IN [ 1, 2, 3, 4, 5, 6, 7, 8, 9, 10 ] " + + " FILTER i > 1 || i >= 2 RETURN i"; + + isRuleUsed(query, {}); + + var expected = [ 2, 3, 4, 5, 6, 7, 8, 9, 10 ]; + var actual = getQueryResults(query); + assertEqual(expected, actual); + assertEqual(executeWithRule(query, {}), executeWithoutRule(query, {})); + }, + + testFiresGtLe1 : function () { + var query = "FOR i IN [ 1, 2, 3, 4, 5, 6, 7, 8, 9, 10 ] " + + " FILTER i > 1 || 2 <= i RETURN i"; + + isRuleUsed(query, {}); + + var expected = [ 2, 3, 4, 5, 6, 7, 8, 9, 10 ]; + var actual = getQueryResults(query); + assertEqual(expected, actual); + assertEqual(executeWithRule(query, {}), executeWithoutRule(query, {})); + }, + + testFiresLtLe1 : function () { + var query = "FOR i IN [ 1, 2, 3, 4, 5, 6, 7, 8, 9, 10 ] " + + " FILTER 1 < i || 2 <= i RETURN i"; + + isRuleUsed(query, {}); + + var expected = [ 2, 3, 4, 5, 6, 7, 8, 9, 10 ]; + var actual = getQueryResults(query); + assertEqual(expected, actual); + assertEqual(executeWithRule(query, {}), executeWithoutRule(query, {})); + }, + + testFiresLtGe1 : function () { + var query = "FOR i IN [ 1, 2, 3, 4, 5, 6, 7, 8, 9, 10 ] " + + " FILTER 1 < i || i >= 2 RETURN i"; + + isRuleUsed(query, {}); + + var expected = [ 2, 3, 4, 5, 6, 7, 8, 9, 10 ]; + var actual = getQueryResults(query); + assertEqual(expected, actual); + assertEqual(executeWithRule(query, {}), executeWithoutRule(query, {})); + }, + +//////////////////////////////////////////////////////////////////////////////// + + testFiresGeGt1 : function () { + var query = "FOR i IN [ 1, 2, 3, 4, 5, 6, 7, 8, 9, 10 ] " + + " FILTER i >= 1 || i > 2 RETURN i"; + + isRuleUsed(query, {}); + + var expected = [ 1, 2, 3, 4, 5, 6, 7, 8, 9, 10 ]; + var actual = getQueryResults(query); + assertEqual(expected, actual); + assertEqual(executeWithRule(query, {}), executeWithoutRule(query, {})); + }, + + testFiresGeLt1 : function () { + var query = "FOR i IN [ 1, 2, 3, 4, 5, 6, 7, 8, 9, 10 ] " + + " FILTER i >= 1 || 2 < i RETURN i"; + + isRuleUsed(query, {}); + + var expected = [ 1, 2, 3, 4, 5, 6, 7, 8, 9, 10 ]; + var actual = getQueryResults(query); + assertEqual(expected, actual); + assertEqual(executeWithRule(query, {}), executeWithoutRule(query, {})); + }, + + testFiresLeLt1 : function () { + var query = "FOR i IN [ 1, 2, 3, 4, 5, 6, 7, 8, 9, 10 ] " + + " FILTER 1 <= i || 2 < i RETURN i"; + + isRuleUsed(query, {}); + + var expected = [ 1, 2, 3, 4, 5, 6, 7, 8, 9, 10 ]; + var actual = getQueryResults(query); + assertEqual(expected, actual); + assertEqual(executeWithRule(query, {}), executeWithoutRule(query, {})); + }, + + testFiresLeGt1 : function () { + var query = "FOR i IN [ 1, 2, 3, 4, 5, 6, 7, 8, 9, 10 ] " + + " FILTER 1 <= i || i > 2 RETURN i"; + + isRuleUsed(query, {}); + + var expected = [ 1, 2, 3, 4, 5, 6, 7, 8, 9, 10 ]; + var actual = getQueryResults(query); + assertEqual(expected, actual); + assertEqual(executeWithRule(query, {}), executeWithoutRule(query, {})); + }, + +//////////////////////////////////////////////////////////////////////////////// + + testFiresGeGe1 : function () { + var query = "FOR i IN [ 1, 2, 3, 4, 5, 6, 7, 8, 9, 10 ] " + + " FILTER i >= 1 || i >= 2 RETURN i"; + + isRuleUsed(query, {}); + + var expected = [ 1, 2, 3, 4, 5, 6, 7, 8, 9, 10 ]; + var actual = getQueryResults(query); + assertEqual(expected, actual); + assertEqual(executeWithRule(query, {}), executeWithoutRule(query, {})); + }, + + testFiresGeLe1 : function () { + var query = "FOR i IN [ 1, 2, 3, 4, 5, 6, 7, 8, 9, 10 ] " + + " FILTER i >= 1 || 2 <= i RETURN i"; + + isRuleUsed(query, {}); + + var expected = [ 1, 2, 3, 4, 5, 6, 7, 8, 9, 10 ]; + var actual = getQueryResults(query); + assertEqual(expected, actual); + assertEqual(executeWithRule(query, {}), executeWithoutRule(query, {})); + }, + + testFiresLeLe1 : function () { + var query = "FOR i IN [ 1, 2, 3, 4, 5, 6, 7, 8, 9, 10 ] " + + " FILTER 1 <= i || 2 <= i RETURN i"; + + isRuleUsed(query, {}); + + var expected = [ 1, 2, 3, 4, 5, 6, 7, 8, 9, 10 ]; + var actual = getQueryResults(query); + assertEqual(expected, actual); + assertEqual(executeWithRule(query, {}), executeWithoutRule(query, {})); + }, + + testFiresLeGe1 : function () { + var query = "FOR i IN [ 1, 2, 3, 4, 5, 6, 7, 8, 9, 10 ] " + + " FILTER 1 <= i || i >= 2 RETURN i"; + + isRuleUsed(query, {}); + + var expected = [ 1, 2, 3, 4, 5, 6, 7, 8, 9, 10 ]; + var actual = getQueryResults(query); + assertEqual(expected, actual); + assertEqual(executeWithRule(query, {}), executeWithoutRule(query, {})); + }, + +//////////////////////////////////////////////////////////////////////////////// +//////////////////////////////////////////////////////////////////////////////// + + testFiresGtGt2 : function () { + var query = "FOR i IN [ 1, 2, 3, 4, 5, 6, 7, 8, 9, 10 ] " + + " FILTER i > 1 || i > 1 RETURN i"; + + isRuleUsed(query, {}); + + var expected = [ 2, 3, 4, 5, 6, 7, 8, 9, 10 ]; + var actual = getQueryResults(query); + assertEqual(expected, actual); + assertEqual(executeWithRule(query, {}), executeWithoutRule(query, {})); + }, + + testFiresGtLt2 : function () { + var query = "FOR i IN [ 1, 2, 3, 4, 5, 6, 7, 8, 9, 10 ] FILTER i > 1 || 1 < i RETURN i"; + + isRuleUsed(query, {}); + + var expected = [ 2, 3, 4, 5, 6, 7, 8, 9, 10 ]; + var actual = getQueryResults(query); + assertEqual(expected, actual); + assertEqual(executeWithRule(query, {}), executeWithoutRule(query, {})); + }, + + testFiresLtLt2 : function () { + var query = "FOR i IN [ 1, 2, 3, 4, 5, 6, 7, 8, 9, 10 ] FILTER 1 < i || 1 < i RETURN i"; + + isRuleUsed(query, {}); + + var expected = [ 2, 3, 4, 5, 6, 7, 8, 9, 10 ]; + var actual = getQueryResults(query); + assertEqual(expected, actual); + assertEqual(executeWithRule(query, {}), executeWithoutRule(query, {})); + }, + + testFiresLtGt2 : function () { + var query = "FOR i IN [ 1, 2, 3, 4, 5, 6, 7, 8, 9, 10 ] FILTER 1 < i || i > 1 RETURN i"; + + isRuleUsed(query, {}); + + var expected = [ 2, 3, 4, 5, 6, 7, 8, 9, 10 ]; + var actual = getQueryResults(query); + assertEqual(expected, actual); + assertEqual(executeWithRule(query, {}), executeWithoutRule(query, {})); + }, + +//////////////////////////////////////////////////////////////////////////////// + + testFiresGtGe2 : function () { + var query = "FOR i IN [ 1, 2, 3, 4, 5, 6, 7, 8, 9, 10 ] " + + " FILTER i > 1 || i >= 1 RETURN i"; + + isRuleUsed(query, {}); + + var expected = [ 1, 2, 3, 4, 5, 6, 7, 8, 9, 10 ]; + var actual = getQueryResults(query); + assertEqual(expected, actual); + assertEqual(executeWithRule(query, {}), executeWithoutRule(query, {})); + }, + + testFiresGtLe2 : function () { + var query = "FOR i IN [ 1, 2, 3, 4, 5, 6, 7, 8, 9, 10 ] " + + " FILTER i > 1 || 1 <= i RETURN i"; + + isRuleUsed(query, {}); + + var expected = [ 1, 2, 3, 4, 5, 6, 7, 8, 9, 10 ]; + var actual = getQueryResults(query); + assertEqual(expected, actual); + assertEqual(executeWithRule(query, {}), executeWithoutRule(query, {})); + }, + + testFiresLtLe2 : function () { + var query = "FOR i IN [ 1, 2, 3, 4, 5, 6, 7, 8, 9, 10 ] FILTER 1 < i || 1 <= i RETURN i"; + + isRuleUsed(query, {}); + + var expected = [ 1, 2, 3, 4, 5, 6, 7, 8, 9, 10 ]; + var actual = getQueryResults(query); + assertEqual(expected, actual); + assertEqual(executeWithRule(query, {}), executeWithoutRule(query, {})); + }, + + testFiresLtGe2 : function () { + var query = "FOR i IN [ 1, 2, 3, 4, 5, 6, 7, 8, 9, 10 ] " + + " FILTER 1 < i || i >= 1 RETURN i"; + + isRuleUsed(query, {}); + + var expected = [ 1, 2, 3, 4, 5, 6, 7, 8, 9, 10 ]; + var actual = getQueryResults(query); + assertEqual(expected, actual); + assertEqual(executeWithRule(query, {}), executeWithoutRule(query, {})); + }, + +//////////////////////////////////////////////////////////////////////////////// + + testFiresGeGt2 : function () { + var query = "FOR i IN [ 1, 2, 3, 4, 5, 6, 7, 8, 9, 10 ] " + + " FILTER i >= 1 || i > 1 RETURN i"; + + isRuleUsed(query, {}); + + var expected = [ 1, 2, 3, 4, 5, 6, 7, 8, 9, 10 ]; + var actual = getQueryResults(query); + assertEqual(expected, actual); + assertEqual(executeWithRule(query, {}), executeWithoutRule(query, {})); + }, + + testFiresGeLt2 : function () { + var query = "FOR i IN [ 1, 2, 3, 4, 5, 6, 7, 8, 9, 10 ] " + + " FILTER i >= 1 || 1 < i RETURN i"; + + isRuleUsed(query, {}); + + var expected = [ 1, 2, 3, 4, 5, 6, 7, 8, 9, 10 ]; + var actual = getQueryResults(query); + assertEqual(expected, actual); + assertEqual(executeWithRule(query, {}), executeWithoutRule(query, {})); + }, + + testFiresLeLt2 : function () { + var query = "FOR i IN [ 1, 2, 3, 4, 5, 6, 7, 8, 9, 10 ] " + + " FILTER 1 <= i || 1 < i RETURN i"; + + isRuleUsed(query, {}); + + var expected = [ 1, 2, 3, 4, 5, 6, 7, 8, 9, 10 ]; + var actual = getQueryResults(query); + assertEqual(expected, actual); + assertEqual(executeWithRule(query, {}), executeWithoutRule(query, {})); + }, + + testFiresLeGt2 : function () { + var query = "FOR i IN [ 1, 2, 3, 4, 5, 6, 7, 8, 9, 10 ] " + + " FILTER 1 <= i || i > 1 RETURN i"; + + isRuleUsed(query, {}); + + var expected = [ 1, 2, 3, 4, 5, 6, 7, 8, 9, 10 ]; + var actual = getQueryResults(query); + assertEqual(expected, actual); + assertEqual(executeWithRule(query, {}), executeWithoutRule(query, {})); + }, + +//////////////////////////////////////////////////////////////////////////////// + + testFiresGeGe2 : function () { + var query = "FOR i IN [ 1, 2, 3, 4, 5, 6, 7, 8, 9, 10 ] " + + " FILTER i >= 1 || i >= 1 RETURN i"; + + isRuleUsed(query, {}); + + var expected = [ 1, 2, 3, 4, 5, 6, 7, 8, 9, 10 ]; + var actual = getQueryResults(query); + assertEqual(expected, actual); + assertEqual(executeWithRule(query, {}), executeWithoutRule(query, {})); + }, + + testFiresGeLe2 : function () { + var query = "FOR i IN [ 1, 2, 3, 4, 5, 6, 7, 8, 9, 10 ] " + + " FILTER i >= 1 || 1 <= i RETURN i"; + + isRuleUsed(query, {}); + + var expected = [ 1, 2, 3, 4, 5, 6, 7, 8, 9, 10 ]; + var actual = getQueryResults(query); + assertEqual(expected, actual); + assertEqual(executeWithRule(query, {}), executeWithoutRule(query, {})); + }, + + testFiresLeLe2 : function () { + var query = "FOR i IN [ 1, 2, 3, 4, 5, 6, 7, 8, 9, 10 ] " + + " FILTER 1 <= i || 1 <= i RETURN i"; + + isRuleUsed(query, {}); + + var expected = [ 1, 2, 3, 4, 5, 6, 7, 8, 9, 10 ]; + var actual = getQueryResults(query); + assertEqual(expected, actual); + assertEqual(executeWithRule(query, {}), executeWithoutRule(query, {})); + }, + + testFiresLeGe2 : function () { + var query = "FOR i IN [ 1, 2, 3, 4, 5, 6, 7, 8, 9, 10 ] " + + " FILTER 1 <= i || i >= 1 RETURN i"; + + isRuleUsed(query, {}); + + var expected = [ 1, 2, 3, 4, 5, 6, 7, 8, 9, 10 ]; + var actual = getQueryResults(query); + assertEqual(expected, actual); + assertEqual(executeWithRule(query, {}), executeWithoutRule(query, {})); + }, + +//////////////////////////////////////////////////////////////////////////////// + + testDudGtGt1 : function () { + var query = "FOR i IN [ 1, 2, 3, 4, 5, 6, 7, 8, 9, 10 ] " + + " FILTER i > 1 || 2 > i RETURN i"; + + ruleIsNotUsed(query, {}); + }, + + testDudGtLt1 : function () { + var query = "FOR i IN [ 1, 2, 3, 4, 5, 6, 7, 8, 9, 10 ] FILTER i > 1 || i < 2 RETURN i"; + + ruleIsNotUsed(query, {}); + }, + + testDudLtLt1 : function () { + var query = "FOR i IN [ 1, 2, 3, 4, 5, 6, 7, 8, 9, 10 ] FILTER 1 < i || i < 2 RETURN i"; + + ruleIsNotUsed(query, {}); + }, + + testDudLtGt1 : function () { + var query = "FOR i IN [ 1, 2, 3, 4, 5, 6, 7, 8, 9, 10 ] FILTER 1 < i || 2 > i RETURN i"; + + ruleIsNotUsed(query, {}); + }, + +//////////////////////////////////////////////////////////////////////////////// + + testDudGtGe1 : function () { + var query = "FOR i IN [ 1, 2, 3, 4, 5, 6, 7, 8, 9, 10 ] " + + " FILTER i > 1 || 2 >= i RETURN i"; + + ruleIsNotUsed(query, {}); + }, + + testDudGtLe1 : function () { + var query = "FOR i IN [ 1, 2, 3, 4, 5, 6, 7, 8, 9, 10 ] " + + " FILTER i > 1 || i <= 2 RETURN i"; + + ruleIsNotUsed(query, {}); + }, + + testDudLtLe1 : function () { + var query = "FOR i IN [ 1, 2, 3, 4, 5, 6, 7, 8, 9, 10 ] " + + " FILTER 1 < i || i <= 2 RETURN i"; + + ruleIsNotUsed(query, {}); + }, + + testDudLtGe1 : function () { + var query = "FOR i IN [ 1, 2, 3, 4, 5, 6, 7, 8, 9, 10 ] " + + " FILTER 1 < i || 2 >= i RETURN i"; + + ruleIsNotUsed(query, {}); + }, + +//////////////////////////////////////////////////////////////////////////////// + + testDudGeGt1 : function () { + var query = "FOR i IN [ 1, 2, 3, 4, 5, 6, 7, 8, 9, 10 ] " + + " FILTER i >= 1 || 2 > i RETURN i"; + ruleIsNotUsed(query, {}); + }, + + testDudGeLt1 : function () { + var query = "FOR i IN [ 1, 2, 3, 4, 5, 6, 7, 8, 9, 10 ] " + + " FILTER i >= 1 || i < 2 RETURN i"; + ruleIsNotUsed(query, {}); + }, + + testDudLeLt1 : function () { + var query = "FOR i IN [ 1, 2, 3, 4, 5, 6, 7, 8, 9, 10 ] " + + " FILTER 1 <= i || i < 2 RETURN i"; + ruleIsNotUsed(query, {}); + }, + + testDudLeGt1 : function () { + var query = "FOR i IN [ 1, 2, 3, 4, 5, 6, 7, 8, 9, 10 ] " + + " FILTER 1 <= i || 2 > i RETURN i"; + + ruleIsNotUsed(query, {}); + }, + +//////////////////////////////////////////////////////////////////////////////// + + testDudGeGe1 : function () { + var query = "FOR i IN [ 1, 2, 3, 4, 5, 6, 7, 8, 9, 10 ] " + + " FILTER i >= 1 || 2 >= i RETURN i"; + ruleIsNotUsed(query, {}); + }, + + testDudGeLe1 : function () { + var query = "FOR i IN [ 1, 2, 3, 4, 5, 6, 7, 8, 9, 10 ] " + + " FILTER i >= 1 || i <= 2 RETURN i"; + ruleIsNotUsed(query, {}); + }, + + testDudLeLe1 : function () { + var query = "FOR i IN [ 1, 2, 3, 4, 5, 6, 7, 8, 9, 10 ] " + + " FILTER 1 <= i || i <= 2 RETURN i"; + ruleIsNotUsed(query, {}); + }, + + testDudLeGe1 : function () { + var query = "FOR i IN [ 1, 2, 3, 4, 5, 6, 7, 8, 9, 10 ] " + + " FILTER 1 <= i || 2 >= i RETURN i"; + + ruleIsNotUsed(query, {}); + }, + +//////////////////////////////////////////////////////////////////////////////// +//////////////////////////////////////////////////////////////////////////////// + + testDudGtGt2 : function () { + var query = "FOR i IN [ 1, 2, 3, 4, 5, 6, 7, 8, 9, 10 ] " + + " FILTER i > 1 || i < 1 RETURN i"; + ruleIsNotUsed(query, {}); + }, + + testDudGtLt2 : function () { + var query = "FOR i IN [ 1, 2, 3, 4, 5, 6, 7, 8, 9, 10 ] FILTER i > 1 || 1 > i RETURN i"; + ruleIsNotUsed(query, {}); + }, + + testDudLtLt2 : function () { + var query = "FOR i IN [ 1, 2, 3, 4, 5, 6, 7, 8, 9, 10 ] FILTER 1 < i || 1 > i RETURN i"; + ruleIsNotUsed(query, {}); + }, + + testDudLtGt2 : function () { + var query = "FOR i IN [ 1, 2, 3, 4, 5, 6, 7, 8, 9, 10 ] FILTER 1 < i || i < 1 RETURN i"; + ruleIsNotUsed(query, {}); + }, + +//////////////////////////////////////////////////////////////////////////////// + + testDudGtGe2 : function () { + var query = "FOR i IN [ 1, 2, 3, 4, 5, 6, 7, 8, 9, 10 ] " + + " FILTER i > 1 || 1 >= i RETURN i"; + ruleIsNotUsed(query, {}); + }, + + testDudGtLe2 : function () { + var query = "FOR i IN [ 1, 2, 3, 4, 5, 6, 7, 8, 9, 10 ] " + + " FILTER i > 1 || i <= 1 RETURN i"; + ruleIsNotUsed(query, {}); + }, + + testDudLtLe2 : function () { + var query = "FOR i IN [ 1, 2, 3, 4, 5, 6, 7, 8, 9, 10 ] FILTER 1 < i || i <= 1 RETURN i"; + ruleIsNotUsed(query, {}); + }, + + testDudLtGe2 : function () { + var query = "FOR i IN [ 1, 2, 3, 4, 5, 6, 7, 8, 9, 10 ] " + + " FILTER 1 < i || 1 >= i RETURN i"; + ruleIsNotUsed(query, {}); + }, + +//////////////////////////////////////////////////////////////////////////////// + + testDudGeGt2 : function () { + var query = "FOR i IN [ 1, 2, 3, 4, 5, 6, 7, 8, 9, 10 ] " + + " FILTER i >= 1 || 1 > i RETURN i"; + ruleIsNotUsed(query, {}); + }, + + testDudGeLt2 : function () { + var query = "FOR i IN [ 1, 2, 3, 4, 5, 6, 7, 8, 9, 10 ] " + + " FILTER i >= 1 || i < 1 RETURN i"; + ruleIsNotUsed(query, {}); + }, + + testDudLeLt2 : function () { + var query = "FOR i IN [ 1, 2, 3, 4, 5, 6, 7, 8, 9, 10 ] " + + " FILTER 1 <= i || i < 1 RETURN i"; + ruleIsNotUsed(query, {}); + }, + + testDudLeGt2 : function () { + var query = "FOR i IN [ 1, 2, 3, 4, 5, 6, 7, 8, 9, 10 ] " + + " FILTER 1 <= i || 1 > i RETURN i"; + ruleIsNotUsed(query, {}); + }, + +//////////////////////////////////////////////////////////////////////////////// + + testDudGeGe2 : function () { + var query = "FOR i IN [ 1, 2, 3, 4, 5, 6, 7, 8, 9, 10 ] " + + " FILTER i >= 1 || 1 >= i RETURN i"; + ruleIsNotUsed(query, {}); + }, + + testDudGeLe2 : function () { + var query = "FOR i IN [ 1, 2, 3, 4, 5, 6, 7, 8, 9, 10 ] " + + " FILTER i >= 1 || i <= 1 RETURN i"; + ruleIsNotUsed(query, {}); + }, + + testDudLeLe2 : function () { + var query = "FOR i IN [ 1, 2, 3, 4, 5, 6, 7, 8, 9, 10 ] " + + " FILTER 1 <= i || i <= 1 RETURN i"; + ruleIsNotUsed(query, {}); + }, + + testDudLeGe2 : function () { + var query = "FOR i IN [ 1, 2, 3, 4, 5, 6, 7, 8, 9, 10 ] " + + " FILTER 1 <= i || 1 >= i RETURN i"; + ruleIsNotUsed(query, {}); + }, + }; +} + +jsunity.run(NewAqlRemoveRedundantORTestSuite); + +return jsunity.done(); + From 14e6370c22a2304c3d6665e8b66537e8fb661bd2 Mon Sep 17 00:00:00 2001 From: James Date: Sat, 8 Nov 2014 17:35:27 +0000 Subject: [PATCH 12/36] cleaning up. --- arangod/Aql/OptimizerRules.cpp | 42 ++++++++++++++-------------------- 1 file changed, 17 insertions(+), 25 deletions(-) diff --git a/arangod/Aql/OptimizerRules.cpp b/arangod/Aql/OptimizerRules.cpp index aca9c8d429..548a510f52 100644 --- a/arangod/Aql/OptimizerRules.cpp +++ b/arangod/Aql/OptimizerRules.cpp @@ -2825,6 +2825,21 @@ struct RemoveRedundantOR { return (cmp * lowhigh == 1); }; + AstNodeType reverseComparison (AstNodeType type) { + if (type == NODE_TYPE_OPERATOR_BINARY_LE) { + return NODE_TYPE_OPERATOR_BINARY_GE; + } + else if (type == NODE_TYPE_OPERATOR_BINARY_LT) { + return NODE_TYPE_OPERATOR_BINARY_GT; + } + else if (type == NODE_TYPE_OPERATOR_BINARY_GT) { + return NODE_TYPE_OPERATOR_BINARY_LT; + } + else { + return NODE_TYPE_OPERATOR_BINARY_LE; + } + } + bool hasRedundantCondition (AstNode const* node) { if(finder.find(node, NODE_TYPE_OPERATOR_BINARY_LT, commonNode, commonName)){ return hasRedundantConditionWalker(node); @@ -2853,19 +2868,7 @@ struct RemoveRedundantOR { && lhs->isConstant()) { if (!isComparisonSet) { - if (type == NODE_TYPE_OPERATOR_BINARY_LE) { - comparison = NODE_TYPE_OPERATOR_BINARY_GE; - } - else if (type == NODE_TYPE_OPERATOR_BINARY_LT) { - comparison = NODE_TYPE_OPERATOR_BINARY_GT; - } - else if (type == NODE_TYPE_OPERATOR_BINARY_GT) { - comparison = NODE_TYPE_OPERATOR_BINARY_LT; - } - else if (type == NODE_TYPE_OPERATOR_BINARY_GE) { - comparison = NODE_TYPE_OPERATOR_BINARY_LE; - } - + comparison = reverseComparison(type); bestValue = lhs; isComparisonSet = true; return true; @@ -2877,18 +2880,7 @@ struct RemoveRedundantOR { } if (compareBounds(type, lhs, lowhigh)) { - if (type == NODE_TYPE_OPERATOR_BINARY_LE) { - comparison = NODE_TYPE_OPERATOR_BINARY_GE; - } - else if (type == NODE_TYPE_OPERATOR_BINARY_LT) { - comparison = NODE_TYPE_OPERATOR_BINARY_GT; - } - else if (type == NODE_TYPE_OPERATOR_BINARY_GT) { - comparison = NODE_TYPE_OPERATOR_BINARY_LT; - } - else if (type == NODE_TYPE_OPERATOR_BINARY_GE) { - comparison = NODE_TYPE_OPERATOR_BINARY_LE; - } + comparison = reverseComparison(type); bestValue = lhs; } return true; From 31de49cccd2eef519e51b7f23db391b39ec7dab5 Mon Sep 17 00:00:00 2001 From: James Date: Sat, 8 Nov 2014 17:35:39 +0000 Subject: [PATCH 13/36] adding comment --- js/server/tests/aql-optimizer-rule-remove-redundant-OR.js | 1 + 1 file changed, 1 insertion(+) diff --git a/js/server/tests/aql-optimizer-rule-remove-redundant-OR.js b/js/server/tests/aql-optimizer-rule-remove-redundant-OR.js index 06371796b7..8d710cfa0d 100644 --- a/js/server/tests/aql-optimizer-rule-remove-redundant-OR.js +++ b/js/server/tests/aql-optimizer-rule-remove-redundant-OR.js @@ -27,6 +27,7 @@ /// @author /// @author Copyright 2014, triAGENS GmbH, Cologne, Germany //////////////////////////////////////////////////////////////////////////////// +// TODO add some test which don't use number values! var internal = require("internal"); var jsunity = require("jsunity"); From 44d5dd2749f952f30dfadf21949d4ba24979c4c2 Mon Sep 17 00:00:00 2001 From: Jan Steemann Date: Sat, 8 Nov 2014 21:06:06 +0100 Subject: [PATCH 14/36] added test to Makefile, fixed jslint warning --- UnitTests/Makefile.unittests | 3 ++- ...dundant-OR.js => aql-optimizer-rule-remove-redundant-or.js} | 1 - 2 files changed, 2 insertions(+), 2 deletions(-) rename js/server/tests/{aql-optimizer-rule-remove-redundant-OR.js => aql-optimizer-rule-remove-redundant-or.js} (99%) diff --git a/UnitTests/Makefile.unittests b/UnitTests/Makefile.unittests index 0796b7f15e..1f42f3b2d7 100755 --- a/UnitTests/Makefile.unittests +++ b/UnitTests/Makefile.unittests @@ -551,11 +551,12 @@ SHELL_SERVER_AQL = @top_srcdir@/js/server/tests/aql-arithmetic.js \ @top_srcdir@/js/server/tests/aql-optimizer-rule-move-calculations-up.js \ @top_srcdir@/js/server/tests/aql-optimizer-rule-move-filters-up.js \ @top_srcdir@/js/server/tests/aql-optimizer-rule-remove-redundant-calculations.js \ + @top_srcdir@/js/server/tests/aql-optimizer-rule-remove-redundant-or.js \ @top_srcdir@/js/server/tests/aql-optimizer-rule-remove-redundant-sorts.js \ @top_srcdir@/js/server/tests/aql-optimizer-rule-remove-unnecessary-calculations.js \ @top_srcdir@/js/server/tests/aql-optimizer-rule-remove-unnecessary-filters.js \ - @top_srcdir@/js/server/tests/aql-optimizer-rule-use-index-for-sort.js \ @top_srcdir@/js/server/tests/aql-optimizer-rule-replace-or-with-in.js \ + @top_srcdir@/js/server/tests/aql-optimizer-rule-use-index-for-sort.js \ @top_srcdir@/js/server/tests/aql-parse.js \ @top_srcdir@/js/server/tests/aql-primary-index-noncluster.js \ @top_srcdir@/js/server/tests/aql-queries-collection.js \ diff --git a/js/server/tests/aql-optimizer-rule-remove-redundant-OR.js b/js/server/tests/aql-optimizer-rule-remove-redundant-or.js similarity index 99% rename from js/server/tests/aql-optimizer-rule-remove-redundant-OR.js rename to js/server/tests/aql-optimizer-rule-remove-redundant-or.js index 8d710cfa0d..f451cc1f2b 100644 --- a/js/server/tests/aql-optimizer-rule-remove-redundant-OR.js +++ b/js/server/tests/aql-optimizer-rule-remove-redundant-or.js @@ -29,7 +29,6 @@ //////////////////////////////////////////////////////////////////////////////// // TODO add some test which don't use number values! -var internal = require("internal"); var jsunity = require("jsunity"); var helper = require("org/arangodb/aql-helper"); var getQueryResults = helper.getQueryResults; From 7695223744fcace4080d9161ebafd2f4f78bb30f Mon Sep 17 00:00:00 2001 From: Jan Steemann Date: Sat, 8 Nov 2014 21:13:41 +0100 Subject: [PATCH 15/36] use emplace_back --- arangod/Aql/ExecutionBlock.cpp | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/arangod/Aql/ExecutionBlock.cpp b/arangod/Aql/ExecutionBlock.cpp index 5ae94e4f46..59c238530c 100644 --- a/arangod/Aql/ExecutionBlock.cpp +++ b/arangod/Aql/ExecutionBlock.cpp @@ -1071,7 +1071,7 @@ bool IndexRangeBlock::readIndex (size_t atMost) { TRI_ASSERT(false); } - return (!_documents.empty()); + return (! _documents.empty()); } int IndexRangeBlock::initializeCursor (AqlItemBlock* items, size_t pos) { @@ -1314,7 +1314,7 @@ void IndexRangeBlock::readPrimaryIndex (IndexOrCondition const& ranges) { auto found = static_cast(TRI_LookupByKeyPrimaryIndex(primaryIndex, key.c_str())); if (found != nullptr) { - _documents.push_back(*found); + _documents.emplace_back(*found); } } } @@ -1382,7 +1382,7 @@ void IndexRangeBlock::readHashIndex (IndexOrCondition const& ranges, size_t atMo size_t const n = list._length; try { for (size_t i = 0; i < n; ++i) { - _documents.push_back(*(list._documents[i])); + _documents.emplace_back(*(list._documents[i])); } _engine->_stats.scannedIndex += static_cast(n); @@ -1439,7 +1439,7 @@ void IndexRangeBlock::readEdgeIndex (IndexOrCondition const& ranges) { // silently ignore all errors due to wrong _from / _to specifications auto&& result = TRI_LookupEdgesDocumentCollection(document, direction, documentCid, (TRI_voc_key_t) documentKey.c_str()); for (auto it : result) { - _documents.push_back((it)); + _documents.emplace_back(it); } _engine->_stats.scannedIndex += static_cast(result.size()); @@ -1568,7 +1568,7 @@ void IndexRangeBlock::readSkiplistIndex (IndexOrCondition const& ranges, size_t if (indexElement == nullptr || nrSent == atMost) { break; } - _documents.push_back(*(indexElement->_document)); + _documents.emplace_back(*(indexElement->_document)); ++nrSent; ++_engine->_stats.scannedIndex; } From e120bdec8c5671939b7905cdbe14228259fd5239 Mon Sep 17 00:00:00 2001 From: Jan Steemann Date: Sat, 8 Nov 2014 21:15:44 +0100 Subject: [PATCH 16/36] do not allocate too much space for hash index elements --- arangod/Aql/ExecutionBlock.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/arangod/Aql/ExecutionBlock.cpp b/arangod/Aql/ExecutionBlock.cpp index 59c238530c..33b1db772e 100644 --- a/arangod/Aql/ExecutionBlock.cpp +++ b/arangod/Aql/ExecutionBlock.cpp @@ -1345,7 +1345,7 @@ void IndexRangeBlock::readHashIndex (IndexOrCondition const& ranges, size_t atMo }; auto setupSearchValue = [&]() { - size_t const n = std::min(hashIndex->_paths._length, atMost); + size_t const n = hashIndex->_paths._length; searchValue._length = 0; searchValue._values = static_cast(TRI_Allocate(TRI_CORE_MEM_ZONE, n * sizeof(TRI_shaped_json_t), true)); From 738636db2d6cd00b4a63e124b041e87a637a510f Mon Sep 17 00:00:00 2001 From: Jan Steemann Date: Sat, 8 Nov 2014 21:26:54 +0100 Subject: [PATCH 17/36] whitespace --- arangod/Aql/ExecutionBlock.cpp | 40 +++++++++++++++++----------------- 1 file changed, 20 insertions(+), 20 deletions(-) diff --git a/arangod/Aql/ExecutionBlock.cpp b/arangod/Aql/ExecutionBlock.cpp index 33b1db772e..57b7df08c6 100644 --- a/arangod/Aql/ExecutionBlock.cpp +++ b/arangod/Aql/ExecutionBlock.cpp @@ -409,7 +409,7 @@ size_t ExecutionBlock::skipSome (size_t atLeast, size_t atMost) { bool ExecutionBlock::skip (size_t number) { size_t skipped = skipSome(number, number); size_t nr = skipped; - while ( nr != 0 && skipped < number ){ + while (nr != 0 && skipped < number) { nr = skipSome(number - skipped, number - skipped); skipped += nr; } @@ -485,7 +485,7 @@ int ExecutionBlock::getOrSkipSome (size_t atLeast, if (cur->size() - _pos > atMost - skipped) { // The current block is too large for atMost: - if (! skipping){ + if (! skipping) { unique_ptr more(cur->slice(_pos, _pos + (atMost - skipped))); collector.push_back(more.get()); more.release(); // do not delete it! @@ -496,7 +496,7 @@ int ExecutionBlock::getOrSkipSome (size_t atLeast, else if (_pos > 0) { // The current block fits into our result, but it is already // half-eaten: - if(! skipping){ + if (! skipping) { unique_ptr more(cur->slice(_pos, cur->size())); collector.push_back(more.get()); more.release(); @@ -590,7 +590,7 @@ int SingletonBlock::getOrSkipSome (size_t, // atLeast, return TRI_ERROR_NO_ERROR; } - if(! skipping){ + if (! skipping) { result = new AqlItemBlock(1, getPlanNode()->getRegisterPlan()->nrRegs[getPlanNode()->getDepth()]); try { if (_inputRegisterValues != nullptr) { @@ -1206,7 +1206,7 @@ size_t IndexRangeBlock::skipSome (size_t atLeast, size_t skipped = 0; - while (skipped < atLeast ){ + while (skipped < atLeast) { if (_buffer.empty()) { if (! ExecutionBlock::getBlock(DefaultBatchSize, DefaultBatchSize)) { _done = true; @@ -1590,7 +1590,7 @@ EnumerateListBlock::EnumerateListBlock (ExecutionEngine* engine, _inVarRegId(ExecutionNode::MaxRegisterId) { auto it = en->getRegisterPlan()->varInfo.find(en->_inVariable->id); - if (it == en->getRegisterPlan()->varInfo.end()){ + if (it == en->getRegisterPlan()->varInfo.end()) { THROW_ARANGO_EXCEPTION_MESSAGE(TRI_ERROR_INTERNAL, "variable not found"); } _inVarRegId = (*it).second.registerId; @@ -1735,7 +1735,7 @@ AqlItemBlock* EnumerateListBlock::getSome (size_t, size_t atMost) { _thisblock = 0; _seen = 0; // advance read position in the current block . . . - if (++_pos == cur->size() ) { + if (++_pos == cur->size()) { delete cur; _buffer.pop_front(); // does not throw _pos = 0; @@ -1757,7 +1757,7 @@ size_t EnumerateListBlock::skipSome (size_t atLeast, size_t atMost) { size_t skipped = 0; - while ( skipped < atLeast ) { + while (skipped < atLeast) { if (_buffer.empty()) { if (! ExecutionBlock::getBlock(DefaultBatchSize, DefaultBatchSize)) { _done = true; @@ -1789,7 +1789,7 @@ size_t EnumerateListBlock::skipSome (size_t atLeast, size_t atMost) { } case AqlValue::DOCVEC: { - if( _index == 0) { // this is a (maybe) new DOCVEC + if (_index == 0) { // this is a (maybe) new DOCVEC _DOCVECsize = 0; //we require the total number of items for (size_t i = 0; i < inVarReg._vector->size(); i++) { @@ -1843,7 +1843,7 @@ AqlValue EnumerateListBlock::getAqlValue (AqlValue inVarReg) { case AqlValue::DOCVEC: { // incoming doc vec has a single column AqlValue out = inVarReg._vector->at(_thisblock)->getValue(_index - _seen, 0).clone(); - if(++_index == (inVarReg._vector->at(_thisblock)->size() + _seen)){ + if (++_index == (inVarReg._vector->at(_thisblock)->size() + _seen)) { _seen += inVarReg._vector->at(_thisblock)->size(); _thisblock++; } @@ -2233,7 +2233,7 @@ int FilterBlock::getOrSkipSome (size_t atLeast, try { result = AqlItemBlock::concatenate(collector); } - catch (...){ + catch (...) { for (auto x : collector) { delete x; } @@ -2402,7 +2402,7 @@ int AggregateBlock::getOrSkipSome (size_t atLeast, if (newGroup) { if (! _currentGroup.groupValues[0].isEmpty()) { - if(! skipping){ + if (! skipping) { // need to emit the current group first emitGroup(cur, res.get(), skipped); } @@ -2449,7 +2449,7 @@ int AggregateBlock::getOrSkipSome (size_t atLeast, // no more input. we're done try { // emit last buffered group - if(! skipping){ + if (! skipping) { emitGroup(cur, res.get(), skipped); ++skipped; TRI_ASSERT(skipped > 0); @@ -2480,7 +2480,7 @@ int AggregateBlock::getOrSkipSome (size_t atLeast, } } - if(! skipping){ + if (! skipping) { TRI_ASSERT(skipped > 0); res->shrink(skipped); } @@ -3523,7 +3523,7 @@ int GatherBlock::initializeCursor (AqlItemBlock* items, size_t pos) { return res; } - if (!_isSimple) { + if (! _isSimple) { for (std::deque& x : _gatherBlockBuffer) { for (AqlItemBlock* y: x) { delete y; @@ -3595,13 +3595,13 @@ bool GatherBlock::hasMore () { if (_isSimple) { for (size_t i = 0; i < _dependencies.size(); i++) { - if(_dependencies.at(i)->hasMore()) { + if (_dependencies.at(i)->hasMore()) { return true; } } } else { - for (size_t i = 0; i < _gatherBlockBuffer.size(); i++){ + for (size_t i = 0; i < _gatherBlockBuffer.size(); i++) { if (! _gatherBlockBuffer.at(i).empty()) { return true; } @@ -3899,7 +3899,7 @@ BlockWithClients::BlockWithClients (ExecutionEngine* engine, : ExecutionBlock(engine, ep), _nrClients(shardIds.size()), _ignoreInitCursor(false), - _ignoreShutdown(false){ + _ignoreShutdown(false) { _shardIdMap.reserve(_nrClients); for (size_t i = 0; i < _nrClients; i++) { @@ -4176,7 +4176,7 @@ int ScatterBlock::getOrSkipSomeForShard (size_t atLeast, skipped = (std::min)(available, atMost); //nr rows in outgoing block - if (! skipping){ + if (! skipping) { result = _buffer.at(pos.first)->slice(pos.second, pos.second + skipped); } @@ -4349,7 +4349,7 @@ int DistributeBlock::getOrSkipSomeForShard (size_t atLeast, skipped = (std::min)(buf.size(), atMost); if (skipping) { - for (size_t i = 0; i < skipped; i++){ + for (size_t i = 0; i < skipped; i++) { buf.pop_front(); } freeCollector(); From 9c5d15086691e14a6efaf18f1761e77ecf6ab43c Mon Sep 17 00:00:00 2001 From: Jan Steemann Date: Sat, 8 Nov 2014 22:31:31 +0100 Subject: [PATCH 18/36] use Ast::ReverseOperator --- arangod/Aql/Ast.cpp | 31 +++++- arangod/Aql/Ast.h | 15 ++- arangod/Aql/Optimizer.cpp | 16 +-- arangod/Aql/Optimizer.h | 2 +- arangod/Aql/OptimizerRules.cpp | 97 ++++++++----------- arangod/Aql/OptimizerRules.h | 2 +- .../aql-optimizer-rule-remove-redundant-or.js | 2 +- .../aql-optimizer-rule-replace-or-with-in.js | 2 +- 8 files changed, 96 insertions(+), 71 deletions(-) diff --git a/arangod/Aql/Ast.cpp b/arangod/Aql/Ast.cpp index db70bf0b33..ba5e11482d 100644 --- a/arangod/Aql/Ast.cpp +++ b/arangod/Aql/Ast.cpp @@ -81,7 +81,7 @@ AstNode const Ast::EmptyStringNode{ "", VALUE_TYPE_STRING }; /// @brief inverse comparison operators //////////////////////////////////////////////////////////////////////////////// -std::unordered_map const Ast::ReverseOperators{ +std::unordered_map const Ast::NegatedOperators{ { static_cast(NODE_TYPE_OPERATOR_BINARY_EQ), NODE_TYPE_OPERATOR_BINARY_NE }, { static_cast(NODE_TYPE_OPERATOR_BINARY_NE), NODE_TYPE_OPERATOR_BINARY_EQ }, { static_cast(NODE_TYPE_OPERATOR_BINARY_GT), NODE_TYPE_OPERATOR_BINARY_LE }, @@ -92,6 +92,18 @@ std::unordered_map const Ast::ReverseOperators{ { static_cast(NODE_TYPE_OPERATOR_BINARY_NIN), NODE_TYPE_OPERATOR_BINARY_IN } }; +//////////////////////////////////////////////////////////////////////////////// +/// @brief reverse comparison operators +//////////////////////////////////////////////////////////////////////////////// + +std::unordered_map const Ast::ReversedOperators{ + { static_cast(NODE_TYPE_OPERATOR_BINARY_EQ), NODE_TYPE_OPERATOR_BINARY_EQ }, + { static_cast(NODE_TYPE_OPERATOR_BINARY_GT), NODE_TYPE_OPERATOR_BINARY_LT }, + { static_cast(NODE_TYPE_OPERATOR_BINARY_GE), NODE_TYPE_OPERATOR_BINARY_LE }, + { static_cast(NODE_TYPE_OPERATOR_BINARY_LT), NODE_TYPE_OPERATOR_BINARY_GT }, + { static_cast(NODE_TYPE_OPERATOR_BINARY_LE), NODE_TYPE_OPERATOR_BINARY_GE } +}; + // ----------------------------------------------------------------------------- // --SECTION-- constructors / destructors // ----------------------------------------------------------------------------- @@ -1083,6 +1095,19 @@ AstNode* Ast::clone (AstNode const* node) { return copy; } +//////////////////////////////////////////////////////////////////////////////// +/// @brief get the reversed operator for a comparison operator +//////////////////////////////////////////////////////////////////////////////// + +AstNodeType Ast::ReverseOperator (AstNodeType type) { + auto it = ReversedOperators.find(static_cast(type)); + if (it == ReversedOperators.end()) { + THROW_ARANGO_EXCEPTION_MESSAGE(TRI_ERROR_INTERNAL, "invalid node type for inversed operator"); + } + + return (*it).second; +} + // ----------------------------------------------------------------------------- // --SECTION-- private methods // ----------------------------------------------------------------------------- @@ -1222,8 +1247,8 @@ AstNode* Ast::optimizeNotExpression (AstNode* node) { auto lhs = operand->getMember(0); auto rhs = operand->getMember(1); - auto it = ReverseOperators.find(static_cast(operand->type)); - TRI_ASSERT(it != ReverseOperators.end()); + auto it = NegatedOperators.find(static_cast(operand->type)); + TRI_ASSERT(it != NegatedOperators.end()); return createNodeBinaryOperator((*it).second, lhs, rhs); } diff --git a/arangod/Aql/Ast.h b/arangod/Aql/Ast.h index 2b7cc5061b..d0da25298b 100644 --- a/arangod/Aql/Ast.h +++ b/arangod/Aql/Ast.h @@ -489,6 +489,12 @@ namespace triagens { AstNode* clone (AstNode const*); +//////////////////////////////////////////////////////////////////////////////// +/// @brief get the reversed operator for a comparison operator +//////////////////////////////////////////////////////////////////////////////// + + static AstNodeType ReverseOperator (AstNodeType); + // ----------------------------------------------------------------------------- // --SECTION-- private methods // ----------------------------------------------------------------------------- @@ -624,11 +630,16 @@ namespace triagens { public: //////////////////////////////////////////////////////////////////////////////// -/// @brief inverse comparison operators +/// @brief negated comparison operators //////////////////////////////////////////////////////////////////////////////// - static std::unordered_map const ReverseOperators; + static std::unordered_map const NegatedOperators; +//////////////////////////////////////////////////////////////////////////////// +/// @brief reverse comparison operators +//////////////////////////////////////////////////////////////////////////////// + + static std::unordered_map const ReversedOperators; // ----------------------------------------------------------------------------- // --SECTION-- private variables diff --git a/arangod/Aql/Optimizer.cpp b/arangod/Aql/Optimizer.cpp index 496d477f79..86df5cd4d6 100644 --- a/arangod/Aql/Optimizer.cpp +++ b/arangod/Aql/Optimizer.cpp @@ -452,15 +452,15 @@ void Optimizer::setupRules () { ////////////////////////////////////////////////////////////////////////////// // try to replace simple OR conditions with IN - registerRule("replace-OR-with-IN", + registerRule("replace-or-with-in", replaceOrWithIn, replaceOrWithIn_pass6, true); - // try to remove redundant OR - registerRule("remove-redundant-OR", - removeRedundantOR, - removeRedundantOR_pass6, + // try to remove redundant OR conditions + registerRule("remove-redundant-or", + removeRedundantOr, + removeRedundantOr_pass6, true); // try to find a filter after an enumerate collection and find an index . . . @@ -474,10 +474,10 @@ void Optimizer::setupRules () { useIndexForSort, useIndexForSort_pass6, true); - -#if 0 + +#if 0 // try to remove filters which are covered by index ranges - // rule seems to work, but tests are still missing + // TODO: rule seems to work, but tests are still missing registerRule("remove-filter-covered-by-index", removeFiltersCoveredByIndex, removeFiltersCoveredByIndex_pass6, diff --git a/arangod/Aql/Optimizer.h b/arangod/Aql/Optimizer.h index 04296aa811..7db3f62c91 100644 --- a/arangod/Aql/Optimizer.h +++ b/arangod/Aql/Optimizer.h @@ -131,7 +131,7 @@ namespace triagens { replaceOrWithIn_pass6 = 810, // remove redundant OR conditions - removeRedundantOR_pass6 = 820, + removeRedundantOr_pass6 = 820, // try to find a filter after an enumerate collection and find an index . . . useIndexRange_pass6 = 830, diff --git a/arangod/Aql/OptimizerRules.cpp b/arangod/Aql/OptimizerRules.cpp index 548a510f52..c2550e8b53 100644 --- a/arangod/Aql/OptimizerRules.cpp +++ b/arangod/Aql/OptimizerRules.cpp @@ -1477,9 +1477,7 @@ int triagens::aql::useIndexForSort (Optimizer* opt, return TRI_ERROR_NO_ERROR; } -#if 0 // TODO: finish rule and test it - struct FilterCondition { std::string variableName; std::string attributeName; @@ -1561,19 +1559,17 @@ struct FilterCondition { lhs = node->getMember(1); rhs = node->getMember(0); - auto it = Ast::ReverseOperators.find(static_cast(node->type)); - TRI_ASSERT(it != Ast::ReverseOperators.end()); - - op = (*it).second; + op = Ast::ReverseOperator(node->type); } if (found) { TRI_ASSERT(lhs->type == NODE_TYPE_VALUE); TRI_ASSERT(rhs->type == NODE_TYPE_ATTRIBUTE_ACCESS); - std::function buildName = [&] (AstNode const* node) -> void { + std::function buildName = + [&] (AstNode const* node, std::string& variableName, std::string& attributeName) -> void { if (node->type == NODE_TYPE_ATTRIBUTE_ACCESS) { - buildName(node->getMember(0)); + buildName(node->getMember(0), variableName, attributeName); if (! attributeName.empty()) { attributeName.push_back('.'); @@ -1588,7 +1584,9 @@ struct FilterCondition { }; if (attributeName.empty()) { - buildName(rhs); + TRI_ASSERT(! variableName.empty()); + + buildName(rhs, variableName, attributeName); if (op == NODE_TYPE_OPERATOR_BINARY_EQ || op == NODE_TYPE_OPERATOR_BINARY_NE) { lowInclusive = true; @@ -1615,10 +1613,19 @@ struct FilterCondition { return true; } - // else if (attributeName == std::string(buffer.c_str(), buffer.length())) { - // same attribute - // TODO - // } + else { + // already have collected something, now check if the next condition + // is for the same variable / attribute + std::string compareVariableName; + std::string compareAttributeName; + buildName(rhs, compareVariableName, compareAttributeName); + + if (variableName == compareVariableName && + attributeName == compareAttributeName) { + // same attribute + // TODO + } + } // fall-through } @@ -1638,7 +1645,6 @@ struct FilterCondition { }; - //////////////////////////////////////////////////////////////////////////////// /// @brief try to remove filters which are covered by indexes //////////////////////////////////////////////////////////////////////////////// @@ -1657,7 +1663,9 @@ int triagens::aql::removeFiltersCoveredByIndex (Optimizer* opt, // auto outVar = cn->getVariablesSetHere(); auto setter = plan->getVarSetBy(inVar[0]->id); - TRI_ASSERT(setter != nullptr); + if (setter == nullptr) { + continue; + } if (setter->getType() != EN::CALCULATION) { continue; @@ -1717,7 +1725,6 @@ int triagens::aql::removeFiltersCoveredByIndex (Optimizer* opt, return TRI_ERROR_NO_ERROR; } -#endif //////////////////////////////////////////////////////////////////////////////// /// @brief helper to compute lots of permutation tuples @@ -2761,8 +2768,7 @@ int triagens::aql::replaceOrWithIn (Optimizer* opt, LEAVE_BLOCK; } -struct RemoveRedundantOR { - +struct RemoveRedundantOr { AstNode const* bestValue = nullptr; AstNodeType comparison; bool inclusive; @@ -2791,19 +2797,22 @@ struct RemoveRedundantOR { (type == NODE_TYPE_OPERATOR_BINARY_GE || type == NODE_TYPE_OPERATOR_BINARY_GT)) { return -1; //high bound - } else if ((comparison == NODE_TYPE_OPERATOR_BINARY_GE + } + else if ((comparison == NODE_TYPE_OPERATOR_BINARY_GE || comparison == NODE_TYPE_OPERATOR_BINARY_GT) && (type == NODE_TYPE_OPERATOR_BINARY_LE || type == NODE_TYPE_OPERATOR_BINARY_LT)) { return 1; //low bound } - } else { + } + else { if ((comparison == NODE_TYPE_OPERATOR_BINARY_LE || comparison == NODE_TYPE_OPERATOR_BINARY_LT) && (type == NODE_TYPE_OPERATOR_BINARY_LE || type == NODE_TYPE_OPERATOR_BINARY_LT)) { return -1; //high bound - } else if ((comparison == NODE_TYPE_OPERATOR_BINARY_GE + } + else if ((comparison == NODE_TYPE_OPERATOR_BINARY_GE || comparison == NODE_TYPE_OPERATOR_BINARY_GT) && (type == NODE_TYPE_OPERATOR_BINARY_GE || type == NODE_TYPE_OPERATOR_BINARY_GT)) { @@ -2823,25 +2832,10 @@ struct RemoveRedundantOR { return (isInclusiveBound(type) ? true : false); } return (cmp * lowhigh == 1); - }; - - AstNodeType reverseComparison (AstNodeType type) { - if (type == NODE_TYPE_OPERATOR_BINARY_LE) { - return NODE_TYPE_OPERATOR_BINARY_GE; - } - else if (type == NODE_TYPE_OPERATOR_BINARY_LT) { - return NODE_TYPE_OPERATOR_BINARY_GT; - } - else if (type == NODE_TYPE_OPERATOR_BINARY_GT) { - return NODE_TYPE_OPERATOR_BINARY_LT; - } - else { - return NODE_TYPE_OPERATOR_BINARY_LE; - } } bool hasRedundantCondition (AstNode const* node) { - if(finder.find(node, NODE_TYPE_OPERATOR_BINARY_LT, commonNode, commonName)){ + if (finder.find(node, NODE_TYPE_OPERATOR_BINARY_LT, commonNode, commonName)) { return hasRedundantConditionWalker(node); } return false; @@ -2855,10 +2849,10 @@ struct RemoveRedundantOR { hasRedundantConditionWalker(node->getMember(1))); } - if( type == NODE_TYPE_OPERATOR_BINARY_LE + if (type == NODE_TYPE_OPERATOR_BINARY_LE || type == NODE_TYPE_OPERATOR_BINARY_LT || type == NODE_TYPE_OPERATOR_BINARY_GE - || type == NODE_TYPE_OPERATOR_BINARY_GT ) { + || type == NODE_TYPE_OPERATOR_BINARY_GT) { auto lhs = node->getMember(0); auto rhs = node->getMember(1); @@ -2867,8 +2861,8 @@ struct RemoveRedundantOR { && ! hasRedundantConditionWalker(lhs) && lhs->isConstant()) { - if (!isComparisonSet) { - comparison = reverseComparison(type); + if (! isComparisonSet) { + comparison = Ast::ReverseOperator(type); bestValue = lhs; isComparisonSet = true; return true; @@ -2880,7 +2874,7 @@ struct RemoveRedundantOR { } if (compareBounds(type, lhs, lowhigh)) { - comparison = reverseComparison(type); + comparison = Ast::ReverseOperator(type); bestValue = lhs; } return true; @@ -2888,7 +2882,7 @@ struct RemoveRedundantOR { if (hasRedundantConditionWalker(lhs) && ! hasRedundantConditionWalker(rhs) && rhs->isConstant()) { - if (!isComparisonSet) { + if (! isComparisonSet) { comparison = type; bestValue = rhs; isComparisonSet = true; @@ -2915,7 +2909,8 @@ struct RemoveRedundantOR { type == NODE_TYPE_INDEXED_ACCESS) { // get a string representation of the node for comparisons return (node->toString() == commonName); - } else if (node->isBoolValue()) { + } + else if (node->isBoolValue()) { return true; } @@ -2923,7 +2918,7 @@ struct RemoveRedundantOR { } }; -int triagens::aql::removeRedundantOR (Optimizer* opt, +int triagens::aql::removeRedundantOr (Optimizer* opt, ExecutionPlan* plan, Optimizer::Rule const* rule) { ENTER_BLOCK; @@ -2951,19 +2946,13 @@ int triagens::aql::removeRedundantOR (Optimizer* opt, continue; } - RemoveRedundantOR remover; - if(remover.hasRedundantCondition(cn->expression()->node())){ + RemoveRedundantOr remover; + if (remover.hasRedundantCondition(cn->expression()->node())) { Expression* expr = nullptr; ExecutionNode* newNode = nullptr; auto astNode = remover.createReplacementNode(plan->getAst()); - try { - expr = new Expression(plan->getAst(), astNode); - } - catch (...) { - delete astNode; - throw; - } + expr = new Expression(plan->getAst(), astNode); try { newNode = new CalculationNode(plan, plan->nextId(), expr, outVar[0]); diff --git a/arangod/Aql/OptimizerRules.h b/arangod/Aql/OptimizerRules.h index 538442068b..3abf7a570c 100644 --- a/arangod/Aql/OptimizerRules.h +++ b/arangod/Aql/OptimizerRules.h @@ -182,7 +182,7 @@ namespace triagens { int replaceOrWithIn (Optimizer*, ExecutionPlan*, Optimizer::Rule const*); - int removeRedundantOR (Optimizer*, ExecutionPlan*, Optimizer::Rule const*); + int removeRedundantOr (Optimizer*, ExecutionPlan*, Optimizer::Rule const*); } // namespace aql } // namespace triagens diff --git a/js/server/tests/aql-optimizer-rule-remove-redundant-or.js b/js/server/tests/aql-optimizer-rule-remove-redundant-or.js index f451cc1f2b..193b70ede5 100644 --- a/js/server/tests/aql-optimizer-rule-remove-redundant-or.js +++ b/js/server/tests/aql-optimizer-rule-remove-redundant-or.js @@ -38,7 +38,7 @@ var getQueryResults = helper.getQueryResults; //////////////////////////////////////////////////////////////////////////////// function NewAqlRemoveRedundantORTestSuite () { - var ruleName = "remove-redundant-OR"; + var ruleName = "remove-redundant-or"; var isRuleUsed = function (query, params) { var result = AQL_EXPLAIN(query, params, { optimizer: { rules: [ "-all", "+" + ruleName ] } }); diff --git a/js/server/tests/aql-optimizer-rule-replace-or-with-in.js b/js/server/tests/aql-optimizer-rule-replace-or-with-in.js index c7a18f22ea..7cc96621a2 100644 --- a/js/server/tests/aql-optimizer-rule-replace-or-with-in.js +++ b/js/server/tests/aql-optimizer-rule-replace-or-with-in.js @@ -39,7 +39,7 @@ var getQueryResults = helper.getQueryResults; function NewAqlReplaceORWithINTestSuite () { var replace; - var ruleName = "replace-OR-with-IN"; + var ruleName = "replace-or-with-in"; var isRuleUsed = function (query, params) { var result = AQL_EXPLAIN(query, params, { optimizer: { rules: [ "-all", "+" + ruleName ] } }); From 23ea59d4056f8ddbda7ebdcd7bc7e95ffadd2ed8 Mon Sep 17 00:00:00 2001 From: James Date: Tue, 11 Nov 2014 08:43:30 +0000 Subject: [PATCH 19/36] cleaning up, using ReverseOperator. --- arangod/Aql/OptimizerRules.cpp | 42 +++++++++++----------------------- 1 file changed, 13 insertions(+), 29 deletions(-) diff --git a/arangod/Aql/OptimizerRules.cpp b/arangod/Aql/OptimizerRules.cpp index 8928b222d5..0d166022be 100644 --- a/arangod/Aql/OptimizerRules.cpp +++ b/arangod/Aql/OptimizerRules.cpp @@ -2868,35 +2868,19 @@ struct RemoveRedundantOr { return (type == NODE_TYPE_OPERATOR_BINARY_GE || type == NODE_TYPE_OPERATOR_BINARY_LE); } - int isCompatibleBound (AstNodeType type, AstNode const* value, bool reverse) { + int isCompatibleBound (AstNodeType type, AstNode const* value) { - if (reverse) { - if ((comparison == NODE_TYPE_OPERATOR_BINARY_LE - || comparison == NODE_TYPE_OPERATOR_BINARY_LT) && - (type == NODE_TYPE_OPERATOR_BINARY_GE - || type == NODE_TYPE_OPERATOR_BINARY_GT)) { - return -1; //high bound - } - else if ((comparison == NODE_TYPE_OPERATOR_BINARY_GE - || comparison == NODE_TYPE_OPERATOR_BINARY_GT) && - (type == NODE_TYPE_OPERATOR_BINARY_LE - || type == NODE_TYPE_OPERATOR_BINARY_LT)) { - return 1; //low bound - } + if ((comparison == NODE_TYPE_OPERATOR_BINARY_LE + || comparison == NODE_TYPE_OPERATOR_BINARY_LT) && + (type == NODE_TYPE_OPERATOR_BINARY_LE + || type == NODE_TYPE_OPERATOR_BINARY_LT)) { + return -1; //high bound } - else { - if ((comparison == NODE_TYPE_OPERATOR_BINARY_LE - || comparison == NODE_TYPE_OPERATOR_BINARY_LT) && - (type == NODE_TYPE_OPERATOR_BINARY_LE - || type == NODE_TYPE_OPERATOR_BINARY_LT)) { - return -1; //high bound - } - else if ((comparison == NODE_TYPE_OPERATOR_BINARY_GE - || comparison == NODE_TYPE_OPERATOR_BINARY_GT) && - (type == NODE_TYPE_OPERATOR_BINARY_GE - || type == NODE_TYPE_OPERATOR_BINARY_GT)) { - return 1; //low bound - } + else if ((comparison == NODE_TYPE_OPERATOR_BINARY_GE + || comparison == NODE_TYPE_OPERATOR_BINARY_GT) && + (type == NODE_TYPE_OPERATOR_BINARY_GE + || type == NODE_TYPE_OPERATOR_BINARY_GT)) { + return 1; //low bound } return 0; //incompatible bounds } @@ -2947,7 +2931,7 @@ struct RemoveRedundantOr { return true; } - int lowhigh = isCompatibleBound(type, lhs, true); + int lowhigh = isCompatibleBound(Ast::ReverseOperator(type), lhs); if (lowhigh == 0) { return false; } @@ -2968,7 +2952,7 @@ struct RemoveRedundantOr { return true; } - int lowhigh = isCompatibleBound(type, rhs, false); + int lowhigh = isCompatibleBound(type, rhs); if (lowhigh == 0) { return false; } From 7d8f2f03eddcfac8ba39cfd490af61befb099d02 Mon Sep 17 00:00:00 2001 From: James Date: Tue, 11 Nov 2014 08:45:03 +0000 Subject: [PATCH 20/36] applying isConstant patch --- arangod/Aql/AstNode.cpp | 26 ++++++++++++++++++++++++++ 1 file changed, 26 insertions(+) diff --git a/arangod/Aql/AstNode.cpp b/arangod/Aql/AstNode.cpp index 776a3a522d..f3d5d5138d 100644 --- a/arangod/Aql/AstNode.cpp +++ b/arangod/Aql/AstNode.cpp @@ -621,6 +621,23 @@ TRI_json_t* AstNode::toJsonValue (TRI_memory_zone_t* zone) const { return array; } + if (type == NODE_TYPE_ATTRIBUTE_ACCESS) { + TRI_json_t* j = getMember(0)->toJsonValue(zone); + + if (j != nullptr) { + if (TRI_IsArrayJson(j)) { + TRI_json_t* v = TRI_LookupArrayJson(j, getStringValue()); + if (v != nullptr) { + TRI_json_t* copy = TRI_CopyJson(zone, v); + TRI_FreeJson(zone, j); + return copy; + } + } + TRI_FreeJson(zone, j); + return TRI_CreateNullJson(zone); + } + } + return nullptr; } @@ -1099,10 +1116,12 @@ bool AstNode::isSimple () const { bool AstNode::isConstant () const { if (hasFlag(FLAG_CONSTANT)) { + TRI_ASSERT(! hasFlag(FLAG_DYNAMIC)); // fast track exit return true; } if (hasFlag(FLAG_DYNAMIC)) { + TRI_ASSERT(! hasFlag(FLAG_CONSTANT)); // fast track exit return false; } @@ -1154,6 +1173,13 @@ bool AstNode::isConstant () const { return true; } + if (type == NODE_TYPE_ATTRIBUTE_ACCESS) { + if (getMember(0)->isConstant()) { + setFlag(FLAG_CONSTANT); + return true; + } + } + setFlag(FLAG_DYNAMIC); return false; } From ed91b1b1cd9ee7cd37e6c32766e762c37ecc5e1d Mon Sep 17 00:00:00 2001 From: James Date: Tue, 11 Nov 2014 08:49:16 +0000 Subject: [PATCH 21/36] fixing test. --- .../tests/aql-optimizer-rule-replace-or-with-in.js | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-) diff --git a/js/server/tests/aql-optimizer-rule-replace-or-with-in.js b/js/server/tests/aql-optimizer-rule-replace-or-with-in.js index 7cc96621a2..438ffde479 100644 --- a/js/server/tests/aql-optimizer-rule-replace-or-with-in.js +++ b/js/server/tests/aql-optimizer-rule-replace-or-with-in.js @@ -389,23 +389,19 @@ function NewAqlReplaceORWithINTestSuite () { }, - testFiresCommonConstant1: function () { + testDudCommonConstant1: function () { var query = "LET x = {a:@a} FOR v IN " + replace.name() + " FILTER x.a == v.value || x.a == v._key RETURN v._key"; var key = replace.any()._key; - isRuleUsed(query, {a: key}); - - var actual = getQueryResults(query, {a: key}); - assertEqual(key, actual.toString()); - + ruleIsNotUsed(query, {a: key}); }, - testFiresCommonConstant2: function () { + testDudCommonConstant2: function () { var query = "LET x = {a:1} FOR v IN " + replace.name() + " FILTER x.a == v.value || x.a == v._key RETURN v._key"; - isRuleUsed(query, {}); + ruleIsNotUsed(query, {}); }, testDudAlwaysTrue: function () { From d6e1971fd3070066dbabbe776fc7a0e3112fd381 Mon Sep 17 00:00:00 2001 From: James Date: Tue, 11 Nov 2014 11:29:46 +0000 Subject: [PATCH 22/36] lazy index working for skiplists --- arangod/Aql/ExecutionBlock.cpp | 143 +++++++++++++++++++++------------ arangod/Aql/ExecutionBlock.h | 6 +- 2 files changed, 97 insertions(+), 52 deletions(-) diff --git a/arangod/Aql/ExecutionBlock.cpp b/arangod/Aql/ExecutionBlock.cpp index 57b7df08c6..57fbee7932 100644 --- a/arangod/Aql/ExecutionBlock.cpp +++ b/arangod/Aql/ExecutionBlock.cpp @@ -129,7 +129,7 @@ void AggregatorGroup::addValues (AqlItemBlock const* src, /// @brief batch size value //////////////////////////////////////////////////////////////////////////////// -size_t const ExecutionBlock::DefaultBatchSize = 1000; +size_t const ExecutionBlock::DefaultBatchSize = 11; // ----------------------------------------------------------------------------- // --SECTION-- constructors / destructors @@ -579,11 +579,12 @@ int SingletonBlock::shutdown (int errorCode) { } int SingletonBlock::getOrSkipSome (size_t, // atLeast, - size_t, // atMost, + size_t atMost, // atMost, bool skipping, AqlItemBlock*& result, size_t& skipped) { + std::cout << "SingletonBlock::getOrSkipSome atMost = " << atMost << "\n"; TRI_ASSERT(result == nullptr && skipped == 0); if (_done) { @@ -848,7 +849,8 @@ IndexRangeBlock::IndexRangeBlock (ExecutionEngine* engine, : ExecutionBlock(engine, en), _collection(en->collection()), _posInDocs(0), - _allBoundsConstant(true) { + _allBoundsConstant(true), + _skiplistIterator(nullptr){ std::vector> const& orRanges = en->_ranges; TRI_ASSERT(en->_index != nullptr); @@ -936,23 +938,11 @@ int IndexRangeBlock::initialize () { return res; } -bool IndexRangeBlock::readIndex (size_t atMost) { - // This is either called from initialize if all bounds are constant, - // in this case it is never called again. If there is at least one - // variable bound, then readIndex is called once for every item coming - // in from our dependency. In that case, it is guaranteed that - // _buffer is not empty, in particular _buffer.front() is defined - // _pos points to a position in _buffer.front() - // Therefore, we can use the register values in _buffer.front() in row - // _pos to evaluate the variable bounds. - - if (_documents.empty()) { - _documents.reserve(atMost); - } - else { //FIXME does this trash some stuff unnecessarily? - _documents.clear(); - } +// init the index for reading, this should be called once per new incoming +// block! +bool IndexRangeBlock::initIndex () { + auto en = static_cast(getPlanNode()); IndexOrCondition const* condition = &en->_ranges; @@ -1052,20 +1042,53 @@ bool IndexRangeBlock::readIndex (size_t atMost) { condition = newCondition.get(); } + + if (en->_index->type == TRI_IDX_TYPE_SKIPLIST_INDEX) { + initSkiplistIndex(*condition); + return (_skiplistIterator != nullptr); + }//TODO the other cases!! + else { + TRI_ASSERT(false); + } + return false; //FIXME remove this +} + +// this is called every time everything in _documents has been passed on + +bool IndexRangeBlock::readIndex (size_t atMost) { + // TODO: update this comment, which is now out of date!! + // This is either called from initialize if all bounds are constant, + // in this case it is never called again. If there is at least one + // variable bound, then readIndex is called once for every item coming + // in from our dependency. In that case, it is guaranteed that + // _buffer is not empty, in particular _buffer.front() is defined + // _pos points to a position in _buffer.front() + // Therefore, we can use the register values in _buffer.front() in row + // _pos to evaluate the variable bounds. + + if (_documents.empty()) { + _documents.reserve(atMost); + } + else { + _documents.clear(); + } + + auto en = static_cast(getPlanNode()); + IndexOrCondition const* condition = &en->_ranges; //TODO remove this line if (en->_index->type == TRI_IDX_TYPE_PRIMARY_INDEX) { // atMost not passed since only equality is supported - readPrimaryIndex(*condition); + readPrimaryIndex(*condition); //TODO correct } else if (en->_index->type == TRI_IDX_TYPE_HASH_INDEX) { - readHashIndex(*condition, atMost); + readHashIndex(*condition, atMost); //TODO correct } else if (en->_index->type == TRI_IDX_TYPE_SKIPLIST_INDEX) { - readSkiplistIndex(*condition, atMost); + readSkiplistIndex(atMost); } else if (en->_index->type == TRI_IDX_TYPE_EDGE_INDEX) { // atMost not passed since only equality is supported - readEdgeIndex(*condition); + readEdgeIndex(*condition); //TODO correct } else { TRI_ASSERT(false); @@ -1095,6 +1118,7 @@ int IndexRangeBlock::initializeCursor (AqlItemBlock* items, size_t pos) { AqlItemBlock* IndexRangeBlock::getSome (size_t atLeast, size_t atMost) { + std::cout << "IndexRangeBlock::getSome atMost = " << atMost << "\n"; if (_done) { return nullptr; } @@ -1115,11 +1139,14 @@ AqlItemBlock* IndexRangeBlock::getSome (size_t atLeast, } _pos = 0; // this is in the first block - // This is a new item, so let's read the index if bounds are variable: - //if (! _allBoundsConstant) { + // This is a new item, so let's init and read the index + if(initIndex()) { //successfully initted the index readIndex(atMost); - //} - + } + else { //failed to init the index, nothing to pass on + _done = true; + return nullptr; + } _posInDocs = 0; // position in _documents . . . } @@ -1171,19 +1198,19 @@ AqlItemBlock* IndexRangeBlock::getSome (size_t atLeast, _posInDocs = 0; - if (++_pos >= cur->size()) { - _buffer.pop_front(); // does not throw - delete cur; - _pos = 0; + if (! readIndex(atMost)) { //no more output from this version of the index + if (++_pos >= cur->size()) { + _buffer.pop_front(); // does not throw + delete cur; + _pos = 0; + } + if (! _buffer.empty()) { + initIndex(); //TODO if this returns false, what to do? + readIndex(atMost); + } + // If _buffer is empty, then we will fetch a new block in the next call + // and then init/read the index. } - - // let's read the index if bounds are variable: - if (! _buffer.empty()) { - readIndex(atMost); - } - // If _buffer is empty, then we will fetch a new block in the next call - // and then read the index. - } } while (res.get() == nullptr); @@ -1345,7 +1372,7 @@ void IndexRangeBlock::readHashIndex (IndexOrCondition const& ranges, size_t atMo }; auto setupSearchValue = [&]() { - size_t const n = hashIndex->_paths._length; + size_t const n = (std::min)(hashIndex->_paths._length, atMost); searchValue._length = 0; searchValue._values = static_cast(TRI_Allocate(TRI_CORE_MEM_ZONE, n * sizeof(TRI_shaped_json_t), true)); @@ -1483,14 +1510,16 @@ void IndexRangeBlock::readEdgeIndex (IndexOrCondition const& ranges) { // (i.e. the 1 in x.c >= 1) cannot be lists or arrays. // -void IndexRangeBlock::readSkiplistIndex (IndexOrCondition const& ranges, size_t atMost) { +void IndexRangeBlock::initSkiplistIndex (IndexOrCondition const& ranges) { + TRI_ASSERT(_skiplistIterator == nullptr) + auto en = static_cast(getPlanNode()); TRI_index_t* idx = en->_index->data; TRI_ASSERT(idx != nullptr); - + TRI_shaper_t* shaper = _collection->documentCollection()->getShaper(); TRI_ASSERT(shaper != nullptr); - + TRI_index_operator_t* skiplistOperator = nullptr; Json parameters(Json::List); @@ -1546,12 +1575,12 @@ void IndexRangeBlock::readSkiplistIndex (IndexOrCondition const& ranges, size_t } } - TRI_skiplist_iterator_t* skiplistIterator = TRI_LookupSkiplistIndex(idx, skiplistOperator, en->_reverse); + _skiplistIterator = TRI_LookupSkiplistIndex(idx, skiplistOperator, en->_reverse); if (skiplistOperator != nullptr) { TRI_FreeIndexOperator(skiplistOperator); } - - if (skiplistIterator == nullptr) { + + if (_skiplistIterator == nullptr) { int res = TRI_errno(); if (res == TRI_RESULT_ELEMENT_NOT_FOUND) { return; @@ -1559,23 +1588,34 @@ void IndexRangeBlock::readSkiplistIndex (IndexOrCondition const& ranges, size_t THROW_ARANGO_EXCEPTION(TRI_ERROR_ARANGO_NO_INDEX); } +} +void IndexRangeBlock::readSkiplistIndex (size_t atMost) { + + if (_skiplistIterator == nullptr) { + return; + } + try { size_t nrSent = 0; - while (true) { - TRI_skiplist_index_element_t* indexElement = skiplistIterator->next(skiplistIterator); + TRI_skiplist_index_element_t* indexElement; + while (nrSent < atMost) { + indexElement = _skiplistIterator->next(_skiplistIterator); - if (indexElement == nullptr || nrSent == atMost) { + if (indexElement == nullptr) { break; } _documents.emplace_back(*(indexElement->_document)); ++nrSent; ++_engine->_stats.scannedIndex; } - TRI_FreeSkiplistIterator(skiplistIterator); + if (indexElement == nullptr) { + TRI_FreeSkiplistIterator(_skiplistIterator); + _skiplistIterator = nullptr; + } } catch (...) { - TRI_FreeSkiplistIterator(skiplistIterator); + TRI_FreeSkiplistIterator(_skiplistIterator); throw; } } @@ -2853,6 +2893,7 @@ int LimitBlock::getOrSkipSome (size_t atLeast, AqlItemBlock* ReturnBlock::getSome (size_t atLeast, size_t atMost) { + std::cout << "ReturnBlock::getSome atMost = " << atMost << "\n"; auto res = ExecutionBlock::getSomeWithoutRegisterClearout(atLeast, atMost); if (res == nullptr) { diff --git a/arangod/Aql/ExecutionBlock.h b/arangod/Aql/ExecutionBlock.h index 14f296583b..18ac8347e2 100644 --- a/arangod/Aql/ExecutionBlock.h +++ b/arangod/Aql/ExecutionBlock.h @@ -567,6 +567,7 @@ namespace triagens { //////////////////////////////////////////////////////////////////////////////// bool readIndex (size_t atMost); + bool initIndex (); //////////////////////////////////////////////////////////////////////////////// /// @brief read using the primary index @@ -584,7 +585,8 @@ namespace triagens { /// @brief read using a skiplist index //////////////////////////////////////////////////////////////////////////////// - void readSkiplistIndex (IndexOrCondition const&, size_t atMost); + void readSkiplistIndex (size_t atMost); + void initSkiplistIndex (IndexOrCondition const&); //////////////////////////////////////////////////////////////////////////////// /// @brief read using a hash index @@ -644,6 +646,8 @@ namespace triagens { std::vector> _inRegs; + TRI_skiplist_iterator_t* _skiplistIterator; + }; // ----------------------------------------------------------------------------- From 14668133139b5f3286f88898fdfb3b594086a4a0 Mon Sep 17 00:00:00 2001 From: James Date: Tue, 11 Nov 2014 11:32:48 +0000 Subject: [PATCH 23/36] cleaning up --- arangod/Aql/ExecutionBlock.cpp | 16 ++++++---------- 1 file changed, 6 insertions(+), 10 deletions(-) diff --git a/arangod/Aql/ExecutionBlock.cpp b/arangod/Aql/ExecutionBlock.cpp index 57fbee7932..7b04891b62 100644 --- a/arangod/Aql/ExecutionBlock.cpp +++ b/arangod/Aql/ExecutionBlock.cpp @@ -1133,20 +1133,16 @@ AqlItemBlock* IndexRangeBlock::getSome (size_t atLeast, // try again! if (_buffer.empty()) { - if (! ExecutionBlock::getBlock(DefaultBatchSize, DefaultBatchSize)) { + if (! ExecutionBlock::getBlock(DefaultBatchSize, DefaultBatchSize) + || (! initIndex())) { _done = true; return nullptr; } _pos = 0; // this is in the first block - // This is a new item, so let's init and read the index - if(initIndex()) { //successfully initted the index - readIndex(atMost); - } - else { //failed to init the index, nothing to pass on - _done = true; - return nullptr; - } + // This is a new item, so let's read the index (it is already + // initialised). + readIndex(atMost); _posInDocs = 0; // position in _documents . . . } @@ -1205,7 +1201,7 @@ AqlItemBlock* IndexRangeBlock::getSome (size_t atLeast, _pos = 0; } if (! _buffer.empty()) { - initIndex(); //TODO if this returns false, what to do? + initIndex(); readIndex(atMost); } // If _buffer is empty, then we will fetch a new block in the next call From f55b252f1b07776f640000a47406f19522d50067 Mon Sep 17 00:00:00 2001 From: James Date: Tue, 11 Nov 2014 12:25:59 +0000 Subject: [PATCH 24/36] snapshot working on IndexRangeBlock and hash indexes --- arangod/Aql/ExecutionBlock.cpp | 33 ++++++++++++++++++++++++--------- arangod/Aql/ExecutionBlock.h | 3 +++ 2 files changed, 27 insertions(+), 9 deletions(-) diff --git a/arangod/Aql/ExecutionBlock.cpp b/arangod/Aql/ExecutionBlock.cpp index 7b04891b62..de4ed60ebb 100644 --- a/arangod/Aql/ExecutionBlock.cpp +++ b/arangod/Aql/ExecutionBlock.cpp @@ -1043,14 +1043,23 @@ bool IndexRangeBlock::initIndex () { condition = newCondition.get(); } + /*if (en->_index->type == TRI_IDX_TYPE_PRIMARY_INDEX) { + initPrimaryIndex(*condition); + } + else */ + if (en->_index->type == TRI_IDX_TYPE_HASH_INDEX) { + return true; //no initialization here! + } if (en->_index->type == TRI_IDX_TYPE_SKIPLIST_INDEX) { initSkiplistIndex(*condition); return (_skiplistIterator != nullptr); - }//TODO the other cases!! + }/* + else if (en->_index->type == TRI_IDX_TYPE_EDGE_INDEX) { + initEdgeIndex(*condition); / + }*/ else { TRI_ASSERT(false); } - return false; //FIXME remove this } // this is called every time everything in _documents has been passed on @@ -1078,17 +1087,17 @@ bool IndexRangeBlock::readIndex (size_t atMost) { if (en->_index->type == TRI_IDX_TYPE_PRIMARY_INDEX) { // atMost not passed since only equality is supported - readPrimaryIndex(*condition); //TODO correct + //readPrimaryIndex(*condition); //TODO correct } else if (en->_index->type == TRI_IDX_TYPE_HASH_INDEX) { - readHashIndex(*condition, atMost); //TODO correct + readHashIndex(*condition, atMost); } else if (en->_index->type == TRI_IDX_TYPE_SKIPLIST_INDEX) { readSkiplistIndex(atMost); } else if (en->_index->type == TRI_IDX_TYPE_EDGE_INDEX) { // atMost not passed since only equality is supported - readEdgeIndex(*condition); //TODO correct + //readEdgeIndex(*condition); //TODO correct } else { TRI_ASSERT(false); @@ -1347,10 +1356,15 @@ void IndexRangeBlock::readPrimaryIndex (IndexOrCondition const& ranges) { //////////////////////////////////////////////////////////////////////////////// void IndexRangeBlock::readHashIndex (IndexOrCondition const& ranges, size_t atMost) { + auto en = static_cast(getPlanNode()); TRI_index_t* idx = en->_index->data; TRI_ASSERT(idx != nullptr); TRI_hash_index_t* hashIndex = (TRI_hash_index_t*) idx; + + if (_posInHashIndex >= hashIndex->_paths._length){ + return; //do nothing + } TRI_shaper_t* shaper = _collection->documentCollection()->getShaper(); TRI_ASSERT(shaper != nullptr); @@ -1368,7 +1382,7 @@ void IndexRangeBlock::readHashIndex (IndexOrCondition const& ranges, size_t atMo }; auto setupSearchValue = [&]() { - size_t const n = (std::min)(hashIndex->_paths._length, atMost); + size_t const n = (std::min)(hashIndex->_paths._length - _posInHashIndex, atMost); searchValue._length = 0; searchValue._values = static_cast(TRI_Allocate(TRI_CORE_MEM_ZONE, n * sizeof(TRI_shaped_json_t), true)); @@ -1379,7 +1393,7 @@ void IndexRangeBlock::readHashIndex (IndexOrCondition const& ranges, size_t atMo searchValue._length = n; - for (size_t i = 0; i < n; ++i) { + for (size_t i = _posInHashIndex; i < n + _posInHashIndex; ++i) { TRI_shape_pid_t pid = *(static_cast(TRI_AtVector(&hashIndex->_paths, i))); TRI_ASSERT(pid != 0); @@ -1391,18 +1405,19 @@ void IndexRangeBlock::readHashIndex (IndexOrCondition const& ranges, size_t atMo // here x->_low->_bound = x->_high->_bound searchValue._values[i] = *shaped; TRI_Free(shaper->_memoryZone, shaped); + //TODO add break here! } } - } }; setupSearchValue(); TRI_index_result_t list = TRI_LookupHashIndex(idx, &searchValue); - destroySearchValue(); + size_t const n = list._length; + _posInHashIndex += n; try { for (size_t i = 0; i < n; ++i) { _documents.emplace_back(*(list._documents[i])); diff --git a/arangod/Aql/ExecutionBlock.h b/arangod/Aql/ExecutionBlock.h index 18ac8347e2..ade4595194 100644 --- a/arangod/Aql/ExecutionBlock.h +++ b/arangod/Aql/ExecutionBlock.h @@ -573,12 +573,14 @@ namespace triagens { /// @brief read using the primary index //////////////////////////////////////////////////////////////////////////////// + //void readPrimaryIndex (size_t atMost); void readPrimaryIndex (IndexOrCondition const&); //////////////////////////////////////////////////////////////////////////////// /// @brief read using the edges index //////////////////////////////////////////////////////////////////////////////// + //void readEdgeIndex (size_t atMost); void readEdgeIndex (IndexOrCondition const&); //////////////////////////////////////////////////////////////////////////////// @@ -647,6 +649,7 @@ namespace triagens { std::vector> _inRegs; TRI_skiplist_iterator_t* _skiplistIterator; + size_t _posInHashIndex; }; From a5006b7617dc685a087d073933e8910602d25ae0 Mon Sep 17 00:00:00 2001 From: James Date: Thu, 13 Nov 2014 08:37:52 +0000 Subject: [PATCH 25/36] hash indexes working. --- arangod/Aql/ExecutionBlock.cpp | 20 +++++++++++--------- arangod/Aql/ExecutionBlock.h | 6 +++++- 2 files changed, 16 insertions(+), 10 deletions(-) diff --git a/arangod/Aql/ExecutionBlock.cpp b/arangod/Aql/ExecutionBlock.cpp index 6d95066096..5b1ef3d14f 100644 --- a/arangod/Aql/ExecutionBlock.cpp +++ b/arangod/Aql/ExecutionBlock.cpp @@ -850,9 +850,10 @@ IndexRangeBlock::IndexRangeBlock (ExecutionEngine* engine, _collection(en->collection()), _posInDocs(0), _allBoundsConstant(true), - _skiplistIterator(nullptr){ + _skiplistIterator(nullptr), + _condition(&en->_ranges) { - std::vector> const& orRanges = en->_ranges; + std::vector> const& orRanges = en->_ranges;//TODO replace this with _condition TRI_ASSERT(en->_index != nullptr); TRI_ASSERT(orRanges.size() == 1); // OR expressions not yet implemented @@ -944,7 +945,7 @@ int IndexRangeBlock::initialize () { bool IndexRangeBlock::initIndex () { auto en = static_cast(getPlanNode()); - IndexOrCondition const* condition = &en->_ranges; + //IndexOrCondition const* _condition = &en->_ranges; TRI_ASSERT(en->_index != nullptr); @@ -1040,7 +1041,7 @@ bool IndexRangeBlock::initIndex () { newCondition.get()->at(0).push_back(actualRange); } - condition = newCondition.get(); + _condition = newCondition.get(); } /*if (en->_index->type == TRI_IDX_TYPE_PRIMARY_INDEX) { @@ -1051,7 +1052,7 @@ bool IndexRangeBlock::initIndex () { return true; //no initialization here! } if (en->_index->type == TRI_IDX_TYPE_SKIPLIST_INDEX) { - initSkiplistIndex(*condition); + initSkiplistIndex(*_condition); return (_skiplistIterator != nullptr); }/* else if (en->_index->type == TRI_IDX_TYPE_EDGE_INDEX) { @@ -1083,14 +1084,13 @@ bool IndexRangeBlock::readIndex (size_t atMost) { } auto en = static_cast(getPlanNode()); - IndexOrCondition const* condition = &en->_ranges; //TODO remove this line if (en->_index->type == TRI_IDX_TYPE_PRIMARY_INDEX) { // atMost not passed since only equality is supported //readPrimaryIndex(*condition); //TODO correct } else if (en->_index->type == TRI_IDX_TYPE_HASH_INDEX) { - readHashIndex(*condition, atMost); + readHashIndex(*_condition, atMost); } else if (en->_index->type == TRI_IDX_TYPE_SKIPLIST_INDEX) { readSkiplistIndex(atMost); @@ -1210,7 +1210,10 @@ AqlItemBlock* IndexRangeBlock::getSome (size_t atLeast, _pos = 0; } if (! _buffer.empty()) { - initIndex(); + if(! initIndex()) {//FIXME is this right? + _done = true; + return nullptr; + } readIndex(atMost); } // If _buffer is empty, then we will fetch a new block in the next call @@ -1414,7 +1417,6 @@ void IndexRangeBlock::readHashIndex (IndexOrCondition const& ranges, size_t atMo setupSearchValue(); TRI_index_result_t list = TRI_LookupHashIndex(idx, &searchValue); destroySearchValue(); - size_t const n = list._length; _posInHashIndex += n; diff --git a/arangod/Aql/ExecutionBlock.h b/arangod/Aql/ExecutionBlock.h index ade4595194..f644384dbc 100644 --- a/arangod/Aql/ExecutionBlock.h +++ b/arangod/Aql/ExecutionBlock.h @@ -637,7 +637,7 @@ namespace triagens { //////////////////////////////////////////////////////////////////////////////// /// @brief _inVars, a vector containing for each expression above /// a vector of Variable*, used to execute the expression -//////////////////////////////////////////////////////////////////////////////// +///////////////////////////////////////////////////////////////////////////////// std::vector> _inVars; @@ -649,8 +649,12 @@ namespace triagens { std::vector> _inRegs; TRI_skiplist_iterator_t* _skiplistIterator; + size_t _posInHashIndex; + // condition for current incoming block + IndexOrCondition const* _condition; + }; // ----------------------------------------------------------------------------- From aba5567b0e2a25fd4fbbd3701922bea7b9f39f1f Mon Sep 17 00:00:00 2001 From: James Date: Thu, 13 Nov 2014 10:51:04 +0000 Subject: [PATCH 26/36] cleaning up --- arangod/Aql/ExecutionBlock.cpp | 74 +++++++++++++++++++--------------- arangod/Aql/ExecutionBlock.h | 40 +++++++++++++++--- 2 files changed, 76 insertions(+), 38 deletions(-) diff --git a/arangod/Aql/ExecutionBlock.cpp b/arangod/Aql/ExecutionBlock.cpp index 5b1ef3d14f..c3b6c85bd2 100644 --- a/arangod/Aql/ExecutionBlock.cpp +++ b/arangod/Aql/ExecutionBlock.cpp @@ -129,7 +129,7 @@ void AggregatorGroup::addValues (AqlItemBlock const* src, /// @brief batch size value //////////////////////////////////////////////////////////////////////////////// -size_t const ExecutionBlock::DefaultBatchSize = 11; +size_t const ExecutionBlock::DefaultBatchSize = 1000; // ----------------------------------------------------------------------------- // --SECTION-- constructors / destructors @@ -584,7 +584,6 @@ int SingletonBlock::getOrSkipSome (size_t, // atLeast, AqlItemBlock*& result, size_t& skipped) { - std::cout << "SingletonBlock::getOrSkipSome atMost = " << atMost << "\n"; TRI_ASSERT(result == nullptr && skipped == 0); if (_done) { @@ -943,16 +942,17 @@ int IndexRangeBlock::initialize () { // block! bool IndexRangeBlock::initIndex () { - + ENTER_BLOCK + _flag = true; auto en = static_cast(getPlanNode()); - //IndexOrCondition const* _condition = &en->_ranges; + _condition = &en->_ranges; TRI_ASSERT(en->_index != nullptr); - std::unique_ptr newCondition; // Find out about the actual values for the bounds in the variable bound case: if (! _allBoundsConstant) { + std::unique_ptr newCondition; // The following are needed to evaluate expressions with local data from // the current incoming item: AqlItemBlock* cur = _buffer.front(); @@ -1041,31 +1041,32 @@ bool IndexRangeBlock::initIndex () { newCondition.get()->at(0).push_back(actualRange); } - _condition = newCondition.get(); + _condition = newCondition.release(); } - /*if (en->_index->type == TRI_IDX_TYPE_PRIMARY_INDEX) { - initPrimaryIndex(*condition); + if (en->_index->type == TRI_IDX_TYPE_PRIMARY_INDEX) { + return true; //no initialization here! } - else */ - if (en->_index->type == TRI_IDX_TYPE_HASH_INDEX) { + else if (en->_index->type == TRI_IDX_TYPE_HASH_INDEX) { return true; //no initialization here! } if (en->_index->type == TRI_IDX_TYPE_SKIPLIST_INDEX) { initSkiplistIndex(*_condition); return (_skiplistIterator != nullptr); - }/* + } else if (en->_index->type == TRI_IDX_TYPE_EDGE_INDEX) { - initEdgeIndex(*condition); / - }*/ + return true; //no initialization here! + } else { TRI_ASSERT(false); } + LEAVE_BLOCK; } // this is called every time everything in _documents has been passed on bool IndexRangeBlock::readIndex (size_t atMost) { + ENTER_BLOCK; // TODO: update this comment, which is now out of date!! // This is either called from initialize if all bounds are constant, // in this case it is never called again. If there is at least one @@ -1086,27 +1087,33 @@ bool IndexRangeBlock::readIndex (size_t atMost) { auto en = static_cast(getPlanNode()); if (en->_index->type == TRI_IDX_TYPE_PRIMARY_INDEX) { - // atMost not passed since only equality is supported - //readPrimaryIndex(*condition); //TODO correct + if (_flag) { + readPrimaryIndex(*_condition); + } } else if (en->_index->type == TRI_IDX_TYPE_HASH_INDEX) { - readHashIndex(*_condition, atMost); + if (_flag) { + readHashIndex(*_condition); + } } else if (en->_index->type == TRI_IDX_TYPE_SKIPLIST_INDEX) { readSkiplistIndex(atMost); } else if (en->_index->type == TRI_IDX_TYPE_EDGE_INDEX) { - // atMost not passed since only equality is supported - //readEdgeIndex(*condition); //TODO correct + if (_flag) { + readEdgeIndex(*_condition); + } } else { TRI_ASSERT(false); } - + _flag = false; return (! _documents.empty()); + LEAVE_BLOCK; } int IndexRangeBlock::initializeCursor (AqlItemBlock* items, size_t pos) { + ENTER_BLOCK; int res = ExecutionBlock::initializeCursor(items, pos); if (res != TRI_ERROR_NO_ERROR) { return res; @@ -1119,6 +1126,7 @@ int IndexRangeBlock::initializeCursor (AqlItemBlock* items, size_t pos) { }*/ return TRI_ERROR_NO_ERROR; + LEAVE_BLOCK; } //////////////////////////////////////////////////////////////////////////////// @@ -1127,12 +1135,11 @@ int IndexRangeBlock::initializeCursor (AqlItemBlock* items, size_t pos) { AqlItemBlock* IndexRangeBlock::getSome (size_t atLeast, size_t atMost) { - std::cout << "IndexRangeBlock::getSome atMost = " << atMost << "\n"; + ENTER_BLOCK; if (_done) { return nullptr; } - //std::cout << "atMost = " << atMost << "\n"; unique_ptr res(nullptr); do { @@ -1226,6 +1233,7 @@ AqlItemBlock* IndexRangeBlock::getSome (size_t atLeast, // Clear out registers no longer needed later: clearRegisters(res.get()); return res.release(); + LEAVE_BLOCK; } //////////////////////////////////////////////////////////////////////////////// @@ -1296,6 +1304,7 @@ size_t IndexRangeBlock::skipSome (size_t atLeast, //////////////////////////////////////////////////////////////////////////////// void IndexRangeBlock::readPrimaryIndex (IndexOrCondition const& ranges) { + ENTER_BLOCK; TRI_primary_index_t* primaryIndex = &(_collection->documentCollection()->_primaryIndex); std::string key; @@ -1352,23 +1361,20 @@ void IndexRangeBlock::readPrimaryIndex (IndexOrCondition const& ranges) { _documents.emplace_back(*found); } } + LEAVE_BLOCK; } //////////////////////////////////////////////////////////////////////////////// /// @brief read documents using a hash index //////////////////////////////////////////////////////////////////////////////// -void IndexRangeBlock::readHashIndex (IndexOrCondition const& ranges, size_t atMost) { - +void IndexRangeBlock::readHashIndex (IndexOrCondition const& ranges) { + ENTER_BLOCK; auto en = static_cast(getPlanNode()); TRI_index_t* idx = en->_index->data; TRI_ASSERT(idx != nullptr); TRI_hash_index_t* hashIndex = (TRI_hash_index_t*) idx; - if (_posInHashIndex >= hashIndex->_paths._length){ - return; //do nothing - } - TRI_shaper_t* shaper = _collection->documentCollection()->getShaper(); TRI_ASSERT(shaper != nullptr); @@ -1385,7 +1391,7 @@ void IndexRangeBlock::readHashIndex (IndexOrCondition const& ranges, size_t atMo }; auto setupSearchValue = [&]() { - size_t const n = (std::min)(hashIndex->_paths._length - _posInHashIndex, atMost); + size_t const n = hashIndex->_paths._length; searchValue._length = 0; searchValue._values = static_cast(TRI_Allocate(TRI_CORE_MEM_ZONE, n * sizeof(TRI_shaped_json_t), true)); @@ -1396,7 +1402,7 @@ void IndexRangeBlock::readHashIndex (IndexOrCondition const& ranges, size_t atMo searchValue._length = n; - for (size_t i = _posInHashIndex; i < n + _posInHashIndex; ++i) { + for (size_t i = 0; i < n; ++i) { TRI_shape_pid_t pid = *(static_cast(TRI_AtVector(&hashIndex->_paths, i))); TRI_ASSERT(pid != 0); @@ -1419,7 +1425,6 @@ void IndexRangeBlock::readHashIndex (IndexOrCondition const& ranges, size_t atMo destroySearchValue(); size_t const n = list._length; - _posInHashIndex += n; try { for (size_t i = 0; i < n; ++i) { _documents.emplace_back(*(list._documents[i])); @@ -1432,6 +1437,7 @@ void IndexRangeBlock::readHashIndex (IndexOrCondition const& ranges, size_t atMo TRI_DestroyIndexResult(&list); throw; } + LEAVE_BLOCK; } //////////////////////////////////////////////////////////////////////////////// @@ -1439,6 +1445,7 @@ void IndexRangeBlock::readHashIndex (IndexOrCondition const& ranges, size_t atMo //////////////////////////////////////////////////////////////////////////////// void IndexRangeBlock::readEdgeIndex (IndexOrCondition const& ranges) { + ENTER_BLOCK; TRI_document_collection_t* document = _collection->documentCollection(); std::string key; @@ -1485,6 +1492,7 @@ void IndexRangeBlock::readEdgeIndex (IndexOrCondition const& ranges) { _engine->_stats.scannedIndex += static_cast(result.size()); } } + LEAVE_BLOCK; } //////////////////////////////////////////////////////////////////////////////// @@ -1524,6 +1532,7 @@ void IndexRangeBlock::readEdgeIndex (IndexOrCondition const& ranges) { // void IndexRangeBlock::initSkiplistIndex (IndexOrCondition const& ranges) { + ENTER_BLOCK; TRI_ASSERT(_skiplistIterator == nullptr) auto en = static_cast(getPlanNode()); @@ -1601,10 +1610,11 @@ void IndexRangeBlock::initSkiplistIndex (IndexOrCondition const& ranges) { THROW_ARANGO_EXCEPTION(TRI_ERROR_ARANGO_NO_INDEX); } + LEAVE_BLOCK; } void IndexRangeBlock::readSkiplistIndex (size_t atMost) { - + ENTER_BLOCK; if (_skiplistIterator == nullptr) { return; } @@ -1631,6 +1641,7 @@ void IndexRangeBlock::readSkiplistIndex (size_t atMost) { TRI_FreeSkiplistIterator(_skiplistIterator); throw; } + LEAVE_BLOCK; } // ----------------------------------------------------------------------------- @@ -2906,7 +2917,6 @@ int LimitBlock::getOrSkipSome (size_t atLeast, AqlItemBlock* ReturnBlock::getSome (size_t atLeast, size_t atMost) { - std::cout << "ReturnBlock::getSome atMost = " << atMost << "\n"; auto res = ExecutionBlock::getSomeWithoutRegisterClearout(atLeast, atMost); if (res == nullptr) { diff --git a/arangod/Aql/ExecutionBlock.h b/arangod/Aql/ExecutionBlock.h index f644384dbc..181c493bee 100644 --- a/arangod/Aql/ExecutionBlock.h +++ b/arangod/Aql/ExecutionBlock.h @@ -567,20 +567,24 @@ namespace triagens { //////////////////////////////////////////////////////////////////////////////// bool readIndex (size_t atMost); + +//////////////////////////////////////////////////////////////////////////////// +/// @brief set up the index for reading. This should be called once per incoming +/// block. +//////////////////////////////////////////////////////////////////////////////// + bool initIndex (); //////////////////////////////////////////////////////////////////////////////// /// @brief read using the primary index //////////////////////////////////////////////////////////////////////////////// - //void readPrimaryIndex (size_t atMost); void readPrimaryIndex (IndexOrCondition const&); //////////////////////////////////////////////////////////////////////////////// /// @brief read using the edges index //////////////////////////////////////////////////////////////////////////////// - //void readEdgeIndex (size_t atMost); void readEdgeIndex (IndexOrCondition const&); //////////////////////////////////////////////////////////////////////////////// @@ -588,13 +592,18 @@ namespace triagens { //////////////////////////////////////////////////////////////////////////////// void readSkiplistIndex (size_t atMost); + +//////////////////////////////////////////////////////////////////////////////// +/// @brief this tries to create a skiplistIterator to read from the index. +//////////////////////////////////////////////////////////////////////////////// + void initSkiplistIndex (IndexOrCondition const&); //////////////////////////////////////////////////////////////////////////////// /// @brief read using a hash index //////////////////////////////////////////////////////////////////////////////// - void readHashIndex (IndexOrCondition const&, size_t atMost); + void readHashIndex (IndexOrCondition const&); // ----------------------------------------------------------------------------- // --SECTION-- private variables @@ -648,12 +657,31 @@ namespace triagens { std::vector> _inRegs; +//////////////////////////////////////////////////////////////////////////////// +/// @brief _skiplistIterator: holds the skiplist iterator found using +/// initSkiplistIndex (if any) so that it can be read in chunks and not +/// necessarily all at once. +//////////////////////////////////////////////////////////////////////////////// + TRI_skiplist_iterator_t* _skiplistIterator; - size_t _posInHashIndex; - - // condition for current incoming block +//////////////////////////////////////////////////////////////////////////////// +/// @brief _condition: holds the IndexOrCondition for the current incoming block, +/// this is just the _ranges member of the plan node if _allBoundsConstant +/// otherwise it is reevaluated every time initIndex is called, i.e. once per +/// incoming block. +//////////////////////////////////////////////////////////////////////////////// + IndexOrCondition const* _condition; + +//////////////////////////////////////////////////////////////////////////////// +/// @brief _flag: since readIndex for primary, hash, edges indexes reads the +/// whole index, this is if initIndex has been called but readIndex has +/// not been called, otherwise it is to avoid rereading the entire index +/// with successive calls to readIndex. +////////////////////////////////////////////////////////////////////////////////// + + bool _flag; }; From 49352cb8c28b6e2c0dc522731c03e434a9fcc010 Mon Sep 17 00:00:00 2001 From: James Date: Thu, 13 Nov 2014 11:08:14 +0000 Subject: [PATCH 27/36] removing redundant code, and updated comments. --- arangod/Aql/ExecutionBlock.cpp | 46 +++++++++++++++++----------------- 1 file changed, 23 insertions(+), 23 deletions(-) diff --git a/arangod/Aql/ExecutionBlock.cpp b/arangod/Aql/ExecutionBlock.cpp index c3b6c85bd2..a3388b581f 100644 --- a/arangod/Aql/ExecutionBlock.cpp +++ b/arangod/Aql/ExecutionBlock.cpp @@ -932,14 +932,24 @@ int IndexRangeBlock::initialize () { throw; } } - //else { // _allBoundsConstant - // readIndex(); - //} return res; } // init the index for reading, this should be called once per new incoming // block! +// +// This is either called every time we get a new incoming block. +// If all the bounds are constant, then in the case of hash, primary or edges +// indexes it does nothing. In the case of a skiplist index, it creates a +// skiplistIterator which is used by readIndex. If at least one bound is +// variable, then this this also evaluates the IndexOrCondition required to +// determine the values of the bounds. +// +// It is guaranteed that +// _buffer is not empty, in particular _buffer.front() is defined +// _pos points to a position in _buffer.front() +// Therefore, we can use the register values in _buffer.front() in row +// _pos to evaluate the variable bounds. bool IndexRangeBlock::initIndex () { ENTER_BLOCK @@ -948,7 +958,6 @@ bool IndexRangeBlock::initIndex () { _condition = &en->_ranges; TRI_ASSERT(en->_index != nullptr); - // Find out about the actual values for the bounds in the variable bound case: if (! _allBoundsConstant) { @@ -1067,15 +1076,13 @@ bool IndexRangeBlock::initIndex () { bool IndexRangeBlock::readIndex (size_t atMost) { ENTER_BLOCK; - // TODO: update this comment, which is now out of date!! - // This is either called from initialize if all bounds are constant, - // in this case it is never called again. If there is at least one - // variable bound, then readIndex is called once for every item coming - // in from our dependency. In that case, it is guaranteed that - // _buffer is not empty, in particular _buffer.front() is defined - // _pos points to a position in _buffer.front() - // Therefore, we can use the register values in _buffer.front() in row - // _pos to evaluate the variable bounds. + // this is called every time we want more in _documents. + // For non-skiplist indexes (currently hash, primary, edge), this + // only reads the index once, and never again (although there might be + // multiple calls to this function). For skiplists indexes, initIndex creates + // a skiplistIterator and readIndex just reads from the iterator until it is + // done. Then initIndex is read again and so on. This is to avoid reading the + // entire index when we only want a small number of documents. if (_documents.empty()) { _documents.reserve(atMost); @@ -1121,10 +1128,6 @@ int IndexRangeBlock::initializeCursor (AqlItemBlock* items, size_t pos) { _pos = 0; _posInDocs = 0; - /*if (_allBoundsConstant && _documents.size() == 0) { - _done = true; - }*/ - return TRI_ERROR_NO_ERROR; LEAVE_BLOCK; } @@ -1217,7 +1220,7 @@ AqlItemBlock* IndexRangeBlock::getSome (size_t atLeast, _pos = 0; } if (! _buffer.empty()) { - if(! initIndex()) {//FIXME is this right? + if(! initIndex()) { _done = true; return nullptr; } @@ -1258,10 +1261,7 @@ size_t IndexRangeBlock::skipSome (size_t atLeast, _pos = 0; // this is in the first block // This is a new item, so let's read the index if bounds are variable: - //if (! _allBoundsConstant) { - readIndex(atMost); - //} - + readIndex(atMost); _posInDocs = 0; // position in _documents . . . } @@ -1414,7 +1414,7 @@ void IndexRangeBlock::readHashIndex (IndexOrCondition const& ranges) { // here x->_low->_bound = x->_high->_bound searchValue._values[i] = *shaped; TRI_Free(shaper->_memoryZone, shaped); - //TODO add break here! + break; } } } From 6822f80129a5a3748e9a055b2b9df90137c43ca4 Mon Sep 17 00:00:00 2001 From: James Date: Thu, 13 Nov 2014 11:19:11 +0000 Subject: [PATCH 28/36] updating the skipSome for index range block --- arangod/Aql/ExecutionBlock.cpp | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/arangod/Aql/ExecutionBlock.cpp b/arangod/Aql/ExecutionBlock.cpp index a3388b581f..4e9cc34974 100644 --- a/arangod/Aql/ExecutionBlock.cpp +++ b/arangod/Aql/ExecutionBlock.cpp @@ -129,7 +129,7 @@ void AggregatorGroup::addValues (AqlItemBlock const* src, /// @brief batch size value //////////////////////////////////////////////////////////////////////////////// -size_t const ExecutionBlock::DefaultBatchSize = 1000; +size_t const ExecutionBlock::DefaultBatchSize = 11; // ----------------------------------------------------------------------------- // --SECTION-- constructors / destructors @@ -1254,7 +1254,8 @@ size_t IndexRangeBlock::skipSome (size_t atLeast, while (skipped < atLeast) { if (_buffer.empty()) { - if (! ExecutionBlock::getBlock(DefaultBatchSize, DefaultBatchSize)) { + if (! ExecutionBlock::getBlock(DefaultBatchSize, DefaultBatchSize) + || (! initIndex())) { _done = true; return skipped; } @@ -1286,14 +1287,17 @@ size_t IndexRangeBlock::skipSome (size_t atLeast, // let's read the index if bounds are variable: if (! _buffer.empty()) { - readIndex(atMost); // TODO: add atMost? + if(! initIndex()) { + _done = true; + return skipped; + } + readIndex(atMost); } _posInDocs = 0; // If _buffer is empty, then we will fetch a new block in the next round // and then read the index. } - } return skipped; From fb1fe3e8b1a59aca67172fc1fdcddbc34236c824 Mon Sep 17 00:00:00 2001 From: James Date: Thu, 13 Nov 2014 11:21:07 +0000 Subject: [PATCH 29/36] reset the defaultbatchsize to 1000 --- arangod/Aql/ExecutionBlock.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/arangod/Aql/ExecutionBlock.cpp b/arangod/Aql/ExecutionBlock.cpp index 4e9cc34974..20100807a3 100644 --- a/arangod/Aql/ExecutionBlock.cpp +++ b/arangod/Aql/ExecutionBlock.cpp @@ -129,7 +129,7 @@ void AggregatorGroup::addValues (AqlItemBlock const* src, /// @brief batch size value //////////////////////////////////////////////////////////////////////////////// -size_t const ExecutionBlock::DefaultBatchSize = 11; +size_t const ExecutionBlock::DefaultBatchSize = 1000; // ----------------------------------------------------------------------------- // --SECTION-- constructors / destructors From 5fd0bd80be6a5e5d0d490d74b58efdabcf813998 Mon Sep 17 00:00:00 2001 From: James Date: Thu, 13 Nov 2014 11:49:25 +0000 Subject: [PATCH 30/36] refactoring to avoid to calls to readIndex when fewer than DefaultBatchSize docs are requested. --- arangod/Aql/ExecutionBlock.cpp | 50 ++++++++++++++++++---------------- 1 file changed, 27 insertions(+), 23 deletions(-) diff --git a/arangod/Aql/ExecutionBlock.cpp b/arangod/Aql/ExecutionBlock.cpp index 20100807a3..e6e358d497 100644 --- a/arangod/Aql/ExecutionBlock.cpp +++ b/arangod/Aql/ExecutionBlock.cpp @@ -1163,6 +1163,33 @@ AqlItemBlock* IndexRangeBlock::getSome (size_t atLeast, // initialised). readIndex(atMost); _posInDocs = 0; // position in _documents . . . + } + else if (_posInDocs >= _documents.size()) { + // we have exhausted our local documents buffer, + + _posInDocs = 0; + AqlItemBlock* cur = _buffer.front(); + + if (! readIndex(atMost)) { //no more output from this version of the index + if (++_pos >= cur->size()) { + _buffer.pop_front(); // does not throw + delete cur; + _pos = 0; + } + if (_buffer.empty()) { + if (! ExecutionBlock::getBlock(DefaultBatchSize, DefaultBatchSize) ) { + _done = true; + return nullptr; + } + _pos = 0; // this is in the first block + } + + if(! initIndex()) { + _done = true; + return nullptr; + } + readIndex(atMost); + } } // If we get here, we do have _buffer.front() and _pos points into it @@ -1207,29 +1234,6 @@ AqlItemBlock* IndexRangeBlock::getSome (size_t atLeast, } } - // Advance read position: - if (_posInDocs >= _documents.size()) { - // we have exhausted our local documents buffer, - - _posInDocs = 0; - - if (! readIndex(atMost)) { //no more output from this version of the index - if (++_pos >= cur->size()) { - _buffer.pop_front(); // does not throw - delete cur; - _pos = 0; - } - if (! _buffer.empty()) { - if(! initIndex()) { - _done = true; - return nullptr; - } - readIndex(atMost); - } - // If _buffer is empty, then we will fetch a new block in the next call - // and then init/read the index. - } - } } while (res.get() == nullptr); From c1670246542dc0e76888e44719a99b500e08f879 Mon Sep 17 00:00:00 2001 From: Jan Steemann Date: Thu, 13 Nov 2014 14:14:27 +0100 Subject: [PATCH 31/36] fixed compile error --- arangod/Aql/ExecutionBlock.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/arangod/Aql/ExecutionBlock.cpp b/arangod/Aql/ExecutionBlock.cpp index e6e358d497..2d1ab9dda4 100644 --- a/arangod/Aql/ExecutionBlock.cpp +++ b/arangod/Aql/ExecutionBlock.cpp @@ -1541,7 +1541,7 @@ void IndexRangeBlock::readEdgeIndex (IndexOrCondition const& ranges) { void IndexRangeBlock::initSkiplistIndex (IndexOrCondition const& ranges) { ENTER_BLOCK; - TRI_ASSERT(_skiplistIterator == nullptr) + TRI_ASSERT(_skiplistIterator == nullptr); auto en = static_cast(getPlanNode()); TRI_index_t* idx = en->_index->data; From 2622f57e076a1cb7eb02f84c0ec0bbc1f0f84ad8 Mon Sep 17 00:00:00 2001 From: Jan Steemann Date: Thu, 13 Nov 2014 14:15:47 +0100 Subject: [PATCH 32/36] added assertions --- lib/Basics/json-utilities.cpp | 3 +++ 1 file changed, 3 insertions(+) diff --git a/lib/Basics/json-utilities.cpp b/lib/Basics/json-utilities.cpp index 784ab3e99b..d4f4a8cad6 100644 --- a/lib/Basics/json-utilities.cpp +++ b/lib/Basics/json-utilities.cpp @@ -203,9 +203,12 @@ int TRI_CompareValuesJson (TRI_json_t const* lhs, return 1; } + TRI_ASSERT(lWeight == rWeight); + // lhs and rhs have equal weights if (lhs == nullptr) { // both lhs and rhs are NULL, so they are equal + TRI_ASSERT(rhs == nullptr); return 0; } From 896e8daec593ea28787d474503acefa353a7ef7c Mon Sep 17 00:00:00 2001 From: Jan Steemann Date: Thu, 13 Nov 2014 14:24:59 +0100 Subject: [PATCH 33/36] removed unused variable --- arangod/Aql/OptimizerRules.cpp | 3 --- 1 file changed, 3 deletions(-) diff --git a/arangod/Aql/OptimizerRules.cpp b/arangod/Aql/OptimizerRules.cpp index 0d166022be..163314319c 100644 --- a/arangod/Aql/OptimizerRules.cpp +++ b/arangod/Aql/OptimizerRules.cpp @@ -1761,9 +1761,6 @@ int triagens::aql::removeFiltersCoveredByIndex (Optimizer* opt, while (current != nullptr) { if (current->getType() == EN::INDEX_RANGE) { // found an index range, now check if the expression is covered by the index - auto variable = static_cast(current)->outVariable(); - TRI_ASSERT(variable != nullptr); - auto const& ranges = static_cast(current)->ranges(); // TODO: this is not prepared for OR conditions From 32013321c521230e0907141357f25df910bb1e29 Mon Sep 17 00:00:00 2001 From: Jan Steemann Date: Thu, 13 Nov 2014 14:25:10 +0100 Subject: [PATCH 34/36] fixed compile warning --- arangod/Aql/ExecutionBlock.cpp | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/arangod/Aql/ExecutionBlock.cpp b/arangod/Aql/ExecutionBlock.cpp index 2d1ab9dda4..cd4f6d6ea5 100644 --- a/arangod/Aql/ExecutionBlock.cpp +++ b/arangod/Aql/ExecutionBlock.cpp @@ -1066,9 +1066,8 @@ bool IndexRangeBlock::initIndex () { else if (en->_index->type == TRI_IDX_TYPE_EDGE_INDEX) { return true; //no initialization here! } - else { - TRI_ASSERT(false); - } + + THROW_ARANGO_EXCEPTION_MESSAGE(TRI_ERROR_INTERNAL, "unexpected index type"); LEAVE_BLOCK; } From cb1444804e1554f2ff74b910c6d838c56887c29c Mon Sep 17 00:00:00 2001 From: Jan Steemann Date: Thu, 13 Nov 2014 15:55:34 +0100 Subject: [PATCH 35/36] added NODE_TYPE_ATTRIBUTE_ACCESS for CompareAstNodes --- arangod/Aql/Ast.h | 4 +-- arangod/Aql/AstNode.cpp | 76 ++++++++++++++++++++++++++++++++++++++++- 2 files changed, 77 insertions(+), 3 deletions(-) diff --git a/arangod/Aql/Ast.h b/arangod/Aql/Ast.h index 7b9506dfa6..79536713f2 100644 --- a/arangod/Aql/Ast.h +++ b/arangod/Aql/Ast.h @@ -392,13 +392,13 @@ namespace triagens { /// @brief create an AST null value node //////////////////////////////////////////////////////////////////////////////// - AstNode* createNodeValueNull (); + static AstNode* createNodeValueNull (); //////////////////////////////////////////////////////////////////////////////// /// @brief create an AST bool value node //////////////////////////////////////////////////////////////////////////////// - AstNode* createNodeValueBool (bool); + static AstNode* createNodeValueBool (bool); //////////////////////////////////////////////////////////////////////////////// /// @brief create an AST int value node diff --git a/arangod/Aql/AstNode.cpp b/arangod/Aql/AstNode.cpp index f3d5d5138d..7f5f6658ca 100644 --- a/arangod/Aql/AstNode.cpp +++ b/arangod/Aql/AstNode.cpp @@ -132,12 +132,74 @@ std::unordered_map const AstNode::valueTypeNames{ // ----------------------------------------------------------------------------- // --SECTION-- static helper functions // ----------------------------------------------------------------------------- + +//////////////////////////////////////////////////////////////////////////////// +/// @brief resolve an attribute access +//////////////////////////////////////////////////////////////////////////////// + +static AstNode const* ResolveAttribute (AstNode const* node) { + TRI_ASSERT(node != nullptr); + TRI_ASSERT(node->type == NODE_TYPE_ATTRIBUTE_ACCESS); + + std::vector attributeNames; + + while (node->type == NODE_TYPE_ATTRIBUTE_ACCESS) { + char const* attributeName = node->getStringValue(); + + TRI_ASSERT(attributeName != nullptr); + attributeNames.push_back(attributeName); + node = node->getMember(0); + } + + while (true) { + TRI_ASSERT(! attributeNames.empty()); + + TRI_ASSERT(node->type == NODE_TYPE_VALUE || + node->type == NODE_TYPE_LIST || + node->type == NODE_TYPE_ARRAY); + + bool found = false; + + if (node->type == NODE_TYPE_ARRAY) { + char const* attributeName = attributeNames.back().c_str(); + attributeNames.pop_back(); + + size_t const n = node->numMembers(); + for (size_t i = 0; i < n; ++i) { + auto member = node->getMember(i); + + if (member != nullptr && strcmp(member->getStringValue(), attributeName) == 0) { + // found the attribute + node = member->getMember(0); + if (attributeNames.empty()) { + // we found what we looked for + return node; + } + else { + // we found the correct attribute but there is now an attribute access on the result + found = true; + break; + } + } + } + } + + if (! found) { + break; + } + } + + // attribute not found or non-array + return Ast::createNodeValueNull(); +} //////////////////////////////////////////////////////////////////////////////// /// @brief get the node type for inter-node comparisons //////////////////////////////////////////////////////////////////////////////// static TRI_json_type_e GetNodeCompareType (AstNode const* node) { + TRI_ASSERT(node != nullptr); + if (node->type == NODE_TYPE_VALUE) { switch (node->value.type) { case VALUE_TYPE_NULL: @@ -158,7 +220,11 @@ static TRI_json_type_e GetNodeCompareType (AstNode const* node) { else if (node->type == NODE_TYPE_ARRAY) { return TRI_JSON_ARRAY; } + + // we should never get here TRI_ASSERT(false); + + // return null in case assertions are turned off return TRI_JSON_NULL; } @@ -166,7 +232,15 @@ static TRI_json_type_e GetNodeCompareType (AstNode const* node) { /// @brief compare two nodes //////////////////////////////////////////////////////////////////////////////// -int triagens::aql::CompareAstNodes (AstNode const* lhs, AstNode const* rhs) { +int triagens::aql::CompareAstNodes (AstNode const* lhs, + AstNode const* rhs) { + if (lhs->type == NODE_TYPE_ATTRIBUTE_ACCESS) { + lhs = ResolveAttribute(lhs); + } + if (rhs->type == NODE_TYPE_ATTRIBUTE_ACCESS) { + rhs = ResolveAttribute(rhs); + } + auto lType = GetNodeCompareType(lhs); auto rType = GetNodeCompareType(rhs); From 3696e3c7c1caddae9c1a19b905bc2e8771f192f1 Mon Sep 17 00:00:00 2001 From: Jan Steemann Date: Thu, 13 Nov 2014 18:04:05 +0100 Subject: [PATCH 36/36] fixed memleak --- arangod/Aql/ExecutionBlock.cpp | 21 +++++++++++++++++++-- arangod/Aql/ExecutionBlock.h | 13 +++++++++++++ 2 files changed, 32 insertions(+), 2 deletions(-) diff --git a/arangod/Aql/ExecutionBlock.cpp b/arangod/Aql/ExecutionBlock.cpp index 3cd8f4edd9..d92747f84a 100644 --- a/arangod/Aql/ExecutionBlock.cpp +++ b/arangod/Aql/ExecutionBlock.cpp @@ -850,7 +850,8 @@ IndexRangeBlock::IndexRangeBlock (ExecutionEngine* engine, _posInDocs(0), _allBoundsConstant(true), _skiplistIterator(nullptr), - _condition(&en->_ranges) { + _condition(&en->_ranges), + _freeCondition(false) { std::vector> const& orRanges = en->_ranges;//TODO replace this with _condition TRI_ASSERT(en->_index != nullptr); @@ -869,6 +870,10 @@ IndexRangeBlock::~IndexRangeBlock () { delete e; } _allVariableBoundExpressions.clear(); + + if (_freeCondition && _condition != nullptr) { + delete _condition; + } } int IndexRangeBlock::initialize () { @@ -955,7 +960,9 @@ bool IndexRangeBlock::initIndex () { ENTER_BLOCK _flag = true; auto en = static_cast(getPlanNode()); + freeCondition(); _condition = &en->_ranges; + _freeCondition = false; TRI_ASSERT(en->_index != nullptr); @@ -1049,8 +1056,10 @@ bool IndexRangeBlock::initIndex () { newCondition.get()->at(0).push_back(actualRange); } - + + freeCondition(); _condition = newCondition.release(); + _freeCondition = true; } if (en->_index->type == TRI_IDX_TYPE_PRIMARY_INDEX) { @@ -1071,6 +1080,14 @@ bool IndexRangeBlock::initIndex () { LEAVE_BLOCK; } +void IndexRangeBlock::freeCondition () { + if (_condition != nullptr && _freeCondition) { + delete _condition; + _condition = nullptr; + _freeCondition = false; + } +} + // this is called every time everything in _documents has been passed on bool IndexRangeBlock::readIndex (size_t atMost) { diff --git a/arangod/Aql/ExecutionBlock.h b/arangod/Aql/ExecutionBlock.h index 181c493bee..2dc55894fb 100644 --- a/arangod/Aql/ExecutionBlock.h +++ b/arangod/Aql/ExecutionBlock.h @@ -562,6 +562,12 @@ namespace triagens { private: +//////////////////////////////////////////////////////////////////////////////// +/// @brief free _condition if it belongs to us +//////////////////////////////////////////////////////////////////////////////// + + void freeCondition (); + //////////////////////////////////////////////////////////////////////////////// /// @brief continue fetching of documents //////////////////////////////////////////////////////////////////////////////// @@ -683,6 +689,13 @@ namespace triagens { bool _flag; +//////////////////////////////////////////////////////////////////////////////// +/// @brief _freeCondition: whether or not the _condition is owned by the +/// IndexRangeBlock and must be freed +//////////////////////////////////////////////////////////////////////////////// + + bool _freeCondition; + }; // -----------------------------------------------------------------------------