From 4300c77d3e10b40df3481a6123465be14dffe83a Mon Sep 17 00:00:00 2001 From: jsteemann Date: Tue, 25 Apr 2017 01:47:07 +0200 Subject: [PATCH] fixes for non-array IN lookups, added tests --- arangod/Indexes/Index.cpp | 26 +++++++++----- arangod/Indexes/IndexIterator.h | 25 +++++++------ arangod/MMFiles/MMFilesEdgeIndex.cpp | 5 +-- arangod/MMFiles/MMFilesHashIndex.cpp | 6 ++-- arangod/MMFiles/MMFilesPrimaryIndex.cpp | 5 +-- arangod/RocksDBEngine/RocksDBCollection.cpp | 5 +-- arangod/RocksDBEngine/RocksDBEdgeIndex.cpp | 20 ++++++----- arangod/RocksDBEngine/RocksDBKeyBounds.cpp | 9 +++++ arangod/RocksDBEngine/RocksDBKeyBounds.h | 10 ++++-- arangod/RocksDBEngine/RocksDBPrimaryIndex.cpp | 5 +-- arangod/RocksDBEngine/RocksDBVPackIndex.cpp | 2 +- js/server/tests/aql/aql-optimizer-indexes.js | 35 ++++++++++++++++++- 12 files changed, 107 insertions(+), 46 deletions(-) diff --git a/arangod/Indexes/Index.cpp b/arangod/Indexes/Index.cpp index d7535a519f..b34f480615 100644 --- a/arangod/Indexes/Index.cpp +++ b/arangod/Indexes/Index.cpp @@ -764,22 +764,30 @@ void Index::expandInSearchValues(VPackSlice const base, VPackSlice current = oneLookup.at(i); if (current.hasKey(StaticStrings::IndexIn)) { VPackSlice inList = current.get(StaticStrings::IndexIn); + if (!inList.isArray()) { + // IN value is a non-array + result.clear(); + result.openArray(); + return; + } + + TRI_ASSERT(inList.isArray()); + VPackValueLength nList = inList.length(); + + if (nList == 0) { + // Empty Array. short circuit, no matches possible + result.clear(); + result.openArray(); + return; + } std::unordered_set - tmp(static_cast(inList.length()), + tmp(static_cast(nList), arangodb::basics::VelocyPackHelper::VPackHash(), arangodb::basics::VelocyPackHelper::VPackEqual()); - TRI_ASSERT(inList.isArray()); - if (inList.length() == 0) { - // Empty Array. short circuit, no matches possible - result.clear(); - result.openArray(); - result.close(); - return; - } for (auto const& el : VPackArrayIterator(inList)) { tmp.emplace(el); } diff --git a/arangod/Indexes/IndexIterator.h b/arangod/Indexes/IndexIterator.h index b1ae2f4115..46517efdfa 100644 --- a/arangod/Indexes/IndexIterator.h +++ b/arangod/Indexes/IndexIterator.h @@ -58,7 +58,6 @@ class LogicalCollection; namespace transaction { class Methods; } -; /// @brief a base class to iterate over the index. An iterator is requested /// at the index itself @@ -102,23 +101,23 @@ class IndexIterator { /// @brief Special iterator if the condition cannot have any result class EmptyIndexIterator final : public IndexIterator { - public: - EmptyIndexIterator(LogicalCollection* collection, transaction::Methods* trx, ManagedDocumentResult* mmdr, arangodb::Index const* index) - : IndexIterator(collection, trx, mmdr, index) {} + public: + EmptyIndexIterator(LogicalCollection* collection, transaction::Methods* trx, ManagedDocumentResult* mmdr, arangodb::Index const* index) + : IndexIterator(collection, trx, mmdr, index) {} - ~EmptyIndexIterator() {} + ~EmptyIndexIterator() {} - char const* typeName() const override { return "empty-index-iterator"; } + char const* typeName() const override { return "empty-index-iterator"; } - bool next(TokenCallback const&, size_t) override { - return false; - } + bool next(TokenCallback const&, size_t) override { + return false; + } - void reset() override {} + void reset() override {} - void skip(uint64_t, uint64_t& skipped) override { - skipped = 0; - } + void skip(uint64_t, uint64_t& skipped) override { + skipped = 0; + } }; /// @brief a wrapper class to iterate over several IndexIterators. diff --git a/arangod/MMFiles/MMFilesEdgeIndex.cpp b/arangod/MMFiles/MMFilesEdgeIndex.cpp index af6a985763..dd70f6e5cd 100644 --- a/arangod/MMFiles/MMFilesEdgeIndex.cpp +++ b/arangod/MMFiles/MMFilesEdgeIndex.cpp @@ -448,14 +448,15 @@ IndexIterator* MMFilesEdgeIndex::iteratorForCondition( if (comp->type == aql::NODE_TYPE_OPERATOR_BINARY_IN) { // a.b IN values if (!valNode->isArray()) { - return nullptr; + // a.b IN non-array + return new EmptyIndexIterator(_collection, trx, mmdr, this); } return createInIterator(trx, mmdr, attrNode, valNode); } // operator type unsupported - return nullptr; + return new EmptyIndexIterator(_collection, trx, mmdr, this); } /// @brief specializes the condition for use with the index diff --git a/arangod/MMFiles/MMFilesHashIndex.cpp b/arangod/MMFiles/MMFilesHashIndex.cpp index 85fb17956f..74c3a2c2da 100644 --- a/arangod/MMFiles/MMFilesHashIndex.cpp +++ b/arangod/MMFiles/MMFilesHashIndex.cpp @@ -120,8 +120,10 @@ MMFilesHashIndexLookupBuilder::MMFilesHashIndexLookupBuilder( TRI_IF_FAILURE("Index::permutationIN") { THROW_ARANGO_EXCEPTION(TRI_ERROR_DEBUG); } - for (auto const& value : VPackArrayIterator(values)) { - tmp.emplace(value); + if (values.isArray()) { + for (auto const& value : VPackArrayIterator(values)) { + tmp.emplace(value); + } } if (tmp.empty()) { // IN [] short-circuit, cannot be fullfilled; diff --git a/arangod/MMFiles/MMFilesPrimaryIndex.cpp b/arangod/MMFiles/MMFilesPrimaryIndex.cpp index 5b26009647..d53f4a2b62 100644 --- a/arangod/MMFiles/MMFilesPrimaryIndex.cpp +++ b/arangod/MMFiles/MMFilesPrimaryIndex.cpp @@ -471,14 +471,15 @@ IndexIterator* MMFilesPrimaryIndex::iteratorForCondition( } else if (comp->type == aql::NODE_TYPE_OPERATOR_BINARY_IN) { // a.b IN values if (!valNode->isArray()) { - return nullptr; + // a.b IN non-array + return new EmptyIndexIterator(_collection, trx, mmdr, this); } return createInIterator(trx, mmdr, attrNode, valNode); } // operator type unsupported - return nullptr; + return new EmptyIndexIterator(_collection, trx, mmdr, this); } /// @brief specializes the condition for use with the index diff --git a/arangod/RocksDBEngine/RocksDBCollection.cpp b/arangod/RocksDBEngine/RocksDBCollection.cpp index 297a5c9527..df42d3b965 100644 --- a/arangod/RocksDBEngine/RocksDBCollection.cpp +++ b/arangod/RocksDBEngine/RocksDBCollection.cpp @@ -464,10 +464,11 @@ void RocksDBCollection::truncate(transaction::Methods* trx, // TODO maybe we could also reuse Index::drop, if we ensure the // implementations // don't do anything beyond deleting their contents - RocksDBKeyBounds indexBounds = - RocksDBKeyBounds::PrimaryIndex(42); // default constructor? for (std::shared_ptr const& index : _indexes) { RocksDBIndex* rindex = static_cast(index.get()); + + RocksDBKeyBounds indexBounds = + RocksDBKeyBounds::Empty(); switch (rindex->type()) { case RocksDBIndex::TRI_IDX_TYPE_PRIMARY_INDEX: indexBounds = RocksDBKeyBounds::PrimaryIndex(rindex->objectId()); diff --git a/arangod/RocksDBEngine/RocksDBEdgeIndex.cpp b/arangod/RocksDBEngine/RocksDBEdgeIndex.cpp index 7463287eaa..df9e6c8213 100644 --- a/arangod/RocksDBEngine/RocksDBEdgeIndex.cpp +++ b/arangod/RocksDBEngine/RocksDBEdgeIndex.cpp @@ -104,7 +104,10 @@ bool RocksDBEdgeIndexIterator::next(TokenCallback const& cb, size_t limit) { // acquire rocksdb collection auto rocksColl = RocksDBCollection::toRocksDBCollection(_collection); - while (limit > 0) { + + while (true) { + TRI_ASSERT(limit > 0); + while (_iterator->Valid() && (_index->_cmp->Compare(_iterator->key(), _bounds.end()) < 0)) { StringRef edgeKey = RocksDBKey::primaryKey(_iterator->key()); @@ -121,14 +124,12 @@ bool RocksDBEdgeIndexIterator::next(TokenCallback const& cb, size_t limit) { } } // TODO do we need to handle failed lookups here? } - if (limit > 0) { - _keysIterator.next(); - if (!updateBounds()) { - return false; - } + + _keysIterator.next(); + if (!updateBounds()) { + return false; } } - return true; } void RocksDBEdgeIndexIterator::reset() { @@ -323,14 +324,15 @@ IndexIterator* RocksDBEdgeIndex::iteratorForCondition( if (comp->type == aql::NODE_TYPE_OPERATOR_BINARY_IN) { // a.b IN values if (!valNode->isArray()) { - return nullptr; + // a.b IN non-array + return new EmptyIndexIterator(_collection, trx, mmdr, this); } return createInIterator(trx, mmdr, attrNode, valNode); } // operator type unsupported - return nullptr; + return new EmptyIndexIterator(_collection, trx, mmdr, this); } /// @brief specializes the condition for use with the index diff --git a/arangod/RocksDBEngine/RocksDBKeyBounds.cpp b/arangod/RocksDBEngine/RocksDBKeyBounds.cpp index 1205b74062..1e00c9cb49 100644 --- a/arangod/RocksDBEngine/RocksDBKeyBounds.cpp +++ b/arangod/RocksDBEngine/RocksDBKeyBounds.cpp @@ -35,6 +35,10 @@ using namespace arangodb::velocypack; const char RocksDBKeyBounds::_stringSeparator = '\0'; +RocksDBKeyBounds RocksDBKeyBounds::Empty() { + return RocksDBKeyBounds(); +} + RocksDBKeyBounds RocksDBKeyBounds::Databases() { return RocksDBKeyBounds(RocksDBEntryType::Database); } @@ -98,6 +102,11 @@ rocksdb::Slice const RocksDBKeyBounds::end() const { return rocksdb::Slice(_endBuffer); } +// constructor for an empty bound. do not use for anything but to +// default-construct a key bound! +RocksDBKeyBounds::RocksDBKeyBounds() + : _type(RocksDBEntryType::Database), _startBuffer(), _endBuffer() {} + RocksDBKeyBounds::RocksDBKeyBounds(RocksDBEntryType type) : _type(type), _startBuffer(), _endBuffer() { switch (_type) { diff --git a/arangod/RocksDBEngine/RocksDBKeyBounds.h b/arangod/RocksDBEngine/RocksDBKeyBounds.h index 5944d60e5b..0802c3cda1 100644 --- a/arangod/RocksDBEngine/RocksDBKeyBounds.h +++ b/arangod/RocksDBEngine/RocksDBKeyBounds.h @@ -38,8 +38,11 @@ namespace arangodb { class RocksDBKeyBounds { public: - RocksDBKeyBounds() = delete; - + ////////////////////////////////////////////////////////////////////////////// + /// @brief empty bounds + ////////////////////////////////////////////////////////////////////////////// + static RocksDBKeyBounds Empty(); + ////////////////////////////////////////////////////////////////////////////// /// @brief Bounds for list of all databases ////////////////////////////////////////////////////////////////////////////// @@ -125,7 +128,8 @@ class RocksDBKeyBounds { rocksdb::Slice const end() const; private: - RocksDBKeyBounds(RocksDBEntryType type); + RocksDBKeyBounds(); + explicit RocksDBKeyBounds(RocksDBEntryType type); RocksDBKeyBounds(RocksDBEntryType type, uint64_t first); RocksDBKeyBounds(RocksDBEntryType type, uint64_t first, std::string const& second); diff --git a/arangod/RocksDBEngine/RocksDBPrimaryIndex.cpp b/arangod/RocksDBEngine/RocksDBPrimaryIndex.cpp index 6c9e85869c..901a71461f 100644 --- a/arangod/RocksDBEngine/RocksDBPrimaryIndex.cpp +++ b/arangod/RocksDBEngine/RocksDBPrimaryIndex.cpp @@ -502,14 +502,15 @@ IndexIterator* RocksDBPrimaryIndex::iteratorForCondition( } else if (comp->type == aql::NODE_TYPE_OPERATOR_BINARY_IN) { // a.b IN values if (!valNode->isArray()) { - return nullptr; + // a.b IN non-array + return new EmptyIndexIterator(_collection, trx, mmdr, this); } return createInIterator(trx, mmdr, attrNode, valNode); } // operator type unsupported - return nullptr; + return new EmptyIndexIterator(_collection, trx, mmdr, this); } /// @brief specializes the condition for use with the index diff --git a/arangod/RocksDBEngine/RocksDBVPackIndex.cpp b/arangod/RocksDBEngine/RocksDBVPackIndex.cpp index 3e8154f678..633459e003 100644 --- a/arangod/RocksDBEngine/RocksDBVPackIndex.cpp +++ b/arangod/RocksDBEngine/RocksDBVPackIndex.cpp @@ -1147,7 +1147,7 @@ IndexIterator* RocksDBVPackIndex::iteratorForCondition( // unsupported right now. Should have been rejected by // supportsFilterCondition TRI_ASSERT(false); - return nullptr; + return new EmptyIndexIterator(_collection, trx, mmdr, this); } value->toVelocyPackValue(searchValues); } diff --git a/js/server/tests/aql/aql-optimizer-indexes.js b/js/server/tests/aql/aql-optimizer-indexes.js index eaa3b5645f..4618b3ef80 100644 --- a/js/server/tests/aql/aql-optimizer-indexes.js +++ b/js/server/tests/aql/aql-optimizer-indexes.js @@ -227,6 +227,28 @@ function optimizerIndexesTestSuite () { assertEqual(0, results.stats.scannedIndex); }, +//////////////////////////////////////////////////////////////////////////////// +/// @brief test _id +//////////////////////////////////////////////////////////////////////////////// + + testUsePrimaryKeyInAttributeAccess : function () { + var values = [ "test1", "test2", "test21", "test30" ]; + var query = "LET data = NOOPT({ ids : " + JSON.stringify(values) + " }) FOR i IN " + c.name() + " FILTER i._key IN data.ids RETURN i.value"; + + var plan = AQL_EXPLAIN(query).plan; + var nodeTypes = plan.nodes.map(function(node) { + return node.type; + }); + + assertEqual("SingletonNode", nodeTypes[0], query); + assertNotEqual(-1, nodeTypes.indexOf("IndexNode"), query); + + var results = AQL_EXECUTE(query); + assertEqual([ 1, 2, 21, 30 ], results.json.sort(), query); + assertEqual(0, results.stats.scannedFull); + assertEqual(4, results.stats.scannedIndex); + }, + //////////////////////////////////////////////////////////////////////////////// /// @brief test _key //////////////////////////////////////////////////////////////////////////////// @@ -2560,7 +2582,18 @@ function optimizerIndexesTestSuite () { [ "FOR i IN " + c.name() + " FILTER i._key IN ['test23', 'test42'] RETURN i._key", [ 'test23', 'test42' ] ], [ "LET a = PASSTHRU([]) FOR i IN " + c.name() + " FILTER i._key IN a RETURN i._key", [ ] ], [ "LET a = PASSTHRU(['test23']) FOR i IN " + c.name() + " FILTER i._key IN a RETURN i._key", [ 'test23' ] ], - [ "LET a = PASSTHRU(['test23', 'test42']) FOR i IN " + c.name() + " FILTER i._key IN a RETURN i._key", [ 'test23', 'test42' ] ] + [ "LET a = PASSTHRU(['test23', 'test42']) FOR i IN " + c.name() + " FILTER i._key IN a RETURN i._key", [ 'test23', 'test42' ] ], + [ "LET a = PASSTHRU({ ids: ['test23', 'test42'] }) FOR i IN " + c.name() + " FILTER i._key IN a.ids RETURN i._key", [ 'test23', 'test42' ] ], + [ "LET a = PASSTHRU({ ids: [23, 42] }) FOR i IN " + c.name() + " FILTER i.value2 IN a.ids RETURN i.value2", [ 23, 42 ] ], + [ "LET a = PASSTHRU({ ids: [23, 42] }) FOR i IN " + c.name() + " FILTER i.value3 IN a.ids RETURN i.value2", [ 23, 42 ] ], + + // non-arrays. should not fail but return no results + [ "LET a = PASSTHRU({}) FOR i IN " + c.name() + " FILTER i._key IN a RETURN i._key", [ ] ], + [ "LET a = PASSTHRU({}) FOR i IN " + c.name() + " FILTER i.value2 IN a RETURN i.value2", [ ] ], + [ "LET a = PASSTHRU({}) FOR i IN " + c.name() + " FILTER i.value3 IN a RETURN i.value2", [ ] ] + + + ]; queries.forEach(function(query) {