Skip to content

Commit

Permalink
Fix expression rewrite (#3614)
Browse files Browse the repository at this point in the history 
* Do not transfer the filter expression if it contains 2 lableAttribute exprs

* Fix expression overflow check

* Fix rewriteRelExpr

* Refactor rewriteRelExpr

* Fix log usage

* Check whether the minus expression contains string

* Add tck cases

* Address comments

* Fix conflicts

* Address comments

* modify metrics in conf files

* Address comments
  • Loading branch information
@Aiee
Aiee committed Jan 11, 2022
1 parent a408673 commit 32394f4
Show file tree
Hide file tree
Showing 9 changed files with 214 additions and 78 deletions.
5 changes: 3 additions & 2 deletions conf/nebula-graphd.conf.default
View file
Original file line number Diff line number Diff line change
Expand Up @@ -84,8 +84,9 @@
# System memory high watermark ratio, cancel the memory checking when the ratio greater than 1.0
--system_memory_high_watermark_ratio=0.8

########## metrics ##########
--enable_space_level_metrics=false

########## experimental feature ##########
# if use experimental features
--enable_experimental_feature=false

--enable_space_level_metrics=false
5 changes: 3 additions & 2 deletions conf/nebula-graphd.conf.production
View file
Original file line number Diff line number Diff line change
Expand Up @@ -83,8 +83,9 @@
# System memory high watermark ratio, cancel the memory checking when the ratio greater than 1.0
--system_memory_high_watermark_ratio=0.8

########## metrics ##########
--enable_space_level_metrics=false

########## experimental feature ##########
# if use experimental features
--enable_experimental_feature=false

--enable_space_level_metrics=false
104 changes: 84 additions & 20 deletions src/graph/util/ExpressionUtils.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,10 @@

#include <memory>
#include <queue>
#include <unordered_set>

#include "common/base/ObjectPool.h"
#include "common/expression/ArithmeticExpression.h"
#include "common/expression/Expression.h"
#include "common/expression/PropertyExpression.h"
#include "common/function/AggFunctionManager.h"
Expand Down Expand Up @@ -415,30 +417,69 @@ Expression *ExpressionUtils::reduceUnaryNotExpr(const Expression *expr) {

Expression *ExpressionUtils::rewriteRelExpr(const Expression *expr) {
ObjectPool *pool = expr->getObjPool();
// Match relational expressions containing at least one arithmetic expr
auto matcher = [](const Expression *e) -> bool {
if (e->isRelExpr()) {
auto relExpr = static_cast<const RelationalExpression *>(e);
if (isEvaluableExpr(relExpr->right())) {
return true;
QueryExpressionContext ctx(nullptr);

auto checkArithmExpr = [&ctx](const ArithmeticExpression *arithmExpr) -> bool {
auto lExpr = const_cast<Expression *>(arithmExpr->left());
auto rExpr = const_cast<Expression *>(arithmExpr->right());

// If the arithExpr has constant expr as member that is a string, do not rewrite
if (lExpr->kind() == Expression::Kind::kConstant) {
if (lExpr->eval(ctx).isStr()) {
return false;
}
// TODO: To match arithmetic expression on both side
auto lExpr = relExpr->left();
if (lExpr->isArithmeticExpr()) {
auto arithmExpr = static_cast<const ArithmeticExpression *>(lExpr);
return isEvaluableExpr(arithmExpr->left()) || isEvaluableExpr(arithmExpr->right());
}
if (rExpr->kind() == Expression::Kind::kConstant) {
if (rExpr->eval(ctx).isStr()) {
return false;
}
}
return false;
return isEvaluableExpr(arithmExpr->left()) || isEvaluableExpr(arithmExpr->right());
};

// Match relational expressions following these rules:
// 1. the right operand of rel expr should be evaluable
// 2. the left operand of rel expr should be:
// 2.a an arithmetic expr that does not contains string and has at least one operand that is
// evaluable
// OR
// 2.b an relational expr so that it might could be simplified:
// ((v.age > 40 == true) => (v.age > 40))
auto matcher = [&checkArithmExpr](const Expression *e) -> bool {
if (!e->isRelExpr()) {
return false;
}

auto relExpr = static_cast<const RelationalExpression *>(e);
// Check right operand
bool checkRightOperand = isEvaluableExpr(relExpr->right());
if (!checkRightOperand) {
return false;
}

// Check left operand
bool checkLeftOperand = false;
auto lExpr = relExpr->left();
// Left operand is arithmetical expr
if (lExpr->isArithmeticExpr()) {
auto arithmExpr = static_cast<const ArithmeticExpression *>(lExpr);
checkLeftOperand = checkArithmExpr(arithmExpr);
} else if (lExpr->isRelExpr() ||
lExpr->kind() == Expression::Kind::kLabelAttribute) { // for expressions that
// contain boolean literals
// such as (v.age <= null)
checkLeftOperand = true;
}
return checkLeftOperand;
};

// Simplify relational expressions involving boolean literals
auto simplifyBoolOperand =
[pool](RelationalExpression *relExpr, Expression *lExpr, Expression *rExpr) -> Expression * {
QueryExpressionContext ctx(nullptr);
auto simplifyBoolOperand = [pool, &ctx](RelationalExpression *relExpr,
Expression *lExpr,
Expression *rExpr) -> Expression * {
if (rExpr->kind() == Expression::Kind::kConstant) {
auto conExpr = static_cast<ConstantExpression *>(rExpr);
auto val = conExpr->eval(ctx(nullptr));
auto val = conExpr->eval(ctx);
auto valType = val.type();
// Rewrite to null if the expression contains any operand that is null
if (valType == Value::Type::NULLVALUE) {
Expand Down Expand Up @@ -521,21 +562,43 @@ Expression *ExpressionUtils::rewriteRelExprHelper(const Expression *expr,
// case Expression::Kind::kMultiply:
// case Expression::Kind::kDivision:
default:
LOG(FATAL) << "Unsupported expression kind: " << static_cast<uint8_t>(kind);
DLOG(ERROR) << "Unsupported expression kind: " << static_cast<uint8_t>(kind);
break;
}

return rewriteRelExprHelper(root, relRightOperandExpr);
}

StatusOr<Expression *> ExpressionUtils::filterTransform(const Expression *filter) {
auto rewrittenExpr = const_cast<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});
// 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);
}

// 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);

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

// Reduce Unary expression
rewrittenExpr = reduceUnaryNotExpr(rewrittenExpr);
return rewrittenExpr;
Expand Down Expand Up @@ -880,8 +943,9 @@ Expression::Kind ExpressionUtils::getNegatedArithmeticType(const Expression::Kin
return Expression::Kind::kDivision;
case Expression::Kind::kDivision:
return Expression::Kind::kMultiply;
// There is no oppsite operation to Mod, return itself
case Expression::Kind::kMod:
LOG(FATAL) << "Unsupported expression kind: " << static_cast<uint8_t>(kind);
return Expression::Kind::kMod;
break;
default:
LOG(FATAL) << "Invalid arithmetic expression kind: " << static_cast<uint8_t>(kind);
Expand Down
61 changes: 34 additions & 27 deletions src/graph/visitor/DeduceTypeVisitor.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -44,33 +44,40 @@ static const std::unordered_map<Value::Type, Value> kConstantValues = {
{Value::Type::DURATION, Value(Duration())},
};

#define DETECT_BIEXPR_TYPE(OP) \
expr->left()->accept(this); \
if (!ok()) return; \
auto left = type_; \
expr->right()->accept(this); \
if (!ok()) return; \
auto right = type_; \
auto lhs = kConstantValues.find(left); \
if (lhs == kConstantValues.end()) { \
status_ = Status::SemanticError("Can't find constant value of `%s' when deduce type.", \
Value::toString(left).c_str()); \
return; \
} \
auto rhs = kConstantValues.find(right); \
if (rhs == kConstantValues.end()) { \
status_ = Status::SemanticError("Can't find constant value of `%s' when deduce type.", \
Value::toString(right).c_str()); \
return; \
} \
auto detectVal = lhs->second OP rhs->second; \
if (detectVal.isBadNull()) { \
std::stringstream ss; \
ss << "`" << expr->toString() << "' is not a valid expression, " \
<< "can not apply `" << #OP << "' to `" << left << "' and `" << right << "'."; \
status_ = Status::SemanticError(ss.str()); \
return; \
} \
#define DETECT_BIEXPR_TYPE(OP) \
expr->left()->accept(this); \
if (!ok()) return; \
auto left = type_; \
expr->right()->accept(this); \
if (!ok()) return; \
auto right = type_; \
if (strcmp(#OP, "-") == 0 && (left == Value::Type::STRING || right == Value::Type::STRING)) { \
std::stringstream ss; \
ss << "`" << expr->toString() << "' is not a valid expression, " \
<< "can not apply `" << #OP << "' to `" << left << "' and `" << right << "'."; \
status_ = Status::SemanticError(ss.str()); \
return; \
} \
auto lhs = kConstantValues.find(left); \
if (lhs == kConstantValues.end()) { \
status_ = Status::SemanticError("Can't find constant value of `%s' when deduce type.", \
Value::toString(left).c_str()); \
return; \
} \
auto rhs = kConstantValues.find(right); \
if (rhs == kConstantValues.end()) { \
status_ = Status::SemanticError("Can't find constant value of `%s' when deduce type.", \
Value::toString(right).c_str()); \
return; \
} \
auto detectVal = lhs->second OP rhs->second; \
if (detectVal.isBadNull()) { \
std::stringstream ss; \
ss << "`" << expr->toString() << "' is not a valid expression, " \
<< "can not apply `" << #OP << "' to `" << left << "' and `" << right << "'."; \
status_ = Status::SemanticError(ss.str()); \
return; \
} \
type_ = detectVal.type()

#define DETECT_NARYEXPR_TYPE(OP) \
Expand Down
69 changes: 43 additions & 26 deletions src/graph/visitor/test/FilterTransformTest.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,27 +23,24 @@ TEST_F(FilterTransformTest, TestComplexExprRewrite) {
ASSERT_EQ(*res.value(), *expected) << res.value()->toString() << " vs. " << expected->toString();
}

// If the expression tranformation causes an overflow, it should not be done.
TEST_F(FilterTransformTest, TestCalculationOverflow) {
// (v.age - 1 < 9223372036854775807) => overflow
// (v.age - 1 < 9223372036854775807) => unchanged
{
auto expr =
ltExpr(minusExpr(laExpr("v", "age"), constantExpr(1)), constantExpr(9223372036854775807));
auto res = ExpressionUtils::filterTransform(expr);
auto expected = Status::SemanticError(
"result of (9223372036854775807+1) cannot be represented as an "
"integer");
ASSERT(!res.status().ok());
ASSERT_EQ(res.status(), expected) << res.status().toString() << " vs. " << expected.toString();
ASSERT(res.ok());
auto expected = expr;
ASSERT_EQ(res.value(), expected) << res.value() << " vs. " << expected->toString();
}
// (v.age + 1 < -9223372036854775808) => overflow
// (v.age + 1 < -9223372036854775808) => unchanged
{
auto expr = ltExpr(addExpr(laExpr("v", "age"), constantExpr(1)), constantExpr(INT64_MIN));
auto res = ExpressionUtils::filterTransform(expr);
auto expected = Status::SemanticError(
"result of (-9223372036854775808-1) cannot be represented as an "
"integer");
ASSERT(!res.status().ok());
ASSERT_EQ(res.status(), expected) << res.status().toString() << " vs. " << expected.toString();
ASSERT(res.ok());
auto expected = expr;
ASSERT_EQ(res.value(), expected) << res.value() << " vs. " << expected->toString();
}
// (v.age - 1 < 9223372036854775807 + 1) => overflow
{
Expand All @@ -53,7 +50,7 @@ TEST_F(FilterTransformTest, TestCalculationOverflow) {
auto expected = Status::SemanticError(
"result of (9223372036854775807+1) cannot be represented as an "
"integer");
ASSERT(!res.status().ok());
ASSERT(!res.ok());
ASSERT_EQ(res.status(), expected) << res.status().toString() << " vs. " << expected.toString();
}
// (v.age + 1 < -9223372036854775808 - 1) => overflow
Expand All @@ -64,30 +61,50 @@ TEST_F(FilterTransformTest, TestCalculationOverflow) {
auto expected = Status::SemanticError(
"result of (-9223372036854775808-1) cannot be represented as an "
"integer");
ASSERT(!res.status().ok());
ASSERT(!res.ok());
ASSERT_EQ(res.status(), expected) << res.status().toString() << " vs. " << expected.toString();
}
// !!!(v.age - 1 < 9223372036854775807) => overflow
// !!!(v.age - 1 < 9223372036854775807) => unchanged
{
auto expr = notExpr(notExpr(notExpr(ltExpr(minusExpr(laExpr("v", "age"), constantExpr(1)),
constantExpr(9223372036854775807)))));
auto res = ExpressionUtils::filterTransform(expr);
auto expected = Status::SemanticError(
"result of (9223372036854775807+1) cannot be represented as an "
"integer");
ASSERT(!res.status().ok());
ASSERT_EQ(res.status(), expected) << res.status().toString() << " vs. " << expected.toString();
ASSERT(res.ok());
auto expected = expr;
ASSERT_EQ(res.value(), expected) << res.value()->toString() << " vs. " << expected->toString();
}
// !!!(v.age + 1 < -9223372036854775808) => overflow
// !!!(v.age + 1 < -9223372036854775808) => unchanged
{
auto expr = notExpr(notExpr(
notExpr(ltExpr(addExpr(laExpr("v", "age"), constantExpr(1)), constantExpr(INT64_MIN)))));
auto res = ExpressionUtils::filterTransform(expr);
auto expected = Status::SemanticError(
"result of (-9223372036854775808-1) cannot be represented as an "
"integer");
ASSERT(!res.status().ok());
ASSERT_EQ(res.status(), expected) << res.status().toString() << " vs. " << expected.toString();
ASSERT(res.ok());
auto expected = expr;
ASSERT_EQ(res.value(), expected) << res.value()->toString() << " vs. " << expected->toString();
}
}

TEST_F(FilterTransformTest, TestNoRewrite) {
// Do not rewrite if the filter contains more than one different LabelAttribute expr
{
// (v.age - 1 < v2.age + 2) => Unchanged
auto expr = ltExpr(minusExpr(laExpr("v", "age"), constantExpr(1)),
addExpr(laExpr("v2", "age"), constantExpr(40)));
auto res = ExpressionUtils::filterTransform(expr);
ASSERT(res.ok());
auto expected = expr;
ASSERT_EQ(res.value()->toString(), expected->toString())
<< res.value()->toString() << " vs. " << expected->toString();
}
// Do not rewrite if the arithmetic expression contains string/char constant
{
// (v.name - "ab" < "name") => Unchanged
auto expr = ltExpr(minusExpr(laExpr("v", "name"), constantExpr("ab")), constantExpr("name"));
auto res = ExpressionUtils::filterTransform(expr);
ASSERT(res.ok());
auto expected = expr;
ASSERT_EQ(res.value()->toString(), expected->toString())
<< res.value()->toString() << " vs. " << expected->toString();
}
}

Expand Down
18 changes: 18 additions & 0 deletions src/graph/visitor/test/RewriteRelExprVisitorTest.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -234,5 +234,23 @@ TEST_F(RewriteRelExprVisitorTest, TestContainer) {
}
}

TEST_F(RewriteRelExprVisitorTest, TestArithmeticalExprWithStr) {
// Do not rewrite if the arithmetic expression contains string/char constant
{
// (v.name - "ab" < "name") => Unchanged
auto expr = ltExpr(minusExpr(laExpr("v", "name"), constantExpr("ab")), constantExpr("name"));
auto res = ExpressionUtils::rewriteRelExpr(expr);
auto expected = expr;
ASSERT_EQ(*res, *expected) << res->toString() << " vs. " << expected->toString();
}
{
// (v.name + "ab" < "name") => Unchanged
auto expr = ltExpr(addExpr(laExpr("v", "name"), constantExpr("ab")), constantExpr("name"));
auto res = ExpressionUtils::rewriteRelExpr(expr);
auto expected = expr;
ASSERT_EQ(*res, *expected) << res->toString() << " vs. " << expected->toString();
}
}

} // namespace graph
} // namespace nebula
Loading

0 comments on commit 32394f4

Please sign in to comment.