From d2cd833b8675f68b9d088623c31d296ed2b52ff3 Mon Sep 17 00:00:00 2001 From: PThorpe92 Date: Tue, 16 Sep 2025 15:45:49 -0400 Subject: [PATCH 1/5] Rewrite exprs in set + where clause for UPSERT --- core/translate/insert.rs | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-) diff --git a/core/translate/insert.rs b/core/translate/insert.rs index 06f3e2bc1..25a8a1b55 100644 --- a/core/translate/insert.rs +++ b/core/translate/insert.rs @@ -120,7 +120,6 @@ pub fn translate_insert( let inserting_multiple_rows = match &mut body { InsertBody::Select(select, upsert) => { - upsert_opt = upsert.as_deref().cloned(); match &mut select.body.select { // TODO see how to avoid clone OneSelect::Values(values_expr) if values_expr.len() <= 1 => { @@ -149,6 +148,21 @@ pub fn translate_insert( } rewrite_expr(expr, &mut param_idx)?; } + if let Some(ref mut upsert) = upsert { + if let UpsertDo::Set { + ref mut sets, + ref mut where_clause, + } = &mut upsert.do_clause + { + for set in sets.iter_mut() { + rewrite_expr(set.expr.as_mut(), &mut param_idx)?; + } + if let Some(ref mut where_expr) = where_clause { + rewrite_expr(where_expr.as_mut(), &mut param_idx)?; + } + } + } + upsert_opt = upsert.as_deref().cloned(); values = values_expr.pop(); false } From 85eee42bf10fec0160c5de83ce78e2a237679924 Mon Sep 17 00:00:00 2001 From: PThorpe92 Date: Tue, 16 Sep 2025 15:46:16 -0400 Subject: [PATCH 2/5] Support quoted qualified identifiers in UPSERT excluded.x clauses --- core/translate/upsert.rs | 37 ++++++++++++++++++------------------- 1 file changed, 18 insertions(+), 19 deletions(-) diff --git a/core/translate/upsert.rs b/core/translate/upsert.rs index 5b208ed38..82b542603 100644 --- a/core/translate/upsert.rs +++ b/core/translate/upsert.rs @@ -39,28 +39,26 @@ pub struct ConflictTarget { // Extract `(column, optional_collate)` from an ON CONFLICT target Expr. // Accepts: Id, Qualified, DoublyQualified, Parenthesized, Collate fn extract_target_key(e: &ast::Expr) -> Option { + use ast::Name::{Ident, Quoted}; match e { - // expr COLLATE c: carry c and keep descending into expr ast::Expr::Collate(inner, c) => { let mut tk = extract_target_key(inner.as_ref())?; let cstr = match c { - ast::Name::Ident(s) => s.as_str(), - _ => return None, + Ident(s) | Quoted(s) => s.as_str(), }; tk.collate = Some(cstr.to_ascii_lowercase()); Some(tk) } ast::Expr::Parenthesized(v) if v.len() == 1 => extract_target_key(&v[0]), - // Bare identifier - ast::Expr::Id(ast::Name::Ident(name)) => Some(ConflictTarget { + + ast::Expr::Id(Ident(name)) | ast::Expr::Id(Quoted(name)) => Some(ConflictTarget { col_name: normalize_ident(name), collate: None, }), - // t.a or db.t.a + // t.a or db.t.a: accept ident or quoted in the column position ast::Expr::Qualified(_, col) | ast::Expr::DoublyQualified(_, _, col) => { let cname = match col { - ast::Name::Ident(s) => s.as_str(), - _ => return None, + Ident(s) | Quoted(s) => s.as_str(), }; Some(ConflictTarget { col_name: normalize_ident(cname), @@ -526,37 +524,38 @@ fn rewrite_upsert_expr_in_place( conflict_rowid_reg: usize, insertion: &Insertion, ) -> crate::Result { - use ast::Expr; + use ast::{Expr, Name}; - // helper: return the CURRENT-row register for a column (including rowid alias) let col_reg = |name: &str| -> Option { if name.eq_ignore_ascii_case("rowid") { return Some(conflict_rowid_reg); } - let (idx, _c) = table.get_column_by_name(&normalize_ident(name))?; + let (idx, _) = table.get_column_by_name(name)?; Some(current_start + idx) }; + walk_expr_mut( e, &mut |expr: &mut ast::Expr| -> crate::Result { match expr { - // EXCLUDED.x -> insertion register - Expr::Qualified(ns, ast::Name::Ident(c)) => { - if ns.as_str().eq_ignore_ascii_case("excluded") { - let Some(reg) = insertion.get_col_mapping_by_name(c.as_str()) else { + // EXCLUDED.x or t.x (t may be quoted) + Expr::Qualified(ns, Name::Ident(c) | Name::Quoted(c)) + | Expr::DoublyQualified(_, ns, Name::Ident(c) | Name::Quoted(c)) => { + let ns = normalize_ident(ns.as_str()); + let c = normalize_ident(c.as_str()); + if ns.eq_ignore_ascii_case("excluded") { + let Some(reg) = insertion.get_col_mapping_by_name(&c) else { bail_parse_error!("no such column in EXCLUDED: {}", c); }; *expr = Expr::Register(reg.register); - } else if ns.as_str().eq_ignore_ascii_case(table_name) - // t.x -> CURRENT, only if t matches the target table name (never "excluded") - { + } else if ns.eq_ignore_ascii_case(table_name) { if let Some(reg) = col_reg(c.as_str()) { *expr = Expr::Register(reg); } } } // Unqualified column id -> CURRENT - Expr::Id(ast::Name::Ident(name)) => { + Expr::Id(Name::Ident(name)) | Expr::Id(Name::Quoted(name)) => { if let Some(reg) = col_reg(name.as_str()) { *expr = Expr::Register(reg); } From 5dd466941e4d6fed41b6854dad7b6af2fa98fe5f Mon Sep 17 00:00:00 2001 From: PThorpe92 Date: Tue, 16 Sep 2025 16:15:17 -0400 Subject: [PATCH 3/5] Handle upsert even in inserting_multiple_rows case --- core/translate/insert.rs | 41 ++++++++++++++++++++-------------------- 1 file changed, 21 insertions(+), 20 deletions(-) diff --git a/core/translate/insert.rs b/core/translate/insert.rs index 25a8a1b55..e2e001bb4 100644 --- a/core/translate/insert.rs +++ b/core/translate/insert.rs @@ -118,7 +118,9 @@ pub fn translate_insert( let mut values: Option>> = None; let mut upsert_opt: Option = None; - let inserting_multiple_rows = match &mut body { + let mut param_idx = 1; + let inserting_multiple_rows; + match &mut body { InsertBody::Select(select, upsert) => { match &mut select.body.select { // TODO see how to avoid clone @@ -126,7 +128,6 @@ pub fn translate_insert( if values_expr.is_empty() { crate::bail_parse_error!("no values to insert"); } - let mut param_idx = 1; for expr in values_expr.iter_mut().flat_map(|v| v.iter_mut()) { match expr.as_mut() { Expr::Id(name) => { @@ -148,28 +149,28 @@ pub fn translate_insert( } rewrite_expr(expr, &mut param_idx)?; } - if let Some(ref mut upsert) = upsert { - if let UpsertDo::Set { - ref mut sets, - ref mut where_clause, - } = &mut upsert.do_clause - { - for set in sets.iter_mut() { - rewrite_expr(set.expr.as_mut(), &mut param_idx)?; - } - if let Some(ref mut where_expr) = where_clause { - rewrite_expr(where_expr.as_mut(), &mut param_idx)?; - } - } - } - upsert_opt = upsert.as_deref().cloned(); values = values_expr.pop(); - false + inserting_multiple_rows = false; } - _ => true, + _ => inserting_multiple_rows = true, } + if let Some(ref mut upsert) = upsert { + if let UpsertDo::Set { + ref mut sets, + ref mut where_clause, + } = &mut upsert.do_clause + { + for set in sets.iter_mut() { + rewrite_expr(set.expr.as_mut(), &mut param_idx)?; + } + if let Some(ref mut where_expr) = where_clause { + rewrite_expr(where_expr.as_mut(), &mut param_idx)?; + } + } + } + upsert_opt = upsert.as_deref().cloned(); } - InsertBody::DefaultValues => false, + InsertBody::DefaultValues => inserting_multiple_rows = false, }; let halt_label = program.allocate_label(); From 8dd1cf11eb9fa5c5af7394e517a26bd56e6ab491 Mon Sep 17 00:00:00 2001 From: PThorpe92 Date: Tue, 16 Sep 2025 16:15:38 -0400 Subject: [PATCH 4/5] Add some more specific tests for upsert --- testing/upsert.test | 25 ++++++++++++++++++++++++- 1 file changed, 24 insertions(+), 1 deletion(-) diff --git a/testing/upsert.test b/testing/upsert.test index 11effbd0a..cab46cc1b 100644 --- a/testing/upsert.test +++ b/testing/upsert.test @@ -305,7 +305,30 @@ do_execsql_test_on_specific_db {:memory:} upsert-omitted-target-updates-unique { SELECT * FROM ou; } {3|y} -# Target qualified with database.table.column should match too (assuming single db). +do_execsql_test_on_specific_db {:memory:} upsert-current-qualified.1 { + CREATE TABLE dq (a INTEGER UNIQUE, b TEXT); + INSERT INTO dq VALUES (1,'old'); + INSERT INTO dq VALUES (1,'new') + ON CONFLICT(dq.a) DO UPDATE SET b = dq.b || '-' || excluded.b; + SELECT * FROM dq; +} {1|old-new} + +do_execsql_test_on_specific_db {:memory:} upsert-multicol-set.1 { + CREATE TABLE dq (a INTEGER UNIQUE, b TEXT); + INSERT INTO dq VALUES (1,'old'); + INSERT INTO dq VALUES (1,'new') + ON CONFLICT(a) DO UPDATE SET (`a`,`b`) = (`excluded`.`a`, `excluded`.`b`); + SELECT * FROM dq; +} {1|new} + +do_execsql_test_on_specific_db {:memory:} upsert-where-predicate.1 { + CREATE TABLE dq (a INTEGER UNIQUE, b TEXT); + INSERT INTO dq VALUES (1,'old'); + INSERT INTO dq VALUES (1,'old') + ON CONFLICT(a) DO UPDATE SET b = excluded.b WHERE dq.b != excluded.b; + SELECT * FROM dq; +} {1|old} + do_execsql_test_on_specific_db {:memory:} upsert-doubly-qualified-target { CREATE TABLE dq (a UNIQUE, b); INSERT INTO dq VALUES (1,'old'); From 97c11898fecffeb583c1ffc6c9477855829925a8 Mon Sep 17 00:00:00 2001 From: PThorpe92 Date: Tue, 16 Sep 2025 16:18:51 -0400 Subject: [PATCH 5/5] Minor refactor in translate/insert --- core/translate/insert.rs | 86 +++++++++++++++++++--------------------- 1 file changed, 40 insertions(+), 46 deletions(-) diff --git a/core/translate/insert.rs b/core/translate/insert.rs index e2e001bb4..e8c1bb91f 100644 --- a/core/translate/insert.rs +++ b/core/translate/insert.rs @@ -119,59 +119,53 @@ pub fn translate_insert( let mut upsert_opt: Option = None; let mut param_idx = 1; - let inserting_multiple_rows; - match &mut body { - InsertBody::Select(select, upsert) => { - match &mut select.body.select { - // TODO see how to avoid clone - OneSelect::Values(values_expr) if values_expr.len() <= 1 => { - if values_expr.is_empty() { - crate::bail_parse_error!("no values to insert"); - } - for expr in values_expr.iter_mut().flat_map(|v| v.iter_mut()) { - match expr.as_mut() { - Expr::Id(name) => { - if name.is_double_quoted() { - *expr = Expr::Literal(ast::Literal::String(name.to_string())) - .into(); - } else { - // an INSERT INTO ... VALUES (...) cannot reference columns - crate::bail_parse_error!("no such column: {name}"); - } - } - Expr::Qualified(first_name, second_name) => { + let mut inserting_multiple_rows = false; + if let InsertBody::Select(select, upsert) = &mut body { + match &mut select.body.select { + // TODO see how to avoid clone + OneSelect::Values(values_expr) if values_expr.len() <= 1 => { + if values_expr.is_empty() { + crate::bail_parse_error!("no values to insert"); + } + for expr in values_expr.iter_mut().flat_map(|v| v.iter_mut()) { + match expr.as_mut() { + Expr::Id(name) => { + if name.is_double_quoted() { + *expr = + Expr::Literal(ast::Literal::String(name.to_string())).into(); + } else { // an INSERT INTO ... VALUES (...) cannot reference columns - crate::bail_parse_error!( - "no such column: {first_name}.{second_name}" - ); + crate::bail_parse_error!("no such column: {name}"); } - _ => {} } - rewrite_expr(expr, &mut param_idx)?; + Expr::Qualified(first_name, second_name) => { + // an INSERT INTO ... VALUES (...) cannot reference columns + crate::bail_parse_error!("no such column: {first_name}.{second_name}"); + } + _ => {} } - values = values_expr.pop(); - inserting_multiple_rows = false; + rewrite_expr(expr, &mut param_idx)?; } - _ => inserting_multiple_rows = true, + values = values_expr.pop(); } - if let Some(ref mut upsert) = upsert { - if let UpsertDo::Set { - ref mut sets, - ref mut where_clause, - } = &mut upsert.do_clause - { - for set in sets.iter_mut() { - rewrite_expr(set.expr.as_mut(), &mut param_idx)?; - } - if let Some(ref mut where_expr) = where_clause { - rewrite_expr(where_expr.as_mut(), &mut param_idx)?; - } - } - } - upsert_opt = upsert.as_deref().cloned(); + _ => inserting_multiple_rows = true, } - InsertBody::DefaultValues => inserting_multiple_rows = false, - }; + if let Some(ref mut upsert) = upsert { + if let UpsertDo::Set { + ref mut sets, + ref mut where_clause, + } = &mut upsert.do_clause + { + for set in sets.iter_mut() { + rewrite_expr(set.expr.as_mut(), &mut param_idx)?; + } + if let Some(ref mut where_expr) = where_clause { + rewrite_expr(where_expr.as_mut(), &mut param_idx)?; + } + } + } + upsert_opt = upsert.as_deref().cloned(); + } let halt_label = program.allocate_label(); let loop_start_label = program.allocate_label();