From e43db9f0284ecb0782724d8c50b67ea22f6a6bad Mon Sep 17 00:00:00 2001 From: TimelordUK Date: Sat, 25 Jul 2026 11:22:10 +0100 Subject: [PATCH 1/2] fix(parity): close P10 (HAVING NOT), P6 (INTERSECT/EXCEPT), P12 (WITH in expr) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three DuckDB-parity gaps, each now AGREEs and is locked by the --check gate (corpus expectations dropped, SQL_PARITY.md entries flipped to FIXED). Board goes 72->76 AGREE, 10->6 GAP. P10 — HAVING NOT (...): added a `Not` arm to ArithmeticEvaluator::evaluate that negates via the existing to_bool helper, with SQL three-valued logic on the NULL edge (NOT NULL -> NULL). Was erroring "Unsupported expression type ... Not". P6 — INTERSECT / EXCEPT: filled the two return-Err stubs in the set-op loop. Both DISTINCT-by-default, filtering the materialized left table against the right using the same Debug row-key apply_distinct uses for UNION, deduping inline. The `except` corpus case threshold moved 2000 -> 3000 so it yields a real {Oceania} difference instead of a trivially-empty result. P12 — WITH in expression position: parse_subquery already dispatched a leading WITH to the CTE parser; widened the subquery-detection guards from Token::Select to Token::Select | Token::With in primary.rs (scalar + tuple IN) and comparison.rs (IN / NOT IN). Retires previously-unreachable CTEHoister arms. Co-Authored-By: Claude Opus 4.8 (1M context) --- docs/SQL_PARITY.md | 77 +++++++++++++++------ src/data/arithmetic_evaluator.rs | 10 +++ src/data/query_engine.rs | 52 ++++++++++++-- src/sql/parser/expressions/comparison.rs | 10 +-- src/sql/parser/expressions/primary.rs | 21 ++++-- tests/comparison/corpus/06_ctes_setops.toml | 20 +++--- tests/comparison/corpus/07_grouping.toml | 6 +- 7 files changed, 148 insertions(+), 48 deletions(-) diff --git a/docs/SQL_PARITY.md b/docs/SQL_PARITY.md index 7c1339b6..f2a4bd7b 100644 --- a/docs/SQL_PARITY.md +++ b/docs/SQL_PARITY.md @@ -153,11 +153,29 @@ annotation be removed. `query_engine.rs`. It produces exactly one row. ### P6 — `INTERSECT` / `EXCEPT` not implemented -- **Status:** 🔴 OPEN -- **Corpus:** `06_ctes_setops.toml :: intersect`, `except` -- **Observed:** "INTERSECT is not yet implemented" / "EXCEPT is not yet implemented". - `UNION` and `UNION ALL` already AGREE. -- **Decision:** **Fix** — implement alongside the existing `UNION` set-op path. +- **Status:** 🟢 FIXED (2026-07-25) +- **Corpus:** `06_ctes_setops.toml :: intersect`, `except` (both now AGREE; + `expect` dropped) +- **Observed (was):** "INTERSECT is not yet implemented" / "EXCEPT is not yet + implemented". `UNION` and `UNION ALL` already AGREE. +- **Fix:** Filled in the two `return Err(...)` stubs in the set-op loop of + `query_engine.rs`. Both operate on the already-materialized `combined_table` + (left) and `next_table` (right): + - **INTERSECT [DISTINCT]** keeps left rows whose key is present in the right, + deduplicated. + - **EXCEPT [DISTINCT]** keeps left rows whose key is *absent* from the right, + deduplicated. + Both are DISTINCT by default (SQL standard), so each filters and dedups inline + in one pass rather than setting `needs_deduplication` (that flag stays + UNION-only). The row key is `format!("{:?}", row.values)` — the **same + equality basis** `apply_distinct` uses for UNION, so set membership is + consistent across all four set ops. Left-to-right evaluation of chained set + ops is unchanged (INTERSECT-binds-tighter precedence remains a separate, + pre-existing limitation, not exercised by the corpus). +- **Corpus note:** the `except` case threshold was moved from `amount > 2000` to + `> 3000`. Every region has a sale > 2000, so the original form was trivially + empty and would have AGREEd for the wrong reason (cf. the tier-7 `having_in_list` + lesson); `> 3000` leaves `{Oceania}`, a genuine left-minus-right difference. ### P7 — Multi-condition join evaluates extra-condition operands by position - **Status:** 🟢 FIXED (2026-07-12) @@ -257,15 +275,21 @@ annotation be removed. two entries were correctly split rather than being one finding. ### P10 — `HAVING NOT (...)` errors in the evaluator -- **Status:** 🔴 OPEN -- **Corpus:** `07_grouping.toml :: having_not` (GAP) -- **Observed:** `HAVING NOT (COUNT(*) > 2)` → +- **Status:** 🟢 FIXED (2026-07-25) +- **Corpus:** `07_grouping.toml :: having_not` (now AGREE; `expect` dropped) +- **Observed (was):** `HAVING NOT (COUNT(*) > 2)` → `Unsupported expression type for arithmetic evaluation: Not { ... }`. - **Distinct from P9:** here the aggregate *is* rewritten correctly (the - transformer does handle `Not`), and the failure is downstream — the arithmetic - evaluator has no `Not` arm for a post-aggregation predicate. Fixing P9 will not - fix this. -- **Decision:** **Fix** — add the missing evaluator arm. Small and independent. + transformer does handle `Not`), and the failure was downstream — the arithmetic + evaluator had no `Not` arm for a post-aggregation predicate. Fixing P9 did not + fix this, exactly as predicted. +- **Fix:** Added a `Not { expr }` arm to `ArithmeticEvaluator::evaluate` + (`src/data/arithmetic_evaluator.rs`), immediately after the `Between` arm. It + evaluates the inner expression and negates it through the existing `to_bool` + helper (the same truthiness the `AND`/`OR` arms use), with SQL three-valued + logic on the NULL edge: `NOT NULL` → NULL rather than a coerced `true`. The + corpus case returns just `Oceania,1` (the one group with `COUNT(*) <= 2`), + matching DuckDB. ### P11 — A `SELECT` alias is not expanded on the LHS of an `IN` subquery - **Status:** 🔴 OPEN @@ -284,20 +308,29 @@ annotation be removed. the required behaviour. ### P12 — `WITH` is rejected in expression position -- **Status:** 🔴 OPEN -- **Corpus:** `06_ctes_setops.toml :: cte_in_expression_position` (GAP) -- **Observed:** `WHERE price > (WITH avg_cte AS (...) SELECT a FROM avg_cte)` → - `Parse error: Unexpected token in primary expression: With`. Rejected in every +- **Status:** 🟢 FIXED (2026-07-25) +- **Corpus:** `06_ctes_setops.toml :: cte_in_expression_position` (now AGREE; + `expect` dropped) +- **Observed (was):** `WHERE price > (WITH avg_cte AS (...) SELECT a FROM avg_cte)` + → `Parse error: Unexpected token in primary expression: With`. Rejected in every expression position tried — scalar subquery, `BETWEEN` operand, `IN`-list - element, and tuple `IN` (which reports "Tuple IN requires a subquery on the + element, and tuple `IN` (which reported "Tuple IN requires a subquery on the right"). DuckDB accepts a CTE inside a scalar subquery. - **Found:** 2026-07-18, while trying to write a regression test for the `cte_hoister` walker migration — the test could not be expressed. -- **Side effect worth noting:** the `ScalarSubquery` / `InSubquery` arms of - `CTEHoister::hoist_from_expression` are therefore **unreachable dead code** - today; expression-position CTE hoisting has never had an input. -- **Decision:** **Fix** — a parser change (accept `WITH` where a subquery is - already accepted). The hoister machinery to handle the result already exists. +- **Fix:** Pure parser change. `parse_subquery()` *already* dispatched a leading + `WITH` to the CTE parser (`parse_with_clause_inner`); the only blockers were the + subquery-detection guards that gated on `Token::Select` alone. Widened them to + `Token::Select | Token::With` in both spots a subquery is recognised: + - `expressions/primary.rs` — the scalar-subquery branch after `(`, plus the two + tuple-`IN` guards (`(a, b) IN (…)` / `NOT IN`). + - `expressions/comparison.rs` — the `x IN (…)` and `x NOT IN (…)` subquery + branches. + So a CTE is now accepted wherever a subquery already was, matching DuckDB. +- **Side effect resolved:** the `ScalarSubquery` / `InSubquery` arms of + `CTEHoister::hoist_from_expression` were previously **unreachable dead code** + (expression-position CTE hoisting never had an input); they now receive real + input. --- diff --git a/src/data/arithmetic_evaluator.rs b/src/data/arithmetic_evaluator.rs index ac7810ef..33bed224 100644 --- a/src/data/arithmetic_evaluator.rs +++ b/src/data/arithmetic_evaluator.rs @@ -167,6 +167,16 @@ impl<'a> ArithmeticEvaluator<'a> { let le = compare_with_op(&val, &hi, "<=", false); Ok(DataValue::Boolean(ge && le)) } + // Logical negation as a value-producing expression. Reached, e.g., by + // a post-aggregation `HAVING NOT (COUNT(*) > 2)` predicate (P10). + // Three-valued logic: NOT NULL is NULL; otherwise negate the boolean. + SqlExpression::Not { expr } => { + let inner = self.evaluate(expr, row_index)?; + match inner { + DataValue::Null => Ok(DataValue::Null), + other => Ok(DataValue::Boolean(!self.to_bool(&other)?)), + } + } // IN / NOT IN as a value-producing expression — needed when they // appear inside CASE branches or other arithmetic contexts. The // WHERE path has its own evaluator; this mirrors its equality diff --git a/src/data/query_engine.rs b/src/data/query_engine.rs index f08d3e70..d3c06db1 100644 --- a/src/data/query_engine.rs +++ b/src/data/query_engine.rs @@ -1972,14 +1972,54 @@ impl QueryEngine { )); } SetOperation::Intersect => { - // INTERSECT: Keep only rows that appear in both - // TODO: Implement intersection logic - return Err(anyhow!("INTERSECT is not yet implemented")); + // INTERSECT [DISTINCT]: keep only rows present in BOTH + // sides, deduplicated. The row key is the Debug form of + // the whole value vector — the same equality basis + // apply_distinct() uses for UNION. + let right_keys: std::collections::HashSet = next_table + .rows + .iter() + .map(|r| format!("{:?}", r.values)) + .collect(); + let mut seen = std::collections::HashSet::new(); + let retained: Vec<_> = combined_table + .rows + .iter() + .filter(|r| { + let key = format!("{:?}", r.values); + right_keys.contains(&key) && seen.insert(key) + }) + .cloned() + .collect(); + combined_table.rows = retained; + plan.add_detail(format!( + "Result: {} rows (intersection, deduplicated)", + combined_table.row_count() + )); } SetOperation::Except => { - // EXCEPT: Keep only rows from left that don't appear in right - // TODO: Implement except logic - return Err(anyhow!("EXCEPT is not yet implemented")); + // EXCEPT [DISTINCT]: keep rows from the left that do NOT + // appear in the right, deduplicated. + let right_keys: std::collections::HashSet = next_table + .rows + .iter() + .map(|r| format!("{:?}", r.values)) + .collect(); + let mut seen = std::collections::HashSet::new(); + let retained: Vec<_> = combined_table + .rows + .iter() + .filter(|r| { + let key = format!("{:?}", r.values); + !right_keys.contains(&key) && seen.insert(key) + }) + .cloned() + .collect(); + combined_table.rows = retained; + plan.add_detail(format!( + "Result: {} rows (difference, deduplicated)", + combined_table.row_count() + )); } } diff --git a/src/sql/parser/expressions/comparison.rs b/src/sql/parser/expressions/comparison.rs index a90b33b7..14518182 100644 --- a/src/sql/parser/expressions/comparison.rs +++ b/src/sql/parser/expressions/comparison.rs @@ -55,8 +55,9 @@ where parser.advance(); // consume IN parser.consume(Token::LeftParen)?; - // Check if this is a subquery (starts with SELECT) - if matches!(parser.current_token(), Token::Select) { + // Check if this is a subquery (starts with SELECT, or WITH for a + // CTE in expression position — P12). + if matches!(parser.current_token(), Token::Select | Token::With) { debug!("Detected NOT IN subquery"); let subquery = parser.parse_subquery()?; parser.consume(Token::RightParen)?; @@ -168,8 +169,9 @@ where parser.advance(); // consume IN parser.consume(Token::LeftParen)?; - // Check if this is a subquery (starts with SELECT) - if matches!(parser.current_token(), Token::Select) { + // Check if this is a subquery (starts with SELECT, or WITH for a CTE + // in expression position — P12). + if matches!(parser.current_token(), Token::Select | Token::With) { debug!("Detected IN subquery"); let subquery = parser.parse_subquery()?; parser.consume(Token::RightParen)?; diff --git a/src/sql/parser/expressions/primary.rs b/src/sql/parser/expressions/primary.rs index 7be30fc3..6a947b81 100644 --- a/src/sql/parser/expressions/primary.rs +++ b/src/sql/parser/expressions/primary.rs @@ -259,9 +259,14 @@ where debug!("Parsing parenthesized expression or subquery"); ExpressionParser::advance(parser); // consume ( - // Check if this is a subquery (starts with SELECT) - if matches!(ExpressionParser::current_token(parser), Token::Select) { - debug!("Detected subquery - parsing SELECT statement"); + // Check if this is a subquery. It starts with SELECT, or with WITH + // for a CTE in expression position (P12) — parse_subquery() dispatches + // a leading WITH to the CTE parser, so both forms flow through here. + if matches!( + ExpressionParser::current_token(parser), + Token::Select | Token::With + ) { + debug!("Detected subquery - parsing SELECT/WITH statement"); let subquery = parser.parse_subquery()?; ExpressionParser::consume(parser, Token::RightParen)?; Ok(SqlExpression::ScalarSubquery { @@ -286,7 +291,10 @@ where Token::In => { ExpressionParser::advance(parser); // consume IN ExpressionParser::consume(parser, Token::LeftParen)?; - if !matches!(ExpressionParser::current_token(parser), Token::Select) { + if !matches!( + ExpressionParser::current_token(parser), + Token::Select | Token::With + ) { return Err("Tuple IN requires a subquery on the right".to_string()); } let subquery = parser.parse_subquery()?; @@ -303,7 +311,10 @@ where } ExpressionParser::advance(parser); // consume IN ExpressionParser::consume(parser, Token::LeftParen)?; - if !matches!(ExpressionParser::current_token(parser), Token::Select) { + if !matches!( + ExpressionParser::current_token(parser), + Token::Select | Token::With + ) { return Err( "Tuple NOT IN requires a subquery on the right".to_string() ); diff --git a/tests/comparison/corpus/06_ctes_setops.toml b/tests/comparison/corpus/06_ctes_setops.toml index e4cbc61a..a6da8ebe 100644 --- a/tests/comparison/corpus/06_ctes_setops.toml +++ b/tests/comparison/corpus/06_ctes_setops.toml @@ -29,15 +29,18 @@ sql = "SELECT region FROM international_sales WHERE amount > 2000 UNION ALL SELE id = "intersect" data = "international_sales.csv" sql = "SELECT region FROM international_sales WHERE amount > 2000 INTERSECT SELECT region FROM international_sales WHERE amount < 1000" -# "INTERSECT is not yet implemented". -expect = "GAP" +# Was P6 ("INTERSECT is not yet implemented"). Implemented 2026-07-25 in the +# set-op path; INTERSECT DISTINCT keeps rows present in both sides. Returns +# {Europe} — a non-trivial result (some regions are on only one side). [[case]] id = "except" data = "international_sales.csv" -sql = "SELECT region FROM international_sales EXCEPT SELECT region FROM international_sales WHERE amount > 2000" -# "EXCEPT is not yet implemented". -expect = "GAP" +sql = "SELECT region FROM international_sales EXCEPT SELECT region FROM international_sales WHERE amount > 3000" +# Was P6 ("EXCEPT is not yet implemented"). Implemented 2026-07-25 alongside +# INTERSECT. Threshold is 3000 (not 2000) deliberately: every region has a sale +# > 2000, so that form is trivially empty and would pass for the wrong reason. +# > 3000 leaves {Oceania} — a real left-minus-right difference. [[case]] id = "recursive_cte" @@ -50,6 +53,7 @@ expect = "GAP" id = "cte_in_expression_position" data = "trades.csv" sql = "SELECT symbol, price FROM trades WHERE price > (WITH avg_cte AS (SELECT AVG(price) AS a FROM trades) SELECT a FROM avg_cte)" -# P12: WITH is rejected anywhere in expression position ("Unexpected token in -# primary expression: With"). DuckDB allows a CTE inside a scalar subquery. -expect = "GAP" +# Was P12: WITH was rejected in expression position ("Unexpected token in +# primary expression: With"). Fixed 2026-07-25 — the subquery-detection guards +# now accept a leading WITH (parse_subquery already dispatched it to the CTE +# parser). DuckDB allows a CTE inside a scalar subquery; now AGREE. diff --git a/tests/comparison/corpus/07_grouping.toml b/tests/comparison/corpus/07_grouping.toml index 7d7864fd..921eff24 100644 --- a/tests/comparison/corpus/07_grouping.toml +++ b/tests/comparison/corpus/07_grouping.toml @@ -59,6 +59,6 @@ sql = "SELECT region, COUNT(*) AS n FROM international_sales GROUP BY region HAV id = "having_not" data = "international_sales.csv" sql = "SELECT region, COUNT(*) AS n FROM international_sales GROUP BY region HAVING NOT (COUNT(*) > 2) ORDER BY region" -# P10: the aggregate IS rewritten here, but the evaluator has no arm for Not, -# so this errors rather than returning wrong rows. -expect = "GAP" +# Was P10: the aggregate IS rewritten here, but the arithmetic evaluator had no +# arm for Not, so this errored. Fixed 2026-07-25 by adding the Not arm +# (three-valued: NOT NULL -> NULL). Now AGREE. From e73485754b6d59575918e21eb23f17875a3218c0 Mon Sep 17 00:00:00 2001 From: TimelordUK Date: Sat, 25 Jul 2026 11:22:19 +0100 Subject: [PATCH 2/2] test(history): make history_protection_integration reliable cross-platform MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The test sandboxed the history dir by redirecting HOME/APPDATA, but dirs::data_dir() on Windows resolves via the Win32 known-folder API and ignores those env vars, so the test silently read the developer's real history and failed. Env-var redirection is also process-global, so #[serial] (which only serializes serial-tagged tests) couldn't stop a concurrent test from writing into the sandbox and tripping the history-protection guard — hence pass-alone / fail-in-suite flakiness. Decouple the test from global state: add CommandHistory::with_history_file(path) and have new() delegate to it. The test now builds against an explicit tempdir file — no env var, no #[serial], no cross-test races. Verified green across repeated full `cargo test --test integration` runs. Also add a SQL_CLI_DATA_DIR override to AppPaths (honored directly, so it works on every platform unlike the dirs-based env vars) — a legitimate way to relocate/sandbox app data. Co-Authored-By: Claude Opus 4.8 (1M context) --- src/history.rs | 8 +++++ src/utils/app_paths.rs | 29 +++++++++++++---- tests/history_protection_integration.rs | 42 ++++++++++--------------- 3 files changed, 47 insertions(+), 32 deletions(-) diff --git a/src/history.rs b/src/history.rs index e8ad342d..72803ea2 100644 --- a/src/history.rs +++ b/src/history.rs @@ -65,7 +65,15 @@ impl CommandHistory { pub fn new() -> Result { let history_file = AppPaths::history_file() .map_err(|e| anyhow::anyhow!("Failed to get history file path: {}", e))?; + Self::with_history_file(history_file) + } + /// Build a `CommandHistory` backed by an explicit history file path, rather + /// than the OS-resolved app data dir. The backup directory is a sibling + /// `history_backups/` next to the file. Intended for tests that need full + /// isolation from the developer's real history without touching the + /// process-global environment (which parallel tests share and race on). + pub fn with_history_file(history_file: PathBuf) -> Result { // Create backup directory let backup_dir = history_file .parent() diff --git a/src/utils/app_paths.rs b/src/utils/app_paths.rs index 8d24fd6c..ac20caee 100644 --- a/src/utils/app_paths.rs +++ b/src/utils/app_paths.rs @@ -5,19 +5,36 @@ use std::path::PathBuf; pub struct AppPaths; impl AppPaths { + /// Env var that overrides the base directory for all app paths. When set, + /// both data and cache resolve under it instead of the OS default. This is + /// the only reliable way to relocate the paths on Windows: `dirs::data_dir` + /// there resolves via the Win32 known-folder API and ignores `APPDATA` / + /// `LOCALAPPDATA` env vars, so tests can't sandbox it by setting those. + const BASE_DIR_ENV: &'static str = "SQL_CLI_DATA_DIR"; + + fn base_override() -> Option { + std::env::var_os(Self::BASE_DIR_ENV) + .filter(|v| !v.is_empty()) + .map(PathBuf::from) + } + pub fn data_dir() -> Result> { - let data_dir = dirs::data_dir() - .ok_or("Cannot determine data directory")? - .join("sql-cli"); + let base = match Self::base_override() { + Some(base) => base, + None => dirs::data_dir().ok_or("Cannot determine data directory")?, + }; + let data_dir = base.join("sql-cli"); fs::create_dir_all(&data_dir)?; Ok(data_dir) } pub fn cache_dir() -> Result> { - let cache_dir = dirs::cache_dir() - .ok_or("Cannot determine cache directory")? - .join("sql-cli"); + let base = match Self::base_override() { + Some(base) => base, + None => dirs::cache_dir().ok_or("Cannot determine cache directory")?, + }; + let cache_dir = base.join("sql-cli"); fs::create_dir_all(&cache_dir)?; Ok(cache_dir) diff --git a/tests/history_protection_integration.rs b/tests/history_protection_integration.rs index 26ed92cc..26722424 100644 --- a/tests/history_protection_integration.rs +++ b/tests/history_protection_integration.rs @@ -1,34 +1,27 @@ -use serial_test::serial; use sql_cli::history::CommandHistory; use std::fs; use tempfile::TempDir; -// `#[serial]` because this test mutates the process-global `HOME` env var. -// Without it, parallel integration tests that resolve `AppPaths::data_dir()` -// race against each other and intermittently fail with "No such file or -// directory" when one test's tempdir is dropped while another is mid-write. +// This test is fully isolated: it builds CommandHistory against an explicit +// history file inside a tempdir via `with_history_file`, so it never touches +// the process-global environment or the developer's real history. That removes +// the old flakiness — the previous version redirected HOME / APPDATA, which +// (a) didn't work on Windows at all (dirs::data_dir resolves via the Win32 +// known-folder API, ignoring those env vars) and (b) is process-global, so +// parallel tests raced on it. No `#[serial]` needed as a result. #[test] -#[serial] fn test_history_protection_integration() { println!("Testing History Protection Integration...\n"); - // Create temp directory for test + // Create temp directory for test, mirroring the real layout: the app keeps + // history under a `sql-cli/` subdirectory of its data dir. let temp_dir = TempDir::new().unwrap(); + let data_dir = temp_dir.path().join("sql-cli"); + fs::create_dir_all(&data_dir).unwrap(); + let history_file = data_dir.join("history.json"); - // Set environment variables for cross-platform compatibility - // Windows uses APPDATA/LOCALAPPDATA, Unix uses HOME - #[cfg(windows)] - { - std::env::set_var("APPDATA", temp_dir.path()); - std::env::set_var("LOCALAPPDATA", temp_dir.path()); - } - #[cfg(unix)] - { - std::env::set_var("HOME", temp_dir.path()); - } - - // Create history instance - let mut history = CommandHistory::new().unwrap(); + // Create history instance backed by the isolated file + let mut history = CommandHistory::with_history_file(history_file.clone()).unwrap(); // Add some entries for i in 1..=5 { @@ -40,12 +33,9 @@ fn test_history_protection_integration() { let entries = history.get_all(); assert_eq!(entries.len(), 5, "Should have 5 entries"); - // Check backup directory exists - use sql-cli directory (cross-platform) - let backup_dir = temp_dir.path().join("sql-cli").join("history_backups"); + // Check backup directory (sibling of the history file) + let backup_dir = data_dir.join("history_backups"); - // Directory might not exist until first backup, so let's trigger one - // by saving after adding entries - let history_file = temp_dir.path().join("sql-cli").join("history.json"); if history_file.exists() { println!("History file exists at: {history_file:?}"); }