Skip to content
This repository has been archived by the owner on Feb 20, 2023. It is now read-only.

Add the framework for the pg_statistic table. #1416

Merged
merged 19 commits into from
Jan 24, 2021

Conversation

preetansh
Copy link
Contributor

@preetansh preetansh commented Dec 25, 2020

Heading

Add the framework for the pg_statistic table.

Description

This PR adds the skeleton of pg_statistic support.
It does not currently track any statistics at all, apart from creating dummy column statistics in pg_statistic and cleaning those entries up.

@lmwnshn lmwnshn added the in-progress This PR is being actively worked on and not ready to be reviewed or merged. Mark PRs with this. label Jan 8, 2021
Copy link
Contributor

@lmwnshn lmwnshn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

very quick review, please change to new-style catalog stuff.

parser::ConstantValueExpression(type::TypeId::INTEGER));
columns.back().SetOid(PgStatistic::STAATTNUM_COL_OID);

// columns.emplace_back("stanullfrac", type::TypeId::DECIMAL, false,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remove dead code.

Copy link
Contributor

@jkosh44 jkosh44 Jan 9, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@whoever-is-implementing the PR, Is this dead or not yet implemented? I think we'll need these columns as well as some binary array columns for topKs and histograms.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jkosh44 You'll probably be the one implementing it.

src/catalog/postgres/pg_core_impl.cpp Outdated Show resolved Hide resolved
src/catalog/postgres/pg_core_impl.cpp Outdated Show resolved Hide resolved
src/catalog/postgres/pg_core_impl.cpp Outdated Show resolved Hide resolved
src/catalog/postgres/pg_core_impl.cpp Outdated Show resolved Hide resolved
src/include/catalog/database_catalog.h Outdated Show resolved Hide resolved
src/include/catalog/postgres/pg_core_impl.h Outdated Show resolved Hide resolved
src/include/catalog/postgres/pg_statistic.h Outdated Show resolved Hide resolved
@apavlo apavlo marked this pull request as ready for review January 19, 2021 20:28
Add missing calls to Bootstrap and BootstrapPRIs.
Rename the attributes to remove redundant _COL_OID suffix.
Style nitpicks.
noisepage is bootable again.
@lmwnshn lmwnshn changed the title Pg stats Add the framework for the pg_statistic table. Jan 20, 2021
@lmwnshn lmwnshn added ready-for-ci Indicate that this build should be run through CI. ready-for-review This PR passes all checks and is ready to be reviewed. Mark PRs with this. and removed in-progress This PR is being actively worked on and not ready to be reviewed or merged. Mark PRs with this. labels Jan 21, 2021
@lmwnshn lmwnshn requested a review from jkosh44 January 21, 2021 04:39
@lmwnshn
Copy link
Contributor

lmwnshn commented Jan 21, 2021

Assuming this passes CI, @jkosh44 can you take a review pass since you'll be working with the PR and I've made some changes so I probably shouldn't be reviewing myself? Thanks!

Copy link
Contributor

@jkosh44 jkosh44 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, though I'm not super familiar with the Catalog

@lmwnshn lmwnshn added in-progress This PR is being actively worked on and not ready to be reviewed or merged. Mark PRs with this. and removed ready-for-ci Indicate that this build should be run through CI. ready-for-review This PR passes all checks and is ready to be reviewed. Mark PRs with this. labels Jan 21, 2021
@lmwnshn lmwnshn added ready-for-ci Indicate that this build should be run through CI. ready-for-review This PR passes all checks and is ready to be reviewed. Mark PRs with this. and removed in-progress This PR is being actively worked on and not ready to be reviewed or merged. Mark PRs with this. labels Jan 21, 2021
Added missing RecoveryManager logic.
Fixed the size of the PRI and PRM for statistic_oid_index in deletion.

wow i was inattentive
@codecov
Copy link

codecov bot commented Jan 22, 2021

Codecov Report

Merging #1416 (f0279e0) into master (3ebdd86) will increase coverage by 0.06%.
The diff coverage is 91.34%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1416      +/-   ##
==========================================
+ Coverage   82.39%   82.45%   +0.06%     
==========================================
  Files         670      671       +1     
  Lines       46004    46107     +103     
==========================================
+ Hits        37903    38016     +113     
+ Misses       8101     8091      -10     
Impacted Files Coverage Δ
src/include/catalog/database_catalog.h 88.88% <ø> (ø)
src/include/storage/projected_row.h 100.00% <ø> (ø)
src/catalog/postgres/builder.cpp 81.28% <77.77%> (-0.21%) ⬇️
src/catalog/postgres/pg_statistic_impl.cpp 95.31% <95.31%> (ø)
src/catalog/database_catalog.cpp 93.36% <100.00%> (+0.24%) ⬆️
src/storage/recovery/recovery_manager.cpp 81.91% <100.00%> (+0.16%) ⬆️
src/include/common/container/concurrent_bitmap.h 91.66% <0.00%> (-6.67%) ⬇️
src/network/connection_handle.cpp 63.28% <0.00%> (-1.57%) ⬇️
src/traffic_cop/traffic_cop.cpp 73.06% <0.00%> (+0.81%) ⬆️
... and 9 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3ebdd86...f0279e0. Read the comment docs.

Copy link
Contributor

@lmwnshn lmwnshn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rubber stamping.

@lmwnshn lmwnshn added ready-to-merge This PR is ready to be merged. Mark PRs with this. and removed ready-for-review This PR passes all checks and is ready to be reviewed. Mark PRs with this. labels Jan 24, 2021
@lmwnshn lmwnshn merged commit d60c55a into cmu-db:master Jan 24, 2021
jkosh44 pushed a commit to jkosh44/noisepage that referenced this pull request Feb 11, 2021
Co-authored-by: Arvind Sai Krishnan <arvindsk@andrew.cmu.edu>
Co-authored-by: Wan Shen Lim <wanshen.lim@gmail.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
ready-for-ci Indicate that this build should be run through CI. ready-to-merge This PR is ready to be merged. Mark PRs with this.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants