diff --git a/crates/qcraft-core/src/ast/conditions.rs b/crates/qcraft-core/src/ast/conditions.rs index 61913b3..1173961 100644 --- a/crates/qcraft-core/src/ast/conditions.rs +++ b/crates/qcraft-core/src/ast/conditions.rs @@ -66,17 +66,12 @@ impl Conditions { /// `field IS NULL` pub fn is_null(field: FieldRef) -> Self { - Self::comparison(field, CompareOp::IsNull, Expr::Value(Value::Null)) + Self::comparison(field, CompareOp::IsNull, Expr::Value(Value::Bool(true))) } /// `field IS NOT NULL` pub fn is_not_null(field: FieldRef) -> Self { - Self::and(vec![ConditionNode::Comparison(Box::new(Comparison { - left: Expr::Field(field), - op: CompareOp::IsNull, - right: Expr::Value(Value::Null), - negate: true, - }))]) + Self::comparison(field, CompareOp::IsNull, Expr::Value(Value::Bool(false))) } /// `field LIKE pattern` (raw — caller provides the full pattern with wildcards) diff --git a/crates/qcraft-postgres/src/lib.rs b/crates/qcraft-postgres/src/lib.rs index 686dadc..eba0262 100644 --- a/crates/qcraft-postgres/src/lib.rs +++ b/crates/qcraft-postgres/src/lib.rs @@ -1318,7 +1318,16 @@ impl Renderer for PostgresRenderer { return Ok(()); } CompareOp::IsNull => { - ctx.keyword("IS NULL"); + match right { + Expr::Value(Value::Bool(true)) => ctx.keyword("IS NULL"), + Expr::Value(Value::Bool(false)) => ctx.keyword("IS NOT NULL"), + _ => { + return Err(RenderError::unsupported( + "IsNull", + "IsNull right operand must be a boolean", + )); + } + }; return Ok(()); } CompareOp::Similar => ctx.keyword("SIMILAR TO"), diff --git a/crates/qcraft-postgres/tests/dql.rs b/crates/qcraft-postgres/tests/dql.rs index 01277ce..0fd4ab2 100644 --- a/crates/qcraft-postgres/tests/dql.rs +++ b/crates/qcraft-postgres/tests/dql.rs @@ -2672,3 +2672,89 @@ fn pg_xor_with_subquery_operand_is_allowed() { let (sql, _params) = render_expr_pg(e); assert!(sql.contains(" # "), "got: {sql}"); } + +// --------------------------------------------------------------------------- +// IS NULL / IS NOT NULL (value-driven) +// --------------------------------------------------------------------------- + +fn users_from() -> Vec { + vec![FromItem::table(SchemaRef::new("users"))] +} + +#[test] +fn where_is_null() { + let stmt = QueryStmt { + columns: vec![SelectColumn::Star(None)], + from: Some(users_from()), + where_clause: Some(Conditions::is_null(FieldRef::new("users", "email"))), + ..simple_query() + }; + assert_eq!( + render(&stmt), + r#"SELECT * FROM "users" WHERE "users"."email" IS NULL"# + ); +} + +#[test] +fn where_is_not_null() { + let stmt = QueryStmt { + columns: vec![SelectColumn::Star(None)], + from: Some(users_from()), + where_clause: Some(Conditions::is_not_null(FieldRef::new("users", "email"))), + ..simple_query() + }; + assert_eq!( + render(&stmt), + r#"SELECT * FROM "users" WHERE "users"."email" IS NOT NULL"# + ); +} + +#[test] +fn where_is_null_negated() { + let stmt = QueryStmt { + columns: vec![SelectColumn::Star(None)], + from: Some(users_from()), + where_clause: Some(Conditions::is_null(FieldRef::new("users", "email")).negated()), + ..simple_query() + }; + assert_eq!( + render(&stmt), + r#"SELECT * FROM "users" WHERE NOT ("users"."email" IS NULL)"# + ); +} + +#[test] +fn where_is_not_null_negated() { + let stmt = QueryStmt { + columns: vec![SelectColumn::Star(None)], + from: Some(users_from()), + where_clause: Some(Conditions::is_not_null(FieldRef::new("users", "email")).negated()), + ..simple_query() + }; + assert_eq!( + render(&stmt), + r#"SELECT * FROM "users" WHERE NOT ("users"."email" IS NOT NULL)"# + ); +} + +#[test] +fn is_null_non_boolean_right_errors() { + let stmt = QueryStmt { + columns: vec![SelectColumn::Star(None)], + from: Some(users_from()), + where_clause: Some(Conditions::and(vec![ConditionNode::Comparison(Box::new( + Comparison { + left: Expr::Field(FieldRef::new("users", "email")), + op: CompareOp::IsNull, + right: Expr::Value(Value::Null), + negate: false, + }, + ))])), + ..simple_query() + }; + let err = PostgresRenderer::new() + .render_query_stmt(&stmt) + .unwrap_err() + .to_string(); + assert!(err.contains("IsNull"), "unexpected error: {err}"); +} diff --git a/crates/qcraft-postgres/tests/integration/dql.rs b/crates/qcraft-postgres/tests/integration/dql.rs index 7632092..4c91a1b 100644 --- a/crates/qcraft-postgres/tests/integration/dql.rs +++ b/crates/qcraft-postgres/tests/integration/dql.rs @@ -518,7 +518,7 @@ fn where_is_null() { Comparison { left: Expr::Field(FieldRef::new("users", "age")), op: CompareOp::IsNull, - right: Expr::Value(Value::Null), + right: Expr::Value(Value::Bool(true)), negate: false, }, ))])), diff --git a/crates/qcraft-sqlite/src/lib.rs b/crates/qcraft-sqlite/src/lib.rs index b363dd7..d9b72fd 100644 --- a/crates/qcraft-sqlite/src/lib.rs +++ b/crates/qcraft-sqlite/src/lib.rs @@ -968,7 +968,16 @@ impl Renderer for SqliteRenderer { return Ok(()); } CompareOp::IsNull => { - ctx.keyword("IS NULL"); + match right { + Expr::Value(Value::Bool(true)) => ctx.keyword("IS NULL"), + Expr::Value(Value::Bool(false)) => ctx.keyword("IS NOT NULL"), + _ => { + return Err(RenderError::unsupported( + "IsNull", + "IsNull right operand must be a boolean", + )); + } + }; return Ok(()); } CompareOp::Regex => ctx.keyword("REGEXP"), diff --git a/crates/qcraft-sqlite/tests/dql.rs b/crates/qcraft-sqlite/tests/dql.rs index 881b0fd..d1f2f14 100644 --- a/crates/qcraft-sqlite/tests/dql.rs +++ b/crates/qcraft-sqlite/tests/dql.rs @@ -1998,3 +1998,72 @@ fn select_with_timedelta_param() { }] ); } + +// --------------------------------------------------------------------------- +// IS NULL / IS NOT NULL (value-driven) +// --------------------------------------------------------------------------- + +#[test] +fn where_is_null() { + let stmt = QueryStmt { + where_clause: Some(Conditions::is_null(FieldRef::new("users", "email"))), + ..simple_query() + }; + assert_eq!( + render(&stmt), + r#"SELECT * FROM "users" WHERE "users"."email" IS NULL"# + ); +} + +#[test] +fn where_is_not_null() { + let stmt = QueryStmt { + where_clause: Some(Conditions::is_not_null(FieldRef::new("users", "email"))), + ..simple_query() + }; + assert_eq!( + render(&stmt), + r#"SELECT * FROM "users" WHERE "users"."email" IS NOT NULL"# + ); +} + +#[test] +fn where_is_null_negated() { + let stmt = QueryStmt { + where_clause: Some(Conditions::is_null(FieldRef::new("users", "email")).negated()), + ..simple_query() + }; + assert_eq!( + render(&stmt), + r#"SELECT * FROM "users" WHERE NOT ("users"."email" IS NULL)"# + ); +} + +#[test] +fn where_is_not_null_negated() { + let stmt = QueryStmt { + where_clause: Some(Conditions::is_not_null(FieldRef::new("users", "email")).negated()), + ..simple_query() + }; + assert_eq!( + render(&stmt), + r#"SELECT * FROM "users" WHERE NOT ("users"."email" IS NOT NULL)"# + ); +} + +#[test] +fn is_null_non_boolean_right_errors() { + let stmt = QueryStmt { + where_clause: Some(Conditions::and(vec![ConditionNode::Comparison(Box::new( + Comparison { + left: Expr::Field(FieldRef::new("users", "email")), + op: CompareOp::IsNull, + right: Expr::Value(Value::Null), + negate: false, + }, + ))])), + ..simple_query() + }; + let err = render_err(&stmt); + assert!(err.contains("IsNull"), "unexpected error: {err}"); +} diff --git a/crates/qcraft-sqlite/tests/integration_dql.rs b/crates/qcraft-sqlite/tests/integration_dql.rs index a1aa723..0a8a704 100644 --- a/crates/qcraft-sqlite/tests/integration_dql.rs +++ b/crates/qcraft-sqlite/tests/integration_dql.rs @@ -562,7 +562,7 @@ fn where_is_null() { Comparison { left: Expr::Field(FieldRef::new("users", "age")), op: CompareOp::IsNull, - right: Expr::Value(Value::Null), + right: Expr::Value(Value::Bool(true)), negate: false, }, ))])), @@ -589,8 +589,8 @@ fn where_is_not_null() { Comparison { left: Expr::Field(FieldRef::new("users", "age")), op: CompareOp::IsNull, - right: Expr::Value(Value::Null), - negate: true, + right: Expr::Value(Value::Bool(false)), + negate: false, }, ))])), ..simple_query() diff --git a/docs/superpowers/plans/2026-07-06-isnull-value-driven.md b/docs/superpowers/plans/2026-07-06-isnull-value-driven.md new file mode 100644 index 0000000..9d47125 --- /dev/null +++ b/docs/superpowers/plans/2026-07-06-isnull-value-driven.md @@ -0,0 +1,386 @@ +# Value-driven IS NULL / IS NOT NULL Implementation Plan + +> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking. + +**Goal:** Make `CompareOp::IsNull` polarity driven by its boolean `right` operand (`Bool(true)` → `IS NULL`, `Bool(false)` → `IS NOT NULL`), keeping `negate` a pure `NOT (...)` wrapper. + +**Architecture:** The renderer's `IsNull` arm branches on `right` to pick the keyword; a non-boolean `right` (including `Value::Null`) is a hard `RenderError`. The `is_null`/`is_not_null` constructors emit a boolean `right` with `negate: false`. Two renderers (`qcraft-sqlite`, `qcraft-postgres`) plus the shared core constructors. + +**Tech Stack:** Rust workspace (`qcraft-core`, `qcraft-sqlite`, `qcraft-postgres`), `cargo test`, `rusqlite` (sqlite integration), `testcontainers` (postgres integration, needs Docker). + +## Global Constraints + +- `right` for `CompareOp::IsNull` is **strictly boolean**. `Bool(true)` → `IS NULL`, `Bool(false)` → `IS NOT NULL`, anything else (incl. `Value::Null`) → `RenderError::unsupported`. +- `negate` / `negated` are unchanged — pure generic `NOT (...)` wrappers. +- The boolean `right` is consumed as a keyword selector only; it is **never** parameterized (`IS $1` stays invalid). +- No AI/Claude attribution anywhere in commits, code, or docs. +- Design reference: `docs/superpowers/specs/2026-07-06-isnull-value-driven-design.md`. +- Version bump (major) + CHANGELOG are handled separately at release time — **not** in this plan. + +--- + +### Task 1: SQLite renderer + core constructors — value-driven IsNull + +**Files:** +- Modify: `crates/qcraft-core/src/ast/conditions.rs:68-80` (constructors) +- Modify: `crates/qcraft-sqlite/src/lib.rs:970-973` (`IsNull` render arm) +- Test: `crates/qcraft-sqlite/tests/dql.rs` (new exact-SQL unit tests) +- Modify: `crates/qcraft-sqlite/tests/integration_dql.rs:558-608` (fix hand-built `Value::Null` sites) + +**Interfaces:** +- Consumes: `Conditions::is_null(FieldRef) -> Conditions`, `Conditions::is_not_null(FieldRef) -> Conditions`, `Conditions::negated(self) -> Self`, `Comparison { left, op, right, negate }`, `CompareOp::IsNull`, `Expr::Value(Value)`, `Value::{Bool, Null}`, `RenderError::unsupported(feature, message)`. +- Produces: after this task `Conditions::is_null` yields `right = Value::Bool(true)` and `Conditions::is_not_null` yields `right = Value::Bool(false)`, both with `negate: false`. Later tasks (postgres) rely on these constructors. + +- [ ] **Step 1: Write the failing unit tests** + +Append to `crates/qcraft-sqlite/tests/dql.rs` (uses existing `render`, `render_err`, `simple_query` helpers already in that file): + +```rust +// --------------------------------------------------------------------------- +// IS NULL / IS NOT NULL (value-driven) +// --------------------------------------------------------------------------- + +#[test] +fn where_is_null() { + let stmt = QueryStmt { + where_clause: Some(Conditions::is_null(FieldRef::new("users", "email"))), + ..simple_query() + }; + assert_eq!( + render(&stmt), + r#"SELECT * FROM "users" WHERE "users"."email" IS NULL"# + ); +} + +#[test] +fn where_is_not_null() { + let stmt = QueryStmt { + where_clause: Some(Conditions::is_not_null(FieldRef::new("users", "email"))), + ..simple_query() + }; + assert_eq!( + render(&stmt), + r#"SELECT * FROM "users" WHERE "users"."email" IS NOT NULL"# + ); +} + +#[test] +fn where_is_null_negated() { + let stmt = QueryStmt { + where_clause: Some(Conditions::is_null(FieldRef::new("users", "email")).negated()), + ..simple_query() + }; + assert_eq!( + render(&stmt), + r#"SELECT * FROM "users" WHERE NOT ("users"."email" IS NULL)"# + ); +} + +#[test] +fn where_is_not_null_negated() { + let stmt = QueryStmt { + where_clause: Some(Conditions::is_not_null(FieldRef::new("users", "email")).negated()), + ..simple_query() + }; + assert_eq!( + render(&stmt), + r#"SELECT * FROM "users" WHERE NOT ("users"."email" IS NOT NULL)"# + ); +} + +#[test] +fn is_null_non_boolean_right_errors() { + let stmt = QueryStmt { + where_clause: Some(Conditions::and(vec![ConditionNode::Comparison(Box::new( + Comparison { + left: Expr::Field(FieldRef::new("users", "email")), + op: CompareOp::IsNull, + right: Expr::Value(Value::Null), + negate: false, + }, + ))])), + ..simple_query() + }; + let err = render_err(&stmt); + assert!(err.contains("IsNull"), "unexpected error: {err}"); +} +``` + +- [ ] **Step 2: Run tests to verify they fail** + +Run: `cargo test -p qcraft-sqlite --test dql -- is_null is_not_null` +Expected: FAIL — `where_is_not_null` (gets `NOT (... IS NULL)`), `where_is_not_null_negated` (gets `NOT (NOT (... IS NULL))`), and `is_null_non_boolean_right_errors` (panics: render returns `Ok`, so `unwrap_err` fails). + +- [ ] **Step 3: Update the core constructors** + +In `crates/qcraft-core/src/ast/conditions.rs`, replace both constructors (currently lines 67-80): + +```rust + /// `field IS NULL` + pub fn is_null(field: FieldRef) -> Self { + Self::comparison(field, CompareOp::IsNull, Expr::Value(Value::Bool(true))) + } + + /// `field IS NOT NULL` + pub fn is_not_null(field: FieldRef) -> Self { + Self::comparison(field, CompareOp::IsNull, Expr::Value(Value::Bool(false))) + } +``` + +(`Self::comparison` already sets `negate: false` and `connector: And`.) + +- [ ] **Step 4: Update the SQLite renderer IsNull arm** + +In `crates/qcraft-sqlite/src/lib.rs`, replace the `CompareOp::IsNull` arm (currently lines 970-973): + +```rust + CompareOp::IsNull => { + match right { + Expr::Value(Value::Bool(true)) => ctx.keyword("IS NULL"), + Expr::Value(Value::Bool(false)) => ctx.keyword("IS NOT NULL"), + _ => { + return Err(RenderError::unsupported( + "IsNull", + "IsNull right operand must be a boolean", + )); + } + }; + return Ok(()); + } +``` + +- [ ] **Step 5: Fix hand-built SQLite integration tests** + +In `crates/qcraft-sqlite/tests/integration_dql.rs`: + +- In `where_is_null` (line ~564), change `right: Expr::Value(Value::Null),` to `right: Expr::Value(Value::Bool(true)),` (keep `negate: false`). +- In `where_is_not_null` (lines ~591-593), change `right: Expr::Value(Value::Null),` to `right: Expr::Value(Value::Bool(false)),` **and** `negate: true,` to `negate: false,`. The `assert_eq!(rows.len(), 4)` stays valid (native `IS NOT NULL`). + +- [ ] **Step 6: Run the full SQLite crate tests** + +Run: `cargo test -p qcraft-sqlite` +Expected: PASS (unit `dql` tests + `integration_dql` in-memory tests all green). + +- [ ] **Step 7: Commit** + +```bash +git add crates/qcraft-core/src/ast/conditions.rs crates/qcraft-sqlite/src/lib.rs crates/qcraft-sqlite/tests/dql.rs crates/qcraft-sqlite/tests/integration_dql.rs +git commit -m "feat(core,sqlite): value-driven IS NULL / IS NOT NULL via boolean right" +``` + +--- + +### Task 2: PostgreSQL renderer — value-driven IsNull + +**Files:** +- Modify: `crates/qcraft-postgres/src/lib.rs:1320-1323` (`IsNull` render arm) +- Test: `crates/qcraft-postgres/tests/dql.rs` (new exact-SQL unit tests) +- Modify: `crates/qcraft-postgres/tests/integration/dql.rs:511-534` (fix hand-built `Value::Null` site) + +**Interfaces:** +- Consumes: the constructors updated in Task 1 (`Conditions::is_null` → `Bool(true)`, `Conditions::is_not_null` → `Bool(false)`), plus `PostgresRenderer::new()`, `render(&QueryStmt) -> String`, `simple_query()`, `SelectColumn::Star`, `FromItem::table`, `SchemaRef::new`, `RenderError::unsupported`. +- Produces: nothing consumed by later tasks. + +- [ ] **Step 1: Write the failing unit tests** + +Append to `crates/qcraft-postgres/tests/dql.rs` (uses existing `render` and `simple_query`; note postgres `simple_query()` has empty `columns` and `from: None`, so set them explicitly): + +```rust +// --------------------------------------------------------------------------- +// IS NULL / IS NOT NULL (value-driven) +// --------------------------------------------------------------------------- + +fn users_from() -> Vec { + vec![FromItem::table(SchemaRef::new("users"))] +} + +#[test] +fn where_is_null() { + let stmt = QueryStmt { + columns: vec![SelectColumn::Star(None)], + from: Some(users_from()), + where_clause: Some(Conditions::is_null(FieldRef::new("users", "email"))), + ..simple_query() + }; + assert_eq!( + render(&stmt), + r#"SELECT * FROM "users" WHERE "users"."email" IS NULL"# + ); +} + +#[test] +fn where_is_not_null() { + let stmt = QueryStmt { + columns: vec![SelectColumn::Star(None)], + from: Some(users_from()), + where_clause: Some(Conditions::is_not_null(FieldRef::new("users", "email"))), + ..simple_query() + }; + assert_eq!( + render(&stmt), + r#"SELECT * FROM "users" WHERE "users"."email" IS NOT NULL"# + ); +} + +#[test] +fn where_is_null_negated() { + let stmt = QueryStmt { + columns: vec![SelectColumn::Star(None)], + from: Some(users_from()), + where_clause: Some(Conditions::is_null(FieldRef::new("users", "email")).negated()), + ..simple_query() + }; + assert_eq!( + render(&stmt), + r#"SELECT * FROM "users" WHERE NOT ("users"."email" IS NULL)"# + ); +} + +#[test] +fn where_is_not_null_negated() { + let stmt = QueryStmt { + columns: vec![SelectColumn::Star(None)], + from: Some(users_from()), + where_clause: Some(Conditions::is_not_null(FieldRef::new("users", "email")).negated()), + ..simple_query() + }; + assert_eq!( + render(&stmt), + r#"SELECT * FROM "users" WHERE NOT ("users"."email" IS NOT NULL)"# + ); +} + +#[test] +fn is_null_non_boolean_right_errors() { + let stmt = QueryStmt { + columns: vec![SelectColumn::Star(None)], + from: Some(users_from()), + where_clause: Some(Conditions::and(vec![ConditionNode::Comparison(Box::new( + Comparison { + left: Expr::Field(FieldRef::new("users", "email")), + op: CompareOp::IsNull, + right: Expr::Value(Value::Null), + negate: false, + }, + ))])), + ..simple_query() + }; + let err = PostgresRenderer::new() + .render_query_stmt(&stmt) + .unwrap_err() + .to_string(); + assert!(err.contains("IsNull"), "unexpected error: {err}"); +} +``` + +- [ ] **Step 2: Run tests to verify they fail** + +Run: `cargo test -p qcraft-postgres --test dql -- is_null is_not_null` +Expected: FAIL — `where_is_not_null` gets `IS NULL` (old renderer ignores `right`), and `is_null_non_boolean_right_errors` panics (render returns `Ok`). + +- [ ] **Step 3: Update the PostgreSQL renderer IsNull arm** + +In `crates/qcraft-postgres/src/lib.rs`, replace the `CompareOp::IsNull` arm (currently lines 1320-1323): + +```rust + CompareOp::IsNull => { + match right { + Expr::Value(Value::Bool(true)) => ctx.keyword("IS NULL"), + Expr::Value(Value::Bool(false)) => ctx.keyword("IS NOT NULL"), + _ => { + return Err(RenderError::unsupported( + "IsNull", + "IsNull right operand must be a boolean", + )); + } + }; + return Ok(()); + } +``` + +- [ ] **Step 4: Fix hand-built PostgreSQL integration test** + +In `crates/qcraft-postgres/tests/integration/dql.rs`, `where_is_null` (line ~521): change `right: Expr::Value(Value::Null),` to `right: Expr::Value(Value::Bool(true)),` (keep `negate: false`). Row assertion (Eve) stays valid. + +- [ ] **Step 5: Run the PostgreSQL unit tests** + +Run: `cargo test -p qcraft-postgres --test dql` +Expected: PASS. + +> Note: `cargo test -p qcraft-postgres --test integration` requires Docker (testcontainers). Run it if Docker is available; otherwise the `dql` unit tests above are the authoritative fast check for this task. + +- [ ] **Step 6: Commit** + +```bash +git add crates/qcraft-postgres/src/lib.rs crates/qcraft-postgres/tests/dql.rs crates/qcraft-postgres/tests/integration/dql.rs +git commit -m "feat(postgres): value-driven IS NULL / IS NOT NULL via boolean right" +``` + +--- + +### Task 3: Documentation + +**Files:** +- Modify: `docs/type-reference.md:65,174` +- Verify: `docs/type-reference.md:274-275`, `docs/select-queries.md:327-337` (already claim `IS NOT NULL` — confirm now correct) + +**Interfaces:** +- Consumes: nothing. Produces: nothing. + +- [ ] **Step 1: Update the IsNull operator mapping** + +In `docs/type-reference.md`, the operator table row (line ~174) currently reads: + +``` +IsNull IS NULL +``` + +Replace with: + +``` +IsNull IS NULL / IS NOT NULL (selected by boolean `right`: true → IS NULL, false → IS NOT NULL) +``` + +- [ ] **Step 2: Clarify the parameterization note** + +In `docs/type-reference.md` (line ~65), replace: + +``` +The only exception is `CompareOp::IsNull` — it always renders as `IS NULL` because `IS $1` is not valid SQL syntax. +``` + +with: + +``` +The exception is `CompareOp::IsNull`: its boolean `right` selects the keyword (`Bool(true)` → `IS NULL`, `Bool(false)` → `IS NOT NULL`) and is never emitted as a bind parameter, since `IS $1` is not valid SQL syntax. A non-boolean `right` is a render error. +``` + +- [ ] **Step 3: Verify the SELECT docs are now accurate** + +Confirm `docs/select-queries.md:327-337` and `docs/type-reference.md:274-275` show `Conditions::is_not_null(...)` → `... IS NOT NULL`. These are now produced by the renderer; no change needed if the text already matches. Adjust only if they still describe the old `NOT (... IS NULL)` output. + +- [ ] **Step 4: Commit** + +```bash +git add docs/type-reference.md docs/select-queries.md +git commit -m "docs: document value-driven IS NULL / IS NOT NULL" +``` + +--- + +## Self-Review + +**Spec coverage:** +- Value-driven semantics table → Task 1 Step 4 + Task 2 Step 3 (renderer branch). ✓ +- Strictly-boolean `right`, error otherwise → renderer `_ =>` arm + error tests. ✓ +- `negate` unchanged → not modified; negated tests assert `NOT (...)` wrapping. ✓ +- Constructors emit boolean `right` → Task 1 Step 3. ✓ +- TDD failing-first for native `IS NOT NULL` → Task 1/2 Step 1-2. ✓ +- Both dialects → Task 1 (sqlite) + Task 2 (postgres). ✓ +- Update hand-built `Value::Null` tests → Task 1 Step 5, Task 2 Step 4. ✓ +- Docs → Task 3. ✓ +- Version/CHANGELOG deferred to release → Global Constraints. ✓ + +**Placeholder scan:** No TBD/TODO; all code blocks concrete. ✓ + +**Type consistency:** `RenderError::unsupported(feature, message)`, `Value::Bool(bool)`, `Expr::Value(Value)`, `Conditions::is_null/is_not_null/negated`, `render`/`render_err`/`simple_query` helpers match their source definitions. ✓ diff --git a/docs/superpowers/specs/2026-07-06-isnull-value-driven-design.md b/docs/superpowers/specs/2026-07-06-isnull-value-driven-design.md new file mode 100644 index 0000000..0dd7f0f --- /dev/null +++ b/docs/superpowers/specs/2026-07-06-isnull-value-driven-design.md @@ -0,0 +1,141 @@ +# Value-driven `IS NULL` / `IS NOT NULL` + +**Date:** 2026-07-06 +**Status:** Approved (design) + +## Problem + +`CompareOp::IsNull` conflates two orthogonal concerns: + +- **Null polarity** (`IS NULL` vs `IS NOT NULL`) — in Django this is driven by the + lookup value: `field__isnull=True` → `IS NULL`, `field__isnull=False` → `IS NOT NULL`. +- **Negation** (`~Q(...)`) — a generic `NOT (...)` wrapper around any predicate. + +Today the renderer **ignores `right`** for `IsNull` and always emits `IS NULL` +(`qcraft-sqlite/src/lib.rs:970`, `qcraft-postgres/src/lib.rs:1320`). Null polarity is +instead smuggled through the `negate` flag: `is_not_null` is built as +`IsNull + negate:true`, which renders `NOT (field IS NULL)`. + +Consequences: + +1. **`right` is dead data** for `IsNull` — the Django boolean has nowhere to live. + A binding that maps `"ISNULL" => CompareOp::IsNull` and drops the boolean silently + collapses `isnull=False` into `IS NULL` (the root of the observed binding bug). +2. **No native `IS NOT NULL` token** — `is_not_null()` emits `NOT (field IS NULL)`. +3. **Docs are wrong** — `type-reference.md:275` and `select-queries.md:334-336` + promise `field IS NOT NULL`, which the renderer never produces. +4. **Ugly double negation** — `~Q(field__isnull=False)` becomes + `NOT (NOT (field IS NULL))`. + +## Approach + +Make null polarity **value-driven** (Approach A: boolean encoded in `right`), matching +Django 1:1. Keep `negate` as a pure, generic `NOT (...)` wrapper (it already is — +`qcraft-sqlite/src/lib.rs:864-870` has no `IsNull` special-casing). + +This was chosen over a dedicated `CompareOp::IsNotNull` variant (Approach B) because it +mirrors Django's "one lookup, boolean rhs" model exactly and makes the PyO3 binding a +pure pass-through of `right` — the strongest fix for the original bug — with a minimal, +focused code change. `right` for `IsNull` is now strictly boolean: any hand-built AST +that passed `Value::Null` will error instead of silently rendering `IS NULL`. + +### AST semantics for `CompareOp::IsNull` + +The `right` operand encodes the Django `isnull` boolean and is consumed by the renderer +as a **keyword selector** — it is never emitted as a bind parameter, preserving the +existing `IS $1 is not valid SQL` invariant. + +`right` **must** be a boolean literal. Anything else — including `Value::Null` — is a +hard error; there is no legacy fallback. + +| `right` | rendered keyword | +| -------------------------- | ---------------- | +| `Expr::Value(Value::Bool(true))` | `IS NULL` | +| `Expr::Value(Value::Bool(false))` | `IS NOT NULL` | +| anything else (incl. `Value::Null`) | `RenderError::unsupported` | + +`Comparison.negate` and `Conditions.negated` are unchanged — they wrap the produced +predicate in `NOT (...)`. This yields the full, orthogonal truth table: + +| AST (`op=IsNull`) | SQL | ≡ | +| -------------------------------- | ---------------------------- | ----------- | +| `right=Bool(true)` | `field IS NULL` | | +| `right=Bool(false)` | `field IS NOT NULL` | | +| `right=Bool(true), negate` | `NOT (field IS NULL)` | `IS NOT NULL` | +| `right=Bool(false), negate` | `NOT (field IS NOT NULL)` | `IS NULL` | + +The two redundant spellings (rows 3/4) are intentional and expected — the same +`Eq+negate` vs `Neq` redundancy — and match Django, which also emits `NOT (x IS NULL)` +for `~Q(x__isnull=True)` rather than simplifying. + +## Changes + +### Renderers (`qcraft-sqlite`, `qcraft-postgres`) + +The `CompareOp::IsNull` arm of `render_compare_op` branches on `right`: + +```rust +CompareOp::IsNull => { + match right { + Expr::Value(Value::Bool(true)) => ctx.keyword("IS NULL"), + Expr::Value(Value::Bool(false)) => ctx.keyword("IS NOT NULL"), + _ => return Err(RenderError::unsupported( + "IsNull", + "IsNull right operand must be a boolean", + )), + }; + return Ok(()); +} +``` + +`left` is already rendered before the `match op` block, so this arm only chooses the +trailing keyword. The boolean is not passed to `render_expr`, so it never becomes a +parameter. During implementation, confirm no separate AST walk parameterizes `right` +(`contains_unbound_param` at `conditions.rs:196` treats a `Value::Bool` literal as bound, +so it is safe). + +### Constructors (`qcraft-core/src/ast/conditions.rs`) + +Update every `is_null` / `is_not_null` constructor (both the `Conditions::*` and any +`Comparison::*` forms): + +- `is_null` → `right = Value::Bool(true)`, `negate = false` +- `is_not_null` → `right = Value::Bool(false)`, `negate = false` + (replacing the current `right = Null` + `negate = true` construction) + +### Tests (TDD — write failing first) + +Exact-SQL unit tests for **both** dialects: + +- `is_null` → `... IS NULL` +- `is_not_null` → `... IS NOT NULL` (fails on current code — proves the fix) +- `is_null().negated()` → `NOT (... IS NULL)` +- `is_not_null().negated()` → `NOT (... IS NOT NULL)` +- `IsNull` with a non-boolean `right` (including `Value::Null`) → `RenderError` + +Update every existing test that constructs `IsNull` by hand with `right: Value::Null` +(`integration_dql.rs:558`, `:585`, and any others) to pass a boolean `right` +(`Bool(true)` for `IS NULL`, `Bool(false)` for `IS NOT NULL`) — otherwise they now error. +Row-count assertions remain valid once the operand is corrected. + +### Docs + +- `type-reference.md:174` — show both `IS NULL` and `IS NOT NULL` for `IsNull`. +- `type-reference.md:65` — clarify that the boolean `right` selects the keyword and is + never parameterized (the `IS $1` invariant still holds). +- `type-reference.md:275`, `select-queries.md:334-336` — already claim `IS NOT NULL`; + they become correct, no text change needed (verify). +- Document the `right` boolean semantics for `IsNull`. + +## Versioning + +Two observable changes: `is_not_null()` output changes textually +(`NOT (field IS NULL)` → `field IS NOT NULL`, semantically identical), and hand-built +`IsNull` ASTs with a non-boolean `right` now error instead of rendering `IS NULL`. The +latter is a breaking change for external callers, so this warrants a **major** bump. +Finalize the version and CHANGELOG entry at release time. + +## Out of scope + +The PyO3 binding (separate `amsdal` repo) is not touched here. After this change its fix +is a trivial pass-through of Django's `isnull` boolean into `right`. diff --git a/docs/type-reference.md b/docs/type-reference.md index 6e968d3..bb98f89 100644 --- a/docs/type-reference.md +++ b/docs/type-reference.md @@ -62,7 +62,7 @@ In SQLite both behave identically (SQLite has a single INTEGER type). In parameterized query mode (SELECT, INSERT, UPDATE, DELETE), `Value::Null` is sent as a bind parameter (`$1` / `?`), not inlined as the `NULL` keyword. This allows drivers to handle NULL correctly via the wire protocol. -The only exception is `CompareOp::IsNull` — it always renders as `IS NULL` because `IS $1` is not valid SQL syntax. +The exception is `CompareOp::IsNull`: its boolean `right` selects the keyword (`Bool(true)` → `IS NULL`, `Bool(false)` → `IS NOT NULL`) and is never emitted as a bind parameter, since `IS $1` is not valid SQL syntax. A non-boolean `right` is a render error. ## Expr @@ -171,7 +171,7 @@ IContains ILIKE '%val%' (PG) / LOWER(col) LIKE LOWER(?) (SQLite) IStartsWith ILIKE 'val%' (PG) / LOWER(col) LIKE LOWER(?) (SQLite) IEndsWith ILIKE '%val' (PG) / LOWER(col) LIKE LOWER(?) (SQLite) Between BETWEEN $1 AND $2 (Value::Array with 2 items) -IsNull IS NULL +IsNull IS NULL / IS NOT NULL (selected by boolean `right`: true → IS NULL, false → IS NOT NULL) Similar SIMILAR TO (PG) Regex ~ (PG) IRegex ~* (PG) / REGEXP '(?i)' || pattern (SQLite)