Modified pg_column_stats initialization by poojanilangekar · Pull Request #1352 · cmu-db/peloton

poojanilangekar · 2018-05-09T06:03:55Z

This PR modifies the ColumnStatsCatalog constructor to use a predefined schema instead of using DDL. Additionally, it creates the pg_column_stats table to a per database basis. Initially this was not done because of the dependencies of the StatsStorage on the view of "Global Stats".

Additionally, it changes the AnalyzeStatsForAllTables to AnalyzeStatsForAllTablesWithDatabaseOid. Now that we maintain ColumnStats on a per database basis, it makes sense to also collect the stats on a per database basis.

apavlo · 2018-05-09T13:32:58Z

@poojanilangekar Nice!

poojanilangekar · 2018-05-18T22:01:13Z

@mengranwo @GustavoAngulo We need to merge this in ASAP as its blocking the performance benchmarks. Please take a look at it when you get the time.

coveralls · 2018-05-18T23:19:20Z

Coverage increased (+0.01%) to 77.565% when pulling 7dfbc59 on poojanilangekar:column_stats_fix into 5686479 on cmu-db:master.

GustavoAngulo · 2018-05-19T02:02:39Z

I think it might be good to keep AnalyzeStatsForAllTables for now, my group in the class this semester noticed that we don't support the default ANALYZE behavior like Postgres does, where not specifying a table analyzes all tables. We might want to support this in the future, so that function/logic might come in handy.

As for the the schema, I'm not sure which inconsistency you're referring to @poojanilangekar ? @chenboy do you see an inconsistency?

poojanilangekar · 2018-05-19T12:59:35Z

@GustavoAngulo I have modified the function to AnalyzeStatsForAllTablesWithDatabaseOid. I checked the Postgres documentation, the ANALYZE command is run on a per database basis. So I think this should solve the problem.

Regarding the inconsistency, the declaration creates two SKEY indexes and the table has no primary key. While the header file states that the table should contain only one index with the primary key.

chenboy · 2018-05-25T18:38:50Z

The ColumnStatsCatalog use both indexes.

peloton/src/catalog/column_stats_catalog.cpp

Line 135 in d68ab71

oid_t index_offset = IndexId::SECONDARY_KEY_0; // Secondary key index

peloton/src/catalog/column_stats_catalog.cpp

Line 186 in d68ab71

oid_t index_offset = IndexId::SECONDARY_KEY_1; // Secondary key index

So I think we should stick to the declaration, not the header.

poojanilangekar · 2018-06-10T23:05:28Z

@GustavoAngulo @camellyx Can you please review these changes?

tli2 · 2018-06-18T14:44:05Z

@pervazea Can you take a look since these other people are out of town?

apavlo · 2018-06-21T13:30:09Z

This is an important PR that we should merge. We don't want to lose track of this.

pervazea

Looks good.
Please add function headers / documentation as requested (and elsewhere too if I've missed anything).

Also, needs merge conflicts resolved :-( due to recent re-factors.

pervazea · 2018-07-03T18:42:28Z

  //===--------------------------------------------------------------------===//
-  bool InsertColumnStats(oid_t database_id, oid_t table_id, oid_t column_id,
-                         int num_rows, double cardinality, double frac_null,
+  bool InsertColumnStats(oid_t table_id, oid_t column_id, int num_rows,


Add comment documenting function purpose and arguments.

pervazea · 2018-07-03T18:42:55Z

                         bool has_index, type::AbstractPool *pool,
                         concurrency::TransactionContext *txn);
-  bool DeleteColumnStats(oid_t database_id, oid_t table_id, oid_t column_id,
+  bool DeleteColumnStats(oid_t table_id, oid_t column_id,


Add a comment documenting function purpose and arguments.

pervazea · 2018-07-03T18:43:32Z

  std::unique_ptr<std::vector<type::Value>> GetColumnStats(
-      oid_t database_id, oid_t table_id, oid_t column_id,
-      concurrency::TransactionContext *txn);
+      oid_t table_id, oid_t column_id, concurrency::TransactionContext *txn);


Add comment document function, args and return information.

pervazea · 2018-07-03T18:44:58Z

  }

+  ColumnStatsCatalog *GetColumnStatsCatalog() {
+    if (!pg_column_stats_) {


Is this a recoverable error? If not, this should be a PELOTON_ASSERT.

pervazea · 2018-07-03T18:45:42Z


-  ResultType AnalyzeStatsForAllTables(
-      concurrency::TransactionContext *txn = nullptr);
+  ResultType AnalyzeStatsForAllTablesWithDatabaseOid(


Add function header describing this function, args, etc.

pervazea · 2018-07-03T18:49:33Z

-      database_id, table_id, column_id, num_rows, cardinality, frac_null,
-      most_common_vals, most_common_freqs, histogram_bounds, column_name,
-      has_index, pool_.get(), txn);
+  pg_column_stats->DeleteColumnStats(table_id, column_id, txn);


DeleteColumnStats if they exist, I assume. May be helpful to add comment stating that, if correct.

pervazea · 2018-07-03T18:59:51Z

@@ -108,14 +110,18 @@ TEST_F(StatsStorageTests, InsertAndGetTableStatsTest) {
                                          table_stats_collector.get());

  VerifyAndPrintColumnStats(data_table.get(), 4);


If I understand correctly, this function VerifyAndPrint... does print but very little verify. Acknowledge that this wasn't modified, but it does mean some of the old tests are not particularly useful.

poojanilangekar added the do not merge label May 9, 2018

poojanilangekar mentioned this pull request May 12, 2018

Schema - Layout decoupling #1327

Merged

poojanilangekar added 6 commits May 18, 2018 13:55

Modified catalog. Need to fix tests

5926cd4

Fixed all tests

5f431de

Fix include statement

c5a0bc9

Fixed AnalyzeAllTables

6ffb0b3

Changed AnalyzeAllTables to AnalyzeAllTablesWithDatabaseOid

71aeb02

Style fix

92d95fa

poojanilangekar force-pushed the column_stats_fix branch from a625053 to 92d95fa Compare May 18, 2018 21:56

poojanilangekar added ready_for_review and removed do not merge labels May 18, 2018

poojanilangekar requested review from GustavoAngulo and mengranwo May 18, 2018 22:00

Insert test tables into pg_table

7dfbc59

Merge branch 'master' into column_stats_fix

9429b86

poojanilangekar requested a review from camellyx June 10, 2018 23:04

tli2 requested a review from pervazea June 18, 2018 14:43

tli2 removed request for GustavoAngulo, camellyx and mengranwo June 18, 2018 14:44

pervazea suggested changes Jul 3, 2018

View reviewed changes

		@@ -108,14 +110,18 @@ TEST_F(StatsStorageTests, InsertAndGetTableStatsTest) {
		table_stats_collector.get());

		VerifyAndPrintColumnStats(data_table.get(), 4);

Uh oh!

Conversation

poojanilangekar commented May 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

apavlo commented May 9, 2018

Uh oh!

poojanilangekar commented May 18, 2018

Uh oh!

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

Uh oh!

Uh oh!

GustavoAngulo commented May 19, 2018

Uh oh!

poojanilangekar commented May 19, 2018

Uh oh!

chenboy commented May 25, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

poojanilangekar commented Jun 10, 2018

Uh oh!

tli2 commented Jun 18, 2018

Uh oh!

apavlo commented Jun 21, 2018

Uh oh!

pervazea left a comment

Choose a reason for hiding this comment

Uh oh!

pervazea Jul 3, 2018

Choose a reason for hiding this comment

Uh oh!

pervazea Jul 3, 2018

Choose a reason for hiding this comment

Uh oh!

pervazea Jul 3, 2018

Choose a reason for hiding this comment

Uh oh!

pervazea Jul 3, 2018

Choose a reason for hiding this comment

Uh oh!

pervazea Jul 3, 2018

Choose a reason for hiding this comment

Uh oh!

pervazea Jul 3, 2018

Choose a reason for hiding this comment

Uh oh!

pervazea Jul 3, 2018

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

poojanilangekar commented May 9, 2018 •

edited

Loading

coveralls commented May 18, 2018 •

edited

Loading

chenboy commented May 25, 2018 •

edited

Loading