-
Notifications
You must be signed in to change notification settings - Fork 267
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
DELETE implementation #475
base: main
Are you sure you want to change the base?
Conversation
- currently doesn't work because in delete_record `cell_idx` is being calculated wrong.
page.set_dirty(); | ||
self.pager.add_dirty(page.get().id); | ||
|
||
return_if_io!(self.balance_after_delete()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would need more time to reviews this. As far as I know sqlite deletes a cell from a page and the there is a loop that calls balance. balance
checks wether there is an overflow (what we do now) or underflow. I want to refactor the balance because it should be as close to sqlite as possible so we should stick to that instead of having different types of balancing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Btw we need a lot of tests for this
); | ||
let start_offset = program.offset(); | ||
|
||
let table_name = &qualified_table_name.name.0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
need to use normalize_ident()
for this to make sure e.g. both "users"
and [users]
match the table users
let table_name = &qualified_table_name.name.0; | ||
let table = match schema.get_table(table_name) { | ||
Some(t) => t, | ||
None => crate::bail_corrupt_error!("No such table: {}", table_name), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this should be bail_parse_error!
let mut expr = *where_expr.clone(); | ||
bind_column_references(&mut expr, &referenced_tables)?; | ||
|
||
// Handle WHERE id = value case |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you should be able to use translate_condition_expr()
for this
returning, | ||
.. | ||
} => { | ||
assert!(returning.is_none()); // Don't handle RETURNING yet |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
bail_parse_error!("DELETE ... RETURNING is not supported yet")
.. | ||
} => { | ||
assert!(returning.is_none()); // Don't handle RETURNING yet | ||
let boxed_where = where_clause.map(Box::new); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why is this boxed?
Is this PR being actively worked on? 🙏 |
@seonWKim, yes I'm working on it. I'm working on making the PR more comprehensive. I was away for the past two weeks. |
@krishvishal PR for delete planning has been added to main. Maybe you can make use of it. Btw, is there any docs or code you are referring to when implementing DELETE on btree.rs? I also want to take a look |
core/storage/btree.rs
delete_record
cell_idx
is being calculated wrong.I'm still studying the sqlite source code and some other material. So this implementation can be wrong.