feat(condition): add column_self_eq_eliminate for condition simplification
#15901
Conversation
| @@ -849,6 +849,7 @@ impl Condition { | |||
| .into_iter() | |||
| .map(push_down_not) | |||
| .map(fold_boolean_constant) | |||
| .map(column_self_eq_eliminate) | |||
There was a problem hiding this comment.
the reason I put this optimization here is because it's hard to catch new variants of predicate(s) after each predicate_pushdown in logical_optimization, and the predicate in ch_benchmark can't be optimized though I've tried to insert as many LOGICAL_FILTER_EXPRESSION_SIMPLIFY as possible.
thus it's better to solve this problem from the root - from my perspective.
| || func_type == ExprType::IsNotFalse | ||
| } | ||
|
|
||
| pub fn rewrite(expr: ExprImpl) -> ExprImpl { |
There was a problem hiding this comment.
and why bother to separate the logic from ExpressionSimplifyRewriter in logical_filter_expression_simplify_rule?
- the former mainly aims to optimize pattern like, e.g.,
(e) [or / and] not(e). - it's hard to maintain the module with two different logic interleaving. (and presumably more in the future)
- this one is especially to simplify
Conditionat constructing time, while the former will be used duringlogical_optimization.
| @@ -612,6 +612,7 @@ impl LogicalOptimizer { | |||
| // Must push down predicates again after split over window so that OverWindow can be | |||
| // optimized to TopN. | |||
| plan = Self::predicate_pushdown(plan, explain_trace, &ctx); | |||
| plan = plan.optimize_by_rules(&LOGICAL_FILTER_EXPRESSION_SIMPLIFY); | |||
There was a problem hiding this comment.
.map(column_self_eq_eliminate)
If we have added column_self_eq_eliminate to the filter simplify method, do we need to add this rule here?
There was a problem hiding this comment.
I forget to delete it 🤣
github-merge-queue
bot
removed this pull request from the merge queue due to a conflict with the base branch
I hereby agree to the terms of the RisingWave Labs, Inc. Contributor License Agreement.
What's changed and what's your intention?
close #12931.
Checklist
./risedev check(or alias,./risedev c)Documentation
Release note
If this PR includes changes that directly affect users or other significant modifications relevant to the community, kindly draft a release note to provide a concise summary of these changes. Please prioritize highlighting the impact these changes will have on users.