Fix: fix for heuristic rule wrapper (#147)

The new expr returned by heuristic rules are not in the original group,
which means it never got an OptimizeExpressionTask with exploring as
false (OptimizeExpressionTask with exploring=False only be called in
OptimizeGroup), it should evoke an OptimizeExpressionTask with
exploring=false for itself to apply all the transform rules and
implementation rules for itself.

Besides, the pr adds checks for whether the original expr equals to the
output expr for heuristic rule. If that's the case, it should prompt an
error as this breaks the heuristic rule's definition. (Silently
accepting it will mark the input expr being as a dead end and there's no
more new exprs to replace it)

Signed-off-by: AveryQi115 <[email protected]>

Author: AveryQi115

optd-core/src/cascades/memo.rs

@@ -285,13 +285,19 @@ impl<T: RelNodeTyp> Memo<T> {
             };
 
             // if the new expr already in the memo table, merge the group and remove old expr
-            if let Some(&expr_id) = self.expr_node_to_expr_id.get(&memo_node) {
-                let group_id = self.get_group_id_of_expr_id(expr_id);
+            if let Some(&new_expr_id) = self.expr_node_to_expr_id.get(&memo_node) {
+                if new_expr_id == expr_id {
+                    // This is not acceptable, as it means the expr returned by a heuristic rule is exactly
+                    // the same as the original expr, which should not happen
+                    // TODO: we can silently ignore this case without marking the original one as a deadend
+                    // But the rule creators should follow the definition of the heuristic rule
+                    // and return an empty vec if their rule does not do the real transformation
+                    unreachable!("replace_group_expr: you're replacing the old expr with the same expr, please check your rules registered as heuristic
+                        and make sure if it does not do any transformation, it should return an empty vec!");
+                }
+                let group_id = self.get_group_id_of_expr_id(new_expr_id);
                 let group_id = self.get_reduced_group_id(group_id);
                 self.merge_group_inner(replace_group_id, group_id);
-
-                // TODO: instead of remove this expr from the old group,
-                // we mark the expr as all rules have been fired to make it a dead end
                 return false;
             }
 

optd-core/src/cascades/tasks/apply_rule.rs

@@ -233,11 +233,11 @@ impl<T: RelNodeTyp> Task<T> for ApplyRuleTask {
 
                     trace!(event = "apply_rule replace", expr_id = %self.expr_id, rule_id = %self.rule_id);
 
-                    // rules registed as heuristics are always logical, exploring its children
-                    tasks.push(
-                        Box::new(OptimizeExpressionTask::new(self.expr_id, self.exploring))
-                            as Box<dyn Task<T>>,
-                    );
+                    // the expr returned by heuristic rule is a brand new one
+                    // so there's no optimizeExpressionTask for it in the original task list
+                    // we should set exploring as false to both envoke tranform rule and impl rule for it
+                    tasks.push(Box::new(OptimizeExpressionTask::new(self.expr_id, false))
+                        as Box<dyn Task<T>>);
                 }
                 continue;
             }