ExtractFilterExprVisitor #3462
ExtractFilterExprVisitor #3462
Conversation
| checkEqual(expr, expect, remain, false); | ||
| } | ||
| { | ||
| // A1 or (A2 and B1) or (A3 and B2) or A4 |
There was a problem hiding this comment.
it seems that there're some parts of this expression could be pushed:
A1 or (A2 and B1) or (A3 and B2) or A4
=>
A1 or A4 or (A2 and B1) or (A3 and B2)
=>
((A1 or A4 or A2) and (A1 or A4 or B1)) or (A3 and B2)
=>
(((A1 or A4 or A2) and (A1 or A4 or B1)) or A3) and (((A1 or A4 or A2) and (A1 or A4 or B1)) or B2)
=>
((A1 or A4 or A2 or A3) and (A1 or A4 or B1 or A3)) and ((A1 or A4 or A2 or B2) and (A1 or A4 or B1 or B2))
=>
(A1 or A4 or A2 or A3) and (A1 or A4 or B1 or A3) and (A1 or A4 or A2 or B2) and (A1 or A4 or B1 or B2)
=>
can be pushed: (A1 or A4 or A2 or A3)
remain: (A1 or A4 or B1 or A3) and (A1 or A4 or A2 or B2) and (A1 or A4 or B1 or B2)
There was a problem hiding this comment.
remained expr could be simplified: (A1 or A4) or ((B1 or A3) and (A2 or B2) and (B1 or B2))
There was a problem hiding this comment.
It is not easy to implement this. In this pr, for simplification, LogicalOr can be split, if and only if just one operand can not be pushed and that operand is LogicalAnd. Maybe this improvement can be implemented in future version.
There was a problem hiding this comment.
More test cases for different scenarios are necessary to verify the correctness of this pr. However, I can't think of more.
| @@ -79,6 +79,15 @@ class LogicalExpression final : public Expression { | |||
|
|
|||
| bool isLogicalExpr() const override { return true; } | |||
|
|
|||
| void setLogicalKind(Kind kind) { | |||
| if ((kind == Expression::Kind::kLogicalOr || kind == Expression::Kind::kLogicalAnd) && | |||
| (kind_ == Expression::Kind::kLogicalOr || kind_ == Expression::Kind::kLogicalAnd)) { | |||
There was a problem hiding this comment.
following check maybe better to understand:
(kind == Expression::Kind::kLogicalOr && kind_ == Expression::Kind::kLogicalAnd) ||
(kind == Expression::Kind::kLogicalAnd && kind_ == Expression::Kind::kLogicalOr)There was a problem hiding this comment.
and this function could be renamed: reverseLogicalKind()
There was a problem hiding this comment.
void reverseLogicalKind() {
if (kind_ == kLogicalAnd) {
kind_ = kLogicalOr;
} else if (kind_ == kLogicalOr) {
kind_ = kLogicalAnd;
} else {
LOG(FATAL) << "Should not reverser logical expression except and/or kind.";
}
}
Codecov Report
@@ Coverage Diff @@
## master #3462 +/- ##
==========================================
+ Coverage 85.19% 85.23% +0.03%
==========================================
Files 1304 1306 +2
Lines 121308 122472 +1164
==========================================
+ Hits 103352 104386 +1034
- Misses 17956 18086 +130
Continue to review full report at Codecov.
|
yixinglu
requested review from
Shylock-Hg,
CPWstatic,
jievince,
Aiee and
critical27
| if (canBePushExprs.empty() || canNotPushExprs.empty()) { | ||
| LOG(ERROR) << "Both can be pushed expr and can not push expr should be existed, but not!"; | ||
| canBePushed_ = false; | ||
| return; | ||
| } |
There was a problem hiding this comment.
DCHECK(!canBePushExprs.empty());
DCHECK(!canNotPushExprs.empty());
| Expression *ExtractFilterExprVisitor::rewriteExpr(std::vector<Expression *> &rel, | ||
| std::vector<Expression *> &sharedExprs) { | ||
| Expression *newAndExpr; | ||
| if (rel.size() > 1) { |
| } else if (expr->kind() == Expression::Kind::kLogicalOr) { | ||
| bool isSplit = visitLogicalOr(expr); | ||
| if (isSplit) { | ||
| DCHECK_EQ(expr->operands().size(), 2); |
There was a problem hiding this comment.
DCHECK_EQ(expr->kind(), Expression::Kind::kLogicalAnd);
| @@ -156,6 +200,152 @@ void ExtractFilterExprVisitor::visit(LogicalExpression *expr) { | |||
| } | |||
| } | |||
|
|
|||
| // @retrun: split or not | |||
| sharedExprs.emplace_back(newAndExpr); | ||
| auto newOrExpr = ExpressionUtils::pushOrs(pool_, sharedExprs); | ||
| ExpressionUtils::pullOrs(newOrExpr); | ||
| sharedExprs.pop_back(); |
There was a problem hiding this comment.
How about deleting this line of code and changing the input parameter type？
| Expression *ExtractFilterExprVisitor::rewriteExpr(std::vector<Expression *> &rel, | ||
| std::vector<Expression *> &sharedExprs) { |
There was a problem hiding this comment.
How about this?
Expression *ExtractFilterExprVisitor::rewriteExpr(std::vector<Expression *> rel,
std::vector<Expression *> sharedExprs)There was a problem hiding this comment.
That's ok, I will modify it later.
| return false; | ||
| } | ||
|
|
||
| void ExtractFilterExprVisitor::ExtractRemainExpr(LogicalExpression *expr, |
There was a problem hiding this comment.
DCHECK_EQ(expr->kind(), Expression::Kind::kLogicalAnd);
| return; | ||
| // @return: whether this logical expr satisfies split condition | ||
| bool ExtractFilterExprVisitor::visitLogicalAnd(LogicalExpression *expr, std::vector<bool> &flags) { | ||
| // if the size of operands greater than 2 |
There was a problem hiding this comment.
DCHECK_EQ(expr->kind(), Expression::Kind::kLogicalAnd);
| @@ -156,6 +200,152 @@ void ExtractFilterExprVisitor::visit(LogicalExpression *expr) { | |||
| } | |||
| } | |||
|
|
|||
| // @retrun: split or not | |||
| bool ExtractFilterExprVisitor::visitLogicalOr(LogicalExpression *expr) { | |||
| if (expr->operands().size() <= 2) { | |||
There was a problem hiding this comment.
DCHECK_EQ(expr->kind(), Expression::Kind::kLogicalOr);
|
That's ok |
|
Hi @yixinglu , when I rebase the latest commit in my branch and run cmake .. It complains that |
|
I have checked that we haven't compiled the third party for your version 😂,, you can fix it by install gcc 9.3.0 with |
|
Thx. Since you have merged this pr, it seems that I do not need to push anymore. |
Hi @yixinglu , when I compile the code, it complains that |
You need upgrade third-party to 3.0 |
Hi @Shylock-Hg , I fail to upgrade third-party to 3.0. It complains that |
What type of PR is this?
What does this PR do?
fix #3372
Which issue(s)/PR(s) this PR relates to?
#3372
Special notes for your reviewer, ex. impact of this fix, etc:
Additional context:
Checklist：
Release notes：
Please confirm whether to reflect in release notes and how to describe: