Skip to content

Commit

Permalink
Eliminate vid predication Filter. (vesoft-inc#870)
Browse files Browse the repository at this point in the history
<!--
Thanks for your contribution!
In order to review PR more efficiently, please add information according to the template.
-->

## What type of PR is this?
- [ ] bug
- [ ] feature
- [x] enhancement

## What problem(s) does this PR solve?
#### Issue(s) number: 
Sub job of vesoft-inc#4122 

#### Description:
GetVertices will retrieve Vertex by Id set directly, so we don't filter them again in Filter operator, so we eliminate this Filter operator.

## How do you solve it?



## Special notes for your reviewer, ex. impact of this fix, design document, etc:



## Checklist:
Tests:
- [ ] Unit test(positive and negative cases)
- [ ] Function test
- [ ] Performance test
- [ ] N/A

Affects:
- [ ] Documentation affected (Please add the label if documentation needs to be modified.)
- [ ] Incompatibility (If it breaks the compatibility, please describe it and add the label.)
- [ ] If it's needed to cherry-pick (If cherry-pick to some branches is required, please label the destination version(s).)
- [ ] Performance impacted: Consumes more CPU/Memory


## Release notes:

Please confirm whether to be reflected in release notes and how to describe:
> ex. Fixed the bug .....


Migrated from vesoft-inc#4249

Co-authored-by: shylock <33566796+Shylock-Hg@users.noreply.github.com>
  • Loading branch information
nebula-bots and Shylock-Hg authored May 16, 2022
1 parent 77fdfca commit 50425ea
Show file tree
Hide file tree
Showing 15 changed files with 136 additions and 37 deletions.
12 changes: 6 additions & 6 deletions src/graph/context/ast/CypherAstContext.h
Original file line number Diff line number Diff line change
Expand Up @@ -204,11 +204,11 @@ struct PatternContext {
};

struct NodeContext final : PatternContext {
NodeContext(QueryContext* q, Expression* b, GraphSpaceID g, NodeInfo* i)
: PatternContext(PatternKind::kNode), qctx(q), bindFilter(b), spaceId(g), info(i) {}
NodeContext(QueryContext* q, WhereClauseContext* b, GraphSpaceID g, NodeInfo* i)
: PatternContext(PatternKind::kNode), qctx(q), bindWhereClause(b), spaceId(g), info(i) {}

QueryContext* qctx;
Expression* bindFilter;
WhereClauseContext* bindWhereClause;
GraphSpaceID spaceId;
NodeInfo* info{nullptr};
std::unordered_set<std::string>* nodeAliasesAvailable;
Expand All @@ -221,11 +221,11 @@ struct NodeContext final : PatternContext {
};

struct EdgeContext final : PatternContext {
EdgeContext(QueryContext* q, Expression* b, GraphSpaceID g, EdgeInfo* i)
: PatternContext(PatternKind::kEdge), qctx(q), bindFilter(b), spaceId(g), info(i) {}
EdgeContext(QueryContext* q, WhereClauseContext* b, GraphSpaceID g, EdgeInfo* i)
: PatternContext(PatternKind::kEdge), qctx(q), bindWhereClause(b), spaceId(g), info(i) {}

QueryContext* qctx;
Expression* bindFilter;
WhereClauseContext* bindWhereClause;
GraphSpaceID spaceId;
EdgeInfo* info{nullptr};

Expand Down
4 changes: 2 additions & 2 deletions src/graph/planner/match/LabelIndexSeek.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -84,8 +84,8 @@ StatusOr<SubPlan> LabelIndexSeek::transformNode(NodeContext* nodeCtx) {
// and it should be converted to an `optRule` after the match validator is
// refactored
auto* pool = nodeCtx->qctx->objPool();
if (nodeCtx->bindFilter != nullptr) {
auto* filter = nodeCtx->bindFilter;
if (nodeCtx->bindWhereClause != nullptr && nodeCtx->bindWhereClause->filter != nullptr) {
auto* filter = nodeCtx->bindWhereClause->filter;
const auto& nodeAlias = nodeCtx->info->alias;
const auto& schemaName = nodeCtx->scanInfo.schemaNames.back();

Expand Down
3 changes: 1 addition & 2 deletions src/graph/planner/match/MatchClausePlanner.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -36,10 +36,9 @@ StatusOr<SubPlan> MatchClausePlanner::transform(CypherClauseContextBase* clauseC
for (auto iter = pathInfos.begin(); iter < pathInfos.end(); ++iter) {
auto& nodeInfos = iter->nodeInfos;
MatchPathPlanner matchPathPlanner;
auto bindFilter = matchClauseCtx->where ? matchClauseCtx->where->filter : nullptr;
auto result = matchPathPlanner.transform(matchClauseCtx->qctx,
matchClauseCtx->space.id,
bindFilter,
matchClauseCtx->where.get(),
matchClauseCtx->aliasesAvailable,
nodeAliasesSeen,
*iter);
Expand Down
10 changes: 5 additions & 5 deletions src/graph/planner/match/MatchPathPlanner.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -117,7 +117,7 @@ static Expression* nodeId(ObjectPool* pool, const NodeInfo& node) {
StatusOr<SubPlan> MatchPathPlanner::transform(
QueryContext* qctx,
GraphSpaceID spaceId,
Expression* bindFilter,
WhereClauseContext* bindWhere,
const std::unordered_map<std::string, AliasType>& aliasesAvailable,
std::unordered_set<std::string> nodeAliasesSeen,
Path& path) {
Expand All @@ -135,7 +135,7 @@ StatusOr<SubPlan> MatchPathPlanner::transform(
edgeInfos,
qctx,
spaceId,
bindFilter,
bindWhere,
aliasesAvailable,
nodeAliasesSeen,
startFromEdge,
Expand All @@ -158,7 +158,7 @@ Status MatchPathPlanner::findStarts(
std::vector<EdgeInfo>& edgeInfos,
QueryContext* qctx,
GraphSpaceID spaceId,
Expression* bindFilter,
WhereClauseContext* bindWhereClause,
const std::unordered_map<std::string, AliasType>& aliasesAvailable,
std::unordered_set<std::string> nodeAliasesSeen,
bool& startFromEdge,
Expand All @@ -178,7 +178,7 @@ Status MatchPathPlanner::findStarts(
// Find the start plan node
for (auto& finder : startVidFinders) {
for (size_t i = 0; i < nodeInfos.size() && !foundStart; ++i) {
auto nodeCtx = NodeContext(qctx, bindFilter, spaceId, &nodeInfos[i]);
auto nodeCtx = NodeContext(qctx, bindWhereClause, spaceId, &nodeInfos[i]);
nodeCtx.nodeAliasesAvailable = &allNodeAliasesAvailable;
auto nodeFinder = finder();
if (nodeFinder->match(&nodeCtx)) {
Expand All @@ -197,7 +197,7 @@ Status MatchPathPlanner::findStarts(
}

if (i != nodeInfos.size() - 1) {
auto edgeCtx = EdgeContext(qctx, bindFilter, spaceId, &edgeInfos[i]);
auto edgeCtx = EdgeContext(qctx, bindWhereClause, spaceId, &edgeInfos[i]);
auto edgeFinder = finder();
if (edgeFinder->match(&edgeCtx)) {
auto plan = edgeFinder->transform(&edgeCtx);
Expand Down
4 changes: 2 additions & 2 deletions src/graph/planner/match/MatchPathPlanner.h
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ class MatchPathPlanner final {

StatusOr<SubPlan> transform(QueryContext* qctx,
GraphSpaceID spaceId,
Expression* bindFilter,
WhereClauseContext* bindWhereClause,
const std::unordered_map<std::string, AliasType>& aliasesAvailable,
std::unordered_set<std::string> nodeAliasesSeen,
Path& path);
Expand All @@ -25,7 +25,7 @@ class MatchPathPlanner final {
std::vector<EdgeInfo>& edgeInfos,
QueryContext* qctx,
GraphSpaceID spaceId,
Expression* bindFilter,
WhereClauseContext* bindWhereClause,
const std::unordered_map<std::string, AliasType>& aliasesAvailable,
std::unordered_set<std::string> nodeAliases,
bool& startFromEdge,
Expand Down
8 changes: 4 additions & 4 deletions src/graph/planner/match/PropIndexSeek.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -25,9 +25,9 @@ bool PropIndexSeek::matchEdge(EdgeContext* edgeCtx) {
}

Expression* filter = nullptr;
if (edgeCtx->bindFilter != nullptr) {
if (edgeCtx->bindWhereClause != nullptr && edgeCtx->bindWhereClause->filter != nullptr) {
filter = MatchSolver::makeIndexFilter(
edge.types.back(), edge.alias, edgeCtx->bindFilter, edgeCtx->qctx, true);
edge.types.back(), edge.alias, edgeCtx->bindWhereClause->filter, edgeCtx->qctx, true);
}
if (filter == nullptr) {
if (edge.props != nullptr && !edge.props->items().empty()) {
Expand Down Expand Up @@ -124,9 +124,9 @@ bool PropIndexSeek::matchNode(NodeContext* nodeCtx) {

Expression* filterInWhere = nullptr;
Expression* filterInPattern = nullptr;
if (nodeCtx->bindFilter != nullptr) {
if (nodeCtx->bindWhereClause != nullptr && nodeCtx->bindWhereClause->filter != nullptr) {
filterInWhere = MatchSolver::makeIndexFilter(
node.labels.back(), node.alias, nodeCtx->bindFilter, nodeCtx->qctx);
node.labels.back(), node.alias, nodeCtx->bindWhereClause->filter, nodeCtx->qctx);
}
if (!node.labelProps.empty()) {
auto props = node.labelProps.back();
Expand Down
3 changes: 3 additions & 0 deletions src/graph/planner/match/SegmentsConnector.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,9 @@ SubPlan SegmentsConnector::cartesianProduct(QueryContext* qctx,
}

SubPlan SegmentsConnector::addInput(const SubPlan& left, const SubPlan& right, bool copyColNames) {
if (left.root == nullptr) {
return right;
}
SubPlan newPlan = left;
DCHECK(left.root->isSingleInput());
if (left.tail->isSingleInput()) {
Expand Down
8 changes: 6 additions & 2 deletions src/graph/planner/match/VertexIdSeek.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ StatusOr<SubPlan> VertexIdSeek::transformEdge(EdgeContext *edgeCtx) {
bool VertexIdSeek::matchNode(NodeContext *nodeCtx) {
auto &node = *nodeCtx->info;
// auto *matchClauseCtx = nodeCtx->matchClauseCtx;
if (nodeCtx->bindFilter == nullptr) {
if (nodeCtx->bindWhereClause == nullptr || nodeCtx->bindWhereClause->filter == nullptr) {
return false;
}

Expand All @@ -37,7 +37,7 @@ bool VertexIdSeek::matchNode(NodeContext *nodeCtx) {
}

VidExtractVisitor vidExtractVisitor(nodeCtx->qctx);
nodeCtx->bindFilter->accept(&vidExtractVisitor);
nodeCtx->bindWhereClause->filter->accept(&vidExtractVisitor);
auto vidResult = vidExtractVisitor.moveVidPattern();
if (vidResult.spec != VidExtractVisitor::VidPattern::Special::kInUsed) {
return false;
Expand All @@ -47,6 +47,10 @@ bool VertexIdSeek::matchNode(NodeContext *nodeCtx) {
if (nodeVid.second.kind == VidExtractVisitor::VidPattern::Vids::Kind::kIn) {
if (nodeVid.first == node.alias) {
nodeCtx->ids = std::move(nodeVid.second.vids);
// Eliminate Filter when it's complete predication about Vertex Id
if (ExpressionUtils::isVidPredication(nodeCtx->bindWhereClause->filter)) {
nodeCtx->bindWhereClause->filter = nullptr;
}
return true;
}
}
Expand Down
4 changes: 2 additions & 2 deletions src/graph/planner/match/WhereClausePlanner.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -19,8 +19,8 @@ StatusOr<SubPlan> WhereClausePlanner::transform(CypherClauseContextBase* ctx) {
}

auto* wctx = static_cast<WhereClauseContext*>(ctx);
SubPlan wherePlan;
if (wctx->filter) {
SubPlan wherePlan;
auto* newFilter = MatchSolver::doRewrite(wctx->qctx, wctx->aliasesAvailable, wctx->filter);
wherePlan.root = Filter::make(wctx->qctx, nullptr, newFilter, needStableFilter_);
wherePlan.tail = wherePlan.root;
Expand All @@ -46,7 +46,7 @@ StatusOr<SubPlan> WhereClausePlanner::transform(CypherClauseContextBase* ctx) {
return wherePlan;
}

return Status::OK();
return wherePlan;
}
} // namespace graph
} // namespace nebula
28 changes: 28 additions & 0 deletions src/graph/util/ExpressionUtils.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1242,5 +1242,33 @@ bool ExpressionUtils::checkExprDepth(const Expression *expr) {
return graph::RewriteVisitor::transform(expr, matcher, rewriter);
}

/*static*/ bool ExpressionUtils::isVidPredication(const Expression *expr) {
if (DCHECK_NOTNULL(expr)->kind() != Expression::Kind::kRelIn &&
expr->kind() != Expression::Kind::kRelEQ) {
return false;
}
const auto *relExpr = static_cast<const RelationalExpression *>(expr);
if (relExpr->left()->kind() != Expression::Kind::kFunctionCall) {
return false;
}
const auto *fCallExpr = static_cast<const FunctionCallExpression *>(relExpr->left());
if (fCallExpr->name() != "id" || fCallExpr->args()->numArgs() != 1 ||
fCallExpr->args()->args().front()->kind() != Expression::Kind::kLabel) {
return false;
}
if (expr->kind() == Expression::Kind::kRelIn) {
// id(V) IN [List]
if (relExpr->right()->kind() != Expression::Kind::kList) {
return false;
}
} else if (expr->kind() == Expression::Kind::kRelEQ) {
// id(V) = Value
if (relExpr->right()->kind() != Expression::Kind::kConstant) {
return false;
}
}
return true;
}

} // namespace graph
} // namespace nebula
4 changes: 4 additions & 0 deletions src/graph/util/ExpressionUtils.h
Original file line number Diff line number Diff line change
Expand Up @@ -211,6 +211,10 @@ class ExpressionUtils {
static Expression* rewriteVertexPropertyFilter(ObjectPool* pool,
const std::string& node,
Expression* expr);

// Whether the whole expression is vertex id predication
// e.g. id(v) == 1, id(v) IN [...]
static bool isVidPredication(const Expression* expr);
};

} // namespace graph
Expand Down
2 changes: 0 additions & 2 deletions src/graph/validator/test/QueryValidatorTest.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1228,7 +1228,6 @@ TEST_F(QueryValidatorTest, TestMatch) {
"RETURN type(r) AS Type, v2.person.name AS Name";
std::vector<PlanNode::Kind> expected = {
PK::kProject,
PK::kFilter,
PK::kProject,
PK::kAppendVertices,
PK::kTraverse,
Expand All @@ -1245,7 +1244,6 @@ TEST_F(QueryValidatorTest, TestMatch) {
"RETURN v1, v2";
std::vector<PlanNode::Kind> expected = {
PK::kProject,
PK::kFilter,
PK::kProject,
PK::kAppendVertices,
PK::kTraverse,
Expand Down
34 changes: 34 additions & 0 deletions tests/tck/features/match/SeekById.feature
Original file line number Diff line number Diff line change
Expand Up @@ -365,3 +365,37 @@ Feature: Match seek by id
| 'Aron Baynes' |
| 'Blake Griffin' |
| 'Grant Hill' |

Scenario: check plan
When profiling query:
"""
MATCH (v)
WHERE id(v) == 'Paul Gasol'
RETURN v.player.name AS Name, v.player.age AS Age
"""
Then the result should be, in any order:
| Name | Age |
| 'Paul Gasol' | 38 |
And the execution plan should be:
| id | name | dependencies | operator info |
| 12 | Project | 2 | |
| 2 | AppendVertices | 16 | |
| 16 | Dedup | 7 | |
| 7 | PassThrough | 0 | |
| 0 | Start | | |
When profiling query:
"""
MATCH (v)
WHERE id(v) IN ['Paul Gasol']
RETURN v.player.name AS Name, v.player.age AS Age
"""
Then the result should be, in any order:
| Name | Age |
| 'Paul Gasol' | 38 |
And the execution plan should be:
| id | name | dependencies | operator info |
| 12 | Project | 2 | |
| 2 | AppendVertices | 16 | |
| 16 | Dedup | 7 | |
| 7 | PassThrough | 0 | |
| 0 | Start | | |
34 changes: 34 additions & 0 deletions tests/tck/features/match/SeekById.intVid.feature
Original file line number Diff line number Diff line change
Expand Up @@ -405,3 +405,37 @@ Feature: Match seek by id
Then the result should be, in any order:
| v |
| (-100 :player{age: 32, name: "Tim"}) |

Scenario: check plan
When profiling query:
"""
MATCH (v)
WHERE id(v) == hash('Paul Gasol')
RETURN v.player.name AS Name, v.player.age AS Age
"""
Then the result should be, in any order:
| Name | Age |
| 'Paul Gasol' | 38 |
And the execution plan should be:
| id | name | dependencies | operator info |
| 12 | Project | 2 | |
| 2 | AppendVertices | 16 | |
| 16 | Dedup | 7 | |
| 7 | PassThrough | 0 | |
| 0 | Start | | |
When profiling query:
"""
MATCH (v)
WHERE id(v) IN [hash('Paul Gasol')]
RETURN v.player.name AS Name, v.player.age AS Age
"""
Then the result should be, in any order:
| Name | Age |
| 'Paul Gasol' | 38 |
And the execution plan should be:
| id | name | dependencies | operator info |
| 12 | Project | 2 | |
| 2 | AppendVertices | 16 | |
| 16 | Dedup | 7 | |
| 7 | PassThrough | 0 | |
| 0 | Start | | |
Loading

0 comments on commit 50425ea

Please sign in to comment.