Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
77 changes: 55 additions & 22 deletions docs/SQL_PARITY.md
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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
Expand All @@ -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.

---

Expand Down
10 changes: 10 additions & 0 deletions src/data/arithmetic_evaluator.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
52 changes: 46 additions & 6 deletions src/data/query_engine.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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<String> = 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<String> = 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()
));
}
}

Expand Down
8 changes: 8 additions & 0 deletions src/history.rs
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,15 @@ impl CommandHistory {
pub fn new() -> Result<Self> {
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<Self> {
// Create backup directory
let backup_dir = history_file
.parent()
Expand Down
10 changes: 6 additions & 4 deletions src/sql/parser/expressions/comparison.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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)?;
Expand Down Expand Up @@ -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)?;
Expand Down
21 changes: 16 additions & 5 deletions src/sql/parser/expressions/primary.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand All @@ -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()?;
Expand All @@ -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()
);
Expand Down
29 changes: 23 additions & 6 deletions src/utils/app_paths.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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<PathBuf> {
std::env::var_os(Self::BASE_DIR_ENV)
.filter(|v| !v.is_empty())
.map(PathBuf::from)
}

pub fn data_dir() -> Result<PathBuf, Box<dyn Error>> {
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<PathBuf, Box<dyn Error>> {
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)
Expand Down
20 changes: 12 additions & 8 deletions tests/comparison/corpus/06_ctes_setops.toml
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand All @@ -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.
6 changes: 3 additions & 3 deletions tests/comparison/corpus/07_grouping.toml
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Loading
Loading