Address comments

Author: chenboy

src/include/expression/expression_util.h

@@ -401,21 +401,24 @@ class ExpressionUtil {
   }
 
   /**
-   * TODO(boweic): this function may not be efficient, in the future we may want
-   * to add expressions to groups so that we do not need to walk through the
-   * expression tree when judging '==' each time
+   * @brief TODO(boweic): this function may not be efficient, in the future we
+   * may want to add expressions to groups so that we do not need to walk
+   * through the expression tree when judging '==' each time
    *
    * Convert all expression in the current expression tree that is in
    * child_expr_map to tuple value expression with corresponding column offset
    * of the input child tuple. This is used for handling projection
    * on situations like aggregate function (e.g. SELECT sum(a)+max(b) FROM ...
    * GROUP BY ...) when input columns contain sum(a) and sum(b). We need to
    * treat them as tuple value expression in the projection plan. This function
-   *should always be called before calling EvaluateExpression
+   * should always be called before calling EvaluateExpression
    *
    * Please notice that this function should only apply to copied expression
-   *since it would modify the current expression. We do not want to modify the
-   *original expression since it may be referenced in other places
+   * since it would modify the current expression. We do not want to modify the
+   * original expression since it may be referenced in other places
+   *
+   * @param expr The expression to modify
+   * @param child_expr_maps map from child column ids to expression
    */
   static void ConvertToTvExpr(AbstractExpression *expr,
                               std::vector<ExprMap> child_expr_maps) {

src/include/optimizer/query_to_operator_transformer.h

@@ -85,10 +85,6 @@ class QueryToOperatorTransformer : public SqlNodeVisitor {
       expression::AbstractExpression *expr,
       std::vector<AnnotatedExpression> predicates = {});
 
-  // TODO(boweic): Since we haven't migrated all the functionalities needed to
-  // generate mark-join and single-join to the optimizer, currently this
-  // function has not been tested, and it may be a bit hard to understand. We
-  // may integrate the unnesting functionality in the next PR
   /**
    * @brief Transform a sub-query in an expression to use
    *

src/include/optimizer/rule_impls.h

@@ -291,7 +291,8 @@ class CombineConsecutiveFilter : public Rule {
 };
 
 /**
- * @brief perform predicate push-down to push a filter through aggregation, also will embed filter into aggregation operator if appropriate. 
+ * @brief perform predicate push-down to push a filter through aggregation, also
+ * will embed filter into aggregation operator if appropriate.
  */
 class PushFilterThroughAggregation : public Rule {
  public:
@@ -323,6 +324,9 @@ class EmbedFilterIntoGet : public Rule {
 
 ///////////////////////////////////////////////////////////////////////////////
 /// Unnesting rules
+// We use this promise to determine which rules should be applied first if
+// multiple rules are applicable, we need to first pull filters up through mark-join
+// then turn mark-join into a regular join operator
 enum class UnnestPromise { Low = 1, High };
 // TODO(boweic): MarkJoin and SingleJoin should not be transformed into inner
 // join. Sometimes MarkJoin could be transformed into semi-join, but for now we

src/include/optimizer/util.h

@@ -33,10 +33,23 @@ class DataTable;
 namespace optimizer {
 namespace util {
 
+  /**
+   * @brief Convert upper case letters into lower case in a string
+   *
+   * @param str The string to operate on
+   */
 inline void to_lower_string(std::string &str) {
   std::transform(str.begin(), str.end(), str.begin(), ::tolower);
 }
 
+/**
+ * @brief Check if a hashset is a subset of the other one
+ *
+ * @param super_set The potential super set
+ * @param child_set The potential child set
+ *
+ * @return True if the second set is a subset of the first one
+ */
 template <class T>
 bool IsSubset(const std::unordered_set<T> &super_set,
               const std::unordered_set<T> &child_set) {
@@ -46,47 +59,107 @@ bool IsSubset(const std::unordered_set<T> &super_set,
   return true;
 }
 
+/**
+ * @brief Perform union operation on 2 hashsets
+ *
+ * @param new_set The first set, will hold the result after the union operation
+ * @param old_set the other set
+ */
 template <class T>
 void SetUnion(std::unordered_set<T> &new_set, std::unordered_set<T> &old_set) {
   for (auto element : old_set) new_set.insert(element);
 }
 
+/**
+ * @breif Split conjunction expression tree into a vector of expressions with
+ * AND
+ */
 void SplitPredicates(expression::AbstractExpression *expr,
                      std::vector<expression::AbstractExpression *> &predicates);
 
+/**
+ * @breif Combine a vector of expressions with AND
+ */
 expression::AbstractExpression *CombinePredicates(
     std::vector<std::shared_ptr<expression::AbstractExpression>> predicates);
 
+/**
+ * @breif Combine a vector of expressions with AND
+ */
 expression::AbstractExpression *CombinePredicates(
     std::vector<AnnotatedExpression> predicates);
 
+/**
+ * @brief Extract single table precates and multi-table predicates from the expr
+ *
+ * @param expr The original predicate
+ * @param annotated_predicates The extracted conjunction predicates
+ */
 std::vector<AnnotatedExpression> ExtractPredicates(
     expression::AbstractExpression *expr,
     std::vector<AnnotatedExpression> annotated_predicates = {});
 
+/**
+ * @breif Construct a qualified join predicate (contains a subset of alias in
+ *  the table_alias_set)
+ *  and remove the multitable expressions in the original join_predicates
+ *
+ * @return The final join predicate
+ */
 expression::AbstractExpression *ConstructJoinPredicate(
     std::unordered_set<std::string> &table_alias_set,
     MultiTablePredicates &join_predicates);
 
+
+/**
+ * @breif Check if there are any join columns in the join expression
+ *  For example, expr = (expr_1) AND (expr_2) AND (expr_3)
+ *  we only extract expr_i that have the format (l_table.a = r_table.b)
+ *  i.e. expr that is equality check for tuple columns from both of
+ *  the underlying tables
+ */
 bool ContainsJoinColumns(const std::unordered_set<std::string> &l_group_alias,
                          const std::unordered_set<std::string> &r_group_alias,
                          const expression::AbstractExpression *expr);
 
+/**
+ * @brief Create a copy plan based on the copy statement
+ */
 std::unique_ptr<planner::AbstractPlan> CreateCopyPlan(
     parser::CopyStatement *copy_stmt);
 
+/**
+ * @brief Construct the map from subquery column name to the actual expression
+ *  at the subquery level, for example SELECT a FROM (SELECT a + b as a FROM
+ *  test), we'll build the map {"a" -> a + b}
+ *
+ * @param select_list The select list of a subquery
+ *
+ * @return The mapping mentioned above
+ */
 std::unordered_map<std::string, std::shared_ptr<expression::AbstractExpression>>
 ConstructSelectElementMap(
     std::vector<std::unique_ptr<expression::AbstractExpression>> &select_list);
 
+/**
+ * @brief Walk a predicate, transform the tuple value expression into the actual
+ *  expression in the sub-query
+ *
+ * @param alias_to_expr_map The column name to expression map
+ * @param expr the predicate
+ *
+ * @return the transformed predicate
+ */
 expression::AbstractExpression *TransformQueryDerivedTablePredicates(
     const std::unordered_map<std::string,
                              std::shared_ptr<expression::AbstractExpression>>
         &alias_to_expr_map,
     expression::AbstractExpression *expr);
 
-// Extract equi-join keys from the join predicates (conjunction is already
-// removed in each unit)
+/**
+ * @brief Walk through a set of join predicates, generate join keys base on the
+ *  left/right table aliases set
+ */
 void ExtractEquiJoinKeys(
     const std::vector<AnnotatedExpression> join_predicates,
     std::vector<std::unique_ptr<expression::AbstractExpression>> &left_keys,

src/optimizer/input_column_deriver.cpp

@@ -219,6 +219,8 @@ void InputColumnDeriver::AggregateHelper(const BaseOperatorNode *op) {
   // Check having predicate, since the predicate may use columns other than
   // output columns
   for (auto &having_expr : having_exprs) {
+    // We perform aggregate here so the output contains aggregate exprs while
+    // input should contain all tuple value exprs used to perform aggregation.
     expression::ExpressionUtil::GetTupleValueExprs(input_cols_set,
                                                    having_expr.expr.get());
     expression::ExpressionUtil::GetTupleAndAggregateExprs(
@@ -275,7 +277,8 @@ void InputColumnDeriver::JoinHelper(const BaseOperatorNode *op) {
   }
   ExprMap output_cols_map;
   for (auto expr : required_cols_) {
-    expression::ExpressionUtil::GetTupleAndAggregateExprs(output_cols_map, expr);
+    expression::ExpressionUtil::GetTupleAndAggregateExprs(output_cols_map,
+                                                          expr);
   }
   for (auto &expr_idx_pair : output_cols_map) {
     input_cols_set.insert(expr_idx_pair.first);

src/optimizer/optimizer_task.cpp

@@ -29,11 +29,7 @@ void OptimizerTask::ConstructValidRules(
     GroupExpression *group_expr, OptimizeContext *context,
     std::vector<std::unique_ptr<Rule>> &rules,
     std::vector<RuleWithPromise> &valid_rules) {
-  //  LOG_DEBUG("Child size %lu", group_expr->GetChildrenGroupsSize());
   for (auto &rule : rules) {
-    // LOG_DEBUG("Op %d, Pattern Op %d",
-    // static_cast<int>(group_expr->Op().type()),
-    //           static_cast<int>(rule->GetMatchPattern()->Type()));
     if (group_expr->Op().type() !=
             rule->GetMatchPattern()->Type() ||  // Root pattern type mismatch
         group_expr->HasRuleExplored(rule.get()) ||  // Rule has been applied
@@ -61,7 +57,7 @@ RuleSet &OptimizerTask::GetRuleSet() const {
 // OptimizeGroup
 //===--------------------------------------------------------------------===//
 void OptimizeGroup::execute() {
-  // LOG_DEBUG("OptimizeGroup::Execute() group %d", group_->GetID());
+  LOG_TRACE("OptimizeGroup::Execute() group %d", group_->GetID());
   if (group_->GetCostLB() > context_->cost_upper_bound ||  // Cost LB > Cost UB
       group_->GetBestExpression(context_->required_prop) !=
           nullptr)  // Has optimized given the context
@@ -98,8 +94,8 @@ void OptimizeExpression::execute() {
                       GetRuleSet().GetImplementationRules(), valid_rules);
 
   std::sort(valid_rules.begin(), valid_rules.end());
-  // LOG_DEBUG("OptimizeExpression::execute() op %d, valid rules : %lu",
-  //           static_cast<int>(group_expr_->Op().type()), valid_rules.size());
+  LOG_TRACE("OptimizeExpression::execute() op %d, valid rules : %lu",
+            static_cast<int>(group_expr_->Op().type()), valid_rules.size());
 
   // Apply rule
   for (auto &r : valid_rules) {
@@ -125,7 +121,7 @@ void OptimizeExpression::execute() {
 //===--------------------------------------------------------------------===//
 void ExploreGroup::execute() {
   if (group_->HasExplored()) return;
-  // LOG_DEBUG("ExploreGroup::execute() ");
+  LOG_TRACE("ExploreGroup::execute() ");
 
   for (auto &logical_expr : group_->GetLogicalExpressions()) {
     PushTask(new ExploreExpression(logical_expr.get(), context_));
@@ -140,7 +136,7 @@ void ExploreGroup::execute() {
 // ExploreExpression
 //===--------------------------------------------------------------------===//
 void ExploreExpression::execute() {
-  // LOG_DEBUG("ExploreExpression::execute() ");
+  LOG_TRACE("ExploreExpression::execute() ");
   std::vector<RuleWithPromise> valid_rules;
 
   // Construct valid transformation rules from rule set
@@ -172,7 +168,7 @@ void ExploreExpression::execute() {
 // ApplyRule
 //===--------------------------------------------------------------------===//
 void ApplyRule::execute() {
-  // LOG_DEBUG("ApplyRule::execute() ");
+  LOG_TRACE("ApplyRule::execute() ");
   if (group_expr_->HasRuleExplored(rule_)) return;
 
   GroupExprBindingIterator iterator(GetMemo(), group_expr_,
@@ -262,7 +258,7 @@ void DeriveStats::execute() {
 //===--------------------------------------------------------------------===//
 void OptimizeInputs::execute() {
   // Init logic: only run once per task
-  // LOG_DEBUG("OptimizeInputs::execute() ");
+  LOG_TRACE("OptimizeInputs::execute() ");
   if (cur_child_idx_ == -1) {
     // TODO(patrick):
     // 1. We can init input cost using non-zero value for pruning

src/optimizer/query_to_operator_transformer.cpp

@@ -74,7 +74,8 @@ void QueryToOperatorTransformer::Visit(parser::SelectStatement *op) {
     // Plain aggregation
     std::shared_ptr<OperatorExpression> agg_expr;
     if (op->group_by == nullptr) {
-      // TODO(boweic): aggregation without groupby could still have having clause
+      // TODO(boweic): aggregation without groupby could still have having
+      // clause
       agg_expr = std::make_shared<OperatorExpression>(
           LogicalAggregateAndGroupBy::make());
       agg_expr->PushChild(output_expr_);
@@ -421,7 +422,7 @@ bool QueryToOperatorTransformer::IsSupportedConjunctivePredicate(
       expr_type == ExpressionType::COMPARE_GREATERTHANOREQUALTO ||
       expr_type == ExpressionType::COMPARE_LESSTHAN ||
       expr_type == ExpressionType::COMPARE_LESSTHANOREQUALTO) {
-    // Supported is one child is subquery and the other is not
+    // Supported if one child is subquery and the other is not
     if ((!expr->GetChild(0)->HasSubquery() &&
          expr->GetChild(1)->GetExpressionType() ==
              ExpressionType::ROW_SUBQUERY) ||
@@ -450,7 +451,9 @@ bool QueryToOperatorTransformer::IsSupportedSubSelect(
   std::vector<expression::AbstractExpression *> predicates;
   util::SplitPredicates(op->where_clause.get(), predicates);
   for (const auto &pred : predicates) {
-    // If correlated predicate
+    // Depth is used to detect correlated subquery, it is set in the binder,
+    // if a TVE has depth less than the depth of the current operator, then it
+    // is correlated predicate
     if (pred->GetDepth() < op->depth) {
       if (pred->GetExpressionType() != ExpressionType::COMPARE_EQUAL) {
         return false;