diff --git a/.github/workflows/pull_request.yml b/.github/workflows/pull_request.yml index 901cb9b572d..e944d415c4a 100644 --- a/.github/workflows/pull_request.yml +++ b/.github/workflows/pull_request.yml @@ -19,7 +19,16 @@ jobs: lint: name: lint if: ${{ contains(github.event.pull_request.labels.*.name, 'ready-for-testing') && github.event.pull_request.merged != true }} - runs-on: ubuntu-20.04 + runs-on: + - self-hosted + - linux + - AMD64 + - medium + container: + image: vesoft/nebula-dev:ubuntu2004 + env: + PIP_CACHE_DIR: /tmp/pip-cache + PYPI_MIRROR: ${{ vars.PYPI_MIRROR }} steps: - uses: actions/checkout@v4 with: @@ -27,10 +36,9 @@ jobs: - uses: actions/checkout@v4 with: clean: false - - name: Check License Header - uses: apache/skywalking-eyes/header@main - name: Ensure clang-format-10 is available run: | + sed -i 's|http://[^ ]*|http://nexus.vesoft-inc.com:8081/repository/ubuntu-2004|g' /etc/apt/sources.list && apt update command -v clang-format-10 > /dev/null || (sudo apt update && sudo apt install -y clang-format-10) - name: Cpplint run: | @@ -43,6 +51,14 @@ jobs: - uses: actions/setup-python@v5 with: python-version: 3.7 + - name: Isolate pip installs to tmp dir + id: pip_prefix + run: | + TMP_DIR=$(mktemp -d) + echo "PYTHONUSERBASE=$TMP_DIR" >> "$GITHUB_ENV" + echo "PATH=$TMP_DIR/bin:$PATH" >> "$GITHUB_ENV" + PY_VER=$(python -c 'import sys; print(f"{sys.version_info[0]}.{sys.version_info[1]}")') + echo "PYTHONPATH=$TMP_DIR/lib/python${PY_VER}/site-packages:$PYTHONPATH" >> "$GITHUB_ENV" - name: Prepare Gherkin exec environ run: make init-all -C tests - name: Check Gherkin feature format diff --git a/src/graph/util/ExpressionUtils.cpp b/src/graph/util/ExpressionUtils.cpp index ca3c68bc412..5a10b1e8d9a 100644 --- a/src/graph/util/ExpressionUtils.cpp +++ b/src/graph/util/ExpressionUtils.cpp @@ -819,65 +819,180 @@ Expression *ExpressionUtils::rewriteRelExpr(const Expression *expr) { return simplifiedExpr; } // Move all evaluable expression to the right side - auto relRightOperandExpr = relExpr->right()->clone(); - auto relLeftOperandExpr = rewriteRelExprHelper(relExpr->left(), relRightOperandExpr); - return RelationalExpression::makeKind( - pool, relExpr->kind(), relLeftOperandExpr->clone(), relRightOperandExpr->clone()); + return rewriteRelExprHelper(pool, relExpr); + // auto relRightOperandExpr = relExpr->right()->clone(); + // auto relLeftOperandExpr = rewriteRelExprHelper(relExpr->left(), relRightOperandExpr); + // return RelationalExpression::makeKind( + // pool, relExpr->kind(), relLeftOperandExpr->clone(), relRightOperandExpr->clone()); }; return RewriteVisitor::transform(expr, matcher, rewriter); } -Expression *ExpressionUtils::rewriteRelExprHelper(const Expression *expr, - Expression *&relRightOperandExpr) { - ObjectPool *pool = expr->getObjPool(); - // TODO: Support rewrite mul/div expression after fixing overflow - auto matcher = [](const Expression *e) -> bool { - if (!e->isArithmeticExpr() || e->kind() == Expression::Kind::kMultiply || - e->kind() == Expression::Kind::kDivision) - return false; - auto arithExpr = static_cast(e); +static std::optional invertRelOp(Expression::Kind kind) { + switch (kind) { + case Expression::Kind::kRelLT: + return Expression::Kind::kRelGT; + case Expression::Kind::kRelLE: + return Expression::Kind::kRelGE; + case Expression::Kind::kRelGT: + return Expression::Kind::kRelLT; + case Expression::Kind::kRelGE: + return Expression::Kind::kRelLE; + case Expression::Kind::kRelEQ: + return Expression::Kind::kRelEQ; + case Expression::Kind::kRelNE: + return Expression::Kind::kRelNE; + default: + // we don't handle this cases + return std::nullopt; + } +} - return ExpressionUtils::isEvaluableExpr(arithExpr->left()) || - ExpressionUtils::isEvaluableExpr(arithExpr->right()); - }; +static Expression *rewriteArith(ObjectPool *pool, const Expression *expr, Expression *&rhs) { + if (!expr->isArithmeticExpr()) { + return const_cast(expr); + } - if (!matcher(expr)) { + auto *arith = static_cast(expr); + auto k = arith->kind(); + // only support add and minus for now + if (k != Expression::Kind::kAdd && k != Expression::Kind::kMinus) { return const_cast(expr); } - auto arithExpr = static_cast(expr); - auto kind = getNegatedArithmeticType(arithExpr->kind()); - auto lexpr = relRightOperandExpr->clone(); - const Expression *root = nullptr; - Expression *rexpr = nullptr; + auto *l = arith->left(); + auto *r = arith->right(); + bool le = ExpressionUtils::isEvaluableExpr(l); + bool re = ExpressionUtils::isEvaluableExpr(r); - // Use left operand as root - if (ExpressionUtils::isEvaluableExpr(arithExpr->right())) { - rexpr = arithExpr->right()->clone(); - root = arithExpr->left(); - } else { - rexpr = arithExpr->left()->clone(); - root = arithExpr->right(); + // finish remove if both sides are not evaluable + if (!le && !re) { + return const_cast(expr); } - switch (kind) { - case Expression::Kind::kAdd: - relRightOperandExpr = ArithmeticExpression::makeAdd(pool, lexpr, rexpr); - break; - case Expression::Kind::kMinus: - relRightOperandExpr = ArithmeticExpression::makeMinus(pool, lexpr, rexpr); - break; - // Unsupported arithmetic kind - // case Expression::Kind::kMultiply: - // case Expression::Kind::kDivision: - default: - DLOG(ERROR) << "Unsupported expression kind: " << static_cast(kind); - break; + + if (k == Expression::Kind::kAdd) { + if (le && !re) { + // swap to make sure r is evaluable + std::swap(l, r); + std::swap(le, re); + } + DCHECK(re && ExpressionUtils::isEvaluableExpr(r)); + // a + C rhs --> a rhs - C + rhs = ArithmeticExpression::makeMinus(pool, rhs->clone(), r->clone()); + // nested rewrite the left operand + return rewriteArith(pool, l, rhs); + } + + if (k == Expression::Kind::kMinus) { + if (re) { + // a - C rhs --> a rhs + C + rhs = ArithmeticExpression::makeAdd(pool, rhs->clone(), r->clone()); + // nested rewrite the left operand + return rewriteArith(pool, l, rhs); + } } - return rewriteRelExprHelper(root, relRightOperandExpr); + return const_cast(expr); } +Expression *ExpressionUtils::rewriteRelExprHelper(ObjectPool *pool, const Expression *expr) { + if (!expr->isRelExpr()) { + return const_cast(expr); + } + + const RelationalExpression *relExpr = static_cast(expr); + auto *lhs = relExpr->left(); + auto *rhs = relExpr->right(); + auto relKind = relExpr->kind(); + + // specially handle C - a rhs case + if (lhs->isArithmeticExpr()) { + auto *arith = static_cast(lhs); + if (arith->kind() == Expression::Kind::kMinus && + ExpressionUtils::isEvaluableExpr(arith->left()) && + !ExpressionUtils::isEvaluableExpr(arith->right())) { + // C - a rhs --> a C - rhs + auto *constantPart = arith->left()->clone(); + auto res = invertRelOp(relKind); + if (!res.has_value()) { + return const_cast(expr); + } + relKind = invertRelOp(relKind).value(); + // let the variable part be negative and move it to the left side of the relational expression + lhs = arith->right(); + rhs = ArithmeticExpression::makeMinus(pool, constantPart, rhs->clone()); + } + } + + // swap the left and right if left is evaluable but right is not + if (ExpressionUtils::isEvaluableExpr(lhs) && !ExpressionUtils::isEvaluableExpr(rhs)) { + std::swap(lhs, rhs); + auto res = invertRelOp(relKind); + if (res.has_value()) { + relKind = res.value(); + } else { + return const_cast(expr); + } + } + + Expression *re = rhs->clone(); + // move evaluable part from left to right, rewrite the right in place and return the new left + Expression *le = rewriteArith(pool, lhs, re); + + return RelationalExpression::makeKind(pool, relKind, le->clone(), re->clone()); +} + +// Expression *ExpressionUtils::rewriteRelExprHelper(const Expression *expr, +// Expression *&relRightOperandExpr) { +// ObjectPool *pool = expr->getObjPool(); +// // TODO: Support rewrite mul/div expression after fixing overflow +// auto matcher = [](const Expression *e) -> bool { +// if (!e->isArithmeticExpr() || e->kind() == Expression::Kind::kMultiply || +// e->kind() == Expression::Kind::kDivision) +// return false; +// auto arithExpr = static_cast(e); + +// return ExpressionUtils::isEvaluableExpr(arithExpr->left()) || +// ExpressionUtils::isEvaluableExpr(arithExpr->right()); +// }; + +// if (!matcher(expr)) { +// return const_cast(expr); +// } + +// auto arithExpr = static_cast(expr); +// auto kind = getNegatedArithmeticType(arithExpr->kind()); +// auto lexpr = relRightOperandExpr->clone(); +// const Expression *root = nullptr; +// Expression *rexpr = nullptr; + +// // Use left operand as root +// if (ExpressionUtils::isEvaluableExpr(arithExpr->right())) { +// rexpr = arithExpr->right()->clone(); +// root = arithExpr->left(); +// } else { +// rexpr = arithExpr->left()->clone(); +// root = arithExpr->right(); +// } +// switch (kind) { +// case Expression::Kind::kAdd: +// relRightOperandExpr = ArithmeticExpression::makeAdd(pool, lexpr, rexpr); +// break; +// case Expression::Kind::kMinus: +// relRightOperandExpr = ArithmeticExpression::makeMinus(pool, lexpr, rexpr); +// break; +// // Unsupported arithmetic kind +// // case Expression::Kind::kMultiply: +// // case Expression::Kind::kDivision: +// default: +// DLOG(ERROR) << "Unsupported expression kind: " << static_cast(kind); +// break; +// } + +// return rewriteRelExprHelper(root, relRightOperandExpr); +// } + StatusOr ExpressionUtils::filterTransform(const Expression *filter) { // Check if any overflow happen before filter transform auto initialConstFold = foldConstantExpr(filter); diff --git a/src/graph/util/ExpressionUtils.h b/src/graph/util/ExpressionUtils.h index c7151b04d5d..81aa4fc6fb2 100644 --- a/src/graph/util/ExpressionUtils.h +++ b/src/graph/util/ExpressionUtils.h @@ -107,7 +107,7 @@ class ExpressionUtils { // Rewrites relational expression, gather all evaluable expressions in the left operands and move // them to the right static Expression* rewriteRelExpr(const Expression* expr); - static Expression* rewriteRelExprHelper(const Expression* expr, Expression*& relRightOperandExpr); + static Expression* rewriteRelExprHelper(ObjectPool* pool, const Expression* expr); // Rewrites IN expression into OR expression or relEQ expression static Expression* rewriteInExpr(const Expression* expr); diff --git a/src/graph/util/test/ExpressionUtilsTest.cpp b/src/graph/util/test/ExpressionUtilsTest.cpp index e376c48e9de..fd7fc8afd5a 100644 --- a/src/graph/util/test/ExpressionUtilsTest.cpp +++ b/src/graph/util/test/ExpressionUtilsTest.cpp @@ -827,5 +827,62 @@ TEST_F(ExpressionUtilsTest, simplifyLogicalExpr) { } } +TEST_F(ExpressionUtilsTest, rewriteRelExpr) { + auto e0 = parse("v.age +3 < 7"); + auto e1 = parse("v.age < 7-3"); + auto eGot = ExpressionUtils::rewriteRelExpr(e0); + ASSERT_EQ(eGot->toString(), e1->toString()) << eGot->toString(); + + e0 = parse("v.age - 3 < 7"); + e1 = parse("v.age < 7+3"); + eGot = ExpressionUtils::rewriteRelExpr(e0); + ASSERT_EQ(eGot->toString(), e1->toString()) << eGot->toString(); + + e0 = parse("v.age - 3 == 7"); + e1 = parse("v.age == 7+3"); + eGot = ExpressionUtils::rewriteRelExpr(e0); + ASSERT_EQ(eGot->toString(), e1->toString()) << eGot->toString(); + + e0 = parse("3-v.age >= 7"); + e1 = parse("v.age <= 3-7"); + eGot = ExpressionUtils::rewriteRelExpr(e0); + ASSERT_EQ(eGot->toString(), e1->toString()) << eGot->toString(); + + e0 = parse("v.age - 3 == 7"); + e1 = parse("v.age == 7+3"); + eGot = ExpressionUtils::rewriteRelExpr(e0); + ASSERT_EQ(eGot->toString(), e1->toString()) << eGot->toString(); + + e0 = parse("3 + v.age <= 7"); + e1 = parse("v.age <= 7-3"); + eGot = ExpressionUtils::rewriteRelExpr(e0); + ASSERT_EQ(eGot->toString(), e1->toString()) << eGot->toString(); + + e0 = parse("a > 5"); + e1 = parse("a > 5"); + eGot = ExpressionUtils::rewriteRelExpr(e0); + ASSERT_EQ(eGot->toString(), e1->toString()) << eGot->toString(); + + e0 = parse("((v.age + 1) - 2) > 1"); + e1 = parse("v.age > (1+2)-1"); + eGot = ExpressionUtils::rewriteRelExpr(e0); + ASSERT_EQ(eGot->toString(), e1->toString()) << eGot->toString(); + + e0 = parse("(v.age + v.player.height) + 5 < 15"); + e1 = parse("v.age+v.player.height < 15-5"); + eGot = ExpressionUtils::rewriteRelExpr(e0); + ASSERT_EQ(eGot->toString(), e1->toString()) << eGot->toString(); + + e0 = parse("v.age * 2 < 10"); + e1 = parse("v.age*2 < 10"); + eGot = ExpressionUtils::rewriteRelExpr(e0); + ASSERT_EQ(eGot->toString(), e1->toString()) << eGot->toString(); + + e0 = parse("x != y"); + e1 = parse("x!=y"); + eGot = ExpressionUtils::rewriteRelExpr(e0); + ASSERT_EQ(eGot->toString(), e1->toString()) << eGot->toString(); +} + } // namespace graph } // namespace nebula