Merge pull request #1242 from chenboy/join_order_fix

Fix 4 table join bug in binding iterator

Author: pervazea

src/include/optimizer/operator_expression.h

@@ -35,6 +35,8 @@ class OperatorExpression {
 
   const Operator &Op() const;
 
+  const std::string GetInfo() const;
+
  private:
   Operator op;
   std::vector<std::shared_ptr<OperatorExpression>> children;

src/optimizer/binding.cpp

@@ -19,11 +19,10 @@
 namespace peloton {
 namespace optimizer {
 
-
 //===--------------------------------------------------------------------===//
 // Group Binding Iterator
 //===--------------------------------------------------------------------===//
-GroupBindingIterator::GroupBindingIterator(Memo& memo, GroupID id,
+GroupBindingIterator::GroupBindingIterator(Memo &memo, GroupID id,
                                            std::shared_ptr<Pattern> pattern)
     : BindingIterator(memo),
       group_id_(id),
@@ -52,7 +51,8 @@ bool GroupBindingIterator::HasNext() {
     // Keep checking item iterators until we find a match
     while (current_item_index_ < num_group_items_) {
       current_iterator_.reset(new GroupExprBindingIterator(
-          memo_, target_group_->GetLogicalExpressions()[current_item_index_].get(),
+          memo_,
+          target_group_->GetLogicalExpressions()[current_item_index_].get(),
           pattern_));
 
       if (current_iterator_->HasNext()) {
@@ -78,24 +78,29 @@ std::shared_ptr<OperatorExpression> GroupBindingIterator::Next() {
 //===--------------------------------------------------------------------===//
 // Item Binding Iterator
 //===--------------------------------------------------------------------===//
-GroupExprBindingIterator::GroupExprBindingIterator(Memo& memo,
-                                         GroupExpression *gexpr,
-                                         std::shared_ptr<Pattern> pattern)
+GroupExprBindingIterator::GroupExprBindingIterator(
+    Memo &memo, GroupExpression *gexpr, std::shared_ptr<Pattern> pattern)
     : BindingIterator(memo),
       gexpr_(gexpr),
       pattern_(pattern),
       first_(true),
       has_next_(false),
       current_binding_(std::make_shared<OperatorExpression>(gexpr->Op())) {
-  LOG_TRACE("Attempting to bind on group %d with expression of type %s",
-            gexpr->GetGroupID(), gexpr->Op().GetName().c_str());
-  if (gexpr->Op().GetType() != pattern->Type()) return;
+  if (gexpr->Op().GetType() != pattern->Type()) {
+    return;
+  }
 
   const std::vector<GroupID> &child_groups = gexpr->GetChildGroupIDs();
   const std::vector<std::shared_ptr<Pattern>> &child_patterns =
       pattern->Children();
 
-  if (child_groups.size() != child_patterns.size()) return;
+  if (child_groups.size() != child_patterns.size()) {
+    return;
+  }
+  LOG_TRACE(
+      "Attempting to bind on group %d with expression of type %s, children "
+      "size %lu",
+      gexpr->GetGroupID(), gexpr->Op().GetName().c_str(), child_groups.size());
 
   // Find all bindings for children
   children_bindings_.resize(child_groups.size(), {});
@@ -104,15 +109,16 @@ GroupExprBindingIterator::GroupExprBindingIterator(Memo& memo,
     // Try to find a match in the given group
     std::vector<std::shared_ptr<OperatorExpression>> &child_bindings =
         children_bindings_[i];
-    GroupBindingIterator iterator(memo_, child_groups[i],
-                                  child_patterns[i]);
+    GroupBindingIterator iterator(memo_, child_groups[i], child_patterns[i]);
 
     // Get all bindings
     while (iterator.HasNext()) {
       child_bindings.push_back(iterator.Next());
     }
 
-    if (child_bindings.size() == 0) return;
+    if (child_bindings.size() == 0) {
+      return;
+    }
 
     current_binding_->PushChild(child_bindings[0]);
   }
@@ -128,33 +134,37 @@ bool GroupExprBindingIterator::HasNext() {
   }
 
   if (has_next_) {
-    size_t size = children_bindings_pos_.size();
-    size_t i;
-    for (i = 0; i < size; ++i) {
+    // The first child to be modified
+    int first_modified_idx = children_bindings_pos_.size() - 1;
+    for (; first_modified_idx >= 0; --first_modified_idx) {
       const std::vector<std::shared_ptr<OperatorExpression>> &child_binding =
-          children_bindings_[size - 1 - i];
+          children_bindings_[first_modified_idx];
 
-      size_t new_pos = ++children_bindings_pos_[size - 1 - i];
+      // Try to increment idx from the back
+      size_t new_pos = ++children_bindings_pos_[first_modified_idx];
       if (new_pos < child_binding.size()) {
         break;
       } else {
-        children_bindings_pos_[size - 1 - i] = 0;
+        children_bindings_pos_[first_modified_idx] = 0;
       }
     }
 
-    if (i == size) {
+    if (first_modified_idx < 0) {
       // We have explored all combinations of the child bindings
       has_next_ = false;
     } else {
-      for (size_t j = 0; j<i; j++)
+      // Pop all updated childrens
+      for (size_t idx = first_modified_idx; idx < children_bindings_pos_.size();
+           idx++) {
         current_binding_->PopChild();
-      // Replay to end
-      size_t offset = size - 1 - i;
-      for (size_t j = 0; j < i + 1; ++j) {
+      }
+      // Add new children to end
+      for (size_t offset = first_modified_idx;
+           offset < children_bindings_pos_.size(); ++offset) {
         const std::vector<std::shared_ptr<OperatorExpression>> &child_binding =
-            children_bindings_[offset + j];
+            children_bindings_[offset];
         std::shared_ptr<OperatorExpression> binding =
-            child_binding[children_bindings_pos_[offset + j]];
+            child_binding[children_bindings_pos_[offset]];
         current_binding_->PushChild(binding);
       }
     }

src/optimizer/memo.cpp

@@ -33,7 +33,7 @@ GroupExpression *Memo::InsertExpression(std::shared_ptr<GroupExpression> gexpr,
   // If leaf, then just return
   if (gexpr->Op().GetType() == OpType::Leaf) {
     const LeafOperator *leaf = gexpr->Op().As<LeafOperator>();
-    assert(target_group == UNDEFINED_GROUP ||
+    PL_ASSERT(target_group == UNDEFINED_GROUP ||
            target_group == leaf->origin_group);
     gexpr->SetGroupID(leaf->origin_group);
     return nullptr;
@@ -43,7 +43,7 @@ GroupExpression *Memo::InsertExpression(std::shared_ptr<GroupExpression> gexpr,
   auto it = group_expressions_.find(gexpr.get());
 
   if (it != group_expressions_.end()) {
-    assert(target_group == UNDEFINED_GROUP ||
+    PL_ASSERT(target_group == UNDEFINED_GROUP ||
            target_group == (*it)->GetGroupID());
     gexpr->SetGroupID((*it)->GetGroupID());
     return *it;

src/optimizer/operator_expression.cpp

@@ -28,12 +28,37 @@ void OperatorExpression::PushChild(std::shared_ptr<OperatorExpression> op) {
 
 void OperatorExpression::PopChild() { children.pop_back(); }
 
-const std::vector<std::shared_ptr<OperatorExpression>> &
-OperatorExpression::Children() const {
+const std::vector<std::shared_ptr<OperatorExpression>>
+    &OperatorExpression::Children() const {
   return children;
 }
 
 const Operator &OperatorExpression::Op() const { return op; }
 
+const std::string OperatorExpression::GetInfo() const {
+  std::string info = "{";
+  {
+    info += "\"Op\":";
+    info += "\"" + op.GetName() + "\",";
+    if (!children.empty()) {
+      info += "\"Children\":[";
+      {
+        bool is_first = true;
+        for (const auto &child : children) {
+          if (is_first == true) {
+            is_first = false;
+          } else {
+            info += ",";
+          }
+          info += child->GetInfo();
+        }
+      }
+      info += "]";
+    }
+  }
+  info += '}';
+  return info;
+}
+
 }  // namespace optimizer
 }  // namespace peloton

src/optimizer/operators.cpp

@@ -426,7 +426,7 @@ Operator PhysicalSeqScan::make(
     oid_t get_id, std::shared_ptr<catalog::TableCatalogObject> table,
     std::string alias, std::vector<AnnotatedExpression> predicates,
     bool update) {
-  assert(table != nullptr);
+  PL_ASSERT(table != nullptr);
   PhysicalSeqScan *scan = new PhysicalSeqScan;
   scan->table_ = table;
   scan->table_alias = alias;
@@ -465,7 +465,7 @@ Operator PhysicalIndexScan::make(
     oid_t index_id, std::vector<oid_t> key_column_id_list,
     std::vector<ExpressionType> expr_type_list,
     std::vector<type::Value> value_list) {
-  assert(table != nullptr);
+  PL_ASSERT(table != nullptr);
   PhysicalIndexScan *scan = new PhysicalIndexScan;
   scan->table_ = table;
   scan->is_for_update = update;

src/optimizer/rule_impls.cpp

@@ -109,7 +109,7 @@ void InnerJoinAssociativity::Transform(
   auto middle = children[0]->Children()[1];
   auto right = children[1];
 
-  LOG_TRACE("Reordered join structured: (%s JOIN %s) JOIN %s",
+  LOG_DEBUG("Reordered join structured: (%s JOIN %s) JOIN %s",
             left->Op().GetName().c_str(), middle->Op().GetName().c_str(),
             right->Op().GetName().c_str());
 

test/sql/optimizer_sql_test.cpp

@@ -494,6 +494,7 @@ TEST_F(OptimizerSQLTests, SelectConstantTest) {
 }
 
 TEST_F(OptimizerSQLTests, JoinTest) {
+  // The original table
   // TestingSQLUtil::ExecuteSQLQuery("INSERT INTO test VALUES (1, 22, 333);");
   // TestingSQLUtil::ExecuteSQLQuery("INSERT INTO test VALUES (2, 11, 000);");
   // TestingSQLUtil::ExecuteSQLQuery("INSERT INTO test VALUES (3, 33, 444);");
@@ -519,6 +520,16 @@ TEST_F(OptimizerSQLTests, JoinTest) {
   TestingSQLUtil::ExecuteSQLQuery("INSERT INTO test2 VALUES (3, 22, 555);");
   TestingSQLUtil::ExecuteSQLQuery("INSERT INTO test2 VALUES (4, 00, 000);");
 
+  // Create the fourth table
+  TestingSQLUtil::ExecuteSQLQuery(
+      "CREATE TABLE test3(a INT PRIMARY KEY, b INT, c INT);");
+
+  // Insert tuples into table
+  TestingSQLUtil::ExecuteSQLQuery("INSERT INTO test3 VALUES (1, 22, 000);");
+  TestingSQLUtil::ExecuteSQLQuery("INSERT INTO test3 VALUES (2, 11, 333);");
+  TestingSQLUtil::ExecuteSQLQuery("INSERT INTO test3 VALUES (3, 22, 555);");
+  TestingSQLUtil::ExecuteSQLQuery("INSERT INTO test3 VALUES (4, 00, 000);");
+
   /************************* Basic Queries (only joins)
    * *******************************/
   // Product
@@ -637,6 +648,14 @@ TEST_F(OptimizerSQLTests, JoinTest) {
       "GROUP BY test.a "
       "ORDER BY test.a",
       {"22", "1", "11", "2", "22", "3", "0", "4"}, true);
+
+  // Basic 4 table join
+  TestUtil(
+      "SELECT test.a, test1.a, test2.a, test3.c FROM test, test1, test2, test3 "
+      "WHERE test.a = test2.a AND test2.a = test1.a and test.b = test3.b",
+      {"1", "1", "1", "0", "1", "1", "1", "555", "2", "2", "2", "333", "4",
+       "4", "4", "0"},
+      false);
 }
 
 TEST_F(OptimizerSQLTests, IndexTest) {