diff --git a/CHANGELOG b/CHANGELOG index 4c5510b53e..3b743b0754 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -1,6 +1,10 @@ devel ----- +* fixed possible segfault when using COLLECT with a LIMIT and an offset + +* fixed COLLECT forgetting top-level variables after 1000 rows + * added sort-limit optimization in AQL; improves memory usage and execution time for some queries diff --git a/arangod/Aql/CollectBlock.cpp b/arangod/Aql/CollectBlock.cpp index 8dc3851b8f..ce0180ed3f 100644 --- a/arangod/Aql/CollectBlock.cpp +++ b/arangod/Aql/CollectBlock.cpp @@ -148,7 +148,8 @@ SortedCollectBlock::SortedCollectBlock(ExecutionEngine* engine, CollectNode cons _lastBlock(nullptr), _expressionRegister(ExecutionNode::MaxRegisterId), _collectRegister(ExecutionNode::MaxRegisterId), - _variableNames() { + _variableNames(), + _registersInherited(false) { for (auto const& p : en->_groupVariables) { // We know that planRegisters() has been run, so // getPlanNode()->_registerPlan is set up @@ -257,6 +258,10 @@ std::pair SortedCollectBlock::initializeCursor _currentGroup.reset(); _pos = 0; _lastBlock = nullptr; + if (_result != nullptr) { + auto r = _result.release(); + returnBlock(r); + } return res; } @@ -354,6 +359,8 @@ std::pair SortedCollectBlock::getOrSkipSome( // group. size_t maxBlockSize = _groupRegisters.empty() ? 1 : atMost; _result.reset(requestBlock(maxBlockSize, nrOutRegs)); + // We got a new block, we need to inherit registers for it + _registersInherited = false; TRI_ASSERT(nrInRegs <= _result->getNrRegs()); } @@ -386,12 +393,9 @@ std::pair SortedCollectBlock::getOrSkipSome( AqlItemBlock* cur = _buffer.front(); TRI_ASSERT(cur != nullptr); - // TODO this is dirty. if you have an idea how to improve this, please do. - // Can't we omit this? - if (_lastBlock == nullptr) { - // call only on the first row of the first block - TRI_ASSERT(_pos == 0); + if (!skipping && !_registersInherited) { inheritRegisters(cur, _result.get(), 0); + _registersInherited = true; } // if the current block changed, move the last block's infos into the @@ -478,9 +482,9 @@ void SortedCollectBlock::emitGroup(AqlItemBlock const* cur, AqlItemBlock* res, size_t row, bool skipping) { TRI_ASSERT(res != nullptr); - // TODO removing this block doesn't seem to have any effect. - // find out if it's necessary, and why, and what it has to do with - // the inheritRegisters call in getOrSkipSome. + // Copy input registers from the first row. Note that the input variables that + // are still available after the block can only be constant over all input + // rows. if (row > 0 && !skipping) { // re-use already copied AqlValues TRI_ASSERT(cur != nullptr); diff --git a/arangod/Aql/CollectBlock.h b/arangod/Aql/CollectBlock.h index 9008f7a99c..b9c2cbbcaf 100644 --- a/arangod/Aql/CollectBlock.h +++ b/arangod/Aql/CollectBlock.h @@ -135,6 +135,8 @@ class SortedCollectBlock final : public ExecutionBlock { /// @brief list of variables names for the registers std::vector _variableNames; + + bool _registersInherited; }; class HashedCollectBlock final : public ExecutionBlock { diff --git a/tests/js/server/aql/aql-optimizer-rule-remove-redundant-sorts.js b/tests/js/server/aql/aql-optimizer-rule-remove-redundant-sorts.js index 6b5e5550dc..ffc3cc031c 100644 --- a/tests/js/server/aql/aql-optimizer-rule-remove-redundant-sorts.js +++ b/tests/js/server/aql/aql-optimizer-rule-remove-redundant-sorts.js @@ -28,9 +28,10 @@ /// @author Copyright 2012, triAGENS GmbH, Cologne, Germany //////////////////////////////////////////////////////////////////////////////// -var jsunity = require("jsunity"); -var helper = require("@arangodb/aql-helper"); -var isEqual = helper.isEqual; +const _ = require('lodash'); +const jsunity = require("jsunity"); +const helper = require("@arangodb/aql-helper"); +const isEqual = helper.isEqual; //////////////////////////////////////////////////////////////////////////////// /// @brief test suite @@ -213,6 +214,57 @@ function optimizerRuleTestSuite () { }); }, +//////////////////////////////////////////////////////////////////////////////// +/// @brief test collect variable pass through +/// +/// Regression test, input registers were only copied in the first block +/// in sorted collect. +//////////////////////////////////////////////////////////////////////////////// + + + testCollectWithVariablePassThrough : function () { + const query = ` + LET x = LENGTH(1..42) + FOR i IN 1..2000 + LET d = {val: i} + SORT d.val + COLLECT v = d.val + RETURN {v, x} + `; + + const expected = _.range(1, 2001).map(i => ({v: i, x: 42})); + + const result = AQL_EXECUTE(query, {}, paramEnabled).json; + assertEqual(result.length, expected.length); + // Don't compare the whole arrays, for a better readable output in case + // of errors. + for (let i = 0; i < expected.length; i++) { + assertEqual(result[i], expected[i], `mismatch at index ${i}`); + } + }, + +//////////////////////////////////////////////////////////////////////////////// +/// @brief test collect with offset +/// +/// Regression test, there was a segfault in sorted collect when at least one +/// row was skipped. +//////////////////////////////////////////////////////////////////////////////// + + + testCollectWithOffset : function () { + const query = ` + FOR i IN 1..2 + LET d = {val: i} + SORT d.val + COLLECT v = d.val + LIMIT 1,1 + RETURN v + `; + + const result = AQL_EXECUTE(query, {}, paramEnabled).json; + assertEqual(result, [2]); + }, + }; }