Skip to content

Commit

Permalink
Bug fix 3.5/fix issue 11303 (#11536)
Browse files Browse the repository at this point in the history
* fixed issue #11303

* added test

* Update CHANGELOG

Co-authored-by: KVS85 <vadim@arangodb.com>
  • Loading branch information
jsteemann and KVS85 committed May 8, 2020
1 parent 2b147cb commit 69a307b
Show file tree
Hide file tree
Showing 3 changed files with 129 additions and 14 deletions.
12 changes: 12 additions & 0 deletions CHANGELOG
@@ -1,3 +1,15 @@
v3.5.5.1 (2020-05-08)
---------------------

* Fixed issue #11303: The result data of filter operation on edge document with
_from or _to field ALWAYS miss some data on version 3.6.2.

This bug could have led to some edges from a RocksDB edge index not being
returned when a non-traversal edge lookup was done in an AQL query in an inner
FOR loop. Whether or not edges were withheld depended on whether edges were
located within the same prefix domain.


v3.5.5 (2020-04-24)
-------------------

Expand Down
27 changes: 13 additions & 14 deletions arangod/RocksDBEngine/RocksDBEdgeIndex.cpp
Expand Up @@ -98,12 +98,6 @@ class RocksDBEdgeIndexLookupIterator final : public IndexIterator {
_lastKey(VPackSlice::nullSlice()) {
TRI_ASSERT(_keys != nullptr);
TRI_ASSERT(_keys->slice().isArray());

auto* mthds = RocksDBTransactionState::toMethods(trx);
// intentional copy of the options
rocksdb::ReadOptions ro = mthds->iteratorReadOptions();
ro.fill_cache = EdgeIndexFillBlockCache;
_iterator = mthds->NewIterator(ro, index->columnFamily());
}

~RocksDBEdgeIndexLookupIterator() {
Expand Down Expand Up @@ -305,29 +299,35 @@ class RocksDBEdgeIndexLookupIterator final : public IndexIterator {

void lookupInRocksDB(VPackStringRef fromTo) {
// Bad case read from RocksDB
auto* mthds = RocksDBTransactionState::toMethods(_trx);
// intentional copy of the options
rocksdb::ReadOptions ro = mthds->iteratorReadOptions();
ro.fill_cache = EdgeIndexFillBlockCache;
std::unique_ptr<rocksdb::Iterator> iterator = mthds->NewIterator(ro, _index->columnFamily());

_bounds = RocksDBKeyBounds::EdgeIndexVertex(_index->_objectId, fromTo);
resetInplaceMemory();
rocksdb::Comparator const* cmp = _index->comparator();
auto end = _bounds.end();

cache::Cache* cc = _cache.get();
_builder.openArray(true);
for (_iterator->Seek(_bounds.start());
_iterator->Valid() && (cmp->Compare(_iterator->key(), end) < 0);
_iterator->Next()) {
for (iterator->Seek(_bounds.start());
iterator->Valid() && (cmp->Compare(iterator->key(), end) < 0);
iterator->Next()) {
LocalDocumentId const documentId =
RocksDBKey::indexDocumentId(RocksDBEntryType::EdgeIndexValue,
_iterator->key());
iterator->key());

// adding documentId and _from or _to value
_builder.add(VPackValue(documentId.id()));
VPackStringRef vertexId = RocksDBValue::vertexId(_iterator->value());
VPackStringRef vertexId = RocksDBValue::vertexId(iterator->value());
_builder.add(VPackValuePair(vertexId.data(), vertexId.size(), VPackValueType::String));
}
_builder.close();

// validate that Iterator is in a good shape and hasn't failed
arangodb::rocksutils::checkIteratorStatus(_iterator.get());
arangodb::rocksutils::checkIteratorStatus(iterator.get());

if (cc != nullptr) {
// TODO Add cache retry on next call
Expand Down Expand Up @@ -368,10 +368,9 @@ class RocksDBEdgeIndexLookupIterator final : public IndexIterator {
std::unique_ptr<arangodb::velocypack::Builder> _keys;
arangodb::velocypack::ArrayIterator _keysIterator;

// the following 2 values are required for correct batch handling
std::unique_ptr<rocksdb::Iterator> _iterator; // iterator position in rocksdb
RocksDBKeyBounds _bounds;

// the following values are required for correct batch handling
arangodb::velocypack::Builder _builder;
arangodb::velocypack::ArrayIterator _builderIterator;
arangodb::velocypack::Slice _lastKey;
Expand Down
104 changes: 104 additions & 0 deletions tests/js/common/shell/shell-edge-index.js
@@ -0,0 +1,104 @@
/*jshint globalstrict:false, strict:false */
/*global assertEqual */

////////////////////////////////////////////////////////////////////////////////
/// @brief test the edge index
///
/// @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 Jan Steemann
/// @author Copyright 2012, triAGENS GmbH, Cologne, Germany
////////////////////////////////////////////////////////////////////////////////

let jsunity = require("jsunity");
let db = require("internal").db;

function EdgeIndexSuite() {
'use strict';
const cn = "UnitTestsCollection";

return {

setUpAll : function () {
db._drop(cn);
let c = db._createEdgeCollection(cn);

let docs = [];
for (let i = 0; i < 1000; ++i) {
docs.push({ _from: "test/v" + i, _to: "test/v" + i });
}
c.insert(docs);
},

tearDownAll : function () {
db._drop(cn);
},

setUp : function () {
db[cn].unload(); // drop caches
},

tearDown : function () {
db[cn].unload(); // drop caches
},

testLookupFrom : function () {
for (let i = 0; i < 1000; ++i) {
let result = db._query("FOR doc IN " + cn + " FILTER doc._from == @from RETURN doc", { from: "test/v" + i }).toArray();
assertEqual(1, result.length);
}
},

testLookupTo : function () {
for (let i = 0; i < 1000; ++i) {
let result = db._query("FOR doc IN " + cn + " FILTER doc._to == @to RETURN doc", { to: "test/v" + i }).toArray();
assertEqual(1, result.length);
}
},

testLookupFromTo : function () {
for (let i = 0; i < 1000; ++i) {
let result = db._query("FOR doc IN " + cn + " FILTER doc._from == @from && doc._to == @to RETURN doc", { from: "test/v" + i, to: "test/v" + i }).toArray();
assertEqual(1, result.length);
}
},

testLookupFromNested : function () {
let result = db._query("FOR i IN 0..999 FOR doc IN " + cn + " FILTER doc._from == CONCAT('test/v', i) RETURN doc").toArray();
assertEqual(1000, result.length);
},

testLookupToNested : function () {
let result = db._query("FOR i IN 0..999 FOR doc IN " + cn + " FILTER doc._to == CONCAT('test/v', i) RETURN doc").toArray();
assertEqual(1000, result.length);
},

testLookupFromToNested : function () {
let result = db._query("FOR i IN 0..999 FOR doc IN " + cn + " FILTER doc._from == CONCAT('test/v', i) && doc._to == CONCAT('test/v', i) RETURN doc").toArray();
assertEqual(1000, result.length);
},

};
}

jsunity.run(EdgeIndexSuite);

return jsunity.done();

0 comments on commit 69a307b

Please sign in to comment.