Skip to content

Commit

Permalink
fix match index (#3694)
Browse files Browse the repository at this point in the history 
* check scheam

* add test case

* fix graph crash

* address comment'
'

* add test case

* fix error

* fix vid select error

* fix unit test error

* address comment
  • Loading branch information
@nevermore3
nevermore3 authored Jan 14, 2022
1 parent fdb4cdc commit b4029d3
Show file tree
Hide file tree
Showing 10 changed files with 234 additions and 90 deletions.
4 changes: 2 additions & 2 deletions src/graph/optimizer/rule/IndexScanRule.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -224,11 +224,11 @@ inline bool verifyType(const Value& val) {
case Value::Type::SET:
case Value::Type::MAP:
case Value::Type::DATASET:
case Value::Type::DURATION:
case Value::Type::GEOGRAPHY: // TODO(jie)
case Value::Type::PATH: {
DLOG(FATAL) << "Not supported value type " << val.type() << "for index.";
return false;
} break;
}
default: {
return true;
}
Expand Down
11 changes: 6 additions & 5 deletions src/graph/planner/match/LabelIndexSeek.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -89,18 +89,20 @@ StatusOr<SubPlan> LabelIndexSeek::transformNode(NodeContext* nodeCtx) {
if (whereCtx && whereCtx->filter) {
auto* filter = whereCtx->filter;
const auto& nodeAlias = nodeCtx->info->alias;
const auto& schemaName = nodeCtx->scanInfo.schemaNames.back();

if (filter->kind() == Expression::Kind::kLogicalOr) {
auto exprs = ExpressionUtils::collectAll(filter, {Expression::Kind::kLabelTagProperty});
bool labelMatched = true;
bool matched = true;
for (auto* expr : exprs) {
auto tagPropExpr = static_cast<const LabelTagPropertyExpression*>(expr);
if (static_cast<const PropertyExpression*>(tagPropExpr->label())->prop() != nodeAlias) {
labelMatched = false;
if (static_cast<const PropertyExpression*>(tagPropExpr->label())->prop() != nodeAlias ||
tagPropExpr->sym() != schemaName) {
matched = false;
break;
}
}
if (labelMatched) {
if (matched) {
auto flattenFilter = ExpressionUtils::flattenInnerLogicalExpr(filter);
DCHECK_EQ(flattenFilter->kind(), Expression::Kind::kLogicalOr);
auto& filterItems = static_cast<LogicalExpression*>(flattenFilter)->operands();
Expand All @@ -120,7 +122,6 @@ StatusOr<SubPlan> LabelIndexSeek::transformNode(NodeContext* nodeCtx) {
storage::cpp2::IndexQueryContext ctx;
ctx.filter_ref() = Expression::encode(*flattenFilter);
scan->setIndexQueryContext({ctx});
whereCtx.reset();
}
}
}
Expand Down
4 changes: 4 additions & 0 deletions src/graph/planner/match/VertexIdSeek.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,10 @@ bool VertexIdSeek::matchNode(NodeContext *nodeCtx) {
if (vidResult.spec != VidExtractVisitor::VidPattern::Special::kInUsed) {
return false;
}
if (vidResult.nodes.size() > 1) {
// where id(v) == 'xxx' or id(t) == 'yyy'
return false;
}
for (auto &nodeVid : vidResult.nodes) {
if (nodeVid.second.kind == VidExtractVisitor::VidPattern::Vids::Kind::kIn) {
if (nodeVid.first == node.alias) {
Expand Down
22 changes: 11 additions & 11 deletions src/graph/util/ExpressionUtils.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -570,32 +570,32 @@ Expression *ExpressionUtils::rewriteRelExprHelper(const Expression *expr,
}

StatusOr<Expression *> ExpressionUtils::filterTransform(const Expression *filter) {
// If the filter contains more than one different LabelAttribute expr, this filter cannot be
// pushed down
auto propExprs = ExpressionUtils::collectAll(filter, {Expression::Kind::kLabelTagProperty});
// Check if any overflow happen before filter tranform
auto initialConstFold = foldConstantExpr(filter);
NG_RETURN_IF_ERROR(initialConstFold);
auto newFilter = initialConstFold.value();

// If the filter contains more than one different Label expr, this filter cannot be
// pushed down, such as where v1.player.name == 'xxx' or v2.player.age == 20
auto propExprs = ExpressionUtils::collectAll(newFilter, {Expression::Kind::kLabel});
// Deduplicate the list
std::unordered_set<std::string> dedupPropExprSet;
for (auto &iter : propExprs) {
dedupPropExprSet.emplace(iter->toString());
}
if (dedupPropExprSet.size() > 1) {
return const_cast<Expression *>(filter);
return const_cast<Expression *>(newFilter);
}

// Check if any overflow happen before filter tranform
auto initialConstFold = foldConstantExpr(filter);
NG_RETURN_IF_ERROR(initialConstFold);

// Rewrite relational expression
auto rewrittenExpr = initialConstFold.value();
rewrittenExpr = rewriteRelExpr(rewrittenExpr);
auto rewrittenExpr = rewriteRelExpr(newFilter->clone());

// Fold constant expression
auto constantFoldRes = foldConstantExpr(rewrittenExpr);
// If errors like overflow happened during the constant fold, stop transferming and return the
// original expression
if (!constantFoldRes.ok()) {
return const_cast<Expression *>(filter);
return const_cast<Expression *>(newFilter);
}
rewrittenExpr = constantFoldRes.value();

Expand Down
52 changes: 29 additions & 23 deletions src/graph/visitor/VidExtractVisitor.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,6 @@ void VidExtractVisitor::visit(ConstantExpression *expr) {

void VidExtractVisitor::visit(UnaryExpression *expr) {
if (expr->kind() == Expression::Kind::kUnaryNot) {
// const auto *expr = static_cast<const UnaryExpression *>(expr);
expr->operand()->accept(this);
auto operandResult = moveVidPattern();
if (operandResult.spec == VidPattern::Special::kInUsed) {
Expand Down Expand Up @@ -119,14 +118,15 @@ void VidExtractVisitor::visit(LabelExpression *expr) {
}

void VidExtractVisitor::visit(LabelAttributeExpression *expr) {
if (expr->kind() == Expression::Kind::kLabelAttribute) {
const auto *labelExpr = static_cast<const LabelAttributeExpression *>(expr);
vidPattern_ =
VidPattern{VidPattern::Special::kInUsed,
{{labelExpr->left()->toString(), {VidPattern::Vids::Kind::kOtherSource, {}}}}};
} else {
vidPattern_ = VidPattern{};
}
const auto &label = expr->left()->toString();
vidPattern_ = VidPattern{VidPattern::Special::kInUsed,
{{label, {VidPattern::Vids::Kind::kOtherSource, {}}}}};
}

void VidExtractVisitor::visit(LabelTagPropertyExpression *expr) {
const auto &label = static_cast<const PropertyExpression *>(expr->label())->prop();
vidPattern_ = VidPattern{VidPattern::Special::kInUsed,
{{label, {VidPattern::Vids::Kind::kOtherSource, {}}}}};
}

void VidExtractVisitor::visit(ArithmeticExpression *expr) {
Expand All @@ -144,7 +144,13 @@ void VidExtractVisitor::visit(RelationalExpression *expr) {
{{label, {VidPattern::Vids::Kind::kOtherSource, {}}}}};
return;
}

if (expr->left()->kind() == Expression::Kind::kLabelTagProperty) {
const auto *tagPropExpr = static_cast<const LabelTagPropertyExpression *>(expr->left());
const auto &label = static_cast<const PropertyExpression *>(tagPropExpr->label())->prop();
vidPattern_ = VidPattern{VidPattern::Special::kInUsed,
{{label, {VidPattern::Vids::Kind::kOtherSource, {}}}}};
return;
}
if (expr->left()->kind() != Expression::Kind::kFunctionCall ||
expr->right()->kind() != Expression::Kind::kList ||
!ExpressionUtils::isEvaluableExpr(expr->right())) {
Expand All @@ -165,7 +171,6 @@ void VidExtractVisitor::visit(RelationalExpression *expr) {
VidPattern{VidPattern::Special::kInUsed,
{{fCallExpr->args()->args().front()->toString(),
{VidPattern::Vids::Kind::kIn, listExpr->eval(ctx(nullptr)).getList()}}}};
return;
} else if (expr->kind() == Expression::Kind::kRelEQ) {
// id(V) == vid
if (expr->left()->kind() == Expression::Kind::kLabelAttribute) {
Expand All @@ -175,6 +180,13 @@ void VidExtractVisitor::visit(RelationalExpression *expr) {
{{label, {VidPattern::Vids::Kind::kOtherSource, {}}}}};
return;
}
if (expr->left()->kind() == Expression::Kind::kLabelTagProperty) {
const auto *tagPropExpr = static_cast<const LabelTagPropertyExpression *>(expr->left());
const auto &label = static_cast<const PropertyExpression *>(tagPropExpr->label())->prop();
vidPattern_ = VidPattern{VidPattern::Special::kInUsed,
{{label, {VidPattern::Vids::Kind::kOtherSource, {}}}}};
return;
}
if (expr->left()->kind() != Expression::Kind::kFunctionCall ||
expr->right()->kind() != Expression::Kind::kConstant) {
vidPattern_ = VidPattern{};
Expand All @@ -194,10 +206,13 @@ void VidExtractVisitor::visit(RelationalExpression *expr) {
vidPattern_ = VidPattern{VidPattern::Special::kInUsed,
{{fCallExpr->args()->args().front()->toString(),
{VidPattern::Vids::Kind::kIn, List({constExpr->value()})}}}};
return;
} else {
vidPattern_ = VidPattern{};
return;
if (ExpressionUtils::isPropertyExpr(expr->left())) {
vidPattern_ = VidPattern{VidPattern::Special::kInUsed,
{{"", {VidPattern::Vids::Kind::kOtherSource, {}}}}};
} else {
vidPattern_ = VidPattern{};
}
}
}

Expand All @@ -213,11 +228,9 @@ void VidExtractVisitor::visit(AttributeExpression *expr) {

void VidExtractVisitor::visit(LogicalExpression *expr) {
if (expr->kind() == Expression::Kind::kLogicalAnd) {
// const auto *expr = static_cast<const LogicalExpression *>(expr);
std::vector<VidPattern> operandsResult;
operandsResult.reserve(expr->operands().size());
for (const auto &operand : expr->operands()) {
// operandsResult.emplace_back(reverseEvalVids(operand.get()));
operand->accept(this);
operandsResult.emplace_back(moveVidPattern());
}
Expand Down Expand Up @@ -273,8 +286,6 @@ void VidExtractVisitor::visit(LogicalExpression *expr) {
vidPattern_ = std::move(inResult);
return;
} else if (expr->kind() == Expression::Kind::kLogicalOr) {
// const auto *andExpr = static_cast<const LogicalExpression
// *>(expr);
std::vector<VidPattern> operandsResult;
operandsResult.reserve(expr->operands().size());
for (const auto &operand : expr->operands()) {
Expand Down Expand Up @@ -351,11 +362,6 @@ void VidExtractVisitor::visit(MapExpression *expr) {
}

// property Expression
void VidExtractVisitor::visit(LabelTagPropertyExpression *expr) {
UNUSED(expr);
vidPattern_ = VidPattern{};
}

void VidExtractVisitor::visit(TagPropertyExpression *expr) {
UNUSED(expr);
vidPattern_ = VidPattern{};
Expand Down
14 changes: 9 additions & 5 deletions src/graph/visitor/test/FilterTransformTest.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -32,15 +32,17 @@ TEST_F(FilterTransformTest, TestCalculationOverflow) {
auto res = ExpressionUtils::filterTransform(expr);
ASSERT(res.ok());
auto expected = expr;
ASSERT_EQ(res.value(), expected) << res.value() << " vs. " << expected->toString();
ASSERT_EQ(res.value()->toString(), expected->toString())
<< res.value()->toString() << " vs. " << expected->toString();
}
// (v.age + 1 < -9223372036854775808) => unchanged
{
auto expr = ltExpr(addExpr(laExpr("v", "age"), constantExpr(1)), constantExpr(INT64_MIN));
auto res = ExpressionUtils::filterTransform(expr);
ASSERT(res.ok());
auto expected = expr;
ASSERT_EQ(res.value(), expected) << res.value() << " vs. " << expected->toString();
ASSERT_EQ(res.value()->toString(), expected->toString())
<< res.value()->toString() << " vs. " << expected->toString();
}
// (v.age - 1 < 9223372036854775807 + 1) => overflow
{
Expand Down Expand Up @@ -71,7 +73,8 @@ TEST_F(FilterTransformTest, TestCalculationOverflow) {
auto res = ExpressionUtils::filterTransform(expr);
ASSERT(res.ok());
auto expected = expr;
ASSERT_EQ(res.value(), expected) << res.value()->toString() << " vs. " << expected->toString();
ASSERT_EQ(res.value()->toString(), expected->toString())
<< res.value()->toString() << " vs. " << expected->toString();
}
// !!!(v.age + 1 < -9223372036854775808) => unchanged
{
Expand All @@ -80,12 +83,13 @@ TEST_F(FilterTransformTest, TestCalculationOverflow) {
auto res = ExpressionUtils::filterTransform(expr);
ASSERT(res.ok());
auto expected = expr;
ASSERT_EQ(res.value(), expected) << res.value()->toString() << " vs. " << expected->toString();
ASSERT_EQ(res.value()->toString(), expected->toString())
<< res.value()->toString() << " vs. " << expected->toString();
}
}

TEST_F(FilterTransformTest, TestNoRewrite) {
// Do not rewrite if the filter contains more than one different LabelAttribute expr
// Do not rewrite if the filter contains more than one different Label expr
{
// (v.age - 1 < v2.age + 2) => Unchanged
auto expr = ltExpr(minusExpr(laExpr("v", "age"), constantExpr(1)),
Expand Down
2 changes: 1 addition & 1 deletion src/storage/exec/IndexScanNode.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -282,7 +282,7 @@ class QualifiedStrategy {
* [start,end) which is generated by `RangeIndex`. Therefore, it is necessary to make additional
* judgment on the truncated string type index
* For example:
* (ab)c meas that string is "abc" but index val has been truncated to "ab". (ab)c > ab is
* (ab)c means that string is "abc" but index val has been truncated to "ab". (ab)c > ab is
* `UNCERTAIN`, and (ab)c > aa is COMPATIBLE.
*
* Args:
Expand Down
Loading

0 comments on commit b4029d3

Please sign in to comment.