-
Notifications
You must be signed in to change notification settings - Fork 362
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
Create plan for Update queries #1189
base: main
Are you sure you want to change the base?
Conversation
Hi @PThorpe92 , I was testing out this branch locally and found at least the following panic: limbo> update users set first_name = 'old bastard' where age > 80;
thread 'main' panicked at core/types.rs:513:21:
index out of bounds: the len is 2 but the index is 2 The My guess is it's incorrectly trying to get all the columns from the index since it's using Quick "not very clean code" fix: diff --git a/core/translate/emitter.rs b/core/translate/emitter.rs
index 9cfb0ac2..2d2a3da8 100644
--- a/core/translate/emitter.rs
+++ b/core/translate/emitter.rs
@@ -469,13 +469,15 @@ fn emit_update_insns(
program: &mut ProgramBuilder,
) -> crate::Result<()> {
let table_ref = &plan.table_references.first().unwrap();
- let cursor_id = match &table_ref.op {
- Operation::Scan { .. } => program.resolve_cursor_id(&table_ref.identifier),
+ let (cursor_id, index) = match &table_ref.op {
+ Operation::Scan { .. } => (program.resolve_cursor_id(&table_ref.identifier), None),
Operation::Search(search) => match search {
&Search::RowidEq { .. } | Search::RowidSearch { .. } => {
- program.resolve_cursor_id(&table_ref.identifier)
+ (program.resolve_cursor_id(&table_ref.identifier), None)
+ }
+ Search::IndexSearch { index, .. } => {
+ (program.resolve_cursor_id(&table_ref.identifier), Some((index.clone(), program.resolve_cursor_id(&index.name))))
}
- Search::IndexSearch { index, .. } => program.resolve_cursor_id(&index.name),
},
_ => return Ok(()),
};
@@ -521,9 +523,13 @@ fn emit_update_insns(
&t_ctx.resolver,
)?;
} else {
+ let table_column = table_ref.table.columns().get(idx).unwrap();
+ let column_idx_in_index = index.as_ref().and_then(|(idx, _)| {
+ idx.columns.iter().position(|c| Some(&c.name) == table_column.name.as_ref())
+ });
program.emit_insn(Insn::Column {
- cursor_id,
- column: idx,
+ cursor_id: *index.as_ref().and_then(|(_, id)| if column_idx_in_index.is_some() { Some(id) } else { None }).unwrap_or(&cursor_id),
+ column: column_idx_in_index.unwrap_or(idx),
dest: first_col_reg + idx,
});
} |
fixed that issue but there is still an issue with doing queries that trigger many updates, e.g. on a copy of the @krishvishal confirmed that this is likely from a balancing issue, and he has an incoming fix. |
closes #1186, or at least works towards it by implementing an actual Plan for update queries instead of translating everything inline. This allows for actually using index's, rewriting const expressions, pushing predicates instead of hardcoding a full scan in the translation.
TODOs:
RETURNING
clause/result columnsOFFSET
clausesLIMIT:
By supporting
LIMIT
directly in update queries, we'll have to put the tests outside of the compatibility tests, maybe in the CLI tests.