From 00c060c884a609d227097749414220bc801e0ce1 Mon Sep 17 00:00:00 2001 From: Dan Larkin-York Date: Mon, 26 Nov 2018 16:41:29 -0500 Subject: [PATCH] [3.4] Fix end condition (hasMore) for EnumerateViewNode. (#7278) * Fix end condition (hasMore) for EnumerateViewNode. * Fix crashes. * Some more fixes. * eliminate code duplication --- arangod/Aql/CalculationBlock.cpp | 1 + arangod/IResearch/IResearchViewBlock.cpp | 72 +++++++++---------- .../js/server/aql/aql-profiler-noncluster.js | 30 ++++++-- 3 files changed, 57 insertions(+), 46 deletions(-) diff --git a/arangod/Aql/CalculationBlock.cpp b/arangod/Aql/CalculationBlock.cpp index de853eb4d1..ed2dc8e60d 100644 --- a/arangod/Aql/CalculationBlock.cpp +++ b/arangod/Aql/CalculationBlock.cpp @@ -186,6 +186,7 @@ CalculationBlock::getSome(size_t atMost) { traceGetSomeBegin(atMost); if (_done) { + traceGetSomeEnd(nullptr, ExecutionState::DONE); return {ExecutionState::DONE, nullptr}; } diff --git a/arangod/IResearch/IResearchViewBlock.cpp b/arangod/IResearch/IResearchViewBlock.cpp index 70788cec3b..d7e2a2d7e7 100644 --- a/arangod/IResearch/IResearchViewBlock.cpp +++ b/arangod/IResearch/IResearchViewBlock.cpp @@ -293,52 +293,30 @@ IResearchViewBlockBase::getSome(size_t atMost) { return {ExecutionState::DONE, nullptr}; } - bool needMore; - AqlItemBlock* cur = nullptr; - ReadContext ctx(getNrInputRegisters()); - RegisterId const nrOutRegs = getNrOutputRegisters(); do { - do { - needMore = false; + if (_buffer.empty()) { + size_t const toFetch = (std::min)(DefaultBatchSize(), atMost); - if (_buffer.empty()) { - size_t const toFetch = (std::min)(DefaultBatchSize(), atMost); - auto upstreamRes = ExecutionBlock::getBlock(toFetch); - if (upstreamRes.first == ExecutionState::WAITING) { - traceGetSomeEnd(nullptr, ExecutionState::WAITING); - return {upstreamRes.first, nullptr}; - } - _upstreamState = upstreamRes.first; - if (!upstreamRes.second) { - _done = true; - traceGetSomeEnd(nullptr, ExecutionState::DONE); - return {ExecutionState::DONE, nullptr}; - } - _pos = 0; // this is in the first block + switch (getBlockIfNeeded(toFetch)) { + case BufferState::NO_MORE_BLOCKS: + TRI_ASSERT(_inflight == 0); + _done = true; + TRI_ASSERT(getHasMoreState() == ExecutionState::DONE); + traceGetSomeEnd(nullptr, ExecutionState::DONE); + return {ExecutionState::DONE, nullptr}; + case BufferState::WAITING: + traceGetSomeEnd(nullptr, ExecutionState::WAITING); + return {ExecutionState::WAITING, nullptr}; + default: reset(); } + } - // If we get here, we do have _buffer.front() - cur = _buffer.front(); - - if (!_hasMore) { - needMore = true; - _hasMore = true; - - if (++_pos >= cur->size()) { - _buffer.pop_front(); // does not throw - returnBlock(cur); - _pos = 0; - } else { - // we have exhausted this cursor - // re-initialize fetching of documents - reset(); - } - } - } while (needMore); + // If we get here, we do have _buffer.front() + auto* cur = _buffer.front(); TRI_ASSERT(cur); TRI_ASSERT(ctx.curRegs == cur->getNrRegs()); @@ -352,19 +330,33 @@ IResearchViewBlockBase::getSome(size_t atMost) { throwIfKilled(); // check if we were aborted - TRI_IF_FAILURE("EnumerateViewBlock::moreDocuments") { + TRI_IF_FAILURE("IResearchViewBlockBase::getSome") { THROW_ARANGO_EXCEPTION(TRI_ERROR_DEBUG); } _hasMore = next(ctx, atMost); + if (!_hasMore) { + _hasMore = true; + + if (++_pos >= cur->size()) { + _buffer.pop_front(); // does not throw + returnBlock(cur); + _pos = 0; + } else { + // we have exhausted this cursor + // re-initialize fetching of documents + reset(); + } + } + // If the collection is actually empty we cannot forward an empty block } while (ctx.pos == 0); TRI_ASSERT(ctx.res); // aggregate stats - _engine->_stats.scannedIndex += static_cast(ctx.pos); + _engine->_stats.scannedIndex += static_cast(ctx.pos); if (ctx.pos < atMost) { // The collection did not have enough results diff --git a/tests/js/server/aql/aql-profiler-noncluster.js b/tests/js/server/aql/aql-profiler-noncluster.js index d47ebbab6b..1690056a2b 100644 --- a/tests/js/server/aql/aql-profiler-noncluster.js +++ b/tests/js/server/aql/aql-profiler-noncluster.js @@ -323,9 +323,15 @@ function ahuacatlProfilerTestSuite () { const query = `FOR d IN @@view SEARCH d.value != 0 OPTIONS { waitForSync: true } RETURN d.value`; const genNodeList = (rows, batches) => { + // EnumerateViewBlock returns HASMORE when asked for the exact number + // of items it has left. This could be improved. + const optimalBatches = Math.ceil(rows / defaultBatchSize); + const maxViewBatches = Math.floor(rows / defaultBatchSize) + 1; + const viewBatches = [optimalBatches, maxViewBatches]; + return [ - {type: SingletonBlock, calls: 2, items: 1}, - {type: EnumerateViewNode, calls: batches + 1, items: rows}, + {type: SingletonBlock, calls: 1, items: 1}, + {type: EnumerateViewNode, calls: viewBatches, items: rows}, {type: CalculationBlock, calls: rows % defaultBatchSize === 0 ? batches + 1 : batches, items: rows}, {type: ReturnBlock, calls: rows % defaultBatchSize === 0 ? batches + 1 : batches, items: rows} ]; @@ -350,9 +356,15 @@ function ahuacatlProfilerTestSuite () { const query = `FOR d IN @@view SEARCH d.value != 0 OPTIONS { waitForSync: true } SORT d.value DESC RETURN d.value`; const genNodeList = (rows, batches) => { + // EnumerateViewBlock returns HASMORE when asked for the exact number + // of items it has left. This could be improved. + const optimalBatches = Math.ceil(rows / defaultBatchSize); + const maxViewBatches = Math.floor(rows / defaultBatchSize) + 1; + const viewBatches = [optimalBatches, maxViewBatches]; + return [ - {type: SingletonBlock, calls: 2, items: 1}, - {type: EnumerateViewNode, calls: batches + 1, items: rows}, + {type: SingletonBlock, calls: 1, items: 1}, + {type: EnumerateViewNode, calls: viewBatches, items: rows}, {type: CalculationBlock, calls: rows % defaultBatchSize === 0 ? batches + 1 : batches, items: rows}, {type: SortBlock, calls: batches, items: rows}, {type: ReturnBlock, calls: batches, items: rows} @@ -378,9 +390,15 @@ function ahuacatlProfilerTestSuite () { const query = `FOR d IN @@view SEARCH d.value != 0 OPTIONS { waitForSync: true } SORT TFIDF(d) ASC, BM25(d) RETURN d.value`; const genNodeList = (rows, batches) => { + // EnumerateViewBlock returns HASMORE when asked for the exact number + // of items it has left. This could be improved. + const optimalBatches = Math.ceil(rows / defaultBatchSize); + const maxViewBatches = Math.floor(rows / defaultBatchSize) + 1; + const viewBatches = [optimalBatches, maxViewBatches]; + return [ - {type: SingletonBlock, calls: 2, items: 1}, - {type: EnumerateViewNode, calls: batches + 1, items: rows}, + {type: SingletonBlock, calls: 1, items: 1}, + {type: EnumerateViewNode, calls: viewBatches, items: rows}, {type: SortBlock, calls: batches, items: rows}, {type: CalculationBlock, calls: batches, items: rows}, {type: ReturnBlock, calls: batches, items: rows}