Skip to content

Conversation

@st1page
Copy link
Contributor

@st1page st1page commented Mar 10, 2022

What's changed and what's your intention?

Checklist

  • I have written necessary docs and comments
  • I have added necessary unit tests and integration tests

Refer to a related PR or issue link (optional)

@st1page st1page marked this pull request as draft March 10, 2022 03:55
@github-actions github-actions bot added the type/feature Type: New feature. label Mar 10, 2022
Comment on lines 5 to 7
enum PhysicalTable {
CellBased(CellBasedTableDesc),
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would prefer simply TableDesc and refactor it once we do have other kind of tables. There is too much overdesign in our system... :(

Comment on lines 15 to 18
pub pk: Vec<OrderedColumnDesc>,
// TODO: the other columns' descriptor, or all the columns? it is actually not used in the
// compute node... for the plan node include its requiring `columnDesc`.
// pub columns: Vec<ColumnDesc>,
Copy link
Collaborator

Choose a reason for hiding this comment

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

May delete these

@codecov
Copy link

codecov bot commented Mar 10, 2022

Codecov Report

Merging #815 (5d5d5bb) into main (597468b) will decrease coverage by 0.00%.
The diff coverage is 0.00%.

Impacted file tree graph

@@             Coverage Diff              @@
##               main     #815      +/-   ##
============================================
- Coverage     72.23%   72.23%   -0.01%     
  Complexity     2766     2766              
============================================
  Files           923      923              
  Lines         53664    53667       +3     
  Branches       1787     1787              
============================================
+ Hits          38764    38765       +1     
- Misses        14010    14012       +2     
  Partials        890      890              
Flag Coverage Δ
java 61.22% <ø> (ø)
rust 76.81% <0.00%> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
rust/common/src/catalog/column.rs 73.68% <0.00%> (-13.82%) ⬇️
rust/common/src/catalog/mod.rs 66.66% <ø> (ø)
rust/meta/src/hummock/compaction.rs 81.65% <0.00%> (+0.59%) ⬆️

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 597468b...5d5d5bb. Read the comment docs.

@st1page st1page marked this pull request as ready for review March 10, 2022 09:42
bool is_hidden = 2;
}

message CellBasedTableDesc {
Copy link
Collaborator

Choose a reason for hiding this comment

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

recommend to add some comments

@st1page st1page enabled auto-merge (squash) March 10, 2022 09:49
@st1page st1page merged commit 054b61d into main Mar 10, 2022
@st1page st1page deleted the sts/refactor_table_catalog branch March 10, 2022 10:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

type/feature Type: New feature.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants