diff --git a/docs/ENGINE_REFACTORING.md b/docs/ENGINE_REFACTORING.md index caa0b649..c07c53ec 100644 --- a/docs/ENGINE_REFACTORING.md +++ b/docs/ENGINE_REFACTORING.md @@ -207,19 +207,58 @@ feature work**, and so we can tell the difference between "this is awkward" and why people reach for `Default`-ish shortcuts. ### R5 — Dead code accumulates undetected -- **Status:** 🔴 OPEN -- **Observed:** `column_dependency_lifter.rs` sat in the tree fully dead — - absent from `query_plan/mod.rs`, referenced nowhere, and no longer compiling - against the current AST (its `SelectStatement` literal was missing six - fields). Deleted in PR #30. `cte_hoister::hoist_from_condition` is still dead - (confirmed pre-existing). -- **Impact:** Modest in isolation, but dead code inflates every audit — the - deleted file alone accounted for 9 deprecated-field sites and 14 - `SqlExpression` match arms in the R2/R3 survey. -- **Decision:** Not worth a dedicated project. Cheapest fix is to stop tolerating - the warnings: the tree currently emits ~368 clippy warnings and ~67 - `#[allow(deprecated)]` annotations, which is enough noise to hide a real - signal. Consider a `[lints]` table in `Cargo.toml` once the count is down. +- **Status:** 🟡 REOPENED 2026-10-03 as a workstream, at the user's request — + a year of refactoring (R2, R8, R13 above all) has left code behind, and + slice 6 of R13 found three uncalled join methods that rustc had been + reporting all along. Was: "not worth a dedicated project". +- **Observed (2026-07):** `column_dependency_lifter.rs` sat in the tree fully + dead — absent from `query_plan/mod.rs`, referenced nowhere, and no longer + compiling against the current AST. Deleted in PR #30. + `cte_hoister::hoist_from_condition` is still dead (confirmed pre-existing). +- **Observed (2026-10-03):** `cargo build` names **~30 dead items** today, + lost among ~350 other warnings. By area: + + | Area | Items rustc reports as never used / never read | + |---|---| + | Engine | `QueryEngine::build_view`, `build_view_internal`, `apply_multi_order_by`; `ArithmeticEvaluator` field `row_index`; `BatchWindowEvaluator` fields `specs`, `contexts`; `DataType::looks_like_datetime`; `DataAnalyzer::looks_like_date{,_fast}`; `CsvDataSource::filter_results`; `analysis::format_cte_as_query` | + | Parser | `recursive_parser`: `contains_aggregate_function`, `parse_select_statement`, `parse_column_reference` (see [R14](#r14)); `formatter`: `find_token_position`, `format_token` | + | CLI | `non_interactive`: `check_temp_table_usage`, `make_transformer_config`; `main_handlers::is_non_interactive`; `main` field `analyze_correlations_arg` | + | TUI | `enhanced_tui`: `TOTAL_UI_CHROME`, `TABLE_CHROME_ROWS`, `render_help_two_column`; `viewport_manager`: `TABLE_CHROME_ROWS`, `visible_row_cache`, `cache_signature`; `ui_layout_utils::TABLE_BORDER_WIDTH`; `tui_app` field `sql_parser`; `buffer::sync_to_input_manager`; `virtual_table` field `row_style` | + | Other | `debug_info::value_to_rust_code`; `http_fetcher::extract_json_path`; `window_functions/aggregates.rs` `has_non_null` assigned, never read | + + And rustc only sees *private* items. Most modules are `pub` from `lib.rs`, + so a `pub fn` with no caller anywhere is invisible to it — R13's deleted + WHERE readers were of that kind. Seven `#[allow(dead_code)]` / `unused` + annotations hide more. The same warning noise hides a neighbouring smell: + `hash_join.rs` ignores nine `add_row` `Result`s (`unused_must_use`). +- **Impact:** dead code inflates every audit (the deleted lifter alone was 9 + deprecated-field sites and 14 `SqlExpression` arms in the R2/R3 survey), + and a fix can land in a copy nothing calls — R13 slice 6 would have had to + change the nested-loop comparator in two dead builders as well as three + live ones, and R14 counted a dead parser reader as a live one. +- **Slices — each deletion its own commit, no behaviour change, parity report + byte-identical and all three suites green:** + 1. **Harvest rustc's list** (the table above), one commit per area. Items + that look *intended* rather than abandoned (TUI layout constants, the + viewport cache fields) are asked about, not deleted. + 2. **Coverage pass for `pub` code.** `cargo llvm-cov` over the Rust tests, + the Python tests, the examples and the parity corpus (the release binary + instrumented, `LLVM_PROFILE_FILE` per run), merged into one list of + functions never executed. Unexecuted is not dead — TUI paths, error + arms, platform and feature code — so the list is triaged: *dead* + (delete), *live but untested* (log a test gap), *out of reach* (TUI, + `system-tables`, network). A by-hand cross-check: `pub fn` with no + reference outside its own file. + 3. **Let the compiler see more.** Narrow module visibility to `pub(crate)` + where nothing outside the crate (bins, `tests/`) uses it, so rustc's + lint covers it; remove the `#[allow(dead_code)]`s that no longer hide + anything. Unused dependencies (`cargo machete`) alongside. + 4. **Keep it dead.** Once the warning count is low enough to read, a + `[lints]` table in `Cargo.toml` (`dead_code`, `unused_must_use`) and CI + `-D` for those two, so the next abandoned function fails the build. +- **Sequencing:** a lull task, independent of R13/R14 — but slice 1's parser + item is worth doing before R14 slice 2, and slice 1 generally before + anyone fixes a finding in a file it touches. ### R6 — `CorrelatedSubqueryAnalyzer` is unwired and untested - **Status:** 🔴 OPEN @@ -478,8 +517,9 @@ feature work**, and so we can tell the difference between "this is awkward" and WHERE *operator* now delegates. **Slice 5 (method calls) done 2026-09-26**, closing P56 and P57 and deciding D3. **Slice 4 finished 2026-10-01** (AND / OR / NOT / CASE, one truth rule — closing P61 and P62, deciding D4): WHERE - evaluates nothing itself any more. **Slice 6 (the last) begun 2026-10-03** - with its pin — no engine change. + evaluates nothing itself any more. **Slice 6 (the last) begun 2026-10-03**: + pinned, then HAVING and `IIF` moved onto the one truth rule (P62 finished). + P52's join fix and retiring the WHERE evaluator remain. **The active workstream:** parity fixes that touch expression evaluation land as slices of this entry, not as patches. - **Where:** `src/data/arithmetic_evaluator.rs` (`ArithmeticEvaluator`, value → @@ -812,6 +852,14 @@ feature work**, and so we can tell the difference between "this is awkward" and NULL ordering never satisfies them — so those three are AGREE guards for when the nested loop stops using the bare comparator. `nested_loop_join_inner` / `nested_loop_join_left` have no callers. +- **Slice 6, HAVING and `IIF` (2026-10-03).** Both read their condition with + `Trilean::from_value(..)?.is_true()`; `is_truthy` and `IIF`'s match are + gone, so all five of P62's copies are now the one function. Exactly the 15 + matrix entries went FIXED; parity moved by exactly the two HAVING cases + (190 → **191 AGREE**, one OURS_ONLY → BOTH_ERR); FORMAL examples unchanged. + Then the three uncalled join methods (`nested_loop_join_inner` / `_left`, + `qualify_column_name`, 264 lines) were deleted ahead of 6c — rustc had been + warning about them all along ([R5](#r5)). Parity report byte-identical. - **Slices, in the R10 pattern — no-op slices kept apart from the one that changes answers:** 1. ✅ **Pin the divergences, no engine change.** *(Done 2026-09-13.)* A per-operator matrix run through @@ -845,12 +893,12 @@ feature work**, and so we can tell the difference between "this is awkward" and too, not only the nested loop. [R8](#r8) stage 2 (the legacy WHERE stack) is a natural lull task alongside. In parts: - 6a ✅ pin (2026-10-03, above). - - 6b HAVING and `IIF` onto `Trilean::from_value` — exactly the 15 - matrix entries and the two HAVING corpus cases move. + - 6b ✅ HAVING and `IIF` onto `Trilean::from_value` (2026-10-03) — + exactly the 15 matrix entries and the two HAVING corpus cases moved. + Dead single-condition join builders deleted with it. - 6c P52: the nested loop compares through the predicate layer; the hash path skips NULL keys on build and probe, LEFT still emitting the - unmatched row. Dead single-condition builders deleted first. Exactly - the seven join cases move. + unmatched row. Exactly the seven join cases move. - 6d `WhereClause`'s connector list → `BinaryOp AND`; the adapter folds into the row filter and `RecursiveWhereEvaluator` goes. Parity byte-identical. R13 closes. @@ -896,7 +944,9 @@ feature work**, and so we can tell the difference between "this is awkward" and are the same string, and every re-split has to guess. P34 and P58 are both that guess going wrong. The parser has three separate `ident . ident` readers too (`parse_primary`, `parse_identifier_list`, `parse_column_reference`) — - P58 fixed only the first. + P58 fixed only the first. (2026-10-03: rustc reports + `parse_column_reference` as never used — so it is two live readers and a + dead one; [R5](#r5) slice 1 deletes it.) - **Impact:** every resolution rule — alias handling, quoting, case sensitivity, the P49 strictness decision, the error wording — has to be changed in N places, and the history says it isn't: P34 fixed ORDER BY's copy, @@ -952,12 +1002,12 @@ R2 walkers ──┬─→ R3 catch-alls retired R1 FROM migration ── independent; deferred (larger than P3) R4 fixtures ──────── adopt opportunistically, per transformer touched -R5 dead code ─────── opportunistic +R5 dead code ─────── REOPENED 2026-10-03 as a lull workstream: 1 rustc's list → 2 coverage pass → 3 pub(crate) → 4 lints in CI R8 legacy WHERE ──── independent; stage 2 is self-contained, do it in a lull R10 Trilean ──────── DONE; closed P18/P19 (parity 125 → 129) R11 ORDER BY resolver ─ independent; small, but a behaviour change — wants its own parity run; now slice 3 of R14 R12 aggregate registries ─ independent; step 1 is a provable no-op, do it before the next aggregate fix -R13 one evaluator ─── ACTIVE from 2026-09-13; slices 1 (pin), 2 (3VL), 3 (construction + case mode) DONE; 4 operators DONE (BETWEEN/IN, comparisons, IS NULL, LIKE); 5 (method calls) DONE; 4 tail (AND/OR/NOT/CASE, D4) DONE 2026-10-01 → 6 (6a pin DONE 2026-10-03 → 6b HAVING/IIF → 6c P52 joins → 6d retire WHERE evaluator) +R13 one evaluator ─── ACTIVE from 2026-09-13; slices 1 (pin), 2 (3VL), 3 (construction + case mode) DONE; 4 operators DONE (BETWEEN/IN, comparisons, IS NULL, LIKE); 5 (method calls) DONE; 4 tail (AND/OR/NOT/CASE, D4) DONE 2026-10-01 → 6 (6a pin, 6b HAVING/IIF DONE 2026-10-03 → 6c P52 joins → 6d retire WHERE evaluator) R14 one column resolver ─ after R13; slice 1 (pin) any time → 2 (stop flattening ColumnRef; P58 join) → 3 (converge copies, absorbs R11) → 4 (P49/P60 strictness) ──→ feeds P3 scope spine ``` @@ -1006,3 +1056,5 @@ AGREE count — which makes it safe to land well before the semantics change. | 2026-10-01 | R14 filed from P58 (`alias."quoted col"`): parser fix landed on a branch and worked in every clause but JOIN ON, whose resolver strips at the last dot. Five resolver copies inventoried; P59 (quoted alias) and P60 (GROUP BY unknown column → NULL group) filed | — | | 2026-10-01 | R13 slice 4 finished: AND / OR / NOT / CASE. Pinned 19 divergences (sixth matrix, values as truth values); P61 (WHERE CASE without ELSE → FALSE) and P62 (five truth tables; `'f'` read as TRUE) filed and closed; D4 decided (DuckDB's cast to boolean). `Trilean::from_value` the one rule; WHERE evaluates nothing itself — 183 → **186 AGREE** / 217 | — | | 2026-10-03 | R13 slice 6 pin: HAVING (new matrix column) and `IIF` as truth-value sites — 15 divergences, the two copies disagreeing with each other and reading NaN as FALSE; P52 cased on all six live join paths plus expressions, only equality wrong, three inequality guards AGREE. No engine change — 217 → 229 cases, 186 → 190 AGREE (the guards) | — | +| 2026-10-03 | R13 slice 6, HAVING and `IIF`: both onto `Trilean::from_value`, finishing P62 — exactly the 15 pinned entries and two HAVING cases moved, 190 → **191 AGREE** / 229. Three uncalled join methods deleted (264 lines), report byte-identical | — | +| 2026-10-03 | R5 reopened as a workstream: rustc already names ~30 dead items (inventoried by area), invisible in the warning noise; four slices — harvest rustc's list, coverage pass for `pub` code, `pub(crate)`, lints in CI | — | diff --git a/docs/SQL_PARITY.md b/docs/SQL_PARITY.md index 6b99775f..832c0a1a 100644 --- a/docs/SQL_PARITY.md +++ b/docs/SQL_PARITY.md @@ -65,11 +65,11 @@ session apiece to fix properly, so **discovery is paused and the effort moves to picking them off**. Widen the corpus again when the open list is short, or opportunistically when a fix needs a case that doesn't exist yet. -Corpus coverage today: tiers 01–10, **229 cases** (190 AGREE / 22 DIFFER / -12 GAP / 2 OURS_ONLY / 3 BOTH_ERR as of 2026-10-03, after R13 slice 6's pin -added P52's join paths and P62's HAVING copy — four guards AGREE, eight cases -await slice 6; [P51](#p51), [P52](#p52) open, [P53](#p53) open at low -priority). The largest single movement so far remains the 2026-09-05 +Corpus coverage today: tiers 01–10, **229 cases** (191 AGREE / 21 DIFFER / +12 GAP / 1 OURS_ONLY / 4 BOTH_ERR as of 2026-10-03, after R13 slice 6 pinned +P52 on every join path and moved HAVING and `IIF` onto the one truth rule, +finishing [P62](#p62); [P51](#p51), [P52](#p52) open — seven join cases +waiting — [P53](#p53) open at low priority). The largest single movement so far remains the 2026-09-05 NULL-ordering slice, which closed [P13](#p13) stage 2 and [P17](#p17) together — eleven cases in one change. **Tier 10 (aggregate & NULL edges) is still deliberately partial** — it holds the P14, P18–P20 and P41 cases and their @@ -94,7 +94,7 @@ Suggested fix order, by silent blast radius: | ~~9d~~ | ~~[P37](#p37) window in `WHERE` returns 0 rows~~ | ✅ **Fixed 2026-09-05** — corpus count unchanged, and that is the finding: the case is `OURS_ONLY` before *and* after, so the harness cannot see this fix or a future regression of it (first entry of that kind — the regression test is a Rust module). The filed root cause was wrong: `ExpressionLifter` *does* lift from `WHERE`. The real defect was one arm in the WHERE evaluator answering FALSE for any bare value used as a predicate — `WHERE true` returned zero rows too. It did **not** close [P15](#p15), which needs the opposite change | | ~~9e~~ | ~~[P41](#p41) `MODE` tie-break is random per run~~ | ✅ **Fixed 2026-09-06** — 152 → **156 AGREE** (four new cases). Small, as predicted, but not where it was filed: the named `ModeState` was a *shadowed* implementation and fixing it moved nothing. Reference does specify a rule and it is **first-occurrence**, not the "smallest value wins" this row proposed. Unblocked both example files, now FORMAL. Spun off [P42](#p42), [P43](#p43), [R12](ENGINE_REFACTORING.md#r12) | | ~~9g~~ | ~~[P46](#p46) column-vs-column `WHERE` returns 0 rows~~ | ✅ **Fixed 2026-09-13** — 157 → **168 AGREE**. Wider than filed: *every* non-literal right-hand operand read as NULL (comparisons, `IN` items, `BETWEEN` bounds), and a literal on the left errored (`WHERE 1=0`). One resolver for all operands closed it. Did **not** need [P48](#p48) alongside, as this row predicted — the fix kept the comparison in the WHERE evaluator. Spun off [P49](#p49) (correlated outer refs bind to the inner table), accepted knowingly | -| **NEXT** | [R13](ENGINE_REFACTORING.md#r13) one expression evaluator — **the active workstream from 2026-09-13** | **Slices 1–2 done 2026-09-13** (value evaluator three-valued, 168 → 174 AGREE, closed P48/P50); **slice 3 done 2026-09-14** (one construction path; case-insensitive mode now honoured outside WHERE, parity unmoved); **slice 4 in progress** (WHERE arms delegate, one operator per commit: BETWEEN/IN 2026-09-15, comparisons 2026-09-18 closing [P54](#p54), IS NULL and LIKE 2026-09-19 closing [P55](#p55), 175 → 179 AGREE; every WHERE operator now delegates; **finished 2026-10-01** with AND/OR/NOT/CASE, closing [P61](#p61) and [P62](#p62) under [D4](#d4), 183 → 186 AGREE); slice 5 (method calls) done 2026-09-26; slice 6 (HAVING, `IIF`, JOIN collapse) next. Not a finding but the reason several are cheap to fix only once. WHERE and the value evaluator implement every boolean operator separately with different NULL rules, so the same predicate answers differently in WHERE, HAVING, SELECT and JOIN ON. **Working rule:** a parity fix that touches expression evaluation lands as an R13 slice, not a patch. [P52](#p52) (NULL join keys) is slice 6 | +| **NEXT** | [R13](ENGINE_REFACTORING.md#r13) one expression evaluator — **the active workstream from 2026-09-13** | **Slices 1–2 done 2026-09-13** (value evaluator three-valued, 168 → 174 AGREE, closed P48/P50); **slice 3 done 2026-09-14** (one construction path; case-insensitive mode now honoured outside WHERE, parity unmoved); **slice 4 in progress** (WHERE arms delegate, one operator per commit: BETWEEN/IN 2026-09-15, comparisons 2026-09-18 closing [P54](#p54), IS NULL and LIKE 2026-09-19 closing [P55](#p55), 175 → 179 AGREE; every WHERE operator now delegates; **finished 2026-10-01** with AND/OR/NOT/CASE, closing [P61](#p61) and [P62](#p62) under [D4](#d4), 183 → 186 AGREE); slice 5 (method calls) done 2026-09-26; slice 6 in progress (2026-10-03: pin, then HAVING and `IIF` onto the one truth rule, finishing P62; JOIN NULL keys and retiring the WHERE evaluator left). Not a finding but the reason several are cheap to fix only once. WHERE and the value evaluator implement every boolean operator separately with different NULL rules, so the same predicate answers differently in WHERE, HAVING, SELECT and JOIN ON. **Working rule:** a parity fix that touches expression evaluation lands as an R13 slice, not a patch. [P52](#p52) (NULL join keys) is slice 6 | | then | [P60](#p60) `GROUP BY` unknown column → NULL group; [P58](#p58) JOIN ON half | Filed 2026-10-01. P60 is silent and a one-line cause, so cheap; P58's join half is a resolver fix — take it as the first slice of [R14](ENGINE_REFACTORING.md#r14) rather than a patch. [P59](#p59) (quoted alias) rides along if convenient | | then | [P47](#p47) `MIN`/`MAX` ranked by type | Silent and cheap, and outside the evaluator (aggregate registries — confirm the live one first, [R12](ENGINE_REFACTORING.md#r12)), so it can land alongside R13 without breaking the working rule | | then | [P14](#p14), [P20](#p20), [P23](#p23) | Smaller, self-contained, decisions already taken. Was row 9b, then NEXT until the 2026-09-12 findings displaced it. **Check each against the R13 working rule first** — P20 (`\|\|` with NULL) is an operator in the value evaluator | @@ -2457,7 +2457,8 @@ out of date. `left_join_on_null_score_inequality` — because the comparator's ordering and `<>` answers with a NULL side are never true; they are there to hold when the nested loop's comparator changes. `nested_loop_join_inner` and - `nested_loop_join_left` (the single-condition forms) have no callers. + `nested_loop_join_left` (the single-condition forms) had no callers — + deleted 2026-10-03 ahead of the fix. - **Observed:** `null_edges a JOIN null_edges b ON a.label = b.label` returns **32** rows; DuckDB **7**. Seven distinct non-NULL labels match themselves; the five NULL-label rows then pair with each other, 5 × 5 = 25. @@ -2694,18 +2695,19 @@ out of date. ### P62 — Text and dates used as a truth value are read as TRUE when non-empty - **Status:** ✅ FIXED 2026-10-01 by [R13](ENGINE_REFACTORING.md#r13) slice 4 under [D4](#d4): `Trilean::from_value` is the one rule, and three of the - five copies below are gone (183 → 185 AGREE). HAVING's `is_truthy` and - `IIF` still carry their own tables — slice 6, **pinned 2026-10-03**: 7 - HAVING and 8 `IIF` divergences recorded in the matrix, two corpus cases - waiting (below). + five copies below are gone (183 → 185 AGREE). **The last two — HAVING's + `is_truthy` and `IIF` — went 2026-10-03 in R13 slice 6** (pinned first: 7 + HAVING and 8 `IIF` matrix entries, all FIXED in one change; 190 → 191 AGREE). + All five copies are now `Trilean::from_value`. `IIF('abc', …)` and + `HAVING word` are errors, as WHERE already was. - **Corpus:** `02_where.toml :: where_text_flag_as_predicate`, `where_not_text_flag` (DIFFER → AGREE), `where_text_not_a_boolean` (OURS_ONLY → `expect = "BOTH_ERR"`), over new `data/predicate_text.csv`. Evaluator matrix: `TEXT_TRUTHY` / `TEXT_REFUSED` / `DATE_TRUTHY` entries. Slice 6 (the two remaining copies): `07_grouping.toml :: - having_text_flag_as_condition` (`expect = "DIFFER"` — `'f'`, `'N'`, `'no'` - keep their groups), `having_text_not_a_boolean` (`expect = "OURS_ONLY"`, to - become BOTH_ERR) and the guard `having_not_text_flag`. Matrix: + having_text_flag_as_condition` (DIFFER → AGREE — `'f'`, `'N'`, `'no'` + kept their groups), `having_text_not_a_boolean` (OURS_ONLY → + `expect = "BOTH_ERR"`) and the guard `having_not_text_flag`. Matrix: `EXPECTED_TRUTH_HAVING` / `KNOWN_TRUTH_HAVING` (a new `Having` column — each row a group of its own through `QueryEngine`) and `EXPECTED_TRUTH_IIF` / `KNOWN_TRUTH_IIF`. **`IIF` has no corpus case:** DuckDB has no `IIF`; its diff --git a/src/data/group_by_expressions.rs b/src/data/group_by_expressions.rs index 90146681..d153a1a4 100644 --- a/src/data/group_by_expressions.rs +++ b/src/data/group_by_expressions.rs @@ -9,6 +9,7 @@ use crate::data::arithmetic_evaluator::ArithmeticEvaluator; use crate::data::data_view::DataView; use crate::data::datatable::{DataColumn, DataRow, DataTable, DataValue}; use crate::data::query_engine::QueryEngine; +use crate::data::trilean::Trilean; use crate::sql::aggregates::contains_aggregate; use crate::sql::parser::ast::{SelectItem, SqlExpression}; use tracing::debug; @@ -340,8 +341,10 @@ impl GroupByExpressions for QueryEngine { ArithmeticEvaluator::new(&temp_table).with_case_insensitive(case_insensitive); let having_result = evaluator.evaluate(having_expr, 0)?; - // Skip this group if HAVING condition is not met - if !is_truthy(&having_result) { + // Skip this group unless the condition is TRUE: FALSE and + // UNKNOWN both drop it. The value is read with the one truth + // rule, so text that is no boolean is an error (D4). + if !Trilean::from_value(&having_result)?.is_true() { groups_filtered += 1; having_time += having_start.elapsed(); continue; @@ -471,14 +474,3 @@ fn expression_references_column(expr: &SqlExpression, column: &str) -> bool { _ => false, } } - -/// Check if a DataValue is truthy (for HAVING evaluation) -fn is_truthy(value: &DataValue) -> bool { - match value { - DataValue::Boolean(b) => *b, - DataValue::Integer(i) => *i != 0, - DataValue::Float(f) => *f != 0.0 && !f.is_nan(), - DataValue::Null => false, - _ => true, - } -} diff --git a/src/data/hash_join.rs b/src/data/hash_join.rs index 787f0ebe..f3eaf698 100644 --- a/src/data/hash_join.rs +++ b/src/data/hash_join.rs @@ -838,41 +838,6 @@ impl HashJoinExecutor { Ok(result) } - /// Qualify column name to avoid conflicts - fn qualify_column_name( - &self, - col_name: &str, - table_side: &str, - left_join_col: &str, - right_join_col: &str, - ) -> String { - // Extract base column name (without table prefix) - let base_name = if let Some(dot_pos) = col_name.rfind('.') { - &col_name[dot_pos + 1..] - } else { - col_name - }; - - let left_base = if let Some(dot_pos) = left_join_col.rfind('.') { - &left_join_col[dot_pos + 1..] - } else { - left_join_col - }; - - let right_base = if let Some(dot_pos) = right_join_col.rfind('.') { - &right_join_col[dot_pos + 1..] - } else { - right_join_col - }; - - // If this column name appears in both join columns, qualify it - if base_name == left_base || base_name == right_base { - format!("{}_{}", table_side, base_name) - } else { - col_name.to_string() - } - } - /// Reverse a join operator for right joins fn reverse_operator(&self, op: &JoinOperator) -> JoinOperator { match op { @@ -904,113 +869,6 @@ impl HashJoinExecutor { compare_with_op(left, right, op_str, self.case_insensitive) } - /// Nested loop join for INNER JOIN with inequality conditions - fn nested_loop_join_inner( - &self, - left_table: Arc, - right_table: Arc, - left_col_idx: usize, - right_col_idx: usize, - operator: &JoinOperator, - join_alias: &Option, - ) -> Result { - let start = std::time::Instant::now(); - - info!( - "Executing nested loop INNER JOIN with {:?} operator: {} x {} rows", - operator, - left_table.row_count(), - right_table.row_count() - ); - - // Create result table with columns from both tables - let mut result = DataTable::new("joined"); - - // Add columns from left table - for col in &left_table.columns { - result.add_column(DataColumn { - name: col.name.clone(), - data_type: col.data_type.clone(), - nullable: col.nullable, - unique_values: col.unique_values, - distinct_values: None, - null_count: col.null_count, - metadata: col.metadata.clone(), - qualified_name: col.qualified_name.clone(), // Preserve qualified name - source_table: col.source_table.clone(), // Preserve source table - }); - } - - // Add columns from right table - for col in &right_table.columns { - if !left_table - .columns - .iter() - .any(|left_col| left_col.name == col.name) - { - result.add_column(DataColumn { - name: col.name.clone(), - data_type: col.data_type.clone(), - nullable: col.nullable, - unique_values: col.unique_values, - distinct_values: None, - null_count: col.null_count, - metadata: col.metadata.clone(), - qualified_name: col.qualified_name.clone(), // Preserve qualified name - source_table: col.source_table.clone(), // Preserve source table - }); - } else { - let (column_name, qualified_name) = if let Some(alias) = join_alias { - // Use the join alias for the column name - ( - format!("{}.{}", alias, col.name), - Some(format!("{}.{}", alias, col.name)), - ) - } else { - // Fall back to _right suffix - (format!("{}_right", col.name), col.qualified_name.clone()) - }; - result.add_column(DataColumn { - name: column_name, - data_type: col.data_type.clone(), - nullable: col.nullable, - unique_values: col.unique_values, - distinct_values: None, - null_count: col.null_count, - metadata: col.metadata.clone(), - qualified_name, - source_table: join_alias.clone().or_else(|| col.source_table.clone()), - }); - } - } - - // Nested loop join - let mut match_count = 0; - for left_row in &left_table.rows { - let left_value = &left_row.values[left_col_idx]; - - for right_row in &right_table.rows { - let right_value = &right_row.values[right_col_idx]; - - if self.compare_values(left_value, right_value, operator) { - let mut joined_row = DataRow { values: Vec::new() }; - joined_row.values.extend_from_slice(&left_row.values); - joined_row.values.extend_from_slice(&right_row.values); - result.add_row(joined_row); - match_count += 1; - } - } - } - - info!( - "Nested loop INNER JOIN complete: {} matches found in {:?}", - match_count, - start.elapsed() - ); - - Ok(result) - } - /// Nested loop join for INNER JOIN with multiple conditions fn nested_loop_join_inner_multi( &self, @@ -1532,129 +1390,6 @@ impl HashJoinExecutor { Ok(result) } - - /// Nested loop join for LEFT JOIN with inequality conditions - fn nested_loop_join_left( - &self, - left_table: Arc, - right_table: Arc, - left_col_idx: usize, - right_col_idx: usize, - operator: &JoinOperator, - join_alias: &Option, - ) -> Result { - let start = std::time::Instant::now(); - - info!( - "Executing nested loop LEFT JOIN with {:?} operator: {} x {} rows", - operator, - left_table.row_count(), - right_table.row_count() - ); - - // Create result table with columns from both tables - let mut result = DataTable::new("joined"); - - // Add columns from left table - for col in &left_table.columns { - result.add_column(DataColumn { - name: col.name.clone(), - data_type: col.data_type.clone(), - nullable: col.nullable, - unique_values: col.unique_values, - distinct_values: None, - null_count: col.null_count, - metadata: col.metadata.clone(), - qualified_name: col.qualified_name.clone(), // Preserve qualified name - source_table: col.source_table.clone(), // Preserve source table - }); - } - - // Add columns from right table (all nullable for LEFT JOIN) - for col in &right_table.columns { - if !left_table - .columns - .iter() - .any(|left_col| left_col.name == col.name) - { - result.add_column(DataColumn { - name: col.name.clone(), - data_type: col.data_type.clone(), - nullable: true, // Always nullable for outer join - unique_values: col.unique_values, - distinct_values: None, - null_count: col.null_count, - metadata: col.metadata.clone(), - qualified_name: col.qualified_name.clone(), // Preserve qualified name - source_table: col.source_table.clone(), // Preserve source table - }); - } else { - let (column_name, qualified_name) = if let Some(alias) = join_alias { - // Use the join alias for the column name - ( - format!("{}.{}", alias, col.name), - Some(format!("{}.{}", alias, col.name)), - ) - } else { - // Fall back to _right suffix - (format!("{}_right", col.name), col.qualified_name.clone()) - }; - result.add_column(DataColumn { - name: column_name, - data_type: col.data_type.clone(), - nullable: true, // Always nullable for outer join - unique_values: col.unique_values, - distinct_values: None, - null_count: col.null_count, - metadata: col.metadata.clone(), - qualified_name, - source_table: join_alias.clone().or_else(|| col.source_table.clone()), - }); - } - } - - // Nested loop join - let mut match_count = 0; - let mut null_count = 0; - - for left_row in &left_table.rows { - let left_value = &left_row.values[left_col_idx]; - let mut found_match = false; - - for right_row in &right_table.rows { - let right_value = &right_row.values[right_col_idx]; - - if self.compare_values(left_value, right_value, operator) { - let mut joined_row = DataRow { values: Vec::new() }; - joined_row.values.extend_from_slice(&left_row.values); - joined_row.values.extend_from_slice(&right_row.values); - result.add_row(joined_row); - match_count += 1; - found_match = true; - } - } - - // If no match found, emit left row with NULLs for right columns - if !found_match { - let mut joined_row = DataRow { values: Vec::new() }; - joined_row.values.extend_from_slice(&left_row.values); - for _ in 0..right_table.column_count() { - joined_row.values.push(DataValue::Null); - } - result.add_row(joined_row); - null_count += 1; - } - } - - info!( - "Nested loop LEFT JOIN complete: {} matches, {} nulls in {:?}", - match_count, - null_count, - start.elapsed() - ); - - Ok(result) - } } #[cfg(test)] diff --git a/src/sql/functions/comparison.rs b/src/sql/functions/comparison.rs index 9873fbf3..0a757c50 100644 --- a/src/sql/functions/comparison.rs +++ b/src/sql/functions/comparison.rs @@ -3,6 +3,7 @@ use std::cmp::Ordering; use super::{ArgCount, FunctionCategory, FunctionSignature, SqlFunction}; use crate::data::datatable::DataValue; +use crate::data::trilean::Trilean; /// Helper to compare two `DataValues` /// Returns None if values are incomparable (different types that can't be coerced) @@ -319,18 +320,10 @@ impl SqlFunction for IifFunction { let true_value = &args[1]; let false_value = &args[2]; - // Evaluate condition as boolean - let is_true = match condition { - DataValue::Boolean(b) => *b, - DataValue::Integer(i) => *i != 0, - DataValue::Float(f) => *f != 0.0 && !f.is_nan(), - DataValue::String(s) => !s.is_empty(), - DataValue::InternedString(s) => !s.is_empty(), - DataValue::Null => false, - _ => false, - }; - - Ok(if is_true { + // The one truth rule (D4): only TRUE takes the second argument - + // FALSE and NULL both take the third - and text that is no boolean + // is an error. + Ok(if Trilean::from_value(condition)?.is_true() { true_value.clone() } else { false_value.clone() diff --git a/tests/comparison/corpus/07_grouping.toml b/tests/comparison/corpus/07_grouping.toml index 3085215a..e7c4a1e4 100644 --- a/tests/comparison/corpus/07_grouping.toml +++ b/tests/comparison/corpus/07_grouping.toml @@ -80,7 +80,7 @@ sql = "SELECT team, COUNT(*) AS n FROM null_edges GROUP BY team HAVING NOT (MIN( id = "having_text_flag_as_condition" data = "predicate_text.csv" sql = "SELECT flag FROM predicate_text GROUP BY flag HAVING flag ORDER BY flag" -expect = "DIFFER" +# FIXED 2026-10-03 by R13 slice 6 (HAVING onto Trilean::from_value); was DIFFER. # P62, pinned 2026-10-03 for R13 slice 6. Text used as a truth value is read as # a boolean (D4): DuckDB keeps 'true' and 'yes'. HAVING collapses the value with # its own is_truthy, which reads any text as TRUE, so 'f', 'N' and 'no' keep @@ -97,6 +97,7 @@ sql = "SELECT flag FROM predicate_text GROUP BY flag HAVING NOT flag ORDER BY fl id = "having_text_not_a_boolean" data = "predicate_text.csv" sql = "SELECT word FROM predicate_text GROUP BY word HAVING word ORDER BY word" -expect = "OURS_ONLY" +expect = "BOTH_ERR" # P62 / D4. Text that does not read as a boolean is an error in DuckDB, and in -# our WHERE; HAVING's is_truthy keeps every non-NULL group instead. +# our WHERE and (since R13 slice 6, 2026-10-03) HAVING; HAVING's is_truthy +# kept every non-NULL group instead (was OURS_ONLY). diff --git a/tests/evaluator_matrix_tests.rs b/tests/evaluator_matrix_tests.rs index d7075edb..37ce8835 100644 --- a/tests/evaluator_matrix_tests.rs +++ b/tests/evaluator_matrix_tests.rs @@ -519,57 +519,11 @@ const EXPECTED_TRUTH_HAVING: &[(&str, &str)] = &[ ("NOT (MIN(n) > 0)", "FTFF"), ]; -/// HAVING collapses the condition's value with its own `is_truthy`, one of -/// P62's five truth tables and one of the two still standing: any text (even -/// empty) and any date are TRUE, so `'f'` and `'No'` keep their group and text -/// that is not a boolean keeps it rather than failing; NaN is FALSE, where -/// DuckDB reads it as non-zero. -const HAVING_IS_TRUTHY: &str = "P62: HAVING's own is_truthy - text and dates TRUE, NaN FALSE"; - -const KNOWN_TRUTH_HAVING: &[Known] = &[ - Known { - evaluator: Evaluator::Having, - predicate: "x", - observed: "TFFF", - why: HAVING_IS_TRUTHY, - }, - Known { - evaluator: Evaluator::Having, - predicate: "s", - observed: "TTFT", - why: HAVING_IS_TRUTHY, - }, - Known { - evaluator: Evaluator::Having, - predicate: "tb", - observed: "TTFT", - why: HAVING_IS_TRUTHY, - }, - Known { - evaluator: Evaluator::Having, - predicate: "d", - observed: "TFFT", - why: HAVING_IS_TRUTHY, - }, - Known { - evaluator: Evaluator::Having, - predicate: "CASE WHEN n > 1 THEN s END", - observed: "TFFF", - why: HAVING_IS_TRUTHY, - }, - Known { - evaluator: Evaluator::Having, - predicate: "MAX(tb)", - observed: "TTFT", - why: HAVING_IS_TRUTHY, - }, - Known { - evaluator: Evaluator::Having, - predicate: "MAX(s)", - observed: "TTFT", - why: HAVING_IS_TRUTHY, - }, -]; +// R13 slice 6 pinned 7 HAVING divergences here (2026-10-03), all now FIXED: +// HAVING collapsed the condition with its own `is_truthy` (any text and any +// date TRUE, NaN FALSE) - one of P62's truth tables. They went when HAVING +// moved onto `Trilean::from_value`. +const KNOWN_TRUTH_HAVING: &[Known] = &[]; /// The same values as `IIF`'s condition (R13 slice 6), through both /// evaluators. DuckDB has no `IIF`; these were generated with its `if()`, @@ -587,61 +541,11 @@ const EXPECTED_TRUTH_IIF: &[(&str, &str)] = &[ ("IIF(CASE WHEN n > 1 THEN tb END, true, false)", "TFFF"), ]; -/// `IIF` reads its condition with a table of its own - P62's other standing -/// copy, and not the same as HAVING's: non-empty text is TRUE (so `'f'` and -/// `'No'` take the second argument) but a date is FALSE, and NaN is FALSE. -const IIF_OWN_TRUTH: &str = "P62: IIF's own truth table - non-empty text TRUE, dates and NaN FALSE"; - -const KNOWN_TRUTH_IIF: &[Known] = &[ - Known { - evaluator: Evaluator::Where, - predicate: "IIF(x, true, false)", - observed: "TFFF", - why: IIF_OWN_TRUTH, - }, - Known { - evaluator: Evaluator::Value, - predicate: "IIF(x, true, false)", - observed: "TFFF", - why: IIF_OWN_TRUTH, - }, - Known { - evaluator: Evaluator::Where, - predicate: "IIF(s, true, false)", - observed: "TFFT", - why: IIF_OWN_TRUTH, - }, - Known { - evaluator: Evaluator::Value, - predicate: "IIF(s, true, false)", - observed: "TFFT", - why: IIF_OWN_TRUTH, - }, - Known { - evaluator: Evaluator::Where, - predicate: "IIF(tb, true, false)", - observed: "TTFT", - why: IIF_OWN_TRUTH, - }, - Known { - evaluator: Evaluator::Value, - predicate: "IIF(tb, true, false)", - observed: "TTFT", - why: IIF_OWN_TRUTH, - }, - Known { - evaluator: Evaluator::Where, - predicate: "IIF(d, true, false)", - observed: "FFFF", - why: IIF_OWN_TRUTH, - }, - Known { - evaluator: Evaluator::Value, - predicate: "IIF(d, true, false)", - observed: "FFFF", - why: IIF_OWN_TRUTH, - }, -]; +// R13 slice 6 pinned 8 `IIF` divergences here (2026-10-03), all now FIXED: +// `IIF` read its condition with its own table (non-empty text TRUE, dates and +// NaN FALSE) - P62's last copy - in both evaluators. They went when it moved +// onto `Trilean::from_value`. +const KNOWN_TRUTH_IIF: &[Known] = &[]; fn trilean_char(t: Trilean) -> char { match t {