Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

drop column primitive #210

Merged
merged 6 commits into from
May 15, 2023
Merged

drop column primitive #210

merged 6 commits into from
May 15, 2023

Conversation

cheme
Copy link
Collaborator

@cheme cheme commented May 11, 2023

Could also be refactor to run on options only.

Should allow truncating a column by removing files.

fix #209

src/db.rs Outdated
@@ -918,9 +918,45 @@ impl Db {
Self::open_inner(options, OpeningMode::ReadOnly)
}

/// Drop a column from the database, optionally changing its options.
/// This shutdown and restart the db in write mode.
pub fn drop_column(&mut self, index: u8, new_options: Option<ColumnOptions>) -> Result<()> {
Copy link
Member

Choose a reason for hiding this comment

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

This should be called clear_column, as the column is not really removed. Also, similar to add_column, this should be an associated function that does not require &self

Choose a reason for hiding this comment

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

Btw there is clear_column in migration.rs:

pub fn clear_column(path: &Path, column: ColId) -> Result<()> {

It removes all data without removing the actual column. I think clear_column for this function too will be confusing.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

oh yes, it is basically doing the same thing (I should have use this 🤦 ), there is also one for moving column, will fix that redundancy (and make that if column is at last index and no new option is provided, we remove the column (as your use case is the last number it will be usefull).

@cheme
Copy link
Collaborator Author

cheme commented May 12, 2023

I switch to using existing code, and add Db::drop_last_column that will suit the use case I think and Db::reset_column that allow truncating a column and optionally changing its option.
Did not implement moving a column, maybe better keep this small.

src/db.rs Outdated

Ok(())
}

fn drop_files_column(options: &mut Options, index: u8) -> Result<()> {
Copy link
Member

Choose a reason for hiding this comment

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

drop_column_files or remove_column_files

@tdimitrov
Copy link

Great work @cheme. Thank you very much!

I switch to using existing code, and add Db::drop_last_column that will suit the use case I think and Db::reset_column that allow truncating a column and optionally changing its option.

Just curious - what does 'truncating a column' means?

@cheme
Copy link
Collaborator Author

cheme commented May 12, 2023

Great work @cheme. Thank you very much!

I switch to using existing code, and add Db::drop_last_column that will suit the use case I think and Db::reset_column that allow truncating a column and optionally changing its option.

Just curious - what does 'truncating a column' means?

Drop all content index from column, but column still exsists (just cleared and new). Probably the work come to my mind from sql truncate table.

@tdimitrov
Copy link

No rush, but if this is ready for merging I'm eager to use it :)

@arkpar arkpar merged commit b697c77 into paritytech:master May 15, 2023
6 checks passed
@arkpar
Copy link
Member

arkpar commented May 15, 2023

published as 0.4.8

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support for dropping columns from a database
3 participants