From be0ef72f02cd071453458bf5a8de23ef148b9fec Mon Sep 17 00:00:00 2001 From: EnRaiha <15997552+EnRaiha@users.noreply.github.com> Date: Sun, 27 Sep 2026 18:44:58 +0800 Subject: [PATCH 1/2] planner: write the vector-primary key guard as is_none_or MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `cargo clippy --profile ci -p nodedb --all-targets --all-features -- -D warnings` aborts on this expression at `vector_primary.rs:161`: error: this boolean expression can be simplified = help: ... #nonminimal_bool so the lint job is red on `main` for every branch, and the first thing it reports is a file no branch has touched. Clippy's own suggested rewrite is this one, and it is the negation identity `is_none_or(f) == !is_some_and(!f)`: `!get(k).is_some_and(|v| !matches!(v, SqlValue::Null))` becomes `get(k).is_none_or(|v| matches!(v, SqlValue::Null))`. All three cases agree, including the one a reader might expect to differ — an absent key inserts the key-column value under both spellings: present non-null -> false / false present Null -> true / true absent -> true / true Verified: `main` exits 101 on this file, this branch exits 0. --- .../control/planner/sql_plan_convert/dml/vector_primary.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/nodedb/src/control/planner/sql_plan_convert/dml/vector_primary.rs b/nodedb/src/control/planner/sql_plan_convert/dml/vector_primary.rs index e57e34c91..a45974aa1 100644 --- a/nodedb/src/control/planner/sql_plan_convert/dml/vector_primary.rs +++ b/nodedb/src/control/planner/sql_plan_convert/dml/vector_primary.rs @@ -161,9 +161,9 @@ pub(in super::super) fn convert_vector_primary_insert( let key_column = declared.as_deref().unwrap_or(primary_key); let mut fields = row.payload_fields.clone(); if !is_auto_rowid_pk(primary_key) - && !fields + && fields .get(key_column) - .is_some_and(|v| !matches!(v, SqlValue::Null)) + .is_none_or(|v| matches!(v, SqlValue::Null)) { fields.insert(key_column.to_string(), SqlValue::String(doc_id.clone())); } From 77cb538939a0d44a64dd863bb61024c359feb34e Mon Sep 17 00:00:00 2001 From: EnRaiha Date: Tue, 29 Sep 2026 03:09:08 +0800 Subject: [PATCH 2/2] test(planner): pin the vector-primary key-column fill rule The fill guard decides whether a synthesised identity is written back under the key column. Its condition must mirror the identity extractor: fill on an absent or NULL key, leave every present value alone. Four cases pin that split. Two bind the fill direction: a missing key column and an explicit NULL both end with the identity under the key. Two cover the "left alone" direction: a present key keeps its bytes, and an empty key stays empty. The empty case binds the identity extractor's classification of `""` rather than the guard, because the guard writes `doc_id` and `doc_id` already equals `""` there. No case is red on `main`. The guard is behaviour-identical there, because `!is_some_and(!p)` and `is_none_or(p)` agree on every input, so the previous commit changes the form only. A mutation that disables the guard fails the two fill cases (exit 100). The two "left alone" cases do not discriminate that mutation, and they cannot discriminate an always-fill mutation either: for a present String the identity the fill writes is that value's own string form. --- .../sql_plan_convert/dml/vector_primary.rs | 107 ++++++++++++++++++ 1 file changed, 107 insertions(+) diff --git a/nodedb/src/control/planner/sql_plan_convert/dml/vector_primary.rs b/nodedb/src/control/planner/sql_plan_convert/dml/vector_primary.rs index a45974aa1..54583f003 100644 --- a/nodedb/src/control/planner/sql_plan_convert/dml/vector_primary.rs +++ b/nodedb/src/control/planner/sql_plan_convert/dml/vector_primary.rs @@ -498,4 +498,111 @@ mod tests { }) if bytes.is_empty() )); } + + /// A row whose payload carries no key column still mints an identity, and + /// that identity must also appear under the key column. A later point read + /// finds the row by `pk_bytes` regardless, so an omitted column leaves the + /// key value absent from the sidecar and from any payload index on it. + #[test] + fn missing_key_column_carries_the_minted_identity() { + let ctx = make_ctx(0); + let rows = vec![VectorPrimaryRow { + surrogate: nodedb_types::Surrogate::ZERO, + vector: vec![0.0f32; 3], + payload_fields: std::collections::HashMap::new(), + }]; + let tasks = convert(&ctx, &rows, VectorPrimaryInsertIntent::Insert).expect("convert"); + let (pk_bytes, carried) = carried_key_column(&tasks[0]); + assert_eq!( + carried, + String::from_utf8(pk_bytes).expect("identity is utf8"), + "the payload's key column must equal the identity the point read resolves" + ); + } + + /// An explicit NULL key column takes the same path as an absent one: both + /// mint, and both must be replaced in the payload. + #[test] + fn null_key_column_carries_the_minted_identity() { + let ctx = make_ctx(0); + let mut fields = std::collections::HashMap::new(); + fields.insert("id".to_string(), SqlValue::Null); + let rows = vec![VectorPrimaryRow { + surrogate: nodedb_types::Surrogate::ZERO, + vector: vec![0.0f32; 3], + payload_fields: fields, + }]; + let tasks = convert(&ctx, &rows, VectorPrimaryInsertIntent::Insert).expect("convert"); + let (pk_bytes, carried) = carried_key_column(&tasks[0]); + assert_eq!( + carried, + String::from_utf8(pk_bytes).expect("identity is utf8"), + "a NULL key column must be filled, not kept" + ); + } + + /// The fill path fills an absent key and a NULL key. A present value stays + /// as the row carries it: `pk_bytes` already holds that value's string + /// form, and the column keeps the value. An overwrite replaces a typed + /// column with its string form, so `Int(7)` lands in the payload as `"7"`. + #[test] + fn present_key_column_is_left_alone() { + let ctx = make_ctx(0); + let rows = vec![row(3, "r1")]; + let tasks = convert(&ctx, &rows, VectorPrimaryInsertIntent::Insert).expect("convert"); + let (_, carried) = carried_key_column(&tasks[0]); + assert_eq!(carried, "r1", "a present key column must survive untouched"); + } + + /// An empty key column is a present key, not a missing one. The identity + /// extractor reads `""` as `Present("")` and returns `""` as the identity, + /// so `pk_bytes` stays empty. The guard writes `doc_id` when it fills, and + /// `doc_id` already equals `""` here, so a fill changes nothing. An + /// extractor that reads `""` as missing mints a fresh identity, and this + /// assertion fails on `pk_bytes`. + #[test] + fn empty_key_column_is_left_alone() { + let ctx = make_ctx(0); + let mut fields = std::collections::HashMap::new(); + fields.insert("id".to_string(), SqlValue::String(String::new())); + let rows = vec![VectorPrimaryRow { + surrogate: nodedb_types::Surrogate::ZERO, + vector: vec![0.0f32; 3], + payload_fields: fields, + }]; + let tasks = convert(&ctx, &rows, VectorPrimaryInsertIntent::Insert).expect("convert"); + let (pk_bytes, carried) = carried_key_column(&tasks[0]); + assert!( + carried.is_empty(), + "an empty key column must not be replaced by a minted identity" + ); + assert!( + pk_bytes.is_empty(), + "the identity must stay the empty key the row already carries" + ); + } + + /// Read the key column back out of the payload a `DirectInsert` carries. + fn carried_key_column(task: &PhysicalTask) -> (Vec, String) { + let (pk_bytes, payload) = match &task.plan { + PhysicalPlan::Vector(VectorOp::DirectInsert { + pk_bytes, payload, .. + }) => (pk_bytes.clone(), payload.clone()), + other => panic!("expected DirectInsert, got {other:?}"), + }; + // Name the cause here: a guard that fills nothing leaves the payload + // empty, and a bare decode error reports a codec failure rather than + // the missing key column this assertion is about. + assert!( + !payload.is_empty(), + "the payload carries no key column, so the guard did not fill it: expected {}", + String::from_utf8_lossy(&pk_bytes) + ); + let decoded: std::collections::HashMap = + zerompk::from_msgpack(&payload).expect("payload decodes"); + match decoded.get("id") { + Some(nodedb_types::Value::String(id)) => (pk_bytes, id.clone()), + other => panic!("expected a string key column in the payload, got {other:?}"), + } + } }