From 23628a601f15a0b3114a439b3ca01067487d10b4 Mon Sep 17 00:00:00 2001 From: Aiee <18348405+Aiee@users.noreply.github.com> Date: Wed, 26 Oct 2022 17:03:52 +0800 Subject: [PATCH 1/3] Fix pattern expr --- src/common/expression/UnaryExpression.h | 2 +- src/graph/executor/query/RollUpApplyExecutor.cpp | 7 +++++-- tests/tck/features/match/PathExpr.feature | 7 +++++++ 3 files changed, 13 insertions(+), 3 deletions(-) diff --git a/src/common/expression/UnaryExpression.h b/src/common/expression/UnaryExpression.h index 1883aa0f118..a053c946351 100644 --- a/src/common/expression/UnaryExpression.h +++ b/src/common/expression/UnaryExpression.h @@ -87,7 +87,7 @@ class UnaryExpression final : public Expression { void resetFrom(Decoder& decoder) override; private: - Expression* operand_; + Expression* operand_{nullptr}; Value result_; }; diff --git a/src/graph/executor/query/RollUpApplyExecutor.cpp b/src/graph/executor/query/RollUpApplyExecutor.cpp index 87c96c8e10d..ca314f8ad15 100644 --- a/src/graph/executor/query/RollUpApplyExecutor.cpp +++ b/src/graph/executor/query/RollUpApplyExecutor.cpp @@ -121,7 +121,10 @@ DataSet RollUpApplyExecutor::probe(std::vector probeKeys, List list; list.values.reserve(probeKeys.size()); for (auto& col : probeKeys) { - Value val = col->eval(ctx(probeIter)); + // Clone the expression so when evaluating the InputPropertyExpression, the propIndex_ will + // not be cached + auto* colCopy = col->clone(); + Value val = colCopy->eval(ctx(probeIter)); list.values.emplace_back(std::move(val)); } @@ -150,7 +153,7 @@ folly::Future RollUpApplyExecutor::rollUpApply() { } else if (rollUpApplyNode->compareCols().size() == 1) { std::unordered_map hashTable; // Clone the expression so when evaluating the InputPropertyExpression, the propIndex_ will not - // be buffered. + // be cached. buildSingleKeyHashTable(rollUpApplyNode->compareCols()[0]->clone(), rollUpApplyNode->collectCol(), rhsIter_.get(), diff --git a/tests/tck/features/match/PathExpr.feature b/tests/tck/features/match/PathExpr.feature index 0cb908646c5..d494a7e9418 100644 --- a/tests/tck/features/match/PathExpr.feature +++ b/tests/tck/features/match/PathExpr.feature @@ -150,6 +150,13 @@ Feature: Basic match | ("Aron Baynes" :player{age: 32, name: "Aron Baynes"}) | | ("Tiago Splitter" :player{age: 34, name: "Tiago Splitter"}) | | ("Boris Diaw" :player{age: 36, name: "Boris Diaw"}) | + When executing query: + """ + MATCH p = (v:player{name:"Tim Duncan"})-[e]->(t) WHERE NOT (v)-[]->(t:player) RETURN t + """ + Then the result should be, in any order: + | t | + | ("Spurs" :team{name: "Spurs"}) | Scenario: In With When executing query: From fe8d003bab9efe86cad579f69e3182a5ded91474 Mon Sep 17 00:00:00 2001 From: Aiee <18348405+Aiee@users.noreply.github.com> Date: Wed, 26 Oct 2022 17:40:45 +0800 Subject: [PATCH 2/3] Avoid copy --- .../executor/query/RollUpApplyExecutor.cpp | 24 +++++++++++++++---- 1 file changed, 20 insertions(+), 4 deletions(-) diff --git a/src/graph/executor/query/RollUpApplyExecutor.cpp b/src/graph/executor/query/RollUpApplyExecutor.cpp index ca314f8ad15..0f952472f9a 100644 --- a/src/graph/executor/query/RollUpApplyExecutor.cpp +++ b/src/graph/executor/query/RollUpApplyExecutor.cpp @@ -42,10 +42,18 @@ void RollUpApplyExecutor::buildHashTable(const std::vector& compare Iterator* iter, std::unordered_map& hashTable) const { QueryExpressionContext ctx(ectx_); + std::vector collectColsCopy; + collectColsCopy.reserve(compareCols.size()); + + // Copy the collectCols to make sure the propIndex_ is not cached in the expr + for (auto* col : compareCols) { + collectColsCopy.emplace_back(col->clone()); + } + for (; iter->valid(); iter->next()) { List list; - list.values.reserve(compareCols.size()); - for (auto& col : compareCols) { + list.values.reserve(collectColsCopy.size()); + for (auto& col : collectColsCopy) { Value val = col->eval(ctx(iter)); list.values.emplace_back(std::move(val)); } @@ -114,13 +122,21 @@ DataSet RollUpApplyExecutor::probeSingleKey(Expression* probeKey, DataSet RollUpApplyExecutor::probe(std::vector probeKeys, Iterator* probeIter, const std::unordered_map& hashTable) { + std::vector probeKeysCopy; + probeKeysCopy.reserve(probeKeys.size()); + + // Copy the collectCols to make sure the propIndex_ is not cached in the expr + for (auto* col : probeKeys) { + probeKeysCopy.emplace_back(col->clone()); + } + DataSet ds; ds.rows.reserve(probeIter->size()); QueryExpressionContext ctx(ectx_); for (; probeIter->valid(); probeIter->next()) { List list; - list.values.reserve(probeKeys.size()); - for (auto& col : probeKeys) { + list.values.reserve(probeKeysCopy.size()); + for (auto& col : probeKeysCopy) { // Clone the expression so when evaluating the InputPropertyExpression, the propIndex_ will // not be cached auto* colCopy = col->clone(); From 4a772af39dfcb0fce4739fd130505dcde7e734b3 Mon Sep 17 00:00:00 2001 From: Aiee <18348405+Aiee@users.noreply.github.com> Date: Wed, 26 Oct 2022 19:19:48 +0800 Subject: [PATCH 3/3] Address comments --- .../executor/query/RollUpApplyExecutor.cpp | 60 ++++++++----------- 1 file changed, 25 insertions(+), 35 deletions(-) diff --git a/src/graph/executor/query/RollUpApplyExecutor.cpp b/src/graph/executor/query/RollUpApplyExecutor.cpp index 0f952472f9a..2d3605d5d6b 100644 --- a/src/graph/executor/query/RollUpApplyExecutor.cpp +++ b/src/graph/executor/query/RollUpApplyExecutor.cpp @@ -42,18 +42,11 @@ void RollUpApplyExecutor::buildHashTable(const std::vector& compare Iterator* iter, std::unordered_map& hashTable) const { QueryExpressionContext ctx(ectx_); - std::vector collectColsCopy; - collectColsCopy.reserve(compareCols.size()); - - // Copy the collectCols to make sure the propIndex_ is not cached in the expr - for (auto* col : compareCols) { - collectColsCopy.emplace_back(col->clone()); - } for (; iter->valid(); iter->next()) { List list; - list.values.reserve(collectColsCopy.size()); - for (auto& col : collectColsCopy) { + list.values.reserve(compareCols.size()); + for (auto& col : compareCols) { Value val = col->eval(ctx(iter)); list.values.emplace_back(std::move(val)); } @@ -122,25 +115,14 @@ DataSet RollUpApplyExecutor::probeSingleKey(Expression* probeKey, DataSet RollUpApplyExecutor::probe(std::vector probeKeys, Iterator* probeIter, const std::unordered_map& hashTable) { - std::vector probeKeysCopy; - probeKeysCopy.reserve(probeKeys.size()); - - // Copy the collectCols to make sure the propIndex_ is not cached in the expr - for (auto* col : probeKeys) { - probeKeysCopy.emplace_back(col->clone()); - } - DataSet ds; ds.rows.reserve(probeIter->size()); QueryExpressionContext ctx(ectx_); for (; probeIter->valid(); probeIter->next()) { List list; - list.values.reserve(probeKeysCopy.size()); - for (auto& col : probeKeysCopy) { - // Clone the expression so when evaluating the InputPropertyExpression, the propIndex_ will - // not be cached - auto* colCopy = col->clone(); - Value val = colCopy->eval(ctx(probeIter)); + list.values.reserve(probeKeys.size()); + for (auto& col : probeKeys) { + Value val = col->eval(ctx(probeIter)); list.values.emplace_back(std::move(val)); } @@ -162,25 +144,33 @@ folly::Future RollUpApplyExecutor::rollUpApply() { DataSet result; mv_ = movable(node()->inputVars()[0]); - if (rollUpApplyNode->compareCols().size() == 0) { + auto compareCols = rollUpApplyNode->compareCols(); + + if (compareCols.size() == 0) { List hashTable; buildZeroKeyHashTable(rollUpApplyNode->collectCol(), rhsIter_.get(), hashTable); result = probeZeroKey(lhsIter_.get(), hashTable); - } else if (rollUpApplyNode->compareCols().size() == 1) { + } else if (compareCols.size() == 1) { std::unordered_map hashTable; - // Clone the expression so when evaluating the InputPropertyExpression, the propIndex_ will not - // be cached. - buildSingleKeyHashTable(rollUpApplyNode->compareCols()[0]->clone(), - rollUpApplyNode->collectCol(), - rhsIter_.get(), - hashTable); - - result = probeSingleKey(rollUpApplyNode->compareCols()[0]->clone(), lhsIter_.get(), hashTable); + buildSingleKeyHashTable( + compareCols[0]->clone(), rollUpApplyNode->collectCol(), rhsIter_.get(), hashTable); + result = probeSingleKey(compareCols[0]->clone(), lhsIter_.get(), hashTable); } else { + // Copy the compareCols to make sure the propIndex_ is not cached in the expr + auto cloneExpr = [](std::vector exprs) { + std::vector collectColsCopy; + collectColsCopy.reserve(exprs.size()); + for (auto& expr : exprs) { + collectColsCopy.emplace_back(expr->clone()); + } + return collectColsCopy; + }; + std::unordered_map hashTable; buildHashTable( - rollUpApplyNode->compareCols(), rollUpApplyNode->collectCol(), rhsIter_.get(), hashTable); - result = probe(rollUpApplyNode->compareCols(), lhsIter_.get(), hashTable); + cloneExpr(compareCols), rollUpApplyNode->collectCol(), rhsIter_.get(), hashTable); + + result = probe(cloneExpr(compareCols), lhsIter_.get(), hashTable); } result.colNames = rollUpApplyNode->colNames(); return finish(ResultBuilder().value(Value(std::move(result))).build());