morrySnow commented on code in PR #49774:
URL: https://github.com/apache/doris/pull/49774#discussion_r2086141562
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/expression/rules/ExtractCommonFactorRule.java:
##########
@@ -87,17 +88,16 @@ private static Expression
extractCommonFactor(CompoundPredicate originExpr) {
// combine and delete some boolean literal predicate
// e.g. (a and true) -> true
Review Comment:
this comment is wrong? a and true -> a?
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/expression/rules/ExtractCommonFactorRule.java:
##########
@@ -144,22 +138,6 @@ private static Expression
extractCommonFactors(CompoundPredicate originPredicate
LinkedHashMap<Set<Integer>, Set<Expression>> commonFactorPartitions
= partitionByMostCommonFactors(commonFactorToPartIds);
- int extractedExpressionNum = 0;
- for (Set<Expression> exprs : commonFactorPartitions.values()) {
- extractedExpressionNum += exprs.size();
- }
-
- // no any common factor
- if
(commonFactorPartitions.entrySet().iterator().next().getKey().size() <= 1
- && !(originPredicate.getWidth() >
leftDeapTreePredicate.getWidth())
- && originExpressionNum <= extractedExpressionNum) {
- // this condition is important because it can avoid deap loop:
- // origin originExpr: A = 1 and (B > 0 and B < 10)
- // after ExtractCommonFactorRule: (A = 1 and B > 0) and (B < 10)
(left deap tree)
- // after SimplifyRange: A = 1 and (B > 0 and B < 10)
(right deap tree)
- return originPredicate;
- }
-
// now we can do extract common factors for each part:
Review Comment:
how can we infer extractedExprs from originPredicate, initPartitions and
commonFactorPartitions
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/expression/rules/ExtractCommonFactorRule.java:
##########
@@ -144,22 +138,6 @@ private static Expression
extractCommonFactors(CompoundPredicate originPredicate
LinkedHashMap<Set<Integer>, Set<Expression>> commonFactorPartitions
= partitionByMostCommonFactors(commonFactorToPartIds);
- int extractedExpressionNum = 0;
- for (Set<Expression> exprs : commonFactorPartitions.values()) {
- extractedExpressionNum += exprs.size();
- }
-
- // no any common factor
- if
(commonFactorPartitions.entrySet().iterator().next().getKey().size() <= 1
- && !(originPredicate.getWidth() >
leftDeapTreePredicate.getWidth())
- && originExpressionNum <= extractedExpressionNum) {
- // this condition is important because it can avoid deap loop:
- // origin originExpr: A = 1 and (B > 0 and B < 10)
- // after ExtractCommonFactorRule: (A = 1 and B > 0) and (B < 10)
(left deap tree)
- // after SimplifyRange: A = 1 and (B > 0 and B < 10)
(right deap tree)
- return originPredicate;
- }
-
// now we can do extract common factors for each part:
// originPredicate: (a and (b and c)) and (b or c)
// leftDeapTreePredicate: ((a and b) and c) and (b or c)
Review Comment:
this comment should update?
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/expression/rules/ExtractCommonFactorRule.java:
##########
@@ -144,22 +138,6 @@ private static Expression
extractCommonFactors(CompoundPredicate originPredicate
LinkedHashMap<Set<Integer>, Set<Expression>> commonFactorPartitions
= partitionByMostCommonFactors(commonFactorToPartIds);
- int extractedExpressionNum = 0;
- for (Set<Expression> exprs : commonFactorPartitions.values()) {
- extractedExpressionNum += exprs.size();
- }
-
- // no any common factor
- if
(commonFactorPartitions.entrySet().iterator().next().getKey().size() <= 1
- && !(originPredicate.getWidth() >
leftDeapTreePredicate.getWidth())
- && originExpressionNum <= extractedExpressionNum) {
- // this condition is important because it can avoid deap loop:
- // origin originExpr: A = 1 and (B > 0 and B < 10)
- // after ExtractCommonFactorRule: (A = 1 and B > 0) and (B < 10)
(left deap tree)
- // after SimplifyRange: A = 1 and (B > 0 and B < 10)
(right deap tree)
- return originPredicate;
- }
-
// now we can do extract common factors for each part:
Review Comment:
the commonFactorPartitions in this comment is different in upper comment,
according to upper comment, the commonFactorPartitions should be
```
{[1, 3]: [b], [2]: [c], [0]: [a]}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
