Address review comments + minor clean-up in the layout.h API

poojanilangekar · poojanilangekar · commit cf631f48f783 · 2018-05-05T12:07:41.000-04:00
diff --git a/src/catalog/catalog.cpp b/src/catalog/catalog.cpp
@@ -162,7 +162,7 @@ void Catalog::BootstrapSystemCatalogs(storage::Database *database,
   system_catalogs->GetSchemaCatalog()->InsertSchema(
       CATALOG_SCHEMA_OID, CATALOG_SCHEMA_NAME, pool_.get(), txn);
   system_catalogs->GetSchemaCatalog()->InsertSchema(
-      DEFUALT_SCHEMA_OID, DEFAULT_SCHEMA_NAME, pool_.get(), txn);
+      DEFAULT_SCHEMA_OID, DEFAULT_SCHEMA_NAME, pool_.get(), txn);
 
   // Insert catalog tables into pg_table
   // pg_database record is shared across different databases
@@ -561,15 +561,6 @@ ResultType Catalog::CreateIndex(
   return ResultType::SUCCESS;
 }
 
-/*
- * @brief   create a new layout for a table
- * @param   database_oid  database to which the table belongs to
- * @param   table_oid     table to which the layout has to be added
- * @param   column_map    column_map of the new layout to be created
- * @param   txn           TransactionContext
- * @return  shared_ptr    shared_ptr to the newly created layout in case of
- *                        success. nullptr in case of failure.
- */
 std::shared_ptr<const storage::Layout> Catalog::CreateLayout(
     oid_t database_oid, oid_t table_oid, const column_map_type &column_map,
     concurrency::TransactionContext *txn) {
@@ -594,16 +585,6 @@ std::shared_ptr<const storage::Layout> Catalog::CreateLayout(
   return new_layout;
 }
 
-/*
- * @brief   create a new layout for a table and make it the deafult if
- *          if the creating is successsful.
- * @param   database_oid  database to which the table belongs to
- * @param   table_oid     table to which the layout has to be added
- * @param   column_map    column_map of the new layout to be created
- * @param   txn           TransactionContext
- * @return  shared_ptr    shared_ptr to the newly created layout in case of
- *                        success. nullptr in case of failure.
- */
 std::shared_ptr<const storage::Layout> Catalog::CreateDefaultLayout(
     oid_t database_oid, oid_t table_oid, const column_map_type &column_map,
     concurrency::TransactionContext *txn) {
@@ -830,14 +811,6 @@ ResultType Catalog::DropIndex(oid_t database_oid, oid_t index_oid,
   return ResultType::SUCCESS;
 }
 
-/*@brief   Drop layout
- * tile_groups
- * @param   database_oid    the database to which the table belongs
- * @param   table_oid       the table to which the layout belongs
- * @param   layout_oid      the layout to be dropped
- * @param   txn             TransactionContext
- * @return  TransactionContext ResultType(SUCCESS or FAILURE)
- */
 ResultType Catalog::DropLayout(oid_t database_oid, oid_t table_oid,
                                oid_t layout_oid,
                                concurrency::TransactionContext *txn) {
diff --git a/src/catalog/table_catalog.cpp b/src/catalog/table_catalog.cpp
@@ -338,7 +338,7 @@ TableCatalog::TableCatalog(
 bool TableCatalogObject::InsertLayout(
     std::shared_ptr<const storage::Layout> layout) {
   // Invalid object
-  if (layout == nullptr) {
+  if (!layout || (layout->GetOid() == INVALID_OID)) {
     return false;
   }
 
diff --git a/src/codegen/proxy/runtime_functions_proxy.cpp b/src/codegen/proxy/runtime_functions_proxy.cpp
@@ -11,6 +11,7 @@
 //===----------------------------------------------------------------------===//
 
 #include "codegen/proxy/runtime_functions_proxy.h"
+
 #include "codegen/proxy/data_table_proxy.h"
 #include "codegen/proxy/tile_group_proxy.h"
 #include "codegen/proxy/zone_map_proxy.h"
@@ -24,8 +25,6 @@ DEFINE_TYPE(ColumnLayoutInfo, "peloton::ColumnLayoutInfo",
 DEFINE_TYPE(AbstractExpression, "peloton::expression::AbstractExpression",
             MEMBER(opaque));
 
-DEFINE_TYPE(Schema, "peloton::catalog::Schema", MEMBER(opaque));
-
 DEFINE_METHOD(peloton::codegen, RuntimeFunctions, HashCrc64);
 DEFINE_METHOD(peloton::codegen, RuntimeFunctions, GetTileGroup);
 DEFINE_METHOD(peloton::codegen, RuntimeFunctions, GetTileGroupLayout);
diff --git a/src/codegen/runtime_functions.cpp b/src/codegen/runtime_functions.cpp
@@ -102,38 +102,38 @@ void RuntimeFunctions::FillPredicateArray(
 // to skip over to find successive values of the column.
 //===----------------------------------------------------------------------===//
 void RuntimeFunctions::GetTileGroupLayout(const storage::TileGroup *tile_group,
-                                          const catalog::Schema *schema,
                                           ColumnLayoutInfo *infos,
-                                          uint32_t num_cols) {
+                                          UNUSED_ATTRIBUTE uint32_t num_cols) {
   const auto &layout = tile_group->GetLayout();
-  // For LayoutType::ROW, the tile group contains a single tile
-  // and all the columns are in the same order as the table schema.
-  if (layout.IsRowStore()) {
-    auto tuple_location = tile_group->GetTile(0)->GetTupleLocation(0);
-    auto stride = schema->GetLength();
-    for (uint32_t col_idx = 0; col_idx < num_cols; col_idx++) {
-      infos[col_idx].column = tuple_location + schema->GetOffset(col_idx);
-      infos[col_idx].stride = stride;
-      infos[col_idx].is_columnar = false;
-    }
-    return;
-  }
-  for (uint32_t col_idx = 0; col_idx < num_cols; col_idx++) {
-    // Map the current column to a tile and a column offset in the tile
-    oid_t tile_offset, tile_column_offset;
-    layout.LocateTileAndColumn(col_idx, tile_offset, tile_column_offset);
+  UNUSED_ATTRIBUTE oid_t last_col_idx = INVALID_OID;
 
-    // Now grab the column information
-    auto *tile = tile_group->GetTile(tile_offset);
-    auto *tile_schema = tile->GetSchema();
-    infos[col_idx].column =
-        tile->GetTupleLocation(0) + tile_schema->GetOffset(tile_column_offset);
-    infos[col_idx].stride = tile_schema->GetLength();
-    infos[col_idx].is_columnar = tile_schema->GetColumnCount() == 1;
-    LOG_TRACE("Col [%u] start: %p, stride: %u, columnar: %s", col_idx,
-              infos[col_idx].column, infos[col_idx].stride,
-              infos[col_idx].is_columnar ? "true" : "false");
+  auto tile_map = layout.GetTileMap();
+  // Find the mapping for each tile in the layout.
+  for (auto tile_entry : tile_map) {
+    // Get the tile schema.
+    auto tile_idx = tile_entry.first;
+    auto *tile = tile_group->GetTile(tile_idx);
+    auto tile_schema = tile->GetSchema();
+    // Map the current column to a tile and a column offset in the tile.
+    for (auto column_entry : tile_entry.second) {
+      // Now grab the column information
+      oid_t col_idx = column_entry.first;
+      oid_t tile_col_offset = column_entry.second;
+      // Ensure that the col_idx is within the num_cols range
+      PELOTON_ASSERT(col_idx < num_cols);
+      infos[col_idx].column =
+          tile->GetTupleLocation(0) + tile_schema->GetOffset(tile_col_offset);
+      infos[col_idx].stride = tile_schema->GetLength();
+      infos[col_idx].is_columnar = tile_schema->GetColumnCount() == 1;
+      last_col_idx = col_idx;
+      LOG_TRACE("Col [%u] start: %p, stride: %u, columnar: %s", col_idx,
+                infos[col_idx].column, infos[col_idx].stride,
+                infos[col_idx].is_columnar ? "true" : "false");
+    }
   }
+  // Ensure that ColumnLayoutInfo for each column has been populated.
+  PELOTON_ASSERT((last_col_idx != INVALID_OID) &&
+                 (last_col_idx == (num_cols - 1)));
 }
 
 void RuntimeFunctions::ThrowDivideByZeroException() {
diff --git a/src/codegen/tile_group.cpp b/src/codegen/tile_group.cpp
@@ -88,15 +88,12 @@ llvm::Value *TileGroup::GetTileGroupId(CodeGen &codegen,
 std::vector<TileGroup::ColumnLayout> TileGroup::GetColumnLayouts(
     CodeGen &codegen, llvm::Value *tile_group_ptr,
     llvm::Value *column_layout_infos) const {
-  llvm::Value *schema_ptr =
-      codegen->CreateIntToPtr(codegen.Const64((int64_t)&schema_),
-                              SchemaProxy::GetType(codegen)->getPointerTo());
 
   // Call RuntimeFunctions::GetTileGroupLayout()
   uint32_t num_cols = schema_.GetColumnCount();
-  codegen.Call(RuntimeFunctionsProxy::GetTileGroupLayout,
-               {tile_group_ptr, schema_ptr, column_layout_infos,
-                codegen.Const32(num_cols)});
+  codegen.Call(
+      RuntimeFunctionsProxy::GetTileGroupLayout,
+      {tile_group_ptr, column_layout_infos, codegen.Const32(num_cols)});
 
   // Collect <start, stride, is_columnar> triplets of all columns
   std::vector<TileGroup::ColumnLayout> layouts;
diff --git a/src/include/catalog/catalog.h b/src/include/catalog/catalog.h
@@ -120,12 +120,29 @@ class Catalog {
                          concurrency::TransactionContext *txn,
                          bool is_catalog = false);
 
-  // Create a new layout
+  /**
+   * @brief   create a new layout for a table
+   * @param   database_oid  database to which the table belongs to
+   * @param   table_oid     table to which the layout has to be added
+   * @param   column_map    column_map of the new layout to be created
+   * @param   txn           TransactionContext
+   * @return  shared_ptr    shared_ptr to the newly created layout in case of
+   *                        success. nullptr in case of failure.
+   */
   std::shared_ptr<const storage::Layout> CreateLayout(
       oid_t database_oid, oid_t table_oid, const column_map_type &column_map,
       concurrency::TransactionContext *txn);
 
-  // Create a new layout and set it as the default for the table
+  /**
+   * @brief   create a new layout for a table and make it the default if
+   *          if the creating is successsful.
+   * @param   database_oid  database to which the table belongs to
+   * @param   table_oid     table to which the layout has to be added
+   * @param   column_map    column_map of the new layout to be created
+   * @param   txn           TransactionContext
+   * @return  shared_ptr    shared_ptr to the newly created layout in case of
+   *                        success. nullptr in case of failure.
+   */
   std::shared_ptr<const storage::Layout> CreateDefaultLayout(
       oid_t database_oid, oid_t table_oid, const column_map_type &column_map,
       concurrency::TransactionContext *txn);
@@ -158,7 +175,14 @@ class Catalog {
   ResultType DropIndex(oid_t database_oid, oid_t index_oid,
                        concurrency::TransactionContext *txn);
 
-  // Delete a layout using its database_oid, table_oid and layout_oid
+  /** @brief   Drop layout
+   * tile_groups
+   * @param   database_oid    the database to which the table belongs
+   * @param   table_oid       the table to which the layout belongs
+   * @param   layout_oid      the layout to be dropped
+   * @param   txn             TransactionContext
+   * @return  ResultType(SUCCESS or FAILURE)
+   */
   ResultType DropLayout(oid_t database_oid, oid_t table_oid, oid_t layout_oid,
                         concurrency::TransactionContext *txn);
   //===--------------------------------------------------------------------===//
diff --git a/src/include/catalog/catalog_defaults.h b/src/include/catalog/catalog_defaults.h
@@ -53,7 +53,7 @@ namespace catalog {
 // Reserved schema oid
 // "public" for default schema, and "pg_catalog" schema for catalog tables
 #define CATALOG_SCHEMA_OID (0 | SCHEMA_OID_MASK)
-#define DEFUALT_SCHEMA_OID (1 | SCHEMA_OID_MASK)
+#define DEFAULT_SCHEMA_OID (1 | SCHEMA_OID_MASK)
 #define CATALOG_SCHEMA_NAME "pg_catalog"
 #define DEFAULT_SCHEMA_NAME "public"
 
diff --git a/src/include/catalog/layout_catalog.h b/src/include/catalog/layout_catalog.h
@@ -39,11 +39,6 @@ namespace catalog {
 
 class LayoutCatalog : public AbstractCatalog {
  public:
-  // Global Singleton, only the first call requires passing parameters.
-  static LayoutCatalog *GetInstance(
-      storage::Database *pg_catalog = nullptr,
-      type::AbstractPool *pool = nullptr,
-      concurrency::TransactionContext *txn = nullptr);
 
   LayoutCatalog(storage::Database *pg_catalog, type::AbstractPool *pool,
                 concurrency::TransactionContext *txn);
diff --git a/src/include/codegen/proxy/runtime_functions_proxy.h b/src/include/codegen/proxy/runtime_functions_proxy.h
@@ -51,7 +51,6 @@ PROXY(RuntimeFunctions) {
 
 TYPE_BUILDER(ColumnLayoutInfo, codegen::RuntimeFunctions::ColumnLayoutInfo);
 TYPE_BUILDER(AbstractExpression, expression::AbstractExpression);
-TYPE_BUILDER(Schema, catalog::Schema);
 
 }  // namespace codegen
 }  // namespace peloton
diff --git a/src/include/codegen/runtime_functions.h b/src/include/codegen/runtime_functions.h
@@ -70,7 +70,6 @@ class RuntimeFunctions {
 
   // Get the column configuration for every column in the tile group
   static void GetTileGroupLayout(const storage::TileGroup *tile_group,
-                                 const catalog::Schema *schema,
                                  ColumnLayoutInfo *infos, uint32_t num_cols);
 
   static void ThrowDivideByZeroException();
diff --git a/src/include/common/internal_types.h b/src/include/common/internal_types.h
@@ -1406,6 +1406,11 @@ typedef std::unordered_set<expression::AbstractExpression *,
  */
 typedef std::map<oid_t, std::pair<oid_t, oid_t>> column_map_type;
 
+/* tile_map_type used to store the mapping between a tile and its columns
+ * <tile index> to vector{<original column index, tile column offset>}
+ */
+typedef std::map<oid_t, std::vector<std::pair<oid_t, oid_t>>> tile_map_type;
+
 //===--------------------------------------------------------------------===//
 // Wire protocol typedefs
 //===--------------------------------------------------------------------===//
diff --git a/src/include/storage/layout.h b/src/include/storage/layout.h
@@ -92,7 +92,10 @@ class Layout : public Printable {
   /** @brief  Returns the number of columns in the layout. */
   oid_t GetColumnCount() const { return num_columns_; }
 
-  /** @brief  Constructs the schema for the given layout.  Thid function
+  /** @brief Returns the tile-columns map for each tile in the TileGroup. */
+  tile_map_type GetTileMap() const;
+
+  /** @brief  Constructs the schema for the given layout.  This function
    *          is used only in TempTables and LogicalTiles.
    */
   std::vector<catalog::Schema> GetLayoutSchemas(
diff --git a/src/storage/data_table.cpp b/src/storage/data_table.cpp
@@ -1239,7 +1239,7 @@ void SetTransformedTileGroup(storage::TileGroup *orig_tile_group,
 
   // Check that both tile groups have the same schema
   // Currently done by checking that the number of columns are equal
-  // TODO Pooja: Handle schena equality for multiple schema versions.
+  // TODO Pooja: Handle schema equality for multiple schema versions.
   UNUSED_ATTRIBUTE auto new_column_count = new_layout.GetColumnCount();
   UNUSED_ATTRIBUTE auto orig_column_count = orig_layout.GetColumnCount();
   PELOTON_ASSERT(new_column_count == orig_column_count);
diff --git a/src/storage/layout.cpp b/src/storage/layout.cpp
@@ -139,6 +139,37 @@ oid_t Layout::GetTileIdFromColumnId(oid_t column_id) const {
   return tile_offset;
 }
 
+tile_map_type Layout::GetTileMap() const {
+  tile_map_type tile_map;
+
+  if (layout_type_ == LayoutType::ROW) {
+    // Row store layout, hence all columns are contained in tile 0.
+    // The column_offset is always the same as column_id.
+    tile_map[0] = {};
+    for (oid_t column_id = 0; column_id < num_columns_; column_id++) {
+      tile_map[0].emplace_back(column_id, column_id);
+    }
+  } else if (layout_type_ == LayoutType::COLUMN) {
+    // Column store layout, hence all columns are contained in separate tiles.
+    // The column_offset within the tile is always 0.
+    for (oid_t column_id = 0; column_id < num_columns_; column_id++) {
+      tile_map[column_id] = {std::make_pair(column_id, 0)};
+    }
+  } else {
+    // Hybrid layout, hence figure out the mapping from the column_layout_.
+    for (auto entry : column_layout_) {
+      auto column_id = entry.first;
+      auto tile_id = entry.second.first;
+      auto column_offset = entry.second.second;
+      if (tile_map.find(tile_id) == tile_map.end()) {
+        tile_map[tile_id] = {};
+      }
+      tile_map[tile_id].emplace_back(column_id, column_offset);
+    }
+  }
+  return tile_map;
+}
+
 oid_t Layout::GetTileColumnOffset(oid_t column_id) const {
   oid_t tile_column_id, tile_offset;
   LocateTileAndColumn(column_id, tile_offset, tile_column_id);
diff --git a/test/catalog/catalog_test.cpp b/test/catalog/catalog_test.cpp
diff --git a/test/executor/drop_test.cpp b/test/executor/drop_test.cpp
diff --git a/test/optimizer/optimizer_test.cpp b/test/optimizer/optimizer_test.cpp
diff --git a/test/storage/database_test.cpp b/test/storage/database_test.cpp
diff --git a/test/tuning/layout_tuner_test.cpp b/test/tuning/layout_tuner_test.cpp

Original file line number	Diff line number	Diff line change
`@@ -338,7 +338,7 @@ TableCatalog::TableCatalog(`
`338`	`338`	`bool TableCatalogObject::InsertLayout(`
`339`	`339`	`std::shared_ptr<const storage::Layout> layout) {`
`340`	`340`	`// Invalid object`
`341`		`- if (layout == nullptr) {`
	`341`	`+ if (!layout \|\| (layout->GetOid() == INVALID_OID)) {`
`342`	`342`	`return false;`
`343`	`343`	`}`
`344`	`344`