diff --git a/src/planner/planners/MatchVertexIndexSeekPlanner.cpp b/src/planner/planners/MatchVertexIndexSeekPlanner.cpp index e76706f3834..efa850323c7 100644 --- a/src/planner/planners/MatchVertexIndexSeekPlanner.cpp +++ b/src/planner/planners/MatchVertexIndexSeekPlanner.cpp @@ -66,7 +66,7 @@ Expression* MatchVertexIndexSeekPlanner::makeIndexFilter(const std::string &labe Expression* MatchVertexIndexSeekPlanner::makeIndexFilter(const std::string &label, const std::string &alias, - const Expression *filter, + Expression *filter, QueryContext* qctx) { static const std::unordered_set kinds = { Expression::Kind::kRelEQ, @@ -81,7 +81,11 @@ Expression* MatchVertexIndexSeekPlanner::makeIndexFilter(const std::string &labe if (kinds.count(kind) == 1) { ands.emplace_back(filter); } else if (kind == Expression::Kind::kLogicalAnd) { - ands = ExpressionUtils::pullAnds(filter); + auto *logic = static_cast(filter); + ExpressionUtils::pullAnds(logic); + for (auto &operand : logic->operands()) { + ands.emplace_back(operand.get()); + } } else { return nullptr; } diff --git a/src/planner/planners/MatchVertexIndexSeekPlanner.h b/src/planner/planners/MatchVertexIndexSeekPlanner.h index b9006690a99..cef6278b7e1 100644 --- a/src/planner/planners/MatchVertexIndexSeekPlanner.h +++ b/src/planner/planners/MatchVertexIndexSeekPlanner.h @@ -34,7 +34,7 @@ class MatchVertexIndexSeekPlanner final : public Planner { static Expression* makeIndexFilter(const std::string& label, const std::string& alias, - const Expression* filter, + Expression* filter, QueryContext* qctx); MatchVertexIndexSeekPlanner() = default; diff --git a/src/util/ExpressionUtils.cpp b/src/util/ExpressionUtils.cpp index 82db2976731..ad8cb9dc432 100644 --- a/src/util/ExpressionUtils.cpp +++ b/src/util/ExpressionUtils.cpp @@ -21,50 +21,46 @@ std::unique_ptr ExpressionUtils::foldConstantExpr(const Expression * return newExpr; } -std::vector ExpressionUtils::pullAnds(const Expression *expr) { +Expression* ExpressionUtils::pullAnds(Expression *expr) { DCHECK(expr->kind() == Expression::Kind::kLogicalAnd); - auto *root = static_cast(expr); - std::vector operands; - - // TODO(dutor) Deal with n-ary operands - if (root->operand(0)->kind() != Expression::Kind::kLogicalAnd) { - operands.emplace_back(root->operand(0)); - } else { - auto ands = pullAnds(root->operand(0)); - operands.insert(operands.end(), ands.begin(), ands.end()); - } - - if (root->operand(1)->kind() != Expression::Kind::kLogicalAnd) { - operands.emplace_back(root->operand(1)); - } else { - auto ands = pullAnds(root->operand(1)); - operands.insert(operands.end(), ands.begin(), ands.end()); - } - - return operands; + auto *logic = static_cast(expr); + std::vector> operands; + pullAndsImpl(logic, operands); + logic->setOperands(std::move(operands)); + return logic; } -std::vector ExpressionUtils::pullOrs(const Expression *expr) { +Expression* ExpressionUtils::pullOrs(Expression *expr) { DCHECK(expr->kind() == Expression::Kind::kLogicalOr); - auto *root = static_cast(expr); - std::vector operands; + auto *logic = static_cast(expr); + std::vector> operands; + pullOrsImpl(logic, operands); + logic->setOperands(std::move(operands)); + return logic; +} - // TODO(dutor) Deal with n-ary operands - if (root->operand(0)->kind() != Expression::Kind::kLogicalOr) { - operands.emplace_back(root->operand(0)); - } else { - auto ands = pullOrs(root->operand(0)); - operands.insert(operands.end(), ands.begin(), ands.end()); +void +ExpressionUtils::pullAndsImpl(LogicalExpression *expr, + std::vector> &operands) { + for (auto &operand : expr->operands()) { + if (operand->kind() != Expression::Kind::kLogicalAnd) { + operands.emplace_back(std::move(operand)); + continue; + } + pullAndsImpl(static_cast(operand.get()), operands); } +} - if (root->operand(1)->kind() != Expression::Kind::kLogicalOr) { - operands.emplace_back(root->operand(1)); - } else { - auto ands = pullOrs(root->operand(1)); - operands.insert(operands.end(), ands.begin(), ands.end()); +void +ExpressionUtils::pullOrsImpl(LogicalExpression *expr, + std::vector> &operands) { + for (auto &operand : expr->operands()) { + if (operand->kind() != Expression::Kind::kLogicalOr) { + operands.emplace_back(std::move(operand)); + continue; + } + pullOrsImpl(static_cast(operand.get()), operands); } - - return operands; } } // namespace graph diff --git a/src/util/ExpressionUtils.h b/src/util/ExpressionUtils.h index 2ab87269851..29d9603a6cf 100644 --- a/src/util/ExpressionUtils.h +++ b/src/util/ExpressionUtils.h @@ -110,9 +110,13 @@ class ExpressionUtils { // Clone and fold constant expression static std::unique_ptr foldConstantExpr(const Expression* expr); - static std::vector pullAnds(const Expression *expr); + static Expression* pullAnds(Expression *expr); + static void pullAndsImpl(LogicalExpression *expr, + std::vector> &operands); - static std::vector pullOrs(const Expression *expr); + static Expression* pullOrs(Expression *expr); + static void pullOrsImpl(LogicalExpression *expr, + std::vector> &operands); }; } // namespace graph diff --git a/src/util/test/ExpressionUtilsTest.cpp b/src/util/test/ExpressionUtilsTest.cpp index ba082c63123..72949309654 100644 --- a/src/util/test/ExpressionUtilsTest.cpp +++ b/src/util/test/ExpressionUtilsTest.cpp @@ -210,10 +210,11 @@ TEST_F(ExpressionUtilsTest, PullAnds) { auto *first = new ConstantExpression(true); auto *second = new ConstantExpression(false); LogicalExpression expr(Kind::kLogicalAnd, first, second); - auto ands = ExpressionUtils::pullAnds(&expr); - ASSERT_EQ(2UL, ands.size()); - ASSERT_EQ(first, ands[0]); - ASSERT_EQ(second, ands[1]); + LogicalExpression expected(Kind::kLogicalAnd, + first->clone().release(), + second->clone().release()); + ExpressionUtils::pullAnds(&expr); + ASSERT_EQ(expected, expr); } // true AND false AND true { @@ -221,12 +222,16 @@ TEST_F(ExpressionUtilsTest, PullAnds) { auto *second = new ConstantExpression(false); auto *third = new ConstantExpression(true); LogicalExpression expr(Kind::kLogicalAnd, - new LogicalExpression(Kind::kLogicalAnd, first, second), third); - auto ands = ExpressionUtils::pullAnds(&expr); - ASSERT_EQ(3UL, ands.size()); - ASSERT_EQ(first, ands[0]); - ASSERT_EQ(second, ands[1]); - ASSERT_EQ(third, ands[2]); + new LogicalExpression(Kind::kLogicalAnd, + first, + second), + third); + LogicalExpression expected(Kind::kLogicalAnd); + expected.addOperand(first->clone().release()); + expected.addOperand(second->clone().release()); + expected.addOperand(third->clone().release()); + ExpressionUtils::pullAnds(&expr); + ASSERT_EQ(expected, expr); } // true AND (false AND true) { @@ -235,12 +240,15 @@ TEST_F(ExpressionUtilsTest, PullAnds) { auto *third = new ConstantExpression(true); LogicalExpression expr(Kind::kLogicalAnd, first, - new LogicalExpression(Kind::kLogicalAnd, second, third)); - auto ands = ExpressionUtils::pullAnds(&expr); - ASSERT_EQ(3UL, ands.size()); - ASSERT_EQ(first, ands[0]); - ASSERT_EQ(second, ands[1]); - ASSERT_EQ(third, ands[2]); + new LogicalExpression(Kind::kLogicalAnd, + second, + third)); + LogicalExpression expected(Kind::kLogicalAnd); + expected.addOperand(first->clone().release()); + expected.addOperand(second->clone().release()); + expected.addOperand(third->clone().release()); + ExpressionUtils::pullAnds(&expr); + ASSERT_EQ(expected, expr); } // (true OR false) AND (true OR false) { @@ -251,10 +259,11 @@ TEST_F(ExpressionUtilsTest, PullAnds) { new ConstantExpression(true), new ConstantExpression(false)); LogicalExpression expr(Kind::kLogicalAnd, first, second); - auto ands = ExpressionUtils::pullAnds(&expr); - ASSERT_EQ(2UL, ands.size()); - ASSERT_EQ(first, ands[0]); - ASSERT_EQ(second, ands[1]); + LogicalExpression expected(Kind::kLogicalAnd); + expected.addOperand(first->clone().release()); + expected.addOperand(second->clone().release()); + ExpressionUtils::pullAnds(&expr); + ASSERT_EQ(expected, expr); } // true AND ((false AND true) OR false) AND true { @@ -267,11 +276,12 @@ TEST_F(ExpressionUtilsTest, PullAnds) { auto *third = new ConstantExpression(true); LogicalExpression expr(Kind::kLogicalAnd, new LogicalExpression(Kind::kLogicalAnd, first, second), third); - auto ands = ExpressionUtils::pullAnds(&expr); - ASSERT_EQ(3UL, ands.size()); - ASSERT_EQ(first, ands[0]); - ASSERT_EQ(second, ands[1]); - ASSERT_EQ(third, ands[2]); + LogicalExpression expected(Kind::kLogicalAnd); + expected.addOperand(first->clone().release()); + expected.addOperand(second->clone().release()); + expected.addOperand(third->clone().release()); + ExpressionUtils::pullAnds(&expr); + ASSERT_EQ(expected, expr); } } @@ -282,10 +292,11 @@ TEST_F(ExpressionUtilsTest, PullOrs) { auto *first = new ConstantExpression(true); auto *second = new ConstantExpression(false); LogicalExpression expr(Kind::kLogicalOr, first, second); - auto ors = ExpressionUtils::pullOrs(&expr); - ASSERT_EQ(2UL, ors.size()); - ASSERT_EQ(first, ors[0]); - ASSERT_EQ(second, ors[1]); + LogicalExpression expected(Kind::kLogicalOr, + first->clone().release(), + second->clone().release()); + ExpressionUtils::pullOrs(&expr); + ASSERT_EQ(expected, expr); } // true OR false OR true { @@ -294,11 +305,12 @@ TEST_F(ExpressionUtilsTest, PullOrs) { auto *third = new ConstantExpression(true); LogicalExpression expr(Kind::kLogicalOr, new LogicalExpression(Kind::kLogicalOr, first, second), third); - auto ors = ExpressionUtils::pullOrs(&expr); - ASSERT_EQ(3UL, ors.size()); - ASSERT_EQ(first, ors[0]); - ASSERT_EQ(second, ors[1]); - ASSERT_EQ(third, ors[2]); + LogicalExpression expected(Kind::kLogicalOr); + expected.addOperand(first->clone().release()); + expected.addOperand(second->clone().release()); + expected.addOperand(third->clone().release()); + ExpressionUtils::pullOrs(&expr); + ASSERT_EQ(expected, expr); } // true OR (false OR true) { @@ -308,11 +320,12 @@ TEST_F(ExpressionUtilsTest, PullOrs) { LogicalExpression expr(Kind::kLogicalOr, first, new LogicalExpression(Kind::kLogicalOr, second, third)); - auto ors = ExpressionUtils::pullOrs(&expr); - ASSERT_EQ(3UL, ors.size()); - ASSERT_EQ(first, ors[0]); - ASSERT_EQ(second, ors[1]); - ASSERT_EQ(third, ors[2]); + LogicalExpression expected(Kind::kLogicalOr); + expected.addOperand(first->clone().release()); + expected.addOperand(second->clone().release()); + expected.addOperand(third->clone().release()); + ExpressionUtils::pullOrs(&expr); + ASSERT_EQ(expected, expr); } // (true AND false) OR (true AND false) { @@ -323,10 +336,11 @@ TEST_F(ExpressionUtilsTest, PullOrs) { new ConstantExpression(true), new ConstantExpression(false)); LogicalExpression expr(Kind::kLogicalOr, first, second); - auto ors = ExpressionUtils::pullOrs(&expr); - ASSERT_EQ(2UL, ors.size()); - ASSERT_EQ(first, ors[0]); - ASSERT_EQ(second, ors[1]); + LogicalExpression expected(Kind::kLogicalOr, + first->clone().release(), + second->clone().release()); + ExpressionUtils::pullOrs(&expr); + ASSERT_EQ(expected, expr); } // true OR ((false OR true) AND false) OR true { @@ -339,11 +353,12 @@ TEST_F(ExpressionUtilsTest, PullOrs) { auto *third = new ConstantExpression(true); LogicalExpression expr(Kind::kLogicalOr, new LogicalExpression(Kind::kLogicalOr, first, second), third); - auto ors = ExpressionUtils::pullOrs(&expr); - ASSERT_EQ(3UL, ors.size()); - ASSERT_EQ(first, ors[0]); - ASSERT_EQ(second, ors[1]); - ASSERT_EQ(third, ors[2]); + LogicalExpression expected(Kind::kLogicalOr); + expected.addOperand(first->clone().release()); + expected.addOperand(second->clone().release()); + expected.addOperand(third->clone().release()); + ExpressionUtils::pullOrs(&expr); + ASSERT_EQ(expected, expr); } }