Schema - Layout decoupling #1327

poojanilangekar · 2018-04-24T01:06:29Z

This PR decouples the logical schema from the physical layout of the data. Previously, each TileGroup contained a vector of schemas for which violates the notion of the storage layer being a physical store. We also had cases where the LayoutTuner modifies the column_map which ended up modifying the schemas vector via the DataTable. There were multiple other instances where the Schema was used to get the layout of a TileGroup while all we needed was the physical mapping between column_ids and a <tile_id, column_offset> pair. This PR now makes it possible to make schema changes like changing column name, adding & dropping columns without making any calls to the storage layer (i.e., It can now be done entirely in the catalog).

Changes:

Added Layout and LayoutCatalog classes to store the layout in a physical object and the pg_layout.
Added a default_layotut_ object to each table (replacing default_partition) and tile_group_layout_ to each TileGroup.
Persist the HYBRID layouts to the pg_catalog table. The two pre-defined layouts, ROW and COLUMN stores are not persisted.
Added support in the LLVM engine to read the Layout on a per TileGroup while invoking the TableScanTranslator.
Modified the LLVM updater and inserter to check that we insert new versions only if the TileGroup has a LayoutType::ROW layout.
Modified LayoutTuner to tune the Layout rather than the column_map. Additionally, the tuning decisions are now transactional.
Added the pg_catalog to the SystemsCatalog to ensure that the each layout is stored in a catalog with respect its database.
Minor cleanup in the catalog (some typos and a couple of magic numbers which caused tests to fail).

Testing:

Modified the LayoutTuner test to ensure it works as expected after the refactor.
Minor changes in the existing TileGroupLayoutTest and SeqScanTest which ensures that the old engine works as expected.
Added tests to the TableScanTranslatorTests to replicate the tests in TileGroupLayoutTest and SeqScanTest. The tests check that the LLVM engine can perform scans on tables different layouts and on tables where each TileGroup has a different Layout.
Added a test to CatalogTests to ensure that the pg_layout stores and retrieves data as expected and its state is transactional.

Disclaimer:

Each Tile still contains a Schema which will be used by the old engine, the GetValue and SetValue functions and (currently) the LLVM engines to determine the stride in case of a HYBRID layout. I believe we have to continue supporting the GetValue and SetValue functions even after the old engine is deprecated because we will be needing these interfaces in the Catalog and possibly the Optimizer.
In order to make the new layout change compatible with the LogicalTile generated by the executor engine, I had to reconstruct the Schemas. This isn't very efficient but the idea is that it is currently only used in tests and will soon be deprecated.
Unlike the other Catalogs (i.e., ColumnCatalog, StatsCatalog), the LayoutCatalog directly returns a Layout rather than a LayoutCatalogObject since the API are designed to be read only. In essence, the layout is immutable because if you were changing the layout, you need to change the physical representation. So it felt redundant to create a read only object to be returned by the catalog.

@mengranwo Can you please review the catalog changes?
@pmenon Can you please review the changes to the LLVM engine? Mainly, RuntimeFunctions, Inserter, Updater, TestingCodegenUtil and TableScanTranslatorTests.
@pervazea Can you please review the overall changes? I have modified a few APIs in the old engine. The current code doesn't break the existing tests and it seems to function as expected. But please let me know if you think there is anything I need to change.
@jarulraj I know you may not have the bandwidth for this, but if possible could you please review the changes to LayoutTuner and LayoutTunerTest. Nobody else has modified that code and it would be great if you could take a look.

coveralls · 2018-04-24T01:29:27Z

Coverage increased (+0.1%) to 77.551% when pulling 38a5614 on poojanilangekar:master into d68ab71 on cmu-db:master.

mengranwo · 2018-05-04T01:19:30Z

src/catalog/catalog.cpp

+  }
+  return new_layout;
+}
+


I just wondered what is the difference between function CreateLayout() and CreateDefaultLayout() cause they both have the same parameters passes in and same returned value. Is CreateLayout() only a helper function that is being called in test cases?

And from my understanding, only HYBRID layout type will be persist in pg_layout catalog table, right?

In the current transformation infrastructure, the layouts are changed in two contexts:

Transform a specific TileGroup. In this case we want to change the layout of that TileGroup but not the default_layout_ of the table (used to allocate new TileGroups).

The LayoutTuner changes the default_layout_ of the entire table based on its tuning mechanism.
I have provided the APIs to support both operations, however, we might later want to change this because currently the Inserter only supports inserts for LayoutType::Row. I believe this can be decided once the tuner is integrated into the system.

Yes, we only need to persist HYBRID layouts because they contain real mapping information. For ROW and COLUMN layouts, you can reconstruct it as long as you know the number of columns.

mengranwo · 2018-05-04T01:39:59Z

src/include/storage/data_table.h


-  column_map_type GetDefaultLayout() const;
+  inline void ResetDefaultLayout(LayoutType type = LayoutType::ROW) {


Can you add more comments to clarify under what circumstances the Default LayoutType would be set to COLUMN(Seem like only appears in test cases)

Yes, you are right, currently it is only done in test cases. However, once the tuner is in place, the brain may decide that a particular table is always used for OLAP workloads and hence it might make sense to transform it to COLUMN layout.

mengranwo · 2018-05-04T01:50:35Z

src/catalog/table_catalog.cpp

+    return false;
+  }
+
+  valid_layout_objects_ = true;


Why set to be true?

I have made this function similar to TableCatalog::InsertColumnObject. My understanding is that this updates the validity of the TableCatalogObject cache and makes sure that subsequent requests are serviced by the cache directly. Was this assumption incorrect? Please let me know if i should change this.

From our logic within TableCatalogObject, we only the parameter valid_table_objects_ when ALL the table objects are inserted into cache. For your case, set valid_layout_objects_ only when all the layout objects from this table is inserted into txn cache.

mengranwo · 2018-05-04T01:57:22Z

src/gc/gc_manager.cpp

-                tile->pool->Free(varlen_ptr);
-              }
-          }
+void GCManager::CheckAndReclaimVarlenColumns(storage::TileGroup *tile_group,


Anything else has been change inside gc_manager.cpp except formatting and change the name of pass in parameter?

Yes, There is no tile_schemas object in the TileGroup. So it changes the way we check whether or not we are freeing a VARCHAR attribute.

mengranwo · 2018-05-04T02:02:25Z

src/include/catalog/table_catalog.h

@@ -73,6 +79,15 @@ class TableCatalogObject {
  std::shared_ptr<ColumnCatalogObject> GetColumnObject(
      const std::string &column_name, bool cached_only = false);

+  // Evict all layouts from the cache


I may remember this wrong, but last time we talked you were saying add an extra column in pg_table to record the largest layout_id for each data table.

mengranwo · 2018-05-04T02:58:03Z

test/catalog/catalog_test.cpp

+  txn_manager.CommitTransaction(txn);
+
+  txn = txn_manager.BeginTransaction();
+  auto table =


Try to use table catalog object instead of data table to obtain information like database oid and table oid.

mengranwo · 2018-05-04T03:06:02Z

@poojanilangekar
Hi, this PR looks good for me except for some minor changes involved and some questions I posted in the comment area. I mainly focus on the logic of catalog & its corresponding test cases and I also take a look at the layout.h/layout.cpp, it stands.

And thank you for fix the typo in DEFAULT_SCHEMA_NAME!

mengranwo · 2018-05-04T13:09:09Z

test/catalog/catalog_test.cpp

+  // Drop the default layout.
+  auto default_layout_oid = default_layout->GetOid();
+  txn = txn_manager.BeginTransaction();
+  EXPECT_EQ(ResultType::SUCCESS, catalog->DropLayout(database_oid, table_oid,


You said Persist the HYBRID layouts to the pg_catalog table. The two pre-defined layouts, ROW and COLUMN stores are not persisted.
I may understand this wrong, the word ** Persist** means store this information in catalog table, right? If that is true, why DropLayout(default_layout_oid) returns SUCCESS? Since default_layout is ROW stored, it should not be inserted into pg_layout?

Yes, you are correct.
I was returning SUCCESS to indicate that the layout has been removed from pg_layout.

The initial default_layout_ of each table is always ROW store. However, it can be changed via CreateDefaultLayout function, wherein it creates a layout, persists it in pg_layout and updates the DataTable. In the case of this test case, I transformed the table to contain a HYBRID layout as the default_layout_ of the table.

Does this test case make sense? Or should I change it?

Yeah, it makes sense now. Thanks for your explanation

mengranwo

One more thing I found confusing.

pervazea

Initial comments added. Will read further. Mostly looks good.

pervazea · 2018-05-04T18:02:44Z

src/catalog/catalog.cpp

@@ -161,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, DEFUALT_SCHEMA_NAME, pool_.get(), txn);
+      DEFUALT_SCHEMA_OID, DEFAULT_SCHEMA_NAME, pool_.get(), txn);


Might as well fix:
DEFUALT_SCHEMA_OID to be DEFAULT_....

pervazea · 2018-05-04T18:14:09Z

src/catalog/catalog.cpp

+}
+
+/*
+ * @brief   create a new layout for a table and make it the deafult if


deafult -> default

pervazea · 2018-05-04T18:15:35Z

src/catalog/catalog.cpp

@@ -555,6 +561,63 @@ ResultType Catalog::CreateIndex(
  return ResultType::SUCCESS;
 }

+/*


The headers documenting the functions should be in the .h file (not the .cpp file).

pervazea · 2018-05-04T18:23:06Z

src/catalog/catalog.cpp

+ * @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)


Delete word TransactionContext? Just a ResultType.

pervazea · 2018-05-04T18:27:56Z

src/catalog/table_catalog.cpp

+bool TableCatalogObject::InsertLayout(
+    std::shared_ptr<const storage::Layout> layout) {
+  // Invalid object
+  if (layout == nullptr) {


Do you want to be nice and permit nullptr or should this be an assertion that it is non-null?

I followed the existing TableCatalog convention. I have updated it to check for INVALID_OID as well.
@mengranwo Do you think this is fine or should we change this and all other instances?

pervazea · 2018-05-04T18:38:28Z

src/storage/data_table.cpp

+
+  // 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.


schena -> schema
Haven't looked at where / how equality is used yet, but ...
Is equality via same number of columns sufficient? Should equality also check equality of column types?

Done.
Currently it is only called in tests. I agree it is not sufficient. The reason this code existed earlier is because we didn't support schema changes. So a table always had the same schema and so the old code worked fine. I plan to add additional checks once we have the schema changes in place.

pervazea · 2018-05-04T18:58:37Z

src/tuning/layout_tuner.cpp

+  if (catalog->CreateDefaultLayout(database_oid, table_oid, column_map, txn) ==
+      nullptr) {
+    txn_manager.AbortTransaction(txn);
+    LOG_DEBUG("Layout Update to failed.");


What is the correct thing to do here?
void UpdateDefaultPartition, so no success/fail returnable at the moment, but the function, I assume, expects success. Therefore throwing an assertion here might be more correct. Failing with just a debug message is probably not ok.

Actually, this is going to be a part of the brain code to Tune the table layout. A transaction may fail because it conflicts with other concurrent updates. In an ideal case it must fail because tuning is a background task while the other transaction would be user specified. Since this is a background process, in case of failure, it would come around in the next iteration and attempt the change again.
I am not sure if it would be appropriate to throw an exception when the failure was due to an internal event rather than a user action. What do you think?

Ok. I think it is fine not to throw an exception.
This is not your issue, but I'm wondering why the tuner doesn't apply a bit more intelligence. Just re-trying an operation without keeping any stats or ensuring you don't end up re-trying it forever, seems optimistic.

I will change this function to return a bool to indicate success. Later, @malin1993ml can determine how to handle failures in the Tuner as a part of thebrain. Can you please review the rest of the changes? This needs to be merged in order to support schema changes.

Also the query_logger_test failure is the same as #1307. So I am assuming we can ignore it.

@pervazea I have changed that part. Please let me know if there are any other changes I need to make.

pervazea · 2018-05-04T19:06:16Z

test/catalog/catalog_test.cpp

  EXPECT_EQ(
-      11,
+      CATALOG_TABLES_COUNT + 3,


The +3 hardcoded number... improvement that now using CATALOG_TABLES_COUNT. Can we fix it all the way by using define constant instead of the 3 or updating CATALOG_TABLES_COUNT if that is the right solution?

pervazea · 2018-05-04T19:10:48Z

test/tuning/layout_tuner_test.cpp

@@ -6,7 +6,7 @@
 //
 // Identification: test/tuning/layout_tuner_test.cpp
 //
-// Copyright (c) 2015-16, Carnegie Mellon University Database Group
+// Copyright (c) 2015-16, Carnegie Mellon University Databa e Group


Accidental deletion of s.

pervazea · 2018-05-04T19:21:20Z

src/include/catalog/layout_catalog.h

+
+class LayoutCatalog : public AbstractCatalog {
+ public:
+  // Global Singleton, only the first call requires passing parameters.


Aren't we trying to eliminate our use of singletons?

That was accidental, removed it.

poojanilangekar

@mengranwo @pervazea Thanks for reviewing the PR. I have addressed all your comments inline. Please take a look at it again and let me know if I need to make any other changes.

poojanilangekar · 2018-05-04T15:43:44Z

src/gc/gc_manager.cpp

-                tile->pool->Free(varlen_ptr);
-              }
-          }
+void GCManager::CheckAndReclaimVarlenColumns(storage::TileGroup *tile_group,


Yes, There is no tile_schemas object in the TileGroup. So it changes the way we check whether or not we are freeing a VARCHAR attribute.

poojanilangekar · 2018-05-04T23:16:21Z

src/catalog/catalog.cpp

@@ -161,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, DEFUALT_SCHEMA_NAME, pool_.get(), txn);
+      DEFUALT_SCHEMA_OID, DEFAULT_SCHEMA_NAME, pool_.get(), txn);


poojanilangekar · 2018-05-04T23:16:34Z

src/catalog/catalog.cpp

+}
+
+/*
+ * @brief   create a new layout for a table and make it the deafult if


poojanilangekar · 2018-05-05T05:57:30Z

test/catalog/catalog_test.cpp

+  txn_manager.CommitTransaction(txn);
+
+  txn = txn_manager.BeginTransaction();
+  auto table =


poojanilangekar · 2018-05-05T06:41:57Z

src/catalog/catalog.cpp

+ * @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)


poojanilangekar · 2018-05-05T07:35:53Z

test/tuning/layout_tuner_test.cpp

@@ -6,7 +6,7 @@
 //
 // Identification: test/tuning/layout_tuner_test.cpp
 //
-// Copyright (c) 2015-16, Carnegie Mellon University Database Group
+// Copyright (c) 2015-16, Carnegie Mellon University Databa e Group


poojanilangekar · 2018-05-05T07:36:43Z

src/include/catalog/layout_catalog.h

+
+class LayoutCatalog : public AbstractCatalog {
+ public:
+  // Global Singleton, only the first call requires passing parameters.


That was accidental, removed it.

poojanilangekar · 2018-05-05T07:43:32Z

test/catalog/catalog_test.cpp

+  // Drop the default layout.
+  auto default_layout_oid = default_layout->GetOid();
+  txn = txn_manager.BeginTransaction();
+  EXPECT_EQ(ResultType::SUCCESS, catalog->DropLayout(database_oid, table_oid,


Yes, you are correct.
I was returning SUCCESS to indicate that the layout has been removed from pg_layout.

The initial default_layout_ of each table is always ROW store. However, it can be changed via CreateDefaultLayout function, wherein it creates a layout, persists it in pg_layout and updates the DataTable. In the case of this test case, I transformed the table to contain a HYBRID layout as the default_layout_ of the table.

Does this test case make sense? Or should I change it?

poojanilangekar · 2018-05-05T07:54:53Z

src/storage/data_table.cpp

+
+  // 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.


Done.
Currently it is only called in tests. I agree it is not sufficient. The reason this code existed earlier is because we didn't support schema changes. So a table always had the same schema and so the old code worked fine. I plan to add additional checks once we have the schema changes in place.

poojanilangekar · 2018-05-05T08:00:23Z

src/tuning/layout_tuner.cpp

+  if (catalog->CreateDefaultLayout(database_oid, table_oid, column_map, txn) ==
+      nullptr) {
+    txn_manager.AbortTransaction(txn);
+    LOG_DEBUG("Layout Update to failed.");


Actually, this is going to be a part of the brain code to Tune the table layout. A transaction may fail because it conflicts with other concurrent updates. In an ideal case it must fail because tuning is a background task while the other transaction would be user specified. Since this is a background process, in case of failure, it would come around in the next iteration and attempt the change again.
I am not sure if it would be appropriate to throw an exception when the failure was due to an internal event rather than a user action. What do you think?

pmenon

I did a quick aesthetic/style review. Will take a closer look at design soon.

pmenon · 2018-05-07T13:34:54Z

src/codegen/updater.cpp

+  // Get the tile offset assuming that it is still a row store
+  // Hence the Tile offset is 0.
+  auto layout = tile_group->GetLayout();
+  PELOTON_ASSERT(layout.IsRowStore());


You probably want to make this a const reference to avoid the copy.

You'll need to add UNUSED_ATTRIBUTE to pass Release builds.

pmenon · 2018-05-07T13:45:01Z

src/include/catalog/table_catalog.h

+
+namespace storage {
+class Layout;
+}


Close namespace.

pmenon · 2018-05-07T13:45:29Z

src/include/codegen/runtime_functions.h

@@ -17,6 +17,10 @@

 namespace peloton {

+namespace catalog {
+class Schema;
+}


Close namespace.

pmenon · 2018-05-07T13:46:53Z

src/include/common/internal_types.h

+/* 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;


I'm not sold that these are common enough types for the whole system that they should be here ... but I guess it's okay.

pmenon · 2018-05-07T13:47:54Z

src/include/storage/data_table.h

@@ -247,9 +249,17 @@ class DataTable : public AbstractTable {

  void ClearLayoutSamples();

-  void SetDefaultLayout(const column_map_type &layout);
+  inline void SetDefaultLayout(std::shared_ptr<const Layout> new_layout) {


You can remove the inline keyword.

pmenon · 2018-05-07T13:50:18Z

src/include/storage/layout.h

+
+namespace catalog {
+class Schema;
+}


Close namespace.

pmenon · 2018-05-07T13:50:40Z

src/include/storage/layout.h

+//
+// Identification: src/include/storage/layout.h
+//
+// Copyright (c) 2015-18, Carnegie Mellon University Database Group


If you use the formatter.py script, it'll add the properly formatted copyright.

pmenon · 2018-05-07T13:50:54Z

src/include/storage/layout.h

+  Layout(const column_map_type &column_map, oid_t layout_oid);
+
+  /** @brief  Check whether this layout is a row store. */
+  inline bool IsRowStore() const { return (layout_type_ == LayoutType::ROW); }


Remove inline.

pmenon · 2018-05-07T13:51:01Z

src/include/storage/layout.h

+  inline bool IsRowStore() const { return (layout_type_ == LayoutType::ROW); }
+
+  /** @brief  Check whether this layout is a column store. */
+  inline bool IsColumnStore() const {


Remove inline.

pmenon · 2018-05-07T13:52:39Z

test/codegen/table_scan_translator_test.cpp

@@ -35,6 +36,122 @@ class TableScanTranslatorTest : public PelotonCodeGenTest {
    CreateAndLoadAllColsTable();
  }

+  void ExecuteTileGroupTest(peloton::LayoutType layout_type) {


Can you modify the base test class functions to accept a LayoutType? Then you should reuse the create/loading functions rather than creating a one-off function. This also allows other tests to use custom table layouts for free.

I have addressed all your other comments except this one.
From what I understood, the PelotonCodeGenTest does not use the LLVM inserter. The LLVM inserter would only insert into LayoutType::ROW.
Can I make that assumption or do we plan to change that later?

Correct, the tests do not use the inserter, but instead directly insert into the table here.

@pmenon I have modified the CreateTestTables function to get the layout as a parameter. However, since its called from the constructor, so the entire test suit would get called with the given layout. I have however moved the CreateAndLoad function to the base class. Please let me know if you want me to change anything else.

pervazea

This looks good. I did not read the logic as completely as I would have liked, as there is a lot of code here.

Comments added are all style issues. The main one is that oid_t is used in many places where uint32_t or other flavor of int should be used.

pervazea · 2018-05-09T19:36:07Z

src/gc/gc_manager.cpp

+void GCManager::CheckAndReclaimVarlenColumns(storage::TileGroup *tile_group,
+                                             oid_t tuple_id) {
+  oid_t tile_count = tile_group->tile_count_;
+  oid_t tile_col_count;


tile_count and tile_col_count should be uint32_t not oid_t.
While use of oid_t will compile and run correctly, it is confusing and incorrect style.

pervazea · 2018-05-09T19:36:30Z

src/include/catalog/catalog.h

@@ -99,7 +100,8 @@ class Catalog {
      const std::string &database_name, const std::string &schema_name,
      const std::string &table_name, std::unique_ptr<catalog::Schema>,
      concurrency::TransactionContext *txn, bool is_catalog = false,
-      oid_t tuples_per_tilegroup = DEFAULT_TUPLES_PER_TILEGROUP);
+      oid_t tuples_per_tilegroup = DEFAULT_TUPLES_PER_TILEGROUP,


oid_t -> uint32_t

pervazea · 2018-05-09T19:38:37Z

src/include/storage/tile_group.h

@@ -214,13 +190,12 @@ class TileGroup : public Printable {
  oid_t num_tuple_slots;


pervazea · 2018-05-09T19:38:48Z

src/include/storage/tile_group.h

@@ -214,13 +190,12 @@ class TileGroup : public Printable {
  oid_t num_tuple_slots;

  // number of tiles
-  oid_t tile_count;
+  oid_t tile_count_;


pervazea · 2018-05-09T19:39:18Z

src/optimizer/stats/tuple_sampler.cpp


    storage::Tile *tile = tile_group->GetTile(tile_itr);
+    const catalog::Schema &schema = *(tile->GetSchema());
+    oid_t tile_column_count = schema.GetColumnCount();


pervazea · 2018-05-09T19:47:57Z

src/include/storage/layout.h

+  double GetLayoutDifference(const storage::Layout &other) const;
+
+  /** @brief  Returns the tile_id of the given column_id. */
+  oid_t GetTileIdFromColumnId(oid_t column_id) const;


I think this should also be oid_t because the function returns a tile_id.

pervazea · 2018-05-09T19:48:08Z

src/include/storage/layout.h

+  oid_t GetTileIdFromColumnId(oid_t column_id) const;
+
+  /** @brief  Returns the column offset of the column_id. */
+  oid_t GetTileColumnOffset(oid_t column_id) const;


I think this should be oid_t because it's the column_oid within the Tile.

pervazea · 2018-05-09T19:48:16Z

src/include/storage/layout.h

+  oid_t GetTileColumnOffset(oid_t column_id) const;
+
+  /** @brief  Returns the number of columns in the layout. */
+  oid_t GetColumnCount() const { return num_columns_; }


pervazea · 2018-05-09T19:50:32Z

src/include/storage/layout.h

+  /** @brief  Sets the tile id and column id w.r.t. of the tile corresponding
+   *          to the specified tile group column id.
+   */
+  void LocateTileAndColumn(oid_t column_offset, oid_t &tile_offset,


Misnamed? More correctly something more like SetTileAndColumn?

I am sorry! This function was previously a part of TileGroup, I copied the comments as is.

I have changed it now. Essentially, we are using references here because we need to return both the tile_id and tile_column_id.

pervazea · 2018-05-09T19:53:28Z

src/storage/layout.cpp

+  PELOTON_ASSERT(num_columns_ > column_offset);
+
+  // For row store layout, tile id is always 0 and the tile
+  // column_id and tile column_id is the same.


Comment text is unclear...
and the tile column_id and tile column_id is the same

pmenon

LGTM. The design is nice, code is pretty well documented. I just had a few questions below.

pmenon · 2018-05-10T22:30:57Z

src/catalog/catalog.cpp

+  bool result =
+      pg_layout->InsertLayout(table_oid, new_layout, pool_.get(), txn);
+  if (!result) {
+    LOG_DEBUG("Failed to create a new layout for table %u", table_oid);


Should this be LOG_ERROR?

pmenon · 2018-05-10T22:39:03Z

src/catalog/catalog.cpp

+      new const storage::Layout(column_map, layout_oid));
+
+  // Add the layout the pg_layout table
+  auto pg_layout = catalog_map_[database_oid]->GetLayoutCatalog();


Not related to this PR, but this map seems to be accessed concurrently without a lock in Catalog::BootstrapSystemCatalogs()

You are right. There are other small bugs I noticed in #1356 and #1352. I will raise a Catalog cleanup PR once this gets merged in.

pmenon · 2018-05-10T22:48:41Z

src/include/storage/layout.h

+/** @brief 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;


Does it make sense to rename this to something more descriptive? Maybe TileToColumnMap?

pmenon · 2018-05-11T00:10:16Z

src/include/storage/tile_group.h

@@ -76,7 +75,7 @@ class TileGroup : public Printable {
  // Tile group constructor
  TileGroup(BackendType backend_type, TileGroupHeader *tile_group_header,
            AbstractTable *table, const std::vector<catalog::Schema> &schemas,
-            const column_map_type &column_map, int tuple_count);
+            std::shared_ptr<const Layout> layout, int tuple_count);


We're passing in a shared pointer, does that mean this can be null? If not, should we make it a reference?

Also, in terms of ownership, does the TileGroup own the layout?

The layout is actually shared among multiple TileGroups which are initialized with the same layout. We would want the ptr to be alive as long as we have atleast one TileGroup with the same.

To make sure a TileGroup always get a non-null layout, I am now throwing an exception in the TileGroupFactory if it passed a nullptr. Let me know if you want me to handle this differently.

* Added layout.h. Modified Runtime functions to handle row stores. * Modify TileGroup api * Changed Updater & Inserter to use Layout object * Remove calls from optimizer and executor * Removed all calls to schemas vector from TileGroup * Removed Calls to LocateTileAndColumn for TileGroup * Moved definition of column_map_type. Removed column_map from tile_group * Added GetInfo function * make check pases. Need to change a few more function calls * Convert PL_ASSERT -> PELOTON_ASSERT * Modified DataTable and LayoutTunerTest * Added Layout Test for codegen * Get the tests to actually use different layouts * Modified storage layer, builds successfully. Need to fix tests * Moved GetColumnMapStats -> GetColumnLayoutStats * Minor change in TableScanTranslator * Modify TransformTileGroup * Change layout.cpp to better handle HYBRID layouts * use std::make_shared * Modified tests to change calls to GetTileGroup() * make check passes. Need to add test with hybrid layout. * Modify print functions * Add TODOs for catalog * Added LayoutCatalog. Yet to modify TableCatalog * Added catalog functions * Added DeleteLayouts to delete all layouts of a table_oid * LayoutTunerTest fixed * Fix build failures * Access LayoutCatalog via the global Catalog object * Added Multi layout scan test * Fixed tests after catalog refactor * Failing LayoutCatalog test because of deserialization * Fix all tests * Added documentation * Style Fix * Address review comments + minor clean-up in the layout.h API * Change valid_layout_objects_ * Revert unused changes * Addressed Prashanth's initial comments * Modify CreateTable for tests + modify LayoutTuner * Address Pervaze's review comments. * Changed after Prashanth's review

poojanilangekar added in progress do not merge labels Apr 24, 2018

poojanilangekar force-pushed the master branch from fb7d735 to 4ed304d Compare April 24, 2018 05:20

poojanilangekar removed the do not merge label Apr 26, 2018

poojanilangekar mentioned this pull request Apr 30, 2018

Namespace(Schema) support + Catalog refactoring #1302

Merged

poojanilangekar force-pushed the master branch from 97b5b9b to 2ecd160 Compare May 1, 2018 14:48

poojanilangekar requested review from mengranwo, pmenon, pervazea and jarulraj May 2, 2018 21:13

poojanilangekar added ready_for_review and removed in progress labels May 2, 2018

mengranwo suggested changes May 4, 2018

View reviewed changes

mengranwo reviewed May 4, 2018

View reviewed changes

mengranwo suggested changes May 4, 2018

View reviewed changes

pervazea suggested changes May 4, 2018

View reviewed changes

poojanilangekar commented May 5, 2018

View reviewed changes

poojanilangekar force-pushed the master branch 3 times, most recently from c16f90a to cf631f4 Compare May 5, 2018 16:08

mengranwo previously approved these changes May 7, 2018

View reviewed changes

pmenon suggested changes May 7, 2018

View reviewed changes

poojanilangekar dismissed mengranwo’s stale review via 80a3b6f May 7, 2018 15:11

poojanilangekar force-pushed the master branch from 80a3b6f to 3f5b719 Compare May 7, 2018 15:16

pervazea suggested changes May 9, 2018

View reviewed changes

pmenon suggested changes May 11, 2018

View reviewed changes

poojanilangekar added 2 commits May 11, 2018 21:14

Added layout.h. Modified Runtime functions to handle row stores.

5d93f4e

Modify TileGroup api

e909f82

poojanilangekar added 20 commits May 11, 2018 21:14

Modify print functions

5410348

Add TODOs for catalog

8f8bf0e

Added LayoutCatalog. Yet to modify TableCatalog

faedaa1

Added catalog functions

fe1e6b4

Added DeleteLayouts to delete all layouts of a table_oid

6b36654

LayoutTunerTest fixed

1965813

Fix build failures

9489f5a

Access LayoutCatalog via the global Catalog object

7408c20

Added Multi layout scan test

4aaee91

Fixed tests after catalog refactor

429c9d0

Failing LayoutCatalog test because of deserialization

f9187cc

Fix all tests

5e62206

Added documentation

6ce9fc8

Style Fix

d7ce6cd

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

c219d7b

Change valid_layout_objects_

2ea5d7e

Revert unused changes

f256082

Addressed Prashanth's initial comments

0ff874d

Modify CreateTable for tests + modify LayoutTuner

ecb95bb

Address Pervaze's review comments.

6630805

poojanilangekar force-pushed the master branch 2 times, most recently from d2ce8d2 to d4e151f Compare May 14, 2018 17:16

Changed after Prashanth's review

cc8751a

poojanilangekar force-pushed the master branch from d4e151f to cc8751a Compare May 14, 2018 17:20

pmenon approved these changes May 14, 2018

View reviewed changes

pervazea approved these changes May 14, 2018

View reviewed changes

pervazea merged commit 5686479 into cmu-db:master May 14, 2018

pervazea added accepted and removed ready_for_review labels Jul 5, 2018


		column_map_type GetDefaultLayout() const;
		inline void ResetDefaultLayout(LayoutType type = LayoutType::ROW) {

		@@ -214,13 +190,12 @@ class TileGroup : public Printable {
		oid_t num_tuple_slots;

Schema - Layout decoupling #1327

Schema - Layout decoupling #1327

Conversation

poojanilangekar commented Apr 24, 2018 • edited Loading

coveralls commented Apr 24, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mengranwo May 5, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mengranwo commented May 4, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mengranwo left a comment

Choose a reason for hiding this comment

pervazea left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

poojanilangekar May 8, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

poojanilangekar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pmenon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pervazea left a comment

poojanilangekar commented Apr 24, 2018 •

edited

Loading

coveralls commented Apr 24, 2018 •

edited

Loading

mengranwo May 5, 2018 •

edited

Loading

mengranwo commented May 4, 2018 •

edited

Loading

poojanilangekar May 8, 2018 •

edited

Loading