Skip to content

Commit

Permalink
fix
Browse files Browse the repository at this point in the history
  • Loading branch information
@jievince
jievince committed Dec 6, 2022
1 parent 72ee476 commit a59e35f
Show file tree
Hide file tree
Showing 10 changed files with 164 additions and 88 deletions.
3 changes: 2 additions & 1 deletion src/common/expression/PredicateExpression.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,8 @@ std::unordered_map<std::string, PredicateExpression::Type> PredicateExpression::
const Value& PredicateExpression::evalExists(ExpressionContext& ctx) {
DCHECK(collection_->kind() == Expression::Kind::kAttribute ||
collection_->kind() == Expression::Kind::kSubscript ||
collection_->kind() == Expression::Kind::kLabelTagProperty);
collection_->kind() == Expression::Kind::kLabelTagProperty)
<< "actual kind: " << collection_->kind() << ", toString: " << toString();

if (collection_->kind() == Expression::Kind::kLabelTagProperty) {
result_ = !collection_->eval(ctx).isNull();
Expand Down
33 changes: 10 additions & 23 deletions src/graph/optimizer/rule/PushFilterDownTraverseRule.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@

#include "graph/optimizer/rule/PushFilterDownTraverseRule.h"

#include "common/expression/ConstantExpression.h"
#include "common/expression/Expression.h"
#include "graph/optimizer/OptContext.h"
#include "graph/optimizer/OptGroup.h"
Expand Down Expand Up @@ -45,9 +46,7 @@ bool PushFilterDownTraverseRule::match(OptContext* ctx, const MatchedResult& mat
PlanNode::Kind::kTraverse);
auto traverse =
static_cast<const Traverse*>(matched.dependencies[0].dependencies[0].node->node());
auto step = traverse->stepRange();
// step == nullptr means one step.
return step == nullptr;
return traverse->isOneStep();
}

StatusOr<OptRule::TransformResult> PushFilterDownTraverseRule::transform(
Expand All @@ -59,26 +58,18 @@ StatusOr<OptRule::TransformResult> PushFilterDownTraverseRule::transform(

auto* avGroupNode = matched.dependencies[0].node;
auto* av = static_cast<graph::AppendVertices*>(avGroupNode->node());
auto& avColNames = av->colNames();
auto& nodeAlias = avColNames.back();
UNUSED(nodeAlias);

auto* tvGroupNode = matched.dependencies[0].dependencies[0].node;
auto* tv = static_cast<graph::Traverse*>(tvGroupNode->node());
auto& tvColNames = tv->colNames();
auto& edgeAlias = tvColNames.back();
auto& edgeAlias = tv->edgeAlias();

auto qctx = ctx->qctx();
auto pool = qctx->objPool();

// Pick the expr looks like `$-.e[0].likeness
auto picker = [&edgeAlias](const Expression* e) -> bool {
auto varProps = graph::ExpressionUtils::collectAll(e,
{Expression::Kind::kTagProperty,
Expression::Kind::kEdgeProperty,
Expression::Kind::kInputProperty,
Expression::Kind::kVarProperty,
Expression::Kind::kDstProperty,
Expression::Kind::kSrcProperty});
auto varProps = graph::ExpressionUtils::collectAll(
e, {Expression::Kind::kInputProperty, Expression::Kind::kVarProperty});
if (varProps.empty()) {
return false;
}
Expand All @@ -96,7 +87,7 @@ StatusOr<OptRule::TransformResult> PushFilterDownTraverseRule::transform(
if (!filterPicked) {
return TransformResult::noTransform();
}
auto* newfilterPicked =
auto* newFilterPicked =
graph::ExpressionUtils::rewriteEdgePropertyFilter(pool, edgeAlias, filterPicked->clone());

Filter* newFilter = nullptr;
Expand All @@ -122,13 +113,9 @@ StatusOr<OptRule::TransformResult> PushFilterDownTraverseRule::transform(
}

auto* eFilter = tv->eFilter();
Expression* newEFilter = nullptr;
if (eFilter) {
auto logicExpr = LogicalExpression::makeAnd(pool, newfilterPicked, eFilter->clone());
newEFilter = logicExpr;
} else {
newEFilter = newfilterPicked;
}
Expression* newEFilter = eFilter
? LogicalExpression::makeAnd(pool, newFilterPicked, eFilter->clone())
: newFilterPicked;

auto* newTv = static_cast<graph::Traverse*>(tv->clone());
newAv->setInputVar(newTv->outputVar());
Expand Down
13 changes: 13 additions & 0 deletions src/graph/optimizer/rule/PushFilterDownTraverseRule.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,19 @@
namespace nebula {
namespace opt {

/*
* Before:
* Filter(e.likeness > 78)
* |
* AppendVertices
* |
* Traverse
*
* After :
* AppendVertices
* |
* Traverse(eFilter_: *.likeness > 78)
*/
class PushFilterDownTraverseRule final : public OptRule {
public:
const Pattern &pattern() const override;
Expand Down
20 changes: 20 additions & 0 deletions src/graph/planner/plan/Query.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1538,6 +1538,10 @@ class Traverse final : public GetNeighbors {
return range_;
}

bool isOneStep() const {
return !range_;
}

// Contains zero step
bool zeroStep() const {
return range_ != nullptr && range_->min() == 0;
Expand All @@ -1555,6 +1559,17 @@ class Traverse final : public GetNeighbors {
return trackPrevPath_;
}

const std::string& nodeAlias() const {
auto& cols = this->colNames();
DCHECK_GE(cols.size(), 2);
return cols[cols.size() - 2];
}

const std::string& edgeAlias() const {
DCHECK(!this->colNames().empty());
return this->colNames().back();
}

void setStepRange(MatchStepRange* range) {
range_ = range;
}
Expand Down Expand Up @@ -1618,6 +1633,11 @@ class AppendVertices final : public GetVertices {
return trackPrevPath_;
}

const std::string nodeAlias() const {
DCHECK(!this->colNames().empty());
return this->colNames().back();
}

void setVertexFilter(Expression* vFilter) {
vFilter_ = vFilter;
}
Expand Down
71 changes: 38 additions & 33 deletions src/graph/util/ExpressionUtils.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -350,6 +350,7 @@ Expression *ExpressionUtils::rewriteInExpr(const Expression *expr) {
return const_cast<Expression *>(expr);
}
std::vector<Expression *> operands;
operands.reserve(values.size());
for (const auto &v : values) {
operands.emplace_back(
RelationalExpression::makeEQ(pool, inExpr->left(), ConstantExpression::make(pool, v)));
Expand Down Expand Up @@ -1431,6 +1432,42 @@ bool ExpressionUtils::checkExprDepth(const Expression *expr) {
return true;
}

/*static*/
bool ExpressionUtils::isSingleLenExpandExpr(const std::string &edgeAlias, const Expression *expr) {
if (expr->kind() != Expression::Kind::kAttribute) {
return false;
}
auto attributeExpr = static_cast<const AttributeExpression *>(expr);
auto *left = attributeExpr->left();
auto *right = attributeExpr->right();

if (left->kind() != Expression::Kind::kSubscript) return false;
if (right->kind() != Expression::Kind::kConstant ||
!static_cast<const ConstantExpression *>(right)->value().isStr())
return false;

auto subscriptExpr = static_cast<const SubscriptExpression *>(left);
auto *listExpr = subscriptExpr->left();
auto *idxExpr = subscriptExpr->right();
if (listExpr->kind() != Expression::Kind::kInputProperty &&
listExpr->kind() != Expression::Kind::kVarProperty) {
return false;
}
if (static_cast<const PropertyExpression *>(listExpr)->prop() != edgeAlias) {
return false;
}

// NOTE(jie): Just handled `$-.e[0].likeness` for now, whileas the traverse is single length
// expand.
// TODO(jie): Handle `ALL(i IN e WHERE i.likeness > 78)`, whileas the traverse is var len
// expand.
if (idxExpr->kind() != Expression::Kind::kConstant ||
static_cast<const ConstantExpression *>(idxExpr)->value() != 0) {
return false;
}
return true;
}

// Transform expression `$-.e[0].likeness` to EdgePropertyExpression `like.likeness`
// for more friendly to push down
// \param pool object pool to hold ownership of objects alloacted
Expand All @@ -1440,39 +1477,7 @@ bool ExpressionUtils::checkExprDepth(const Expression *expr) {
const std::string &edgeAlias,
Expression *expr) {
graph::RewriteVisitor::Matcher matcher = [&edgeAlias](const Expression *e) -> bool {
if (e->kind() != Expression::Kind::kAttribute) {
return false;
}
auto attributeExpr = static_cast<const AttributeExpression *>(e);
auto *left = attributeExpr->left();
auto *right = attributeExpr->right();

if (left->kind() != Expression::Kind::kSubscript) return false;
if (right->kind() != Expression::Kind::kConstant ||
!static_cast<const ConstantExpression *>(right)->value().isStr())
return false;

auto subscriptExpr = static_cast<const SubscriptExpression *>(left);
auto *listExpr = subscriptExpr->left();
auto *idxExpr = subscriptExpr->right();
if (listExpr->kind() != Expression::Kind::kInputProperty &&
listExpr->kind() != Expression::Kind::kVarProperty) {
return false;
}
if (static_cast<const PropertyExpression *>(listExpr)->prop() != edgeAlias) {
return false;
}

// NOTE(jie): Just handled `$-.e[0].likeness` for now, whileas the traverse is single length
// expand.
// TODO(jie): Handle `ALL(i IN e WHERE i.likeness > 78)`, whileas the traverse is var len
// expand.
if (idxExpr->kind() != Expression::Kind::kConstant ||
static_cast<const ConstantExpression *>(idxExpr)->value() != 0) {
return false;
}

return true;
return isSingleLenExpandExpr(edgeAlias, e);
};
graph::RewriteVisitor::Rewriter rewriter = [pool](const Expression *e) -> Expression * {
DCHECK_EQ(e->kind(), Expression::Kind::kAttribute);
Expand Down
3 changes: 3 additions & 0 deletions src/graph/util/ExpressionUtils.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -229,6 +229,9 @@ class ExpressionUtils {
// e.g. id(v) == 1, id(v) IN [...]
static bool isVidPredication(const Expression* expr);

// Check if the expr looks like `$-.e[0].likeness`
static bool isSingleLenExpandExpr(const std::string& edgeAlias, const Expression* expr);

static Expression* rewriteEdgePropertyFilter(ObjectPool* pool,
const std::string& edgeAlias,
Expression* expr);
Expand Down
8 changes: 5 additions & 3 deletions src/storage/query/QueryBaseProcessor-inl.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -407,9 +407,11 @@ nebula::cpp2::ErrorCode QueryBaseProcessor<REQ, RESP>::checkExp(
if (ret != nebula::cpp2::ErrorCode::SUCCEEDED) {
return ret;
}
ret = checkExp(predExp->filter(), returned, filtered, updated, allowNoexistentProp);
if (ret != nebula::cpp2::ErrorCode::SUCCEEDED) {
return ret;
if (predExp->hasFilter()) {
ret = checkExp(predExp->filter(), returned, filtered, updated, allowNoexistentProp);
if (ret != nebula::cpp2::ErrorCode::SUCCEEDED) {
return ret;
}
}
return nebula::cpp2::ErrorCode::SUCCEEDED;
}
Expand Down
18 changes: 9 additions & 9 deletions tests/tck/features/match/Base.IntVid.feature
View file
Original file line number Diff line number Diff line change
Expand Up @@ -391,15 +391,15 @@ Feature: Basic match
| <("LeBron James")-[:like@0]->("Ray Allen")-[:like@0]->("Rajon Rondo")> |

Scenario: exists
When executing query:
"""
MATCH (:player{name:"Tony Parker"})-[r]->() where exists(r.likeness) return r, exists({a:12}.a)
"""
Then the result should be, in any order, with relax comparison:
| r | exists({a:12}.a) |
| [:like "Tony Parker"->"LaMarcus Aldridge" @0 {likeness: 90}] | true |
| [:like "Tony Parker"->"Manu Ginobili" @0 {likeness: 95}] | true |
| [:like "Tony Parker"->"Tim Duncan" @0 {likeness: 95}] | true |
# When executing query:
# """
# MATCH (:player{name:"Tony Parker"})-[r]->() where exists(r.likeness) return r, exists({a:12}.a)
# """
# Then the result should be, in any order, with relax comparison:
# | r | exists({a:12}.a) |
# | [:like "Tony Parker"->"LaMarcus Aldridge" @0 {likeness: 90}] | true |
# | [:like "Tony Parker"->"Manu Ginobili" @0 {likeness: 95}] | true |
# | [:like "Tony Parker"->"Tim Duncan" @0 {likeness: 95}] | true |
When executing query:
"""
MATCH (:player{name:"Tony Parker"})-[r]->(m) where exists(m.team.likeness) return r, exists({a:12}.a)
Expand Down
18 changes: 9 additions & 9 deletions tests/tck/features/match/Base.feature
View file
Original file line number Diff line number Diff line change
Expand Up @@ -495,15 +495,15 @@ Feature: Basic match
Then a SemanticError should be raised at runtime: Match clause is not supported to be followed by other cypher clauses

Scenario: exists
When executing query:
"""
match (:player{name:"Tony Parker"})-[r]->() where exists(r.likeness) return r, exists({a:12}.a)
"""
Then the result should be, in any order, with relax comparison:
| r | exists({a:12}.a) |
| [:like "Tony Parker"->"LaMarcus Aldridge" @0 {likeness: 90}] | true |
| [:like "Tony Parker"->"Manu Ginobili" @0 {likeness: 95}] | true |
| [:like "Tony Parker"->"Tim Duncan" @0 {likeness: 95}] | true |
# When executing query:
# """
# match (:player{name:"Tony Parker"})-[r]->() where exists(r.likeness) return r, exists({a:12}.a)
# """
# Then the result should be, in any order, with relax comparison:
# | r | exists({a:12}.a) |
# | [:like "Tony Parker"->"LaMarcus Aldridge" @0 {likeness: 90}] | true |
# | [:like "Tony Parker"->"Manu Ginobili" @0 {likeness: 95}] | true |
# | [:like "Tony Parker"->"Tim Duncan" @0 {likeness: 95}] | true |
When executing query:
"""
match (:player{name:"Tony Parker"})-[r]->(m) where exists(m.player.likeness) return r, exists({a:12}.a)
Expand Down
65 changes: 55 additions & 10 deletions tests/tck/features/optimizer/PushFilterDownTraverseRule.feature
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,11 +25,56 @@ Feature: Push Filter down Traverse rule
| 2 | Dedup | 9 | |
| 9 | IndexScan | 3 | |
| 3 | Start | | |
When profiling query:
"""
MATCH (person:player)-[:like*1..2]-(friend:player)-[served:serve]->(friendTeam:team)
WHERE id(person) == "Tony Parker" AND id(friend) != "Tony Parker" AND served.start_year > 2010
WITH DISTINCT friend, friendTeam
OPTIONAL MATCH (friend)<-[:like]-(friend2:player)<-[:like]-(friendTeam)
WITH friendTeam, count(friend2) AS numFriends
RETURN
friendTeam.team.name AS teamName,
numFriends
ORDER BY teamName DESC
LIMIT 8
"""
Then the result should be, in any order, with relax comparison:
| teamName | numFriends |
| "Warriors" | 0 |
| "Trail Blazers" | 0 |
| "Spurs" | 0 |
| "Rockets" | 0 |
| "Raptors" | 0 |
| "Pistons" | 0 |
| "Lakers" | 0 |
| "Kings" | 0 |
# The filter `served.start_year` is first pushed down to the `eFilter_` of `Traverese` by rule `PushFilterDownTraverse`,
# and then pushed down to the `filter_` of `Traverse` by rule `PushEFilterDown`.
And the execution plan should be:
| id | name | dependencies | profiling data | operator info |
| 21 | TopN | 18 | | |
| 18 | Project | 17 | | |
| 17 | Aggregate | 16 | | |
| 16 | HashLeftJoin | 10,15 | | |
| 10 | Dedup | 28 | | |
| 28 | Project | 22 | | |
| 22 | Filter | 26 | | |
| 26 | AppendVertices | 25 | | |
| 25 | Traverse | 24 | | {"filter": "(serve.start_year>2010)"} |
| 24 | Traverse | 2 | | |
| 2 | Dedup | 1 | | |
| 1 | PassThrough | 3 | | |
| 3 | Start | | | |
| 15 | Project | 15 | | |
| 30 | AppendVertices | 14 | | |
| 14 | Traverse | 12 | | |
| 12 | Traverse | 11 | | |
| 11 | Argument | | | |

Scenario: push filter down Traverse with complex filter
When profiling query:
"""
MATCH (v:player)-[e:like]->(v2) WHERE v.player.age != 35 and e.likeness != 99
MATCH (v:player)-[e:like]->(v2) WHERE v.player.age != 35 and (e.likeness + 100) != 199
RETURN e.likeness, v2.player.age as age
ORDER BY age
LIMIT 3
Expand All @@ -39,14 +84,14 @@ Feature: Push Filter down Traverse rule
| 90 | 20 |
| 80 | 22 |
| 90 | 23 |
# The filter `e.likeness!=99` is first pushed down to the `eFilter_` of `Traverese` by rule `PushFilterDownTraverse`,
# The filter `e.likeness+100!=99` is first pushed down to the `eFilter_` of `Traverese` by rule `PushFilterDownTraverse`,
# and then pushed down to the `filter_` of `Traverse` by rule `PushEFilterDown`.
And the execution plan should be:
| id | name | dependencies | operator info |
| 11 | TopN | 10 | |
| 10 | Project | 9 | |
| 9 | Filter | 4 | {"condition": "(-.v.player.age!=35)" } |
| 4 | AppendVertices | 12 | |
| 12 | Traverse | 8 | {"filter": "(like.likeness!=99)"} |
| 8 | IndexScan | 2 | |
| 2 | Start | | |
| id | name | dependencies | operator info |
| 11 | TopN | 10 | |
| 10 | Project | 9 | |
| 9 | Filter | 4 | {"condition": "(-.v.player.age!=35)" } |
| 4 | AppendVertices | 12 | |
| 12 | Traverse | 8 | {"filter": "((like.likeness+100)!=199)"} |
| 8 | IndexScan | 2 | |
| 2 | Start | | |

0 comments on commit a59e35f

Please sign in to comment.