Skip to content

Commit

Permalink
rewrite param in subgraph & path (vesoft-inc#2635)
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?
- [x] bug
- [ ] feature
- [ ] enhancement

## What problem(s) does this PR solve?
#### Issue(s) number: 
close vesoft-inc/nebula-ent#2571
close vesoft-inc/nebula-ent#2570

#### Description:


## 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#5500

Co-authored-by: jimingquan <mingquan.ji@vesoft.com>
  • Loading branch information
nebula-bots and nevermore3 authored Apr 17, 2023
1 parent c51a8cf commit 7cb8287
Show file tree
Hide file tree
Showing 3 changed files with 73 additions and 11 deletions.
21 changes: 17 additions & 4 deletions src/graph/validator/FindPathValidator.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -37,15 +37,28 @@ Status FindPathValidator::validateWhere(WhereClause* where) {
return Status::OK();
}
// Not Support $-、$var、$$.tag.prop、$^.tag.prop、agg
auto expr = where->filter();
if (ExpressionUtils::findAny(expr,
auto filterExpr = where->filter();
if (ExpressionUtils::findAny(filterExpr,
{Expression::Kind::kSrcProperty,
Expression::Kind::kDstProperty,
Expression::Kind::kVarProperty,
Expression::Kind::kInputProperty})) {
return Status::SemanticError("Not support `%s' in where sentence.", expr->toString().c_str());
return Status::SemanticError("Not support `%s' in where sentence.",
filterExpr->toString().c_str());
}
auto colExpr = ExpressionUtils::rewriteLabelAttr2EdgeProp(expr);

auto undefinedParams = graph::ExpressionUtils::ExtractInnerVars(filterExpr, qctx_);
if (!undefinedParams.empty()) {
return Status::SemanticError(
"Undefined parameters: " +
std::accumulate(++undefinedParams.begin(),
undefinedParams.end(),
*undefinedParams.begin(),
[](auto& lhs, auto& rhs) { return lhs + ", " + rhs; }));
}
auto* newFilter = graph::ExpressionUtils::rewriteParameter(filterExpr, qctx_);

auto colExpr = ExpressionUtils::rewriteLabelAttr2EdgeProp(newFilter);
auto edgePropExprs = ExpressionUtils::collectAll(colExpr, {Expression::Kind::kEdgeProperty});
for (const auto* e : edgePropExprs) {
DCHECK_EQ(e->kind(), Expression::Kind::kEdgeProperty);
Expand Down
24 changes: 18 additions & 6 deletions src/graph/validator/GetSubgraphValidator.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -141,17 +141,29 @@ Status GetSubgraphValidator::validateWhere(WhereClause* where) {
if (where == nullptr) {
return Status::OK();
}
auto* expr = where->filter();
if (ExpressionUtils::findAny(expr,
auto* filterExpr = where->filter();
if (ExpressionUtils::findAny(filterExpr,
{Expression::Kind::kAggregate,
Expression::Kind::kSrcProperty,
Expression::Kind::kVarProperty,
Expression::Kind::kInputProperty,
Expression::Kind::kLogicalOr})) {
return Status::SemanticError("Not support `%s' in where sentence.", expr->toString().c_str());
return Status::SemanticError("Not support `%s' in where sentence.",
filterExpr->toString().c_str());
}

where->setFilter(ExpressionUtils::rewriteLabelAttr2EdgeProp(expr));
auto undefinedParams = graph::ExpressionUtils::ExtractInnerVars(filterExpr, qctx_);
if (!undefinedParams.empty()) {
return Status::SemanticError(
"Undefined parameters: " +
std::accumulate(++undefinedParams.begin(),
undefinedParams.end(),
*undefinedParams.begin(),
[](auto& lhs, auto& rhs) { return lhs + ", " + rhs; }));
}
auto* newFilter = graph::ExpressionUtils::rewriteParameter(filterExpr, qctx_);

where->setFilter(ExpressionUtils::rewriteLabelAttr2EdgeProp(newFilter));
auto filter = where->filter();
auto typeStatus = deduceExprType(filter);
NG_RETURN_IF_ERROR(typeStatus);
Expand Down Expand Up @@ -184,11 +196,11 @@ Status GetSubgraphValidator::validateWhere(WhereClause* where) {
}

auto condition = filter->clone();
if (ExpressionUtils::findAny(expr, {Expression::Kind::kDstProperty})) {
if (ExpressionUtils::findAny(filter, {Expression::Kind::kDstProperty})) {
auto visitor = ExtractFilterExprVisitor::makePushGetVertices(qctx_->objPool());
filter->accept(&visitor);
if (!visitor.ok()) {
return Status::SemanticError("Push target vertices filter error: " + expr->toString());
return Status::SemanticError("Push target vertices filter error: " + filter->toString());
}
subgraphCtx_->edgeFilter = visitor.remainedExpr();
auto tagFilter = visitor.extractedExpr() ? visitor.extractedExpr() : filter;
Expand Down
39 changes: 38 additions & 1 deletion tests/tck/features/yield/parameter.feature
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ Feature: Parameter
Background:
Given an empty graph
And load "nba" csv data to a new space
Given parameters: {"p1":1,"p2":true,"p3":"Tim Duncan","p4":3.3,"p5":[1,true,3],"p6":{"a":3,"b":false,"c":"Tim Duncan"},"p7":{"a":{"b":{"c":"Tim Duncan","d":[1,2,3,true,"Tim Duncan"]}}},"p8":"Manu Ginobili", "p9":["Tim Duncan","Tony Parker"]}
Given parameters: {"p1":1,"p2":true,"p3":"Tim Duncan","p4":3.3,"p5":[1,true,3],"p6":{"a":3,"b":false,"c":"Tim Duncan"},"p7":{"a":{"b":{"c":"Tim Duncan","d":[1,2,3,true,"Tim Duncan"]}}},"p8":"Manu Ginobili", "p9":["Tim Duncan","Tony Parker"], "p10":90}

Scenario: [param-test-001] without define param
When executing query:
Expand Down Expand Up @@ -269,6 +269,21 @@ Feature: Parameter
MATCH (v:player) where v.player.age < $unknown_distance RETURN v
"""
Then a SemanticError should be raised at runtime: Undefined parameters: unknown_distance
When executing query:
"""
GET SUBGRAPH FROM 'Tim Duncan' WHERE like.likeness < $unknown_distance YIELD edges as e
"""
Then a SemanticError should be raised at runtime: Undefined parameters: unknown_distance
When executing query:
"""
FIND ALL PATH FROM 'Tim Duncan' TO 'Tony Parker' OVER like WHERE like.likeness > $unknown_distance YIELD path as p
"""
Then a SemanticError should be raised at runtime: Undefined parameters: unknown_distance
When executing query:
"""
FIND SHORTEST PATH FROM 'Tim Duncan' TO 'Tony Parker' OVER like WHERE like.likeness > $unknown_distance YIELD path as p
"""
Then a SemanticError should be raised at runtime: Undefined parameters: unknown_distance
When executing query:
"""
MATCH (v:player) RETURN v LIMIT $p6
Expand Down Expand Up @@ -345,6 +360,28 @@ Feature: Parameter
| v |
| BAD_TYPE |
| BAD_TYPE |
When executing query:
"""
GET SUBGRAPH FROM 'Tim Duncan' WHERE like.likeness > $p10 YIELD edges AS e
"""
Then the result should be, in any order:
| e |
| [[:like "Tim Duncan"->"Manu Ginobili" @0 {likeness: 95}], [:like "Tim Duncan"->"Tony Parker" @0 {likeness: 95}], [:like "Dejounte Murray"->"Tim Duncan" @0 {likeness: 99}], [:like "Tony Parker"->"Tim Duncan" @0 {likeness: 95}]] |
| [[:like "Tony Parker"->"Manu Ginobili" @0 {likeness: 95}], [:like "Dejounte Murray"->"Manu Ginobili" @0 {likeness: 99}], [:like "Dejounte Murray"->"Tony Parker" @0 {likeness: 99}]] |
When executing query:
"""
FIND ALL PATH FROM 'Tim Duncan' TO 'Tony Parker' OVER like WHERE like.likeness > $p10-1 YIELD path AS p
"""
Then the result should be, in any order, with relax comparison:
| p |
| <("Tim Duncan")-[:like@0 {likeness: 95}]->("Tony Parker")> |
| <("Tim Duncan")-[:like@0 {likeness: 95}]->("Manu Ginobili")-[:like@0 {likeness: 90}]->("Tim Duncan")-[:like@0 {likeness: 95}]->("Tony Parker")> |
When executing query:
"""
FIND ALL PATH FROM 'Tim Duncan' TO 'Tony Parker' OVER like WHERE like.likeness > $p5[10] YIELD path AS p
"""
Then the result should be, in any order:
| p |
Scenario: [param-test-013] DML
Given an empty graph
Expand Down

0 comments on commit 7cb8287

Please sign in to comment.