Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enhancement/push filter down AppendVertices #5490

Draft
wants to merge 4 commits into
base: master
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3 changes: 2 additions & 1 deletion src/common/expression/RelationalExpression.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,8 @@ const Value& RelationalExpression::eval(ExpressionContext& ctx) {
case Kind::kRelREG: {
if (lhs.isBadNull() || rhs.isBadNull()) {
result_ = Value::kNullBadType;
} else if ((!lhs.isNull() && !lhs.isStr()) || (!rhs.isNull() && !rhs.isStr())) {
} else if ((!lhs.isNull() && !lhs.empty() && !lhs.isStr()) ||
(!rhs.isNull() && !lhs.empty() && !rhs.isStr())) {
result_ = Value::kNullBadType;
} else if (lhs.isStr() && rhs.isStr()) {
try {
Expand Down
1 change: 1 addition & 0 deletions src/graph/optimizer/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,7 @@ nebula_add_library(
rule/RemoveAppendVerticesBelowJoinRule.cpp
rule/EmbedEdgeAllPredIntoTraverseRule.cpp
rule/PushFilterThroughAppendVerticesRule.cpp
rule/PushFilterDownAppendVerticesRule.cpp
)

nebula_add_subdirectory(test)
3 changes: 3 additions & 0 deletions src/graph/optimizer/rule/EmbedEdgeAllPredIntoTraverseRule.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,9 @@ bool isEdgeAllPredicate(const Expression* e,
return false;
}
auto ves = graph::ExpressionUtils::collectAll(pe->filter(), {Expression::Kind::kAttribute});
if (ves.empty()) {
return false;
}
for (const auto& ve : ves) {
auto iv = static_cast<const AttributeExpression*>(ve)->left();

Expand Down
118 changes: 118 additions & 0 deletions src/graph/optimizer/rule/PushFilterDownAppendVerticesRule.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,118 @@
/* Copyright (c) 2023 vesoft inc. All rights reserved.
*
* This source code is licensed under Apache 2.0 License.
*/

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

#include "common/expression/Expression.h"
#include "graph/optimizer/OptContext.h"
#include "graph/optimizer/OptGroup.h"
#include "graph/planner/plan/PlanNode.h"
#include "graph/planner/plan/Query.h"
#include "graph/util/ExpressionUtils.h"
#include "graph/visitor/ExtractFilterExprVisitor.h"

using nebula::Expression;
using nebula::graph::AppendVertices;
using nebula::graph::Filter;
using nebula::graph::PlanNode;
using nebula::graph::QueryContext;

namespace nebula {
namespace opt {

std::unique_ptr<OptRule> PushFilterDownAppendVerticesRule::kInstance =
std::unique_ptr<PushFilterDownAppendVerticesRule>(new PushFilterDownAppendVerticesRule());

PushFilterDownAppendVerticesRule::PushFilterDownAppendVerticesRule() {
RuleSet::QueryRules().addRule(this);
}

const Pattern &PushFilterDownAppendVerticesRule::pattern() const {
static Pattern pattern =
Pattern::create(PlanNode::Kind::kFilter, {Pattern::create(PlanNode::Kind::kAppendVertices)});
return pattern;
}

StatusOr<OptRule::TransformResult> PushFilterDownAppendVerticesRule::transform(
OptContext *ctx, const MatchedResult &matched) const {
auto filterGroupNode = matched.node;
auto appendVerticesGroupNode = matched.dependencies.front().node;
auto filter = static_cast<const Filter *>(filterGroupNode->node());
auto appendVertices = static_cast<const AppendVertices *>(appendVerticesGroupNode->node());
auto qctx = ctx->qctx();
auto pool = qctx->objPool();
auto condition = graph::ExpressionUtils::rewriteVertexPropertyFilter(
pool, appendVertices->colNames().back(), filter->condition()->clone());

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need to port the latest version of this file from the ent repo. I introduced a bug fix here at line:48.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Which pr?

auto visitor = graph::ExtractFilterExprVisitor::makePushGetVertices(
pool, appendVertices->space(), qctx->schemaMng());
condition->accept(&visitor);
if (!visitor.ok()) {
return TransformResult::noTransform();
}

auto remainedExpr = std::move(visitor).remainedExpr();
OptGroupNode *newFilterGroupNode = nullptr;
PlanNode *newFilter = nullptr;
if (remainedExpr != nullptr) {
auto *found = graph::ExpressionUtils::findAny(remainedExpr, {Expression::Kind::kTagProperty});
if (found != nullptr) { // Some tag property expression don't push down
// TODO(shylock): we could push down a part.
return TransformResult::noTransform();
}
newFilter = Filter::make(qctx, nullptr, remainedExpr);
newFilter->setOutputVar(filter->outputVar());
newFilter->setInputVar(filter->inputVar());
newFilterGroupNode = OptGroupNode::create(ctx, newFilter, filterGroupNode->group());
}

auto newAppendVerticesFilter = condition;
if (condition->isLogicalExpr()) {
const auto *le = static_cast<const LogicalExpression *>(condition);
if (le->operands().size() == 1) {
newAppendVerticesFilter = le->operands().front();
}
}
if (appendVertices->filter() != nullptr) {
auto logicExpr = LogicalExpression::makeAnd(
pool, newAppendVerticesFilter, appendVertices->filter()->clone());
newAppendVerticesFilter = logicExpr;
}

auto newAppendVertices = static_cast<AppendVertices *>(appendVertices->clone());
newAppendVertices->setFilter(newAppendVerticesFilter);

OptGroupNode *newAppendVerticesGroupNode = nullptr;
if (newFilterGroupNode != nullptr) {
// Filter(A&&B)<-AppendVertices(C) => Filter(A)<-AppendVertices(B&&C)
auto newGroup = OptGroup::create(ctx);
newAppendVerticesGroupNode = newGroup->makeGroupNode(newAppendVertices);
newFilterGroupNode->dependsOn(newGroup);
newFilter->setInputVar(newAppendVertices->outputVar());
} else {
// Filter(A)<-AppendVertices(C) => AppendVertices(A&&C)
newAppendVerticesGroupNode =
OptGroupNode::create(ctx, newAppendVertices, filterGroupNode->group());
newAppendVertices->setOutputVar(filter->outputVar());
newAppendVertices->setColNames(appendVertices->colNames());
}

for (auto dep : appendVerticesGroupNode->dependencies()) {
newAppendVerticesGroupNode->dependsOn(dep);
}

TransformResult result;
result.eraseCurr = true;
result.newGroupNodes.emplace_back(newFilterGroupNode ? newFilterGroupNode
: newAppendVerticesGroupNode);
return result;
}

std::string PushFilterDownAppendVerticesRule::toString() const {
return "PushFilterDownAppendVerticesRule";
}

} // namespace opt
} // namespace nebula
31 changes: 31 additions & 0 deletions src/graph/optimizer/rule/PushFilterDownAppendVerticesRule.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
/* Copyright (c) 2023 vesoft inc. All rights reserved.
*
* This source code is licensed under Apache 2.0 License.
*/

#ifndef GRAPH_OPTIMIZER_RULE_PUSHLIMITDOWNAPPENDVERTICES_H_
#define GRAPH_OPTIMIZER_RULE_PUSHLIMITDOWNAPPENDVERTICES_H_

#include "graph/optimizer/OptRule.h"

namespace nebula {
namespace opt {

class PushFilterDownAppendVerticesRule final : public OptRule {
public:
const Pattern &pattern() const override;

StatusOr<TransformResult> transform(OptContext *ctx, const MatchedResult &matched) const override;

std::string toString() const override;

private:
PushFilterDownAppendVerticesRule();

static std::unique_ptr<OptRule> kInstance;
};

} // namespace opt
} // namespace nebula

#endif // GRAPH_OPTIMIZER_RULE_PUSHFILTERDOWNAPPENDVERTICES_H_
17 changes: 8 additions & 9 deletions tests/tck/features/bugfix/PushFilterDownProject.feature
Original file line number Diff line number Diff line change
Expand Up @@ -19,12 +19,11 @@ Feature: Test push filter down project
| count(*) |
| 2 |
And the execution plan should be:
| id | name | dependencies | operator info |
| 9 | Aggregate | 12 | |
| 12 | Project | 11 | |
| 11 | Filter | 5 | {"condition": "(($-.n1.player.age-($-.n1.player.age+(($-.n1.player.age%$-.n1.player.age)+($-.n1.player.age+$-.n1.player.age))))<=$-.n1.player.age)"} |
| 5 | AppendVertices | 4 | |
| 4 | Traverse | 2 | |
| 2 | Dedup | 1 | |
| 1 | PassThrough | 3 | |
| 3 | Start | | |
| id | name | dependencies | operator info |
| 9 | Aggregate | 12 | |
| 12 | Project | 5 | |
| 5 | AppendVertices | 4 | {"filter": "((player.age-(player.age+((player.age%player.age)+(player.age+player.age))))<=player.age)"} |
| 4 | Traverse | 2 | |
| 2 | Dedup | 1 | |
| 1 | PassThrough | 3 | |
| 3 | Start | | |
2 changes: 0 additions & 2 deletions tests/tck/features/expression/RelationalExpr.feature
Original file line number Diff line number Diff line change
Expand Up @@ -228,8 +228,6 @@ Feature: RelationalExpression
| ("Steve Nash" :player{age: 45, name: "Steve Nash"}) |
And the execution plan should be:
| id | name | dependencies | operator info |
| 9 | Project | 8 | |
| 8 | Filter | 2 | |
| 2 | AppendVertices | 6 | |
| 6 | IndexScan | 0 | {"indexCtx": {"columnHints":{"scanType":"RANGE"}}} |
| 0 | Start | | |
2 changes: 0 additions & 2 deletions tests/tck/features/expression/UnaryExpr.feature
Original file line number Diff line number Diff line change
Expand Up @@ -96,8 +96,6 @@ Feature: UnaryExpression
| ("Shaquille O'Neal" :player{age: 47, name: "Shaquille O'Neal"}) |
And the execution plan should be:
| id | name | dependencies | operator info |
| 9 | Project | 8 | |
| 8 | Filter | 2 | |
| 2 | AppendVertices | 6 | |
| 6 | IndexScan | 0 | |
| 0 | Start | | |
5 changes: 0 additions & 5 deletions tests/tck/features/match/Base.feature
Original file line number Diff line number Diff line change
Expand Up @@ -814,11 +814,6 @@ Feature: Basic match
MATCH (v:player{name:"abc"})
"""
Then a SyntaxError should be raised at runtime: syntax error near `)'
When executing query:
"""
MATCH (v:player) where v.player.name RETURN v
"""
Then a ExecutionError should be raised at runtime: Failed to evaluate condition: v.player.name. For boolean conditions, please write in their full forms like <condition> == <true/false> or <condition> IS [NOT] NULL.

Scenario: Scan
When executing query:
Expand Down
17 changes: 5 additions & 12 deletions tests/tck/features/match/IndexSelecting.feature
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,6 @@ Feature: Index selecting for match statement
And the execution plan should be:
| id | name | dependencies | operator info |
| 6 | Project | 2 | |
| 2 | Filter | 5 | |
| 2 | AppendVertices | 5 | |
| 5 | IndexScan | 0 | {"indexCtx": {"columnHints":{"scanType":"PREFIX"}}} |
| 0 | Start | | |
Expand All @@ -82,7 +81,6 @@ Feature: Index selecting for match statement
And the execution plan should be:
| id | name | dependencies | operator info |
| 6 | Project | 2 | |
| 2 | Filter | 5 | |
| 2 | AppendVertices | 5 | |
| 5 | IndexScan | 0 | {"indexCtx": {"columnHints":{"scanType":"PREFIX"}}} |
| 0 | Start | | |
Expand All @@ -95,8 +93,7 @@ Feature: Index selecting for match statement
| "Tim Duncan" |
And the execution plan should be:
| id | name | dependencies | operator info |
| 9 | Project | 7 | |
| 7 | Filter | 2 | |
| 9 | Project | 2 | |
| 2 | AppendVertices | 6 | |
| 6 | IndexScan | 0 | {"indexCtx": {"columnHints":{"scanType":"PREFIX"}}} |
| 0 | Start | | |
Expand All @@ -109,8 +106,7 @@ Feature: Index selecting for match statement
| "Tim Duncan" |
And the execution plan should be:
| id | name | dependencies | operator info |
| 9 | Project | 7 | |
| 7 | Filter | 2 | |
| 9 | Project | 2 | |
| 2 | AppendVertices | 6 | |
| 6 | IndexScan | 0 | {"indexCtx": {"columnHints":{"scanType":"PREFIX"}}} |
| 0 | Start | | |
Expand All @@ -123,8 +119,7 @@ Feature: Index selecting for match statement
| "Tim Duncan" |
And the execution plan should be:
| id | name | dependencies | operator info |
| 9 | Project | 7 | |
| 7 | Filter | 2 | |
| 9 | Project | 2 | |
| 2 | AppendVertices | 6 | |
| 6 | IndexScan | 0 | {"indexCtx": {"columnHints":{"scanType":"PREFIX"}}} |
| 0 | Start | | |
Expand All @@ -139,8 +134,7 @@ Feature: Index selecting for match statement
| "Tim Duncan" |
And the execution plan should be:
| id | name | dependencies | operator info |
| 9 | Project | 7 | |
| 7 | Filter | 2 | |
| 9 | Project | 2 | |
| 2 | AppendVertices | 6 | |
| 6 | IndexScan | 0 | {"indexCtx": {"columnHints":{"scanType":"RANGE"}}} |
| 0 | Start | | |
Expand All @@ -162,8 +156,7 @@ Feature: Index selecting for match statement
| "Tim Duncan" |
And the execution plan should be:
| id | name | dependencies | operator info |
| 9 | Project | 7 | |
| 7 | Filter | 2 | |
| 9 | Project | 2 | |
| 2 | AppendVertices | 6 | |
| 6 | IndexScan | 0 | {"indexCtx": {"columnHints":{"scanType":"PREFIX"}}} |
| 0 | Start | | |
Expand Down