-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Generate an plan base on the output #5437
Generate an plan base on the output #5437
Conversation
src/graph/validator/GoValidator.cpp
Outdated
goCtx_->joinDst = true; | ||
} else { | ||
auto& name = static_cast<const VertexExpression*>(filterExpr)->name(); | ||
if (name == "VERTEX") { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just return false if you dont care this scenario.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is code logic you should check elsewhere than here, IMHO.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should report error if user input
go from ... yield vertex as v
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Didn't the previous implementation check that, I mean you shouldn't mess with the logic of checking if there is an endpoint.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think checking whether there is information about the destination vertex and whether to joinDst is logically consistent and does not need to be implemented separately
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
got what's u mean
52cfb78
to
b157103
Compare
b157103
to
c9b552b
Compare
bool GoValidator::checkDstPropOrVertexExist(const Expression* expr) { | ||
auto filterExprs = ExpressionUtils::collectAll( | ||
expr, {Expression::Kind::kVertex, Expression::Kind::kDstProperty}); | ||
for (auto filterExpr : filterExprs) { | ||
if (filterExpr->kind() == Expression::Kind::kDstProperty) { | ||
goCtx_->joinDst = true; | ||
} else { | ||
auto& name = static_cast<const VertexExpression*>(filterExpr)->name(); | ||
if (name == "VERTEX") { | ||
return false; | ||
} | ||
if (name == "$$") { | ||
goCtx_->joinDst = true; | ||
} | ||
} | ||
} | ||
return true; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe this is better:
bool ExpressionUtils::existAnyDstVertex(const Expression* expr) {
auto finder = [](const Expression* e) -> bool {
return (e->kind() == ExprKind::kDstProperty) ||
(e->kind == ExprKind::kVertex &&
static_cast<const VertexExpression*>(e)->name() == "$$");
}
FindVisitor visitor(finder);
const_cast<Expression*>(expr)->accept(&visitor);
return !visitor.results().empty();
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
goCtx_->joinDst = ExpressionUtils::existAnyDstVertex(filter);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
although the functions are separated, it also causes repeated verification
What type of PR is this?
What problem(s) does this PR solve?
Issue(s) number:
Description:
How do you solve it?
Special notes for your reviewer, ex. impact of this fix, design document, etc:
Checklist:
Tests:
Affects:
Release notes:
Please confirm whether to be reflected in release notes and how to describe: