From 7fd1d3844268063d8a6758e473bef35765fc6832 Mon Sep 17 00:00:00 2001 From: Shylock Hg <33566796+Shylock-Hg@users.noreply.github.com> Date: Tue, 19 Apr 2022 12:01:19 +0800 Subject: [PATCH 1/4] Move input rows of Traverse and AppendVertices. --- src/graph/context/Iterator.h | 18 +++++++++++++--- src/graph/executor/Executor.cpp | 21 +++++++++++++++++++ src/graph/executor/Executor.h | 4 ++++ .../executor/query/AppendVerticesExecutor.cpp | 3 ++- src/graph/executor/query/TraverseExecutor.cpp | 3 ++- 5 files changed, 44 insertions(+), 5 deletions(-) diff --git a/src/graph/context/Iterator.h b/src/graph/context/Iterator.h index 8a3a379255a..bde9341ea45 100644 --- a/src/graph/context/Iterator.h +++ b/src/graph/context/Iterator.h @@ -73,6 +73,8 @@ class Iterator { virtual const Row* row() const = 0; + virtual Row moveRow() = 0; + // erase range, no include last position, if last > size(), erase to the end // position virtual void eraseRange(size_t first, size_t last) = 0; @@ -228,6 +230,11 @@ class DefaultIter final : public Iterator { return nullptr; } + Row moveRow() override { + DLOG(FATAL) << "This method should not be invoked"; + return Row{}; + } + private: void doReset(size_t pos) override { DCHECK((pos == 0 && size() == 0) || (pos < size())); @@ -318,6 +325,10 @@ class GetNeighborsIter final : public Iterator { return currentEdge_; } + Row moveRow() override { + return std::move(*currentEdge_); + } + private: void doReset(size_t pos) override { UNUSED(pos); @@ -475,6 +486,10 @@ class SequentialIter : public Iterator { Value getEdge() const override; + Row moveRow() override { + return std::move(*iter_); + } + protected: const Row* row() const override { return &*iter_; @@ -484,9 +499,6 @@ class SequentialIter : public Iterator { friend class DataCollectExecutor; friend class AppendVerticesExecutor; friend class TraverseExecutor; - Row&& moveRow() { - return std::move(*iter_); - } void doReset(size_t pos) override; diff --git a/src/graph/executor/Executor.cpp b/src/graph/executor/Executor.cpp index 37706f21c6a..0b31365fe8c 100644 --- a/src/graph/executor/Executor.cpp +++ b/src/graph/executor/Executor.cpp @@ -682,6 +682,27 @@ void Executor::dropBody(const PlanNode *body) { } } +bool Executor::movable(const Variable *var) { + // Only support input variables of current executor + DCHECK(std::find(node_->inputVars().begin(), node_->inputVars().end(), DCHECK_NOTNULL(var)) != + node_->inputVars().end()); + // TODO support executor in loop + if (node()->kind() == PlanNode::Kind::kLoop) { + return false; + } + if (node()->loopLayers() != 0) { + // The lifetime of loop body is managed by Loop node + return false; + } + + if (node()->kind() == PlanNode::Kind::kSelect) { + return false; + } + // Normal node + // Make sure drop happened-after count decrement + return var->userCount.load(std::memory_order_acquire) == 1; +} + Status Executor::finish(Result &&result) { if (!FLAGS_enable_lifetime_optimize || node()->outputVarPtr()->userCount.load(std::memory_order_relaxed) != 0) { diff --git a/src/graph/executor/Executor.h b/src/graph/executor/Executor.h index 6e9b932f86b..a2ba45888ac 100644 --- a/src/graph/executor/Executor.h +++ b/src/graph/executor/Executor.h @@ -94,6 +94,10 @@ class Executor : private boost::noncopyable, private cpp::NonMovable { void drop(const PlanNode *node); void dropBody(const PlanNode *body); + // Check whether the variable is movable, it's movable when reach end of lifetime + // This method shouldn't call after `finish` method! + bool movable(const Variable *var); + // Store the result of this executor to execution context Status finish(Result &&result); // Store the default result which not used for later executor diff --git a/src/graph/executor/query/AppendVerticesExecutor.cpp b/src/graph/executor/query/AppendVerticesExecutor.cpp index a7aa369ded4..d30c82eabe7 100644 --- a/src/graph/executor/query/AppendVerticesExecutor.cpp +++ b/src/graph/executor/query/AppendVerticesExecutor.cpp @@ -101,12 +101,13 @@ Status AppendVerticesExecutor::handleResp( } auto *src = av->src(); + bool mv = movable(av->inputVars().front()); for (; inputIter->valid(); inputIter->next()) { auto dstFound = map.find(src->eval(ctx(inputIter.get()))); if (dstFound == map.end()) { continue; } - Row row = *inputIter->row(); + Row row = mv ? inputIter->moveRow() : *inputIter->row(); row.values.emplace_back(dstFound->second); ds.rows.emplace_back(std::move(row)); } diff --git a/src/graph/executor/query/TraverseExecutor.cpp b/src/graph/executor/query/TraverseExecutor.cpp index 0210a5ce6c5..c6cadc3edf7 100644 --- a/src/graph/executor/query/TraverseExecutor.cpp +++ b/src/graph/executor/query/TraverseExecutor.cpp @@ -48,6 +48,7 @@ Status TraverseExecutor::buildRequestDataSet() { auto* src = traverse_->src(); QueryExpressionContext ctx(ectx_); + bool mv = movable(traverse_->inputVars().front()); for (; iter->valid(); iter->next()) { auto vid = src->eval(ctx(iter)); if (!SchemaUtil::isValidVid(vid, vidType)) { @@ -56,7 +57,7 @@ Status TraverseExecutor::buildRequestDataSet() { continue; } // Need copy here, Argument executor may depends on this variable. - auto prePath = *iter->row(); + auto prePath = mv ? iter->moveRow() : *iter->row(); buildPath(prev, vid, std::move(prePath)); if (!uniqueSet.emplace(vid).second) { continue; From f9a873b78bd3c07c96d0959cf5030d80687c34e1 Mon Sep 17 00:00:00 2001 From: Shylock Hg <33566796+Shylock-Hg@users.noreply.github.com> Date: Tue, 19 Apr 2022 15:04:22 +0800 Subject: [PATCH 2/4] Avoid skip validate pattern expression with aggregate. --- src/graph/validator/MatchValidator.cpp | 3 +-- .../bugfix/AggPatternExpression.feature | 17 +++++++++++++++++ 2 files changed, 18 insertions(+), 2 deletions(-) create mode 100644 tests/tck/features/bugfix/AggPatternExpression.feature diff --git a/src/graph/validator/MatchValidator.cpp b/src/graph/validator/MatchValidator.cpp index 93cc0fc6be9..1e42f0849cf 100644 --- a/src/graph/validator/MatchValidator.cpp +++ b/src/graph/validator/MatchValidator.cpp @@ -803,6 +803,7 @@ Status MatchValidator::validateGroup(YieldClauseContext &yieldCtx, DCHECK(!cols.empty()); for (auto *col : cols) { auto *colExpr = col->expr(); + NG_RETURN_IF_ERROR(validateMatchPathExpr(colExpr, yieldCtx.aliasesAvailable, matchs)); auto colOldName = col->name(); if (colExpr->kind() != Expression::Kind::kAggregate) { auto collectAggCol = colExpr->clone(); @@ -834,8 +835,6 @@ Status MatchValidator::validateGroup(YieldClauseContext &yieldCtx, yieldCtx.groupKeys_.emplace_back(colExpr); } - NG_RETURN_IF_ERROR(validateMatchPathExpr(colExpr, yieldCtx.aliasesAvailable, matchs)); - yieldCtx.groupItems_.emplace_back(colExpr); yieldCtx.projCols_->addColumn( diff --git a/tests/tck/features/bugfix/AggPatternExpression.feature b/tests/tck/features/bugfix/AggPatternExpression.feature new file mode 100644 index 00000000000..e84a13353ac --- /dev/null +++ b/tests/tck/features/bugfix/AggPatternExpression.feature @@ -0,0 +1,17 @@ +# Copyright (c) 2022 vesoft inc. All rights reserved. +# +# This source code is licensed under Apache 2.0 License. +# #4175 +Feature: Test crash when aggregate with pattern expression + + Background: + Given a graph with space named "nba" + + Scenario: Crash when aggregate with pattern expression + When executing query: + """ + MATCH (v:player) WHERE id(v) == 'Tim Duncan' return v.player.name AS name, size((v)--(:team)) AS len, count(v.player.name) * 2 AS count + """ + Then the result should be, in any order, with relax comparison: + | name | len | count | + | 'Tim Duncan' | 1 | 2 | From a01616e8f897977077d85d718a4b05efab4e77c6 Mon Sep 17 00:00:00 2001 From: Shylock Hg <33566796+Shylock-Hg@users.noreply.github.com> Date: Tue, 19 Apr 2022 15:24:03 +0800 Subject: [PATCH 3/4] Fix case. --- tests/tck/features/bugfix/AggPatternExpression.feature | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/tests/tck/features/bugfix/AggPatternExpression.feature b/tests/tck/features/bugfix/AggPatternExpression.feature index e84a13353ac..31b83f860d9 100644 --- a/tests/tck/features/bugfix/AggPatternExpression.feature +++ b/tests/tck/features/bugfix/AggPatternExpression.feature @@ -8,10 +8,11 @@ Feature: Test crash when aggregate with pattern expression Given a graph with space named "nba" Scenario: Crash when aggregate with pattern expression + # TODO aggregate should bypass all input, like `(v)--(:team)` here When executing query: """ - MATCH (v:player) WHERE id(v) == 'Tim Duncan' return v.player.name AS name, size((v)--(:team)) AS len, count(v.player.name) * 2 AS count + MATCH (v:player) WHERE id(v) == 'Tim Duncan' return v.player.name AS name, size((v)--(:team)) + count(v.player.name) * 2 AS count """ Then the result should be, in any order, with relax comparison: - | name | len | count | - | 'Tim Duncan' | 1 | 2 | + | name | count | + | 'Tim Duncan' | NULL | From 63ca8fda7b697e5413609bf723504cf2487722bf Mon Sep 17 00:00:00 2001 From: Shylock Hg <33566796+Shylock-Hg@users.noreply.github.com> Date: Tue, 19 Apr 2022 15:25:35 +0800 Subject: [PATCH 4/4] Revert "Move input rows of Traverse and AppendVertices." This reverts commit 7fd1d3844268063d8a6758e473bef35765fc6832. --- src/graph/context/Iterator.h | 18 +++------------- src/graph/executor/Executor.cpp | 21 ------------------- src/graph/executor/Executor.h | 4 ---- .../executor/query/AppendVerticesExecutor.cpp | 3 +-- src/graph/executor/query/TraverseExecutor.cpp | 3 +-- 5 files changed, 5 insertions(+), 44 deletions(-) diff --git a/src/graph/context/Iterator.h b/src/graph/context/Iterator.h index bde9341ea45..8a3a379255a 100644 --- a/src/graph/context/Iterator.h +++ b/src/graph/context/Iterator.h @@ -73,8 +73,6 @@ class Iterator { virtual const Row* row() const = 0; - virtual Row moveRow() = 0; - // erase range, no include last position, if last > size(), erase to the end // position virtual void eraseRange(size_t first, size_t last) = 0; @@ -230,11 +228,6 @@ class DefaultIter final : public Iterator { return nullptr; } - Row moveRow() override { - DLOG(FATAL) << "This method should not be invoked"; - return Row{}; - } - private: void doReset(size_t pos) override { DCHECK((pos == 0 && size() == 0) || (pos < size())); @@ -325,10 +318,6 @@ class GetNeighborsIter final : public Iterator { return currentEdge_; } - Row moveRow() override { - return std::move(*currentEdge_); - } - private: void doReset(size_t pos) override { UNUSED(pos); @@ -486,10 +475,6 @@ class SequentialIter : public Iterator { Value getEdge() const override; - Row moveRow() override { - return std::move(*iter_); - } - protected: const Row* row() const override { return &*iter_; @@ -499,6 +484,9 @@ class SequentialIter : public Iterator { friend class DataCollectExecutor; friend class AppendVerticesExecutor; friend class TraverseExecutor; + Row&& moveRow() { + return std::move(*iter_); + } void doReset(size_t pos) override; diff --git a/src/graph/executor/Executor.cpp b/src/graph/executor/Executor.cpp index 0b31365fe8c..37706f21c6a 100644 --- a/src/graph/executor/Executor.cpp +++ b/src/graph/executor/Executor.cpp @@ -682,27 +682,6 @@ void Executor::dropBody(const PlanNode *body) { } } -bool Executor::movable(const Variable *var) { - // Only support input variables of current executor - DCHECK(std::find(node_->inputVars().begin(), node_->inputVars().end(), DCHECK_NOTNULL(var)) != - node_->inputVars().end()); - // TODO support executor in loop - if (node()->kind() == PlanNode::Kind::kLoop) { - return false; - } - if (node()->loopLayers() != 0) { - // The lifetime of loop body is managed by Loop node - return false; - } - - if (node()->kind() == PlanNode::Kind::kSelect) { - return false; - } - // Normal node - // Make sure drop happened-after count decrement - return var->userCount.load(std::memory_order_acquire) == 1; -} - Status Executor::finish(Result &&result) { if (!FLAGS_enable_lifetime_optimize || node()->outputVarPtr()->userCount.load(std::memory_order_relaxed) != 0) { diff --git a/src/graph/executor/Executor.h b/src/graph/executor/Executor.h index a2ba45888ac..6e9b932f86b 100644 --- a/src/graph/executor/Executor.h +++ b/src/graph/executor/Executor.h @@ -94,10 +94,6 @@ class Executor : private boost::noncopyable, private cpp::NonMovable { void drop(const PlanNode *node); void dropBody(const PlanNode *body); - // Check whether the variable is movable, it's movable when reach end of lifetime - // This method shouldn't call after `finish` method! - bool movable(const Variable *var); - // Store the result of this executor to execution context Status finish(Result &&result); // Store the default result which not used for later executor diff --git a/src/graph/executor/query/AppendVerticesExecutor.cpp b/src/graph/executor/query/AppendVerticesExecutor.cpp index d30c82eabe7..a7aa369ded4 100644 --- a/src/graph/executor/query/AppendVerticesExecutor.cpp +++ b/src/graph/executor/query/AppendVerticesExecutor.cpp @@ -101,13 +101,12 @@ Status AppendVerticesExecutor::handleResp( } auto *src = av->src(); - bool mv = movable(av->inputVars().front()); for (; inputIter->valid(); inputIter->next()) { auto dstFound = map.find(src->eval(ctx(inputIter.get()))); if (dstFound == map.end()) { continue; } - Row row = mv ? inputIter->moveRow() : *inputIter->row(); + Row row = *inputIter->row(); row.values.emplace_back(dstFound->second); ds.rows.emplace_back(std::move(row)); } diff --git a/src/graph/executor/query/TraverseExecutor.cpp b/src/graph/executor/query/TraverseExecutor.cpp index c6cadc3edf7..0210a5ce6c5 100644 --- a/src/graph/executor/query/TraverseExecutor.cpp +++ b/src/graph/executor/query/TraverseExecutor.cpp @@ -48,7 +48,6 @@ Status TraverseExecutor::buildRequestDataSet() { auto* src = traverse_->src(); QueryExpressionContext ctx(ectx_); - bool mv = movable(traverse_->inputVars().front()); for (; iter->valid(); iter->next()) { auto vid = src->eval(ctx(iter)); if (!SchemaUtil::isValidVid(vid, vidType)) { @@ -57,7 +56,7 @@ Status TraverseExecutor::buildRequestDataSet() { continue; } // Need copy here, Argument executor may depends on this variable. - auto prePath = mv ? iter->moveRow() : *iter->row(); + auto prePath = *iter->row(); buildPath(prev, vid, std::move(prePath)); if (!uniqueSet.emplace(vid).second) { continue;