🐛 Add missing statement and refactor

 #51

src/lib/optimizer/optimizer.cpp

@@ -118,7 +118,7 @@ std::shared_ptr<Optimizer> Optimizer::create_default_optimizer() {
 
   optimizer->add_rule(std::make_unique<IndexScanRule>());
 
-  optimizer->add_rule(std::make_unique<PredicateMergeRule>());
+  optimizer->add_rule(std::make_unique<PredicateMergeRule>(4));
 
   return optimizer;
 }

src/lib/optimizer/strategy/predicate_merge_rule.cpp

@@ -5,14 +5,51 @@
 #include "logical_query_plan/lqp_utils.hpp"
 #include "logical_query_plan/union_node.hpp"
 
+using namespace opossum::expression_functional;  // NOLINT
+
 namespace opossum {
 
+PredicateMergeRule::PredicateMergeRule(const size_t optimization_threshold) :
+    _optimization_threshold(optimization_threshold) {}
+
+void PredicateMergeRule::apply_to(const std::shared_ptr<AbstractLQPNode>& node) const {
+  if (!node) {
+    return;
+  }
+
+  std::set<std::shared_ptr<AbstractLQPNode>> break_nodes;
+  size_t lqp_complexity = 0;
+  visit_lqp(node, [&](const auto& sub_node) {
+    switch (sub_node->type) {
+      case LQPNodeType::Predicate:
+        return LQPVisitation::VisitInputs;
+
+      case LQPNodeType::Union:
+        lqp_complexity++;
+        return LQPVisitation::VisitInputs;
+
+      default:
+        break_nodes.insert(sub_node);
+        return LQPVisitation::DoNotVisitInputs;
+    }
+  });
+
+  for (const auto& next_node : break_nodes) {
+    apply_to(next_node->left_input());
+    apply_to(next_node->right_input());
+  }
+
+  // Simple heuristic: The PredicateMergeRule is more likely to improve the performance for complex LQPs with many
+  // UNIONs. TODO(jj): Insert issue reference to find better heuristic
+  if (lqp_complexity >= _optimization_threshold) {
+    merge_subplan(node, std::nullopt);
+  }
+}
+
 /**
- * Function creates a boolean expression from an lqp. It traverses the passed lqp from top to bottom. However, an lqp is
- * evaluated from bottom to top. This requires that the order in which the translated expressions are added to the
- * output expression is the reverse order of how the nodes are traversed. The subsequent_expression parameter passes the
- * translated expressions to the translation of its children nodes, which enables to add the translated expression of
- * child node before its parent node to the output expression.
+ * Merge an LQP that only consists of PredicateNodes and UnionNodes into a single PredicateNode. The
+ * subsequent_expression parameter passes the translated expressions to the translation of its children nodes, which
+ * enables to add the translated expression of child node before its parent node to the output expression.
  */
 std::shared_ptr<AbstractExpression> PredicateMergeRule::merge_subplan(
     const std::shared_ptr<AbstractLQPNode>& begin,
@@ -23,7 +60,7 @@ std::shared_ptr<AbstractExpression> PredicateMergeRule::merge_subplan(
       auto expression = predicate_node->predicate();
       // todo(jj): Add comment
       if (subsequent_expression && begin->output_count() == 1) {
-        expression_functional::and_(expression, *subsequent_expression);
+        expression = and_(expression, *subsequent_expression);
       }
       const auto left_input_expression = merge_subplan(begin->left_input(), expression);
       if (begin->left_input()->output_count() == 1 && left_input_expression) {
@@ -44,9 +81,9 @@ std::shared_ptr<AbstractExpression> PredicateMergeRule::merge_subplan(
       const auto right_input_expression = merge_subplan(begin->right_input(), std::nullopt);
       if (left_input_expression && right_input_expression) {
         // todo(jj): Add comment
-        auto expression = expression_functional::or_(left_input_expression, right_input_expression);
+        auto expression = or_(left_input_expression, right_input_expression);
         if (subsequent_expression && begin->output_count() == 1) {
-          expression_functional::and_(expression, *subsequent_expression);
+          expression = and_(expression, *subsequent_expression);
         }
         lqp_remove_node(begin->left_input());
         lqp_remove_node(begin->right_input());
@@ -68,38 +105,4 @@ std::shared_ptr<AbstractExpression> PredicateMergeRule::merge_subplan(
   }
 }
 
-void PredicateMergeRule::apply_to(const std::shared_ptr<AbstractLQPNode>& node) const {
-  if (!node) {
-    return;
-  }
-
-  std::set<std::shared_ptr<AbstractLQPNode>> break_nodes;
-  auto lqp_complexity = 0;
-  visit_lqp(node, [&](const auto& sub_node) {
-    switch (sub_node->type) {
-      case LQPNodeType::Predicate:
-        return LQPVisitation::VisitInputs;
-
-      case LQPNodeType::Union:
-        lqp_complexity++;
-        return LQPVisitation::VisitInputs;
-
-      default:
-        break_nodes.insert(sub_node);
-        return LQPVisitation::DoNotVisitInputs;
-    }
-  });
-
-  for (const auto& next_node : break_nodes) {
-    apply_to(next_node->left_input());
-    apply_to(next_node->right_input());
-  }
-
-  // Simple heuristic: The PredicateMergeRule is more likely to improve the performance for complex LQPs with many
-  // UNIONs. TODO(jj): Insert issue reference to find better heuristic
-  if (lqp_complexity > 3) {
-    merge_subplan(node, std::nullopt);
-  }
-}
-
 }  // namespace opossum

src/lib/optimizer/strategy/predicate_merge_rule.hpp

@@ -27,12 +27,15 @@ namespace opossum {
  */
 class PredicateMergeRule : public AbstractRule {
  public:
+  PredicateMergeRule(const size_t optimization_threshold);
   void apply_to(const std::shared_ptr<AbstractLQPNode>& node) const override;
 
  private:
   std::shared_ptr<AbstractExpression> merge_subplan(
       const std::shared_ptr<AbstractLQPNode>& begin,
       const std::optional<const std::shared_ptr<AbstractExpression>>& subsequent_expression) const;
+
+  size_t _optimization_threshold;
 };
 
 }  // namespace opossum

src/lib/optimizer/strategy/predicate_split_up_rule.cpp

@@ -7,9 +7,30 @@
 
 namespace opossum {
 
-PredicateSplitUpRule::PredicateSplitUpRule(const bool split_disjunction) : _split_disjunction(split_disjunction) {}
+PredicateSplitUpRule::PredicateSplitUpRule(const bool should_split_disjunction)
+    : _should_split_disjunction(should_split_disjunction) {}
 
-bool PredicateSplitUpRule::_splitConjunction(const std::shared_ptr<PredicateNode>& predicate_node) const {
+void PredicateSplitUpRule::apply_to(const std::shared_ptr<AbstractLQPNode>& root) const {
+  Assert(root->type == LQPNodeType::Root, "PredicateSplitUpRule needs root to hold onto");
+
+  auto predicate_nodes = std::vector<std::shared_ptr<PredicateNode>>{};
+  visit_lqp(root, [&](const auto& sub_node) {
+    if (const auto predicate_node = std::dynamic_pointer_cast<PredicateNode>(sub_node)) {
+      predicate_nodes.emplace_back(predicate_node);
+    }
+    return LQPVisitation::VisitInputs;
+  });
+
+  // _split_conjunction() and _split_disjunction() split up logical expressions by calling each other recursively
+  for (const auto& predicate_node : predicate_nodes) {
+    if (!_split_conjunction(predicate_node)) {
+      // If there is no conjunction at the top level, try to split disjunction first
+      _split_disjunction(predicate_node);
+    }
+  }
+}
+
+bool PredicateSplitUpRule::_split_conjunction(const std::shared_ptr<PredicateNode>& predicate_node) const {
   const auto flat_conjunction = flatten_logical_expressions(predicate_node->predicate(), LogicalOperator::And);
   if (flat_conjunction.size() <= 1) {
     return false;
@@ -22,15 +43,15 @@ bool PredicateSplitUpRule::_splitConjunction(const std::shared_ptr<PredicateNode
   for (const auto& predicate_expression : flat_conjunction) {
     const auto& new_predicate_node = PredicateNode::make(predicate_expression);
     lqp_insert_node(predicate_node, LQPInputSide::Left, new_predicate_node);
-    _splitDisjunction(new_predicate_node);
+    _split_disjunction(new_predicate_node);
   }
   lqp_remove_node(predicate_node);
 
   return true;
 }
 
-void PredicateSplitUpRule::_splitDisjunction(const std::shared_ptr<PredicateNode>& predicate_node) const {
-  if (!_split_disjunction) {
+void PredicateSplitUpRule::_split_disjunction(const std::shared_ptr<PredicateNode>& predicate_node) const {
+  if (!_should_split_disjunction) {
     return;
   }
 
@@ -49,11 +70,11 @@ void PredicateSplitUpRule::_splitDisjunction(const std::shared_ptr<PredicateNode
 
   auto new_predicate_node = PredicateNode::make(flat_disjunction[0], left_input);
   previous_union_node->set_left_input(new_predicate_node);
-  _splitConjunction(new_predicate_node);
+  _split_conjunction(new_predicate_node);
 
   new_predicate_node = PredicateNode::make(flat_disjunction[1], left_input);
   previous_union_node->set_right_input(new_predicate_node);
-  _splitConjunction(new_predicate_node);
+  _split_conjunction(new_predicate_node);
 
   for (auto disjunction_idx = size_t{2}; disjunction_idx < flat_disjunction.size(); ++disjunction_idx) {
     const auto& predicate_expression = flat_disjunction[disjunction_idx];
@@ -62,30 +83,10 @@ void PredicateSplitUpRule::_splitDisjunction(const std::shared_ptr<PredicateNode
 
     new_predicate_node = PredicateNode::make(predicate_expression, left_input);
     next_union_node->set_right_input(new_predicate_node);
-    _splitConjunction(new_predicate_node);
+    _split_conjunction(new_predicate_node);
 
     previous_union_node = next_union_node;
   }
 }
 
-void PredicateSplitUpRule::apply_to(const std::shared_ptr<AbstractLQPNode>& root) const {
-  Assert(root->type == LQPNodeType::Root, "PredicateSplitUpRule needs root to hold onto");
-
-  auto predicate_nodes = std::vector<std::shared_ptr<PredicateNode>>{};
-  visit_lqp(root, [&](const auto& sub_node) {
-    if (const auto predicate_node = std::dynamic_pointer_cast<PredicateNode>(sub_node)) {
-      predicate_nodes.emplace_back(predicate_node);
-    }
-    return LQPVisitation::VisitInputs;
-  });
-
-  // _splitConjunction() and _splitDisjunction() split up logical expressions by calling each other recursively
-  for (const auto& predicate_node : predicate_nodes) {
-    if (!_splitConjunction(predicate_node)) {
-      // If there is no conjunction at the top level, try to split disjunction first
-      _splitDisjunction(predicate_node);
-    }
-  }
-}
-
 }  // namespace opossum

src/lib/optimizer/strategy/predicate_split_up_rule.hpp

@@ -26,17 +26,17 @@ namespace opossum {
  */
 class PredicateSplitUpRule : public AbstractRule {
  public:
-  explicit PredicateSplitUpRule(const bool split_disjunction = true);
+  explicit PredicateSplitUpRule(const bool should_split_disjunction = true);
   void apply_to(const std::shared_ptr<AbstractLQPNode>& root) const override;
 
  private:
   /**
    * @return true if a conjunction was split up
    */
-  bool _splitConjunction(const std::shared_ptr<PredicateNode>& predicate_node) const;
-  void _splitDisjunction(const std::shared_ptr<PredicateNode>& predicate_node) const;
+  bool _split_conjunction(const std::shared_ptr<PredicateNode>& predicate_node) const;
+  void _split_disjunction(const std::shared_ptr<PredicateNode>& predicate_node) const;
 
-  bool _split_disjunction;
+  bool _should_split_disjunction;
 };
 
 }  // namespace opossum