Move parse tree binding from optimizer to catalog (#1133)

* Move parse tree binding from optimizer to catalog

* remove use of using directive from binder_test

* remove the ||true from make check

requested by @pervazea

* Move parse tree binding from optimizer to catalog

* stack allocation of BindNodeVisitor

Author: nappelson

src/catalog/abstract_catalog.cpp

@@ -12,9 +12,10 @@
 
 #include "catalog/abstract_catalog.h"
 
+#include "binder/bind_node_visitor.h"
+
 #include "common/statement.h"
 
-#include "catalog/catalog.h"
 #include "catalog/database_catalog.h"
 #include "catalog/table_catalog.h"
 
@@ -55,25 +56,38 @@ AbstractCatalog::AbstractCatalog(oid_t catalog_table_oid,
 
 AbstractCatalog::AbstractCatalog(const std::string &catalog_table_ddl,
                                  concurrency::TransactionContext *txn) {
-  // get catalog table schema
+  // Get catalog table schema
   auto &peloton_parser = parser::PostgresParser::GetInstance();
+
+  // Build the parse tree
+  const auto parse_tree_list = peloton_parser.BuildParseTree(catalog_table_ddl);
+  if (parse_tree_list->GetStatements().empty()) {
+    throw CatalogException(
+        "Parse tree list has no parse trees. Cannot build plan");
+  }
+  // TODO: support multi-statement queries
+  auto parse_tree = parse_tree_list->GetStatement(0);
+
+  // Run binder
+  auto bind_node_visitor = binder::BindNodeVisitor(txn, DATABASE_CATALOG_NAME);
+  bind_node_visitor.BindNameToNode(parse_tree);
+
+  // Create the plan tree
   auto create_plan = std::dynamic_pointer_cast<planner::CreatePlan>(
-      optimizer::Optimizer().BuildPelotonPlanTree(
-          peloton_parser.BuildParseTree(catalog_table_ddl),
-          DATABASE_CATALOG_NAME, txn));
+      optimizer::Optimizer().BuildPelotonPlanTree(parse_tree_list, txn));
   auto catalog_table_schema = create_plan->GetSchema();
   auto catalog_table_name = create_plan->GetTableName();
 
-  // create catalog table
+  // Create catalog table
   Catalog::GetInstance()->CreateTable(
       CATALOG_DATABASE_NAME, catalog_table_name,
       std::unique_ptr<catalog::Schema>(catalog_table_schema), txn, true);
 
-  // get catalog table oid
+  // Get catalog table oid
   auto catalog_table_object = Catalog::GetInstance()->GetTableObject(
       CATALOG_DATABASE_NAME, catalog_table_name, txn);
 
-  // set catalog_table_
+  // Set catalog_table_
   try {
     catalog_table_ = storage::StorageManager::GetInstance()->GetTableWithOid(
         CATALOG_DATABASE_OID, catalog_table_object->GetTableOid());

src/include/optimizer/abstract_optimizer.h

@@ -50,8 +50,7 @@ class AbstractOptimizer {
   virtual ~AbstractOptimizer();
 
   virtual std::shared_ptr<planner::AbstractPlan> BuildPelotonPlanTree(
-      const std::unique_ptr<parser::SQLStatementList> &parse_tree, 
-      const std::string default_database_name,
+      const std::unique_ptr<parser::SQLStatementList> &parse_tree,
       concurrency::TransactionContext *txn) = 0;
 
   virtual void Reset(){};

src/include/optimizer/optimizer.h

@@ -72,8 +72,7 @@ class Optimizer : public AbstractOptimizer {
   Optimizer();
 
   std::shared_ptr<planner::AbstractPlan> BuildPelotonPlanTree(
-      const std::unique_ptr<parser::SQLStatementList> &parse_tree,
-      const std::string default_database_name,
+      const std::unique_ptr<parser::SQLStatementList> &parse_tree_list,
       concurrency::TransactionContext *txn) override;
 
   void OptimizeLoop(int root_group_id,

src/optimizer/optimizer.cpp

@@ -44,8 +44,6 @@
 
 #include "storage/data_table.h"
 
-#include "binder/bind_node_visitor.h"
-
 using std::vector;
 using std::unordered_map;
 using std::shared_ptr;
@@ -92,21 +90,18 @@ void Optimizer::OptimizeLoop(int root_group_id,
 }
 
 shared_ptr<planner::AbstractPlan> Optimizer::BuildPelotonPlanTree(
-    const unique_ptr<parser::SQLStatementList> &parse_tree_list,
-    const std::string default_database_name,
+    const std::unique_ptr<parser::SQLStatementList> &parse_tree_list,
     concurrency::TransactionContext *txn) {
-  // Base Case
-  if (parse_tree_list->GetStatements().size() == 0) return nullptr;
+  if (parse_tree_list->GetStatements().empty()) {
+    // TODO: create optimizer exception
+    throw CatalogException(
+        "Parse tree list has no parse trees. Cannot build plan");
+  }
+  // TODO: support multi-statement queries
+  auto parse_tree = parse_tree_list->GetStatement(0);
 
   unique_ptr<planner::AbstractPlan> child_plan = nullptr;
 
-  auto parse_tree = parse_tree_list->GetStatements().at(0).get();
-
-  // Run binder
-  auto bind_node_visitor =
-      make_shared<binder::BindNodeVisitor>(txn, default_database_name);
-  bind_node_visitor->BindNameToNode(parse_tree);
-
   // Handle ddl statement
   bool is_ddl_stmt;
   auto ddl_plan = HandleDDLStatement(parse_tree, is_ddl_stmt, txn);

src/traffic_cop/traffic_cop.cpp

@@ -312,9 +312,13 @@ std::shared_ptr<Statement> TrafficCop::PrepareStatement(
   // TODO(Tianyi) Move Statement Planing into Statement's method
   // to increase coherence
   try {
+    // Run binder
+    auto bind_node_visitor = binder::BindNodeVisitor(
+        tcop_txn_state_.top().first, default_database_name_);
+    bind_node_visitor.BindNameToNode(
+        statement->GetStmtParseTreeList()->GetStatement(0));
     auto plan = optimizer_->BuildPelotonPlanTree(
-        statement->GetStmtParseTreeList(), default_database_name_,
-        tcop_txn_state_.top().first);
+        statement->GetStmtParseTreeList(), tcop_txn_state_.top().first);
     statement->SetPlanTree(plan);
     // Get the tables that our plan references so that we know how to
     // invalidate it at a later point when the catalog changes
@@ -362,8 +366,8 @@ void TrafficCop::ProcessInvalidStatement() {
 }
 
 bool TrafficCop::BindParamsForCachePlan(
-    const std::vector<std::unique_ptr<expression::AbstractExpression>> &
-        parameters,
+    const std::vector<std::unique_ptr<expression::AbstractExpression>>
+        &parameters,
     const size_t thread_id UNUSED_ATTRIBUTE) {
   if (tcop_txn_state_.empty()) {
     single_statement_txn_ = true;
@@ -377,8 +381,8 @@ bool TrafficCop::BindParamsForCachePlan(
     tcop_txn_state_.emplace(txn, ResultType::SUCCESS);
   }
   // Run binder
-  auto bind_node_visitor = std::make_shared<binder::BindNodeVisitor>(
-      tcop_txn_state_.top().first, default_database_name_);
+  auto bind_node_visitor = binder::BindNodeVisitor(tcop_txn_state_.top().first,
+                                                   default_database_name_);
 
   std::vector<type::Value> param_values;
   for (const std::unique_ptr<expression::AbstractExpression> &param :
@@ -387,7 +391,7 @@ bool TrafficCop::BindParamsForCachePlan(
       error_message_ = "Invalid Expression Type";
       return false;
     }
-    param->Accept(bind_node_visitor.get());
+    param->Accept(&bind_node_visitor);
     // TODO(Yuchen): need better check for nullptr argument
     param_values.push_back(param->Evaluate(nullptr, nullptr, nullptr));
   }
@@ -580,9 +584,12 @@ ResultType TrafficCop::ExecuteStatement(
         if (statement->GetNeedsReplan()) {
           // TODO(Tianyi) Move Statement Replan into Statement's method
           // to increase coherence
+          auto bind_node_visitor = binder::BindNodeVisitor(
+              tcop_txn_state_.top().first, default_database_name_);
+          bind_node_visitor.BindNameToNode(
+              statement->GetStmtParseTreeList()->GetStatement(0));
           auto plan = optimizer_->BuildPelotonPlanTree(
-              statement->GetStmtParseTreeList(), default_database_name_,
-              tcop_txn_state_.top().first);
+              statement->GetStmtParseTreeList(), tcop_txn_state_.top().first);
           statement->SetPlanTree(plan);
           statement->SetNeedsReplan(true);
         }

test/binder/binder_test.cpp

@@ -32,6 +32,7 @@ using std::string;
 using std::unique_ptr;
 using std::vector;
 using std::make_tuple;
+using std::make_shared;
 
 namespace peloton {
 namespace test {
@@ -77,9 +78,14 @@ void SetupTables(std::string database_name) {
     vector<ResultValue> result;
     vector<int> result_format;
     unique_ptr<Statement> statement(new Statement("CREATE", sql));
-    auto parse_tree = parser.BuildParseTree(sql);
+
+    auto parse_tree_list = parser.BuildParseTree(sql);
+    auto parse_tree = parse_tree_list->GetStatement(0);
+    auto bind_node_visitor = binder::BindNodeVisitor(txn, database_name);
+    bind_node_visitor.BindNameToNode(parse_tree);
+
     statement->SetPlanTree(
-        optimizer.BuildPelotonPlanTree(parse_tree, database_name, txn));
+        optimizer.BuildPelotonPlanTree(parse_tree_list, txn));
     TestingSQLUtil::counter_.store(1);
     auto status = traffic_cop.ExecuteHelper(statement->GetPlanTree(), params,
                                             result, result_format);
@@ -319,7 +325,8 @@ TEST_F(BinderCorrectnessTest, BindDepthTest) {
   auto exists_sub_expr_select =
       dynamic_cast<const expression::SubqueryExpression *>(exists_sub_expr)
           ->GetSubSelect();
-  auto exists_sub_expr_select_where = exists_sub_expr_select->where_clause.get();
+  auto exists_sub_expr_select_where =
+      exists_sub_expr_select->where_clause.get();
   auto exists_sub_expr_select_ele =
       exists_sub_expr_select->select_list[0].get();
   auto in_tv_expr = in_expr->GetChild(0);
@@ -337,8 +344,7 @@ TEST_F(BinderCorrectnessTest, BindDepthTest) {
       in_sub_expr_select_where_right->GetChild(1);
   auto in_sub_expr_select_where_right_sub_select =
       dynamic_cast<const expression::SubqueryExpression *>(
-          in_sub_expr_select_where_right_sub)
-          ->GetSubSelect();
+          in_sub_expr_select_where_right_sub)->GetSubSelect();
   auto in_sub_expr_select_where_right_sub_select_where =
       in_sub_expr_select_where_right_sub_select->where_clause.get();
   auto in_sub_expr_select_where_right_sub_select_ele =

test/executor/copy_test.cpp

@@ -13,6 +13,7 @@
 #include <cstdio>
 #include "sql/testing_sql_util.h"
 
+#include "binder/bind_node_visitor.h"
 #include "catalog/catalog.h"
 #include "common/harness.h"
 #include "common/logger.h"
@@ -41,7 +42,8 @@ TEST_F(CopyTests, Copying) {
   auto catalog = catalog::Catalog::GetInstance();
   auto &txn_manager = concurrency::TransactionManagerFactory::GetInstance();
   auto txn = txn_manager.BeginTransaction();
-  catalog->CreateDatabase("emp_db", txn);
+  const auto &db_name = "emp_db";
+  catalog->CreateDatabase(db_name, txn);
   txn_manager.CommitTransaction(txn);
 
   std::unique_ptr<optimizer::AbstractOptimizer> optimizer(
@@ -119,9 +121,14 @@ TEST_F(CopyTests, Copying) {
   auto &peloton_parser = parser::PostgresParser::GetInstance();
   auto copy_stmt = peloton_parser.BuildParseTree(copy_sql);
 
+  LOG_TRACE("Binding parse tree...");
+  auto parse_tree = copy_stmt->GetStatement(0);
+  auto bind_node_visitor = binder::BindNodeVisitor(txn, db_name);
+  bind_node_visitor.BindNameToNode(parse_tree);
+  LOG_TRACE("Binding parse tree completed!");
+
   LOG_TRACE("Building plan tree...");
-  auto copy_plan =
-      optimizer->BuildPelotonPlanTree(copy_stmt, DEFAULT_DB_NAME, txn);
+  auto copy_plan = optimizer->BuildPelotonPlanTree(copy_stmt, txn);
   statement->SetPlanTree(copy_plan);
 
   LOG_TRACE("Building executor tree...");

test/executor/create_index_test.cpp

@@ -14,6 +14,7 @@
 #include <cstdio>
 #include "sql/testing_sql_util.h"
 
+#include "binder/bind_node_visitor.h"
 #include "catalog/catalog.h"
 #include "common/harness.h"
 #include "common/logger.h"
@@ -80,9 +81,14 @@ TEST_F(CreateIndexTests, CreatingIndex) {
       "dept_name TEXT);");
   LOG_INFO("Building parse tree completed!");
 
+  LOG_INFO("Binding parse tree...");
+  auto parse_tree = create_stmt->GetStatement(0);
+  auto bind_node_visitor = binder::BindNodeVisitor(txn, DEFAULT_DB_NAME);
+  bind_node_visitor.BindNameToNode(parse_tree);
+  LOG_INFO("Binding parse tree completed!");
+
   LOG_INFO("Building plan tree...");
-  statement->SetPlanTree(
-      optimizer->BuildPelotonPlanTree(create_stmt, DEFAULT_DB_NAME, txn));
+  statement->SetPlanTree(optimizer->BuildPelotonPlanTree(create_stmt, txn));
   LOG_INFO("Building plan tree completed!");
 
   std::vector<type::Value> params;
@@ -131,9 +137,14 @@ TEST_F(CreateIndexTests, CreatingIndex) {
       "(1,52,'hello_1');");
   LOG_INFO("Building parse tree completed!");
 
+  LOG_INFO("Binding parse tree...");
+  parse_tree = insert_stmt->GetStatement(0);
+  bind_node_visitor = binder::BindNodeVisitor(txn, DEFAULT_DB_NAME);
+  bind_node_visitor.BindNameToNode(parse_tree);
+  LOG_INFO("Binding parse tree completed!");
+
   LOG_INFO("Building plan tree...");
-  statement->SetPlanTree(
-      optimizer->BuildPelotonPlanTree(insert_stmt, DEFAULT_DB_NAME, txn));
+  statement->SetPlanTree(optimizer->BuildPelotonPlanTree(insert_stmt, txn));
   LOG_INFO("Building plan tree completed!\n%s",
            planner::PlanUtil::GetInfo(statement->GetPlanTree().get()).c_str());
 
@@ -168,9 +179,14 @@ TEST_F(CreateIndexTests, CreatingIndex) {
       "CREATE INDEX saif ON department_table (student_id);");
   LOG_INFO("Building parse tree completed!");
 
+  LOG_INFO("Binding parse tree...");
+  parse_tree = update_stmt->GetStatement(0);
+  bind_node_visitor = binder::BindNodeVisitor(txn, DEFAULT_DB_NAME);
+  bind_node_visitor.BindNameToNode(parse_tree);
+  LOG_INFO("Binding parse tree completed!");
+
   LOG_INFO("Building plan tree...");
-  statement->SetPlanTree(
-      optimizer->BuildPelotonPlanTree(update_stmt, DEFAULT_DB_NAME, txn));
+  statement->SetPlanTree(optimizer->BuildPelotonPlanTree(update_stmt, txn));
   LOG_INFO("Building plan tree completed!\n%s",
            planner::PlanUtil::GetInfo(statement->GetPlanTree().get()).c_str());