Apply PR review suggestions, add comments to partial indexes

core/translate/emitter.rs

@@ -1073,10 +1073,16 @@ fn emit_update_insns(
     }
 
     for (index, (idx_cursor_id, record_reg)) in plan.indexes_to_update.iter().zip(&index_cursors) {
+        // We need to know whether or not the OLD values satisfied the predicate on the
+        // partial index, so we can know whether or not to delete the old index entry,
+        // as well as whether or not the NEW values satisfy the predicate, to determine whether
+        // or not to insert a new index entry for a partial index
         let (old_satisfies_where, new_satisfies_where) =
             if let Some(where_clause) = &index.where_clause {
                 let mut where_copy = where_clause.as_ref().clone();
                 let mut param_state = ParamState::disallow();
+                // This means that we need to bind the column references to a copy of the index Expr,
+                // so we can emit Insn::Column instructions and refer to the old values.
                 bind_and_rewrite_expr(
                     &mut where_copy,
                     Some(&mut plan.table_references),
@@ -1094,6 +1100,9 @@ fn emit_update_insns(
                 )?;
 
                 let mut new_where = where_clause.as_ref().clone();
+                // Now we need to rewrite the Expr::Id and Expr::Qualified/Expr::RowID (from a copy of the original, un-bound `where` expr),
+                // to refer to the new values, which are already loaded into registers starting at
+                // `start`.
                 rewrite_where_for_update_registers(
                     &mut new_where,
                     unsafe { &*table_ref }.columns(),
@@ -1110,6 +1119,9 @@ fn emit_update_insns(
                     &t_ctx.resolver,
                 )?;
 
+                // now we have two registers that tell us whether or not the old and new values satisfy
+                // the partial index predicate, and we can use those to decide whether or not to
+                // delete/insert a new index entry for this partial index.
                 (Some(old_satisfied_reg), Some(new_satisfied_reg))
             } else {
                 (None, None)
@@ -1121,6 +1133,7 @@ fn emit_update_insns(
         // Handle deletion for partial indexes
         if let Some(old_satisfied) = old_satisfies_where {
             skip_delete_label = Some(program.allocate_label());
+            // If the old values don't satisfy the WHERE clause, skip the delete
             program.emit_insn(Insn::IfNot {
                 reg: old_satisfied,
                 target_pc: skip_delete_label.unwrap(),
@@ -1146,7 +1159,7 @@ fn emit_update_insns(
             start_reg: delete_start_reg,
             num_regs,
             cursor_id: *idx_cursor_id,
-            raise_error_if_no_matching_entry: old_satisfies_where.is_none(),
+            raise_error_if_no_matching_entry: true,
         });
 
         // Resolve delete skip label if it exists
@@ -1157,6 +1170,7 @@ fn emit_update_insns(
         // Check if we should insert into partial index
         if let Some(new_satisfied) = new_satisfies_where {
             skip_insert_label = Some(program.allocate_label());
+            // If the new values don't satisfy the WHERE clause, skip the idx insert
             program.emit_insn(Insn::IfNot {
                 reg: new_satisfied,
                 target_pc: skip_insert_label.unwrap(),
@@ -1200,7 +1214,7 @@ fn emit_update_insns(
         });
 
         // Handle unique constraint
-        if !index.unique {
+        if index.unique {
             let constraint_check = program.allocate_label();
             // check if the record already exists in the index for unique indexes and abort if so
             program.emit_insn(Insn::NoConflict {
@@ -1765,6 +1779,10 @@ fn init_limit(
     }
 }
 
+/// We have `Expr`s which have *not* had column references bound to them,
+/// so they are in the state of Expr::Id/Expr::Qualified, etc, and instead of binding Expr::Column
+/// we need to bind Expr::Register, as we have already loaded the *new* column values from the
+/// UPDATE statement into registers starting at `columns_start_reg`, which we want to reference.
 fn rewrite_where_for_update_registers(
     expr: &mut Expr,
     columns: &[Column],

core/translate/index.rs

@@ -4,12 +4,15 @@ use crate::schema::Table;
 use crate::translate::emitter::{
     emit_cdc_full_record, emit_cdc_insns, prepare_cdc_if_necessary, OperationMode, Resolver,
 };
-use crate::translate::expr::{bind_and_rewrite_expr, translate_expr, ParamState};
+use crate::translate::expr::{
+    bind_and_rewrite_expr, translate_condition_expr, translate_expr, ConditionMetadata, ParamState,
+};
 use crate::translate::plan::{
     ColumnUsedMask, IterationDirection, JoinedTable, Operation, Scan, TableReferences,
 };
 use crate::vdbe::builder::CursorKey;
 use crate::vdbe::insn::{CmpInsFlags, Cookie};
+use crate::vdbe::BranchOffset;
 use crate::SymbolTable;
 use crate::{
     schema::{BTreeTable, Column, Index, IndexColumn, PseudoCursorType, Schema},
@@ -208,20 +211,18 @@ pub fn translate_create_index(
     // Then insert the record into the sorter
     let mut skip_row_label = None;
     if let Some(where_clause) = where_clause {
-        let reg = program.alloc_register();
         let label = program.allocate_label();
-        let pr = translate_expr(
+        translate_condition_expr(
             &mut program,
-            Some(&table_references),
+            &table_references,
             &where_clause,
-            reg,
+            ConditionMetadata {
+                jump_if_condition_is_true: false,
+                jump_target_when_false: label,
+                jump_target_when_true: BranchOffset::Placeholder,
+            },
             &resolver,
         )?;
-        program.emit_insn(Insn::IfNot {
-            reg: pr,
-            target_pc: label,
-            jump_if_null: true,
-        });
         skip_row_label = Some(label);
     }
 

core/translate/update.rs

@@ -395,13 +395,14 @@ pub fn prepare_update_plan(
                         let mut param = ParamState::disallow();
                         let mut tr =
                             TableReferences::new(table_references.joined_tables().to_vec(), vec![]);
-                        let _ = bind_and_rewrite_expr(
+                        bind_and_rewrite_expr(
                             &mut where_copy,
                             Some(&mut tr),
                             None,
                             connection,
                             &mut param,
-                        );
+                        )
+                        .ok()?;
                         let cols_used = collect_cols_used_in_expr(&where_copy);
                         // if any of the columns used in the partial index WHERE clause is being
                         // updated, we need to update this index
@@ -447,6 +448,8 @@ fn build_scan_op(table: &Table, iter_dir: IterationDirection) -> Operation {
     }
 }
 
+/// Returns a set of column indices used in the expression.
+/// *Must* be used on an Expr already processed by `bind_and_rewrite_expr`
 fn collect_cols_used_in_expr(expr: &Expr) -> HashSet<usize> {
     let mut acc = HashSet::new();
     let _ = walk_expr(expr, &mut |expr| match expr {