Catalog code cleanup by tli2 · Pull Request #1414 · cmu-db/peloton

tli2 · 2018-06-18T21:13:40Z

Addresses issue #1398.

This PR fixes 1, 2 and renames CatalogObjects to Entries. Other minor code style fixes are included as well.

coveralls · 2018-06-18T22:22:48Z

Coverage increased (+0.04%) to 77.899% when pulling 2b04223 on tli2:tianyu-catalog-cleanup into bf7ff62 on cmu-db:master.

apavlo

Thanks for doing this. Changes are requested.

One potential problem with this PR is that we are switching catalog objects to be called 'entries', but then we are using object identifiers (oid_t) to reference them.

apavlo · 2018-06-19T11:40:19Z

  // using catalog object to retrieve meta-data
-  auto table_object = catalog::Catalog::GetInstance()->GetTableObject(
-      db_name, schema_name, table_name, txn);
+  auto table_object = catalog::Catalog::GetInstance()->GetTableObject(txn,


We should probably rename this as GetTableEntry too.

I thought about this and decided to leave this as is for the following reason. A "Table" is a CatalogEntry inside the TableCatalog, and we are getting a "Table" outside of it, which makes sense. Naming this GetTableObject instead of GetTable makes this clear that we are getting a bunch of information we have on the table (the table "object" in a system), and not the contents of the table itself. (This would make even more sense if we have a glossary somewhere explaining this and use it as a naming convention.) "GetTableEntry" has the same confusing double meaning, and "GetTableCatalogEntry" is not ambiguous but long and doesn't make much sense without looking at the return type name. So the naming here is fine, but the typename needs to renamed because the type "TableObject" wouldn't make sense on its own.

Let me know what you think.

We should vote on this. I think Get*CatalogEntry would be best.

I vote Get*CatalogEntry. I think the accuracy of function name is more important than length. And I think this name is not so long.

I'd go with apavlo, ksaito7 and vote for ... Entry.

Fine, I'll rename them to GetxxxCatalogEntry

apavlo · 2018-06-19T11:41:32Z

-  Catalog::GetInstance()->CreateTable(
-      catalog_database_name, catalog_schema_name, catalog_table_name,
-      std::unique_ptr<catalog::Schema>(catalog_table_schema), txn, true);
+  Catalog::GetInstance()->CreateTable(txn,


Unless this function also creates the DataTable object (which it shouldn't), maybe we should rename this to CreateTableEntry.

Yes. Sorry I missed this. I will rename this to TableObject or TableEntry depending on what you think makes sense for the above comment.

Ironically, I think it does create the DataTable object (Not that it should)

apavlo · 2018-06-19T11:44:52Z

-      storage::Database *pg_catalog = nullptr,
-      type::AbstractPool *pool = nullptr,
-      concurrency::TransactionContext *txn = nullptr);
+  static DatabaseCatalog *GetInstance(concurrency::TransactionContext *txn = nullptr,


Can we remove the default values?

I tried but CLion's refactor feature really doesn't like our code and couldn't handle it. We can add another issue so somebody who has time to go through these by hand can do so in the future. (I will add this under #1398 later)

apavlo · 2018-06-19T11:45:59Z

  // Insert peloton database into pg_database
-  DatabaseCatalog::GetInstance()->InsertDatabase(
-      CATALOG_DATABASE_OID, CATALOG_DATABASE_NAME, pool_.get(), txn);
+  DatabaseCatalog::GetInstance(nullptr, nullptr, nullptr)->InsertDatabase(txn,


Why pass a null txn pointer to DatabaseCatalog::GetInstance() when you actually have the txn pointer?

Oops, that CLion refactor thing I was talking about.

apavlo · 2018-06-19T11:46:50Z


  auto database_object =
-      DatabaseCatalog::GetInstance()->GetDatabaseObject(database_name, txn);
+      DatabaseCatalog::GetInstance(nullptr,


Same here. You actually have the txn pointer.

apavlo · 2018-06-19T11:47:54Z

-    const type::TypeId return_type, oid_t prolang, const std::string &func_src,
-    std::shared_ptr<peloton::codegen::CodeContext> code_context,
-    concurrency::TransactionContext *txn) {
+void Catalog::AddPlpgsqlFunction(concurrency::TransactionContext *txn,


I don't think that this should be called AddPlpgsqlFunction. You are passing in the prolang argument, so it should just be called AddFunction, right? Furthermore, we are are referring to UDFs as procedures (i.e., pg_proc table), so it really should be called AddProcedure.

I had no idea what this is supposed to do. Will change.

apavlo · 2018-06-19T11:49:33Z

    // add "internal" language
-    if (!LanguageCatalog::GetInstance().InsertLanguage("internal", pool_.get(),
-                                                       txn)) {
+    if (!LanguageCatalog::GetInstance().InsertLanguage(txn,


This is not your problem, but we should not be initializing the language table in this function.

Can you make an issue for this so that we don't forget it?

Done. #1421

into tianyu-catalog-cleanup

pervazea

First, let me say that this looks good. The changes in formatting help readability significantly.

Have added comments, but the things I feel could use some additional attention (mostly pre-existing):

Exceptions vs. PELOTON_ASSERT. Haven't analyzed the code, but it looks to me as if there are quite a few locations where they should be asserts. We are trying to enforce an internal requirement, it isn't a recoverable run-time error.
LOG_DEBUG. Unnecessary LOG_DEBUG where it should probably be LOG_TRACE. We should have a discussion about debug logging sometime, because the noise level, when one turns on tracing, makes it almost useless. To improve that situation, I think LOG_DEBUG should be used sparingly.
Use of the Proc abbreviation in function / class names. I think this should be more explicitly Procedure. While the it is mostly clear that it is Procedure and not Process, we should just be explicit.

pervazea · 2018-06-26T22:38:18Z

+    concurrency::TransactionContext *txn,
+    expression::AbstractExpression *predicate,
+    std::vector<oid_t> column_offsets) {
  if (txn == nullptr) throw CatalogException("Scan table requires transaction");


CatalogException vs. PELOTON_ASSERT.
Should this be an assert? Is it ever legitimate to call this function without a transaction? If not, it should be an ASSERT.

pervazea · 2018-06-26T22:39:11Z

+                                          std::vector<type::Value> scan_values,
+                                          std::vector<oid_t> update_columns,
+                                          std::vector<type::Value> update_values) {
  if (txn == nullptr) throw CatalogException("Scan table requires transaction");


ditto for exception vs. PELOTON_ASSERT comment above.

pervazea · 2018-06-26T22:46:57Z

-                                 concurrency::TransactionContext *txn) {
+ResultType Catalog::CreateSchema(concurrency::TransactionContext *txn,
+                                 const std::string &database_name,
+                                 const std::string &schema_name) {
  if (txn == nullptr)


Exception vs. PELOTON_ASSERT

pervazea · 2018-06-26T22:50:03Z

+                  index_name,
+                  {column_id},
+                  true,
+                  IndexType::BWTREE);
      LOG_DEBUG("Added a UNIQUE index on %s in %s.", col_name.c_str(),


LOG_DEBUG -> LOG_TRACE?

pervazea · 2018-06-26T23:05:11Z

  // Check if UDF already exists
  auto proc_catalog_obj =
-      ProcCatalog::GetInstance().GetProcByName(name, argument_types, txn);
+      ProcCatalog::GetInstance().GetProcByName(txn, name, argument_types);


I think it would be clearer and more consistent to not use Proc. So rename to Procedure

ProcedureCatalog
GetProcedureByName
InsertProcedure
etc.
The local variables IMO can stay as is, the class names and class methods though, should change.

pervazea · 2018-06-26T23:30:09Z

-                               concurrency::TransactionContext *txn) {
+void SystemCatalogs::Bootstrap(concurrency::TransactionContext *txn,
+                               const std::string &database_name) {
  LOG_DEBUG("Bootstrapping database: %s", database_name.c_str());


pervazea · 2018-06-26T23:30:52Z

+    pg_trigger_ = new TriggerCatalog(txn, database_name);
  }

  // if (!pg_proc) {


If this is dead code, remove?

pervazea · 2018-06-26T23:56:02Z


  // Maximum column name size for catalog schemas
-  static const size_t max_name_size = 64;
+  static const size_t max_name_size_ = 64;


Since we are going to change it ... lets replace with a more descriptive name. e.g. max_column_name_size_
max_name_size is very generic, could be any name.

pervazea · 2018-06-27T00:03:13Z


  enum ColumnId {
    OID = 0,
    LANNAME = 1,


Poor abbreviation. Should be LANG or LANGUAGE

pervazea · 2018-06-27T00:12:55Z

  // using catalog object to retrieve meta-data
-  auto table_object = catalog::Catalog::GetInstance()->GetTableObject(
-      db_name, schema_name, table_name, txn);
+  auto table_object = catalog::Catalog::GetInstance()->GetTableObject(txn,


I'd go with apavlo, ksaito7 and vote for ... Entry.

…u-catalog-cleanup

pervazea

As per conversation with Tian Yu, naming changes done, exception/assert and logging added to follow on issue.

stale

* Catalog code cleanup * Rename "XXXObject" to "CatalogEntry" * Rename AddPlpgsqlFunction

Catalog code cleanup

bf70821

tli2 requested a review from pervazea June 18, 2018 21:13

tli2 added the ready_for_review label Jun 18, 2018

Merge branch 'master' into tianyu-catalog-cleanup

6ba5b94

apavlo previously requested changes Jun 19, 2018

View reviewed changes

This was referenced Jun 19, 2018

Clean-up Catalog Infrastructure #1398

Open

Constraint refactoring #1415

Merged

tli2 added 3 commits June 19, 2018 11:00

Address some code review comments.

39353a2

Merge branch 'tianyu-catalog-cleanup' of https://github.com/tli2/peloton

ca376df

into tianyu-catalog-cleanup

Merge branch 'master' into tianyu-catalog-cleanup

14327a4

ksaito7 mentioned this pull request Jun 19, 2018

Add a column length in ColumnCatalogObject and pg_attribute #1319

Closed

Merge branch 'master' into tianyu-catalog-cleanup

2b04223

ksaito7 mentioned this pull request Jun 26, 2018

Settings refactoring #1432

Open

pervazea suggested changes Jun 27, 2018

View reviewed changes

Merge branch 'master' of https://github.com/cmu-db/peloton into tiany…

13dcf3f

…u-catalog-cleanup

tli2 added in progress and removed ready_for_review labels Jun 27, 2018

tli2 added 2 commits June 27, 2018 12:07

Rename "XXXObject" to "CatalogEntry"

364769d

Rename AddPlpgsqlFunction

85baabb

pervazea approved these changes Jun 27, 2018

View reviewed changes

Merge branch 'master' into tianyu-catalog-cleanup

b8868eb

tli2 added accepted and removed in progress labels Jun 27, 2018

tli2 merged commit d22bd24 into cmu-db:master Jun 27, 2018

tli2 deleted the tianyu-catalog-cleanup branch June 27, 2018 19:41

mtunique pushed a commit to mtunique/peloton that referenced this pull request Apr 16, 2019

Catalog code cleanup (cmu-db#1414)

c2a0763

* Catalog code cleanup * Rename "XXXObject" to "CatalogEntry" * Rename AddPlpgsqlFunction

Conversation

tli2 commented Jun 18, 2018

Uh oh!

coveralls commented Jun 18, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

apavlo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tli2 Jun 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tli2 Jun 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tli2 Jun 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pervazea left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pervazea left a comment

coveralls commented Jun 18, 2018 •

edited

Loading

tli2 Jun 19, 2018 •

edited

Loading

tli2 Jun 19, 2018 •

edited

Loading

tli2 Jun 19, 2018 •

edited

Loading

pervazea left a comment •

edited

Loading