Merge 'Fix update queries to set n_changes ' from Kim Seon Woo

- `Update` query doesn't update `n_changes`. Let's make it work
- Add `InsertFlags` to add meta information related to insert operations
- For update query, add `UPDATE` flag
- Currently, the update query executes `Insn::Delete` and `Insn::Insert`
internally, it increases `n_change` by 2. So, for the update query,
let's skip increasing `n_change` for the `Insn::Insert`
https://github.com/tursodatabase/limbo/issues/1681

Reviewed-by: Pere Diaz Bou <pere-altea@homail.com>

Closes #1683
This commit is contained in:
Pekka Enberg
2025-06-16 16:30:20 +03:00
10 changed files with 71 additions and 21 deletions

View File

@@ -11,7 +11,6 @@ import java.sql.SQLException;
import java.sql.Statement;
import java.util.Properties;
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Disabled;
import org.junit.jupiter.api.Test;
import tech.turso.TestUtils;
@@ -83,11 +82,9 @@ class JDBC4StatementTest {
}
@Test
@Disabled("limbo's total_changes() works differently from sqlite's total_changes()")
void execute_update_should_return_number_of_updated_elements() throws Exception {
assertThat(stmt.executeUpdate("CREATE TABLE s1 (c1 INT);")).isEqualTo(0);
assertThat(stmt.executeUpdate("INSERT INTO s1 VALUES (1), (2), (3);")).isEqualTo(3);
assertThat(stmt.executeUpdate("UPDATE s1 SET c1 = 0;")).isEqualTo(3);
}

View File

@@ -138,7 +138,7 @@ pub fn translate_alter_table(
cursor: cursor_id,
key_reg: rowid,
record_reg: record,
flag: 0,
flag: crate::vdbe::insn::InsertFlags(0),
table_name: table_name.clone(),
});
});
@@ -284,7 +284,7 @@ pub fn translate_alter_table(
cursor: cursor_id,
key_reg: rowid,
record_reg: record,
flag: 0,
flag: crate::vdbe::insn::InsertFlags(0),
table_name: table_name.clone(),
});
});
@@ -362,7 +362,7 @@ pub fn translate_alter_table(
cursor: cursor_id,
key_reg: rowid,
record_reg: record,
flag: 0,
flag: crate::vdbe::insn::InsertFlags(0),
table_name: table_name.clone(),
});
});

View File

@@ -26,7 +26,7 @@ use crate::translate::plan::{DeletePlan, Plan, Search};
use crate::translate::values::emit_values;
use crate::util::exprs_are_equivalent;
use crate::vdbe::builder::{CursorKey, CursorType, ProgramBuilder};
use crate::vdbe::insn::{CmpInsFlags, IdxInsertFlags, RegisterOrLiteral};
use crate::vdbe::insn::{CmpInsFlags, IdxInsertFlags, InsertFlags, RegisterOrLiteral};
use crate::vdbe::{insn::Insn, BranchOffset};
use crate::{Result, SymbolTable};
@@ -1063,7 +1063,7 @@ fn emit_update_insns(
cursor: cursor_id,
key_reg: rowid_set_clause_reg.unwrap_or(beg),
record_reg,
flag: 0,
flag: InsertFlags::new().update(true),
table_name: table_ref.identifier.clone(),
});
} else if let Some(_) = table_ref.virtual_table() {

View File

@@ -8,7 +8,7 @@ use crate::error::{SQLITE_CONSTRAINT_NOTNULL, SQLITE_CONSTRAINT_PRIMARYKEY};
use crate::schema::{IndexColumn, Table};
use crate::util::normalize_ident;
use crate::vdbe::builder::{ProgramBuilderOpts, QueryMode};
use crate::vdbe::insn::{IdxInsertFlags, RegisterOrLiteral};
use crate::vdbe::insn::{IdxInsertFlags, InsertFlags, RegisterOrLiteral};
use crate::vdbe::BranchOffset;
use crate::{
schema::{Column, Schema},
@@ -212,7 +212,7 @@ pub fn translate_insert(
cursor: temp_cursor_id,
key_reg: rowid_reg,
record_reg,
flag: 0,
flag: InsertFlags::new(),
table_name: "".to_string(),
});
@@ -539,7 +539,7 @@ pub fn translate_insert(
cursor: cursor_id,
key_reg: rowid_reg,
record_reg: record_register,
flag: 0,
flag: InsertFlags::new(),
table_name: table_name.to_string(),
});

View File

@@ -66,7 +66,10 @@ pub fn translate(
) -> Result<Program> {
let change_cnt_on = matches!(
stmt,
ast::Stmt::CreateIndex { .. } | ast::Stmt::Delete(..) | ast::Stmt::Insert(..)
ast::Stmt::CreateIndex { .. }
| ast::Stmt::Delete(..)
| ast::Stmt::Insert(..)
| ast::Stmt::Update(..)
);
// These options will be extended whithin each translate program

View File

@@ -15,7 +15,7 @@ use crate::translate::ProgramBuilderOpts;
use crate::translate::QueryMode;
use crate::util::PRIMARY_KEY_AUTOMATIC_INDEX_NAME_PREFIX;
use crate::vdbe::builder::CursorType;
use crate::vdbe::insn::{CmpInsFlags, Insn};
use crate::vdbe::insn::{CmpInsFlags, InsertFlags, Insn};
use crate::LimboError;
use crate::SymbolTable;
use crate::{bail_parse_error, Result};
@@ -227,7 +227,7 @@ pub fn emit_schema_entry(
cursor: sqlite_schema_cursor_id,
key_reg: rowid_reg,
record_reg,
flag: 0,
flag: InsertFlags::new(),
table_name: tbl_name.to_string(),
});
}
@@ -828,7 +828,7 @@ pub fn translate_drop_table(
cursor: ephemeral_cursor_id,
key_reg: schema_row_id_register,
record_reg: schema_data_register,
flag: 0,
flag: InsertFlags::new(),
table_name: "scratch_table".to_string(),
});
@@ -894,7 +894,7 @@ pub fn translate_drop_table(
cursor: sqlite_schema_cursor_id_1,
key_reg: schema_row_id_register,
record_reg: new_record_register,
flag: 0,
flag: InsertFlags::new(),
table_name: SQLITE_TABLEID.to_string(),
});

View File

@@ -84,6 +84,7 @@ use crate::{
};
use super::{get_new_rowid, make_record, Program, ProgramState, Register};
use crate::vdbe::insn::InsertFlags;
use crate::{
bail_constraint_error, must_be_btree_cursor, resolve_ext_path, MvStore, Pager, Result,
DATABASE_VERSION,
@@ -4195,7 +4196,7 @@ pub fn op_insert(
cursor,
key_reg,
record_reg,
flag: _,
flag,
table_name: _,
} = insn
else {
@@ -4217,8 +4218,12 @@ pub fn op_insert(
if cursor.root_page() != 1 {
if let Some(rowid) = return_if_io!(cursor.rowid()) {
program.connection.update_last_rowid(rowid);
let prev_changes = program.n_change.get();
program.n_change.set(prev_changes + 1);
// n_change is increased when Insn::Delete is executed, so we can skip for Insn::Insert
if !flag.has(InsertFlags::UPDATE) {
let prev_changes = program.n_change.get();
program.n_change.set(prev_changes + 1);
}
}
}
}

View File

@@ -1107,7 +1107,7 @@ pub fn insn_to_str(
*record_reg as i32,
*key_reg as i32,
Value::build_text(&table_name),
*flag as u16,
flag.0 as u16,
format!("intkey=r[{}] data=r[{}]", key_reg, record_reg),
),
Insn::Delete { cursor_id } => (

View File

@@ -95,6 +95,30 @@ impl IdxInsertFlags {
}
}
#[derive(Clone, Copy, Debug, Default)]
pub struct InsertFlags(pub u8);
impl InsertFlags {
pub const UPDATE: u8 = 0x01; // Flag indicating this is part of an UPDATE statement
pub fn new() -> Self {
InsertFlags(0)
}
pub fn has(&self, flag: u8) -> bool {
(self.0 & flag) != 0
}
pub fn update(mut self, is_update: bool) -> Self {
if is_update {
self.0 |= InsertFlags::UPDATE;
} else {
self.0 &= !InsertFlags::UPDATE;
}
self
}
}
#[derive(Clone, Copy, Debug)]
pub enum RegisterOrLiteral<T: Copy + std::fmt::Display> {
Register(usize),
@@ -688,7 +712,7 @@ pub enum Insn {
cursor: CursorID,
key_reg: usize, // Must be int.
record_reg: usize, // Blob of record data.
flag: usize, // Flags used by insert, for now not used.
flag: InsertFlags, // Flags used by insert, for now not used.
table_name: String,
},

View File

@@ -21,3 +21,24 @@ do_execsql_test_on_specific_db {:memory:} total-changes-on-multiple-inserts {
insert into temp values (4), (5), (6), (7);
select total_changes();
} {7}
do_execsql_test_on_specific_db {:memory:} total-changes-on-update-single-row {
create table temp (t1 integer primary key, t2 text);
insert into temp values (1, 'a'), (2, 'b'), (3, 'c');
update temp set t2 = 'z' where t1 = 2;
select total_changes();
} {4}
do_execsql_test_on_specific_db {:memory:} total-changes-on-update-multiple-rows {
create table temp (t1 integer primary key, t2 text);
insert into temp values (1, 'a'), (2, 'b'), (3, 'c');
update temp set t2 = 'x' where t1 > 1;
select total_changes();
} {5}
do_execsql_test_on_specific_db {:memory:} total-changes-on-update-no-match {
create table temp (t1 integer primary key, t2 text);
insert into temp values (1, 'a'), (2, 'b');
update temp set t2 = 'y' where t1 = 99;
select total_changes();
} {2}