diff --git a/data/pattern_matching.csv b/data/pattern_matching.csv new file mode 100644 index 00000000..94d256c4 --- /dev/null +++ b/data/pattern_matching.csv @@ -0,0 +1,7 @@ +id,s +1,abc +2,a.c +3,a(bc +4,ABC +5,[a]bc +6,a.b.c diff --git a/docs/ENGINE_REFACTORING.md b/docs/ENGINE_REFACTORING.md index ee59baa5..4d816f41 100644 --- a/docs/ENGINE_REFACTORING.md +++ b/docs/ENGINE_REFACTORING.md @@ -474,7 +474,9 @@ feature work**, and so we can tell the difference between "this is awkward" and - **Status:** 🟡 IN PROGRESS — filed 2026-09-13 out of [P46](SQL_PARITY.md#p46); **slices 1 and 2 done 2026-09-13, slice 3 (with 3b) done 2026-09-14**, slice 4 in progress (`BETWEEN`, `IN`, `NOT IN` delegated 2026-09-15; comparisons - 2026-09-18, closing P54). + 2026-09-18, closing P54; IS NULL and LIKE 2026-09-19, closing P55). Every + WHERE *operator* now delegates; what remains of slice 4 (AND / OR / NOT / + CASE) lands after slice 5. **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 → @@ -666,14 +668,42 @@ feature work**, and so we can tell the difference between "this is awkward" and parity moved by exactly one case (P54, 175 → **176 AGREE** / 206). `evaluate_binary_op` now gets the node itself, so neither the delegation nor the P37 value-as-predicate branch clones it per row. - - *Remaining in slice 4:* `LIKE` (WHERE compiles through `EvaluationContext`'s - regex cache; the value evaluator has its own matcher, and they disagree on a - non-string operand — `5.0 LIKE '5%'` is FALSE in WHERE, TRUE in the value - evaluator, and an error in DuckDB), `IS NULL` / `IS NOT NULL` (small), and - then `AND` / `OR` / `NOT` / `CASE`, which wait on slice 5 for their - method-call children. WHERE's own readers (`number_literal_value`, the - `DATETIME` arms) are now only reached through a legacy left-side method - call or LIKE. + - *Left after part 2:* LIKE and IS NULL (part 3, below), then `AND` / `OR` / + `NOT` / `CASE`, which wait on slice 5 for their method-call children. +- **Slice 4, third part (2026-09-19) — IS NULL and LIKE.** Six commits: + - *IS NULL / IS NOT NULL delegate* — a true no-op: the parser always puts a + `Null` literal on the right, and no reader difference changes whether the + left operand is NULL. WHERE's bare `IS` / `IS NOT` arms and `==` went with + it: the parser never emits them. + - *Pin LIKE.* A fourth matrix (`EXPECTED_LIKE`, DuckDB-generated) aimed at + what separated the two matchers. WHERE built its matcher by replacing `%` + and `_` in the pattern and escaping nothing else, so it got six of ten + wrong — **P55**: `LIKE 'a.c'` matched `abc`, `'[a]bc'` was a character + class, `'a(b%'` failed the whole query, and `%` stopped at a newline. Three + corpus cases over new `data/pattern_matching.csv`. + - *The value evaluator's matcher was exponential.* Right on all ten patterns, + but recursive over every split for every `%`: one 80-character value + against `'%a%a%a%a%a%a%a%b'` ran for over a minute (SELECT has had this all + along). Replaced with the iterative matcher — backtrack only to the most + recent `%`, O(text × pattern) — **before** WHERE delegated to it, so the fix + did not swap wrong rows for a hang. 28 unit cases plus a 2,000-character + pathological one on a 1 s budget. + - *LIKE over a number — a decision, D2.* DuckDB rejects it, so there is no + reference answer. Chosen: match the number's displayed text, as the value + evaluator already did (`5.0` displays as `5`). Pinned as a fifth matrix set + whose expected answers are the decision, not DuckDB output; WHERE's + "always FALSE" recorded against it. + - *Delegate.* All of LIKE is now one function, `sql_like`, called by the + value evaluator and by WHERE's one remaining legacy arm. Exactly the ten + WHERE pins flipped; parity moved by exactly the three P55 cases, + 176 → **179 AGREE** / 209. Flat on the benchmarks without the regex cache. + - *Plumbing removed.* `EvaluationContext` (the regex cache), WHERE's + `'ctx` lifetime and context constructors, and `with_config` (no callers) — + 147 lines out, parity byte-identical. + - *Left in slice 4:* `AND` / `OR` / `NOT` / `CASE` and WHERE's legacy + left-side method-call arm, all waiting on slice 5. WHERE's readers + (`number_literal_value`, the `DATETIME` arms, `compare_trilean`) are now + reached only through that arm. - **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 @@ -739,7 +769,7 @@ R8 legacy WHERE ──── independent; stage 2 is self-contained, do it in a R10 Trilean ──────── DONE; closed P18/P19 (parity 125 → 129) R11 ORDER BY resolver ─ independent; small, but a behaviour change — wants its own parity run 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 IN PROGRESS (BETWEEN/IN, comparisons done; LIKE, IS NULL next) → 5–6 +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) → 4 tail (AND/OR/NOT/CASE) → 6 ``` **A note on ordering, from the P18/P19 work being next.** The WHERE evaluator @@ -782,3 +812,4 @@ AGREE count — which makes it safe to land well before the semantics change. | 2026-09-14 | R13 slice 3: registries built once per process; one `ArithmeticEvaluator` constructor, dead date-notation plumbing deleted — parity byte-identical, high-cardinality GROUP BY ~2× faster. 3b: case-insensitive mode pinned (11 matrix + 5 clause cases) then handed in at every construction site; all flipped, parity byte-identical | — | | 2026-09-15 | R13 slice 4 (first part): WHERE's `BETWEEN`, `IN`, `NOT IN` delegate to the value evaluator. Latent alias-resolution fault pinned and fixed first; per-node TRACE logging found to be most of the value evaluator's cost and removed (SELECT/GROUP BY expressions ~2 s faster over 100k rows). Parity byte-identical at every commit | — | | 2026-09-18 | R13 slice 4 (second part): WHERE comparisons delegate. Operand readers pinned first (third matrix); P53 and P54 filed; value evaluator's `DATETIME()` moved to local time before delegating. Closes P54 — 175 → **176 AGREE**. Part 1 found to have fixed P54 for IN/BETWEEN unnoticed | — | +| 2026-09-19 | R13 slice 4 (third part): IS NULL and LIKE delegate; one `sql_like` for both evaluators. P55 (LIKE compiled as an unescaped regex) filed and closed — 176 → **179 AGREE**; the value evaluator's exponential LIKE matcher replaced first; D2 (LIKE over a number matches its displayed text) decided; `EvaluationContext` removed | — | diff --git a/docs/SQL_PARITY.md b/docs/SQL_PARITY.md index d95887b6..4fbb0426 100644 --- a/docs/SQL_PARITY.md +++ b/docs/SQL_PARITY.md @@ -65,10 +65,10 @@ 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, **206 cases** (176 AGREE / 15 DIFFER / -12 GAP / 1 OURS_ONLY / 2 BOTH_ERR as of 2026-09-18, after R13 slice 4's -comparison delegation closed [P54](#p54); [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, **209 cases** (179 AGREE / 15 DIFFER / +12 GAP / 1 OURS_ONLY / 2 BOTH_ERR as of 2026-09-19, after R13 slice 4 closed +[P54](#p54) and [P55](#p55); [P51](#p51), [P52](#p52) open, [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 @@ -93,7 +93,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), 175 → 176 AGREE; LIKE and IS NULL 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 — AND/OR/NOT/CASE follow slice 5). 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 | [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 | | 9f | [P42](#p42) `MODE` is numeric-only | Companion to [R12](ENGINE_REFACTORING.md#r12), and cheap if taken with it: the shadowed implementation already handles non-numerics and preserves type, so the fix is largely to stop the live path throwing away what it knows. Also buys the corpus its clearest tie-break case | @@ -2510,6 +2510,38 @@ out of date. the next such change is seen. - **Found:** 2026-09-18, pinning the operand readers for R13 slice 4. +### P55 — A `LIKE` pattern in `WHERE` is read as a regex +- **Status:** ✅ FIXED 2026-09-19 by [R13](ENGINE_REFACTORING.md#r13) slice 4: + WHERE's LIKE delegates to the value evaluator, and both now call one + function, `sql_like`, which treats only `%` and `_` as special. The regex + path is gone. 176 → **179 AGREE**; the six matrix entries went FIXED and + nothing else moved. +- **Corpus:** `02_where.toml :: where_like_dot_is_literal`, + `where_like_bracket_is_literal`, `where_like_paren_is_literal` (all AGREE). + The newline shapes are pinned in the evaluator matrix only (`EXPECTED_LIKE`); + a CSV value with an embedded newline would test the loader as much as LIKE. +- **Found on the way:** the value evaluator's own matcher, correct on all ten + patterns, was exponential — one 80-character value against + `'%a%a%a%a%a%a%a%b'` ran for over a minute. Replaced with an iterative + matcher before WHERE delegated to it, so the fix did not trade wrong rows for + a hang. +- **Observed:** `WHERE s LIKE 'a.c'` returns `abc` as well as `a.c`; + `LIKE '[a]bc'` returns `abc` and misses `[a]bc`; `LIKE 'a(b%'` fails the + query with "regex parse error â€Ļ unclosed group"; `LIKE '%'` misses a value + containing a newline. +- **Cause:** WHERE builds its matcher by string replacement — + `pattern.replace('%', ".*").replace('_', ".")` — in both + `EvaluationContext::get_or_compile_like_regex` (the cached path every query + takes) and the uncached fallback in `recursive_where_evaluator.rs`. Nothing + else is escaped, so every regex metacharacter (`. [ ] ( ) + * ? ^ $ | \ { }`) + in a pattern is live, and `.` does not match `\n`. The value evaluator's + `sql_like_match` walks characters and treats only `%` and `_` specially — it + gets all ten matrix patterns right. +- **Why it matters:** `.` and `(` are ordinary in the data people filter — + file names (`LIKE '%.csv'` matches `xcsv`), versions, email addresses, + `'price (usd)%'`. +- **Found:** 2026-09-19, pinning LIKE for R13 slice 4. + --- ## Deferred / won't fix (intentional) @@ -2525,6 +2557,25 @@ out of date. for the current "vanilla SQL consistency" effort. Revisit if/when that scope layer is pursued. Not a bug; do not let the corpus case churn — keep `expect = "GAP"`. +### D2 — `LIKE` over a number matches the number's displayed text +- **Status:** âšĒ DELIBERATE (decided 2026-09-19, R13 slice 4). +- **Corpus:** none — DuckDB errors, so there is no reference answer to agree + with. Pinned in the evaluator matrix (`EXPECTED_NUMBER_LIKE`), whose expected + answers for these rows are this decision, not DuckDB output. +- **Behaviour:** a non-text operand of `LIKE` is matched as the text sql-cli + displays for it: `51.5 LIKE '5%'` is TRUE; `5.0` displays as `5`, so it + matches `'5%'` but not `'5.0'`. DuckDB rejects the query (no + `LIKE(DOUBLE, VARCHAR)`) and asks for an explicit cast. +- **Rationale:** the tool is for exploring data, often loosely typed CSVs where + an id or code column is inferred as a number; "does this id start with + `9007`" should just work. Matching the displayed text means the pattern + matches what the user can see. The value evaluator (SELECT, HAVING, CASE) + already behaved this way; before R13 slice 4, WHERE answered FALSE for every + number, so the same predicate disagreed with itself between clauses. +- **Considered:** erroring like DuckDB (strict, but breaks existing SELECTs that + rely on it), and FALSE everywhere (a silent non-match is the least useful + answer). + _When we consciously diverge from the reference engine on results (rather than simply not implementing a feature), record it here with the rationale so the DIFFER is understood, not mistaken for a bug._ diff --git a/src/data/arithmetic_evaluator.rs b/src/data/arithmetic_evaluator.rs index 43791c78..c05b0c32 100644 --- a/src/data/arithmetic_evaluator.rs +++ b/src/data/arithmetic_evaluator.rs @@ -367,20 +367,7 @@ impl<'a> ArithmeticEvaluator<'a> { "IS NOT NULL" => Ok(DataValue::Boolean(!matches!(left_val, DataValue::Null))), "AND" => Ok(truth_of(&left_val)?.and(truth_of(&right_val)?).to_value()), "OR" => Ok(truth_of(&left_val)?.or(truth_of(&right_val)?).to_value()), - // LIKE operator - SQL pattern matching - "LIKE" => { - if matches!(left_val, DataValue::Null) || matches!(right_val, DataValue::Null) { - return Ok(DataValue::Null); - } - let mut text = self.value_to_string(&left_val); - let mut pattern = self.value_to_string(&right_val); - if self.case_insensitive { - text = text.to_lowercase(); - pattern = pattern.to_lowercase(); - } - let matches = self.sql_like_match(&text, &pattern); - Ok(DataValue::Boolean(matches)) - } + "LIKE" => Ok(sql_like(&left_val, &right_val, self.case_insensitive).to_value()), _ => Err(anyhow!("Unsupported arithmetic operator: {}", op)), } } @@ -467,77 +454,6 @@ impl<'a> ArithmeticEvaluator<'a> { } } - /// Convert DataValue to string for pattern matching - fn value_to_string(&self, value: &DataValue) -> String { - match value { - DataValue::String(s) => s.clone(), - DataValue::InternedString(s) => s.to_string(), - DataValue::Integer(i) => i.to_string(), - DataValue::Float(f) => f.to_string(), - DataValue::Boolean(b) => b.to_string(), - DataValue::DateTime(dt) => dt.to_string(), - DataValue::Vector(v) => { - // Format as "[x,y,z]" - let components: Vec = v.iter().map(|f| f.to_string()).collect(); - format!("[{}]", components.join(",")) - } - DataValue::Null => String::new(), - } - } - - /// SQL LIKE pattern matching - /// Supports % (any chars) and _ (single char) - fn sql_like_match(&self, text: &str, pattern: &str) -> bool { - let pattern_chars: Vec = pattern.chars().collect(); - let text_chars: Vec = text.chars().collect(); - - self.like_match_recursive(&text_chars, 0, &pattern_chars, 0) - } - - /// Recursive helper for LIKE matching - fn like_match_recursive( - &self, - text: &[char], - text_pos: usize, - pattern: &[char], - pattern_pos: usize, - ) -> bool { - // If we've consumed both text and pattern, it's a match - if pattern_pos >= pattern.len() { - return text_pos >= text.len(); - } - - // Handle % wildcard (matches zero or more characters) - if pattern[pattern_pos] == '%' { - // Try matching zero characters (skip the %) - if self.like_match_recursive(text, text_pos, pattern, pattern_pos + 1) { - return true; - } - // Try matching one or more characters - if text_pos < text.len() { - return self.like_match_recursive(text, text_pos + 1, pattern, pattern_pos); - } - return false; - } - - // If text is consumed but pattern isn't, no match - if text_pos >= text.len() { - return false; - } - - // Handle _ wildcard (matches exactly one character) - if pattern[pattern_pos] == '_' { - return self.like_match_recursive(text, text_pos + 1, pattern, pattern_pos + 1); - } - - // Handle literal character match - if text[text_pos] == pattern[pattern_pos] { - return self.like_match_recursive(text, text_pos + 1, pattern, pattern_pos + 1); - } - - false - } - /// Evaluate a function call fn evaluate_function_with_distinct( &mut self, @@ -1628,6 +1544,84 @@ fn compare_trilean( Trilean::from_bool(compare_with_op(left, right, op, case_insensitive)) } +/// `value LIKE pattern`, three-valued: the one implementation of LIKE, used by +/// the value evaluator and by the WHERE evaluator's remaining legacy arm. +/// NULL on either side is UNKNOWN. A non-text operand is matched as the text +/// sql-cli displays for it (D2 in docs/SQL_PARITY.md - DuckDB rejects LIKE on +/// a number instead). +pub(crate) fn sql_like(value: &DataValue, pattern: &DataValue, case_insensitive: bool) -> Trilean { + if matches!(value, DataValue::Null) || matches!(pattern, DataValue::Null) { + return Trilean::Unknown; + } + let (text, pattern) = (like_text(value), like_text(pattern)); + Trilean::from_bool(if case_insensitive { + like_match(&text.to_lowercase(), &pattern.to_lowercase()) + } else { + like_match(&text, &pattern) + }) +} + +/// The text a LIKE operand is matched as: what sql-cli displays for it. +fn like_text(value: &DataValue) -> String { + match value { + DataValue::String(s) => s.clone(), + DataValue::InternedString(s) => s.to_string(), + DataValue::Integer(i) => i.to_string(), + DataValue::Float(f) => f.to_string(), + DataValue::Boolean(b) => b.to_string(), + DataValue::DateTime(dt) => dt.to_string(), + DataValue::Vector(v) => { + // Format as "[x,y,z]" + let components: Vec = v.iter().map(|f| f.to_string()).collect(); + format!("[{}]", components.join(",")) + } + DataValue::Null => String::new(), + } +} + +/// SQL `LIKE`: `%` matches any run of characters (including none, and across +/// newlines), `_` exactly one character, and every other character only +/// itself - nothing in a pattern is regex syntax (P55). +/// +/// Iterative, backtracking only to the most recent `%`: when a literal fails +/// after a `%`, the `%` absorbs one more character and matching resumes from +/// there. That is enough because a later `%` can always absorb whatever an +/// earlier one would have, so earlier choices never need revisiting - which +/// keeps the worst case at O(text × pattern). The recursive matcher this +/// replaced tried every split for every `%`: one 60-character value against +/// `'%a%a%a%a%a%a%a%b'` took 7 s, and 80 characters over a minute. +fn like_match(text: &str, pattern: &str) -> bool { + let text: Vec = text.chars().collect(); + let pattern: Vec = pattern.chars().collect(); + let (mut t, mut p) = (0, 0); + // Pattern position just after the most recent `%`, and the text position + // that `%` has absorbed up to. + let mut resume: Option<(usize, usize)> = None; + + while t < text.len() { + match pattern.get(p) { + Some(&'%') => { + p += 1; + resume = Some((p, t)); + } + Some(&c) if c == '_' || c == text[t] => { + t += 1; + p += 1; + } + _ => match resume { + Some((after_percent, absorbed)) => { + p = after_percent; + t = absorbed + 1; + resume = Some((after_percent, t)); + } + None => return false, + }, + } + } + // Text used up: only trailing `%`s may remain. + pattern[p..].iter().all(|&c| c == '%') +} + /// The truth value of an operand of AND / OR / NOT. A value with no truth value /// (a string, a date) is an error, as it was before three-valued logic. fn truth_of(value: &DataValue) -> Result { @@ -1779,6 +1773,65 @@ mod tests { assert_eq!(result, DataValue::Float(10.0 / 3.0)); } + #[test] + fn like_match_semantics() { + for (text, pattern, expected) in [ + ("abc", "abc", true), + ("abc", "a%", true), + ("abc", "%c", true), + ("abc", "%b%", true), + ("abc", "a_c", true), + ("abc", "a_", false), + ("abc", "", false), + ("", "", true), + ("", "%", true), + ("", "_", false), + ("abc", "%%%", true), + ("abcd", "_%_", true), + ("a", "_%_", false), + // Only % and _ are special. + ("a.c", "a.c", true), + ("abc", "a.c", false), + ("[a]bc", "[a]bc", true), + ("abc", "[a]bc", false), + ("a(bc", "a(b%", true), + // A literal % in the data is matched by the wildcard. + ("50%", "50%", true), + // % spans a newline. + ("line1\nline2", "%2", true), + ("line1\nline2", "line1%", true), + // Backtracking to the most recent % is enough. + ("aab", "%a%b", true), + ("ab", "%a%a", false), + ("mississippi", "%iss%pi", true), + ("mississippi", "m%ss%ss%", true), + ("mississippi", "m%ss%ss%ss%", false), + // _ is one character, not one byte. + ("Ê", "_", true), + ("Êa", "_a", true), + ] { + assert_eq!( + like_match(text, pattern), + expected, + "{text:?} LIKE {pattern:?}" + ); + } + } + + #[test] + fn like_match_has_no_exponential_backtracking() { + // The recursive matcher this replaced took 7 s for 60 characters and + // over a minute for 80, on one value. Here: 2,000 characters. + let text = "a".repeat(2000); + let start = std::time::Instant::now(); + assert!(!like_match(&text, "%a%a%a%a%a%a%a%b")); + assert!( + start.elapsed() < std::time::Duration::from_secs(1), + "took {:?}", + start.elapsed() + ); + } + #[test] fn test_division_by_zero() { let table = create_test_table(); diff --git a/src/data/evaluation_context.rs b/src/data/evaluation_context.rs deleted file mode 100644 index 15a7d7ef..00000000 --- a/src/data/evaluation_context.rs +++ /dev/null @@ -1,69 +0,0 @@ -use regex::Regex; -use std::collections::HashMap; -use std::sync::Arc; - -/// Context for query evaluation that can cache expensive computations -/// like compiled regexes across multiple row evaluations -#[derive(Debug, Clone)] -pub struct EvaluationContext { - /// Cache of compiled regexes for LIKE patterns - /// Key is the SQL LIKE pattern, value is the compiled regex - regex_cache: HashMap>, - - /// Case insensitive mode - case_insensitive: bool, - - /// Statistics for debugging - regex_compilations: usize, - regex_cache_hits: usize, -} - -impl EvaluationContext { - pub fn new(case_insensitive: bool) -> Self { - Self { - regex_cache: HashMap::new(), - case_insensitive, - regex_compilations: 0, - regex_cache_hits: 0, - } - } - - /// Get or compile a regex for a SQL LIKE pattern - pub fn get_or_compile_like_regex(&mut self, like_pattern: &str) -> Result, String> { - // Check cache first - if let Some(regex) = self.regex_cache.get(like_pattern) { - self.regex_cache_hits += 1; - return Ok(Arc::clone(regex)); - } - - // Convert SQL LIKE pattern to regex pattern - let regex_pattern = like_pattern.replace('%', ".*").replace('_', "."); - - // Compile the regex - let regex = regex::RegexBuilder::new(&format!("^{}$", regex_pattern)) - .case_insensitive(self.case_insensitive) - .build() - .map_err(|e| format!("Invalid LIKE pattern '{}': {}", like_pattern, e))?; - - let regex_arc = Arc::new(regex); - self.regex_cache - .insert(like_pattern.to_string(), Arc::clone(®ex_arc)); - self.regex_compilations += 1; - - Ok(regex_arc) - } - - pub fn get_stats(&self) -> (usize, usize) { - (self.regex_compilations, self.regex_cache_hits) - } - - pub fn is_case_insensitive(&self) -> bool { - self.case_insensitive - } -} - -impl Default for EvaluationContext { - fn default() -> Self { - Self::new(false) - } -} diff --git a/src/data/mod.rs b/src/data/mod.rs index d180601b..fd08e923 100644 --- a/src/data/mod.rs +++ b/src/data/mod.rs @@ -36,7 +36,6 @@ pub mod stream_loader; pub mod arithmetic_evaluator; pub mod batch_window_evaluator; // Batch evaluation for window functions pub mod column_resolution_error; // One home for the 'a.path not found' message -pub mod evaluation_context; pub mod group_by_expressions; pub mod hash_join; pub mod query_engine; diff --git a/src/data/query_engine.rs b/src/data/query_engine.rs index 5eeb34ab..a4eef90d 100644 --- a/src/data/query_engine.rs +++ b/src/data/query_engine.rs @@ -10,7 +10,6 @@ use crate::config::config::BehaviorConfig; use crate::data::arithmetic_evaluator::ArithmeticEvaluator; use crate::data::data_view::DataView; use crate::data::datatable::{DataColumn, DataRow, DataTable, DataValue}; -use crate::data::evaluation_context::EvaluationContext; use crate::data::group_by_expressions::GroupByExpressions; use crate::data::hash_join::HashJoinExecutor; use crate::data::recursive_where_evaluator::RecursiveWhereEvaluator; @@ -1624,15 +1623,13 @@ impl QueryEngine { } let filter_start = Instant::now(); - // Create an evaluation context for caching compiled regexes - let mut eval_context = EvaluationContext::new(self.case_insensitive); // Create evaluator ONCE before the loop for performance let mut evaluator = if let Some(exec_ctx) = exec_context { - // Use both contexts: exec_context for alias resolution, eval_context for regex caching - RecursiveWhereEvaluator::with_both_contexts(&table, &mut eval_context, exec_ctx) + // The execution context is for alias resolution + RecursiveWhereEvaluator::with_exec_context(&table, exec_ctx, self.case_insensitive) } else { - RecursiveWhereEvaluator::with_context(&table, &mut eval_context) + RecursiveWhereEvaluator::with_case_insensitive(&table, self.case_insensitive) }; // Filter visible rows based on WHERE clause @@ -1670,14 +1667,6 @@ impl QueryEngine { } } - // Log regex cache statistics - let (compilations, cache_hits) = eval_context.get_stats(); - if compilations > 0 || cache_hits > 0 { - debug!( - "LIKE pattern cache: {} compilations, {} cache hits", - compilations, cache_hits - ); - } visible_rows = filtered_rows; let filter_duration = filter_start.elapsed(); info!( diff --git a/src/data/recursive_where_evaluator.rs b/src/data/recursive_where_evaluator.rs index 137f25d2..4e2e2bea 100644 --- a/src/data/recursive_where_evaluator.rs +++ b/src/data/recursive_where_evaluator.rs @@ -1,6 +1,5 @@ -use crate::data::arithmetic_evaluator::ArithmeticEvaluator; +use crate::data::arithmetic_evaluator::{sql_like, ArithmeticEvaluator}; use crate::data::datatable::{DataTable, DataValue}; -use crate::data::evaluation_context::EvaluationContext; use crate::data::query_engine::ExecutionContext; use crate::data::trilean::Trilean; use crate::data::value_comparisons::compare_with_op; @@ -12,10 +11,9 @@ use std::cell::RefCell; use tracing::debug; /// Evaluates WHERE clauses from `recursive_parser` directly against `DataTable` -pub struct RecursiveWhereEvaluator<'a, 'ctx, 'exec> { +pub struct RecursiveWhereEvaluator<'a, 'exec> { table: &'a DataTable, case_insensitive: bool, - context: Option<&'ctx mut EvaluationContext>, exec_context: Option<&'exec ExecutionContext>, /// One `ArithmeticEvaluator` for the whole evaluation, built on first use. /// Constructing one builds the function and aggregate registries, and its @@ -24,25 +22,20 @@ pub struct RecursiveWhereEvaluator<'a, 'ctx, 'exec> { arithmetic: RefCell>>, } -impl<'a, 'ctx, 'exec> RecursiveWhereEvaluator<'a, 'ctx, 'exec> { +impl<'a, 'exec> RecursiveWhereEvaluator<'a, 'exec> { #[must_use] - pub fn new(table: &'a DataTable) -> RecursiveWhereEvaluator<'a, 'static, 'static> { - RecursiveWhereEvaluator { - table, - case_insensitive: false, - context: None, - exec_context: None, - arithmetic: RefCell::new(None), - } + pub fn new(table: &'a DataTable) -> RecursiveWhereEvaluator<'a, 'static> { + Self::with_case_insensitive(table, false) } - /// Create evaluator with an evaluation context for caching - pub fn with_context(table: &'a DataTable, context: &'ctx mut EvaluationContext) -> Self { - let case_insensitive = context.is_case_insensitive(); - Self { + #[must_use] + pub fn with_case_insensitive( + table: &'a DataTable, + case_insensitive: bool, + ) -> RecursiveWhereEvaluator<'a, 'static> { + RecursiveWhereEvaluator { table, case_insensitive, - context: Some(context), exec_context: None, arithmetic: RefCell::new(None), } @@ -57,23 +50,6 @@ impl<'a, 'ctx, 'exec> RecursiveWhereEvaluator<'a, 'ctx, 'exec> { Self { table, case_insensitive, - context: None, - exec_context: Some(exec_context), - arithmetic: RefCell::new(None), - } - } - - /// Create evaluator with both execution context (for alias resolution) and evaluation context (for regex caching) - pub fn with_both_contexts( - table: &'a DataTable, - context: &'ctx mut EvaluationContext, - exec_context: &'exec ExecutionContext, - ) -> Self { - let case_insensitive = context.is_case_insensitive(); - Self { - table, - case_insensitive, - context: Some(context), exec_context: Some(exec_context), arithmetic: RefCell::new(None), } @@ -132,35 +108,6 @@ impl<'a, 'ctx, 'exec> RecursiveWhereEvaluator<'a, 'ctx, 'exec> { matrix[len1][len2] } - #[must_use] - pub fn with_case_insensitive( - table: &'a DataTable, - case_insensitive: bool, - ) -> RecursiveWhereEvaluator<'a, 'static, 'static> { - RecursiveWhereEvaluator { - table, - case_insensitive, - context: None, - exec_context: None, - arithmetic: RefCell::new(None), - } - } - - #[must_use] - pub fn with_config( - table: &'a DataTable, - case_insensitive: bool, - _date_notation: String, // No longer needed since we use centralized parse_datetime - ) -> RecursiveWhereEvaluator<'a, 'static, 'static> { - RecursiveWhereEvaluator { - table, - case_insensitive, - context: None, - exec_context: None, - arithmetic: RefCell::new(None), - } - } - /// Compare two values under SQL three-valued logic: if **either** operand /// is NULL the answer is UNKNOWN, never TRUE or FALSE. /// @@ -606,12 +553,14 @@ impl<'a, 'ctx, 'exec> RecursiveWhereEvaluator<'a, 'ctx, 'exec> { return self.evaluate_value_as_predicate(expr, row_index); } - // A comparison is the value evaluator's (R13 slice 4): its operand - // readers and three-valued `compare_trilean` are the one - // implementation. Two shapes stay below for now - a legacy method call - // on the left (`Length()`, `IndexOf()`, `Trim*()`), whose registry - // twins are slice 5's to check, and the operators with their own arms. - if is_comparison_operator(&op_upper) && !matches!(left, SqlExpression::MethodCall { .. }) { + // Comparisons, NULL tests and LIKE are the value evaluator's (R13 + // slice 4): its operand readers, three-valued `compare_trilean` and + // `sql_like` are the one implementation. One shape stays below for + // now - a legacy method call on the left (`Length()`, `IndexOf()`, + // `Trim*()`), whose registry twins are slice 5's to check. + if delegates_to_value_evaluator(&op_upper) + && !matches!(left, SqlExpression::MethodCall { .. }) + { reject_unlifted_window(left)?; reject_unlifted_window(right)?; return self.evaluate_delegated(expr, row_index); @@ -688,42 +637,13 @@ impl<'a, 'ctx, 'exec> RecursiveWhereEvaluator<'a, 'ctx, 'exec> { // Handle special operators that aren't standard comparisons match op_upper.as_str() { // LIKE operator - handle specially - "LIKE" => { - let table_value = cell_value.unwrap_or(DataValue::Null); - let pattern = match compare_value { - DataValue::String(s) => s, - DataValue::InternedString(s) => s.to_string(), - // A NULL pattern makes the whole predicate UNKNOWN; any - // other non-string pattern is simply not a match. - DataValue::Null => return Ok(Trilean::Unknown), - _ => return Ok(Trilean::False), - }; - - let text = match &table_value { - DataValue::String(s) => s.as_str(), - DataValue::InternedString(s) => s.as_str(), - // Likewise on the value side: NULL LIKE '...' is UNKNOWN, - // which matters under NOT — see P19. - DataValue::Null => return Ok(Trilean::Unknown), - _ => return Ok(Trilean::False), - }; - - // Use cached regex if context is available, otherwise compile fresh - if let Some(ctx) = &mut self.context { - let regex = ctx - .get_or_compile_like_regex(&pattern) - .map_err(|e| anyhow::anyhow!("{}", e))?; - Ok(Trilean::from_bool(regex.is_match(text))) - } else { - // Fallback to compiling regex each time (old behavior) - let regex_pattern = pattern.replace('%', ".*").replace('_', "."); - let regex = regex::RegexBuilder::new(&format!("^{regex_pattern}$")) - .case_insensitive(self.case_insensitive) - .build() - .map_err(|e| anyhow::anyhow!("Invalid LIKE pattern: {}", e))?; - Ok(Trilean::from_bool(regex.is_match(text))) - } - } + // Reached only with a legacy method call on the left; every other + // LIKE delegated above. Same function, so the same rules. + "LIKE" => Ok(sql_like( + &cell_value.unwrap_or(DataValue::Null), + &compare_value, + self.case_insensitive, + )), // IS NULL / IS NOT NULL "IS NULL" => Ok(Trilean::from_bool( @@ -733,16 +653,6 @@ impl<'a, 'ctx, 'exec> RecursiveWhereEvaluator<'a, 'ctx, 'exec> { cell_value.is_some() && !matches!(cell_value, Some(DataValue::Null)), )), - // `x IS NULL` written with an explicit NULL operand. Matched on the - // expression, not the value: `a IS b` where b happens to be NULL on - // this row is not a NULL test. - "IS" if matches!(right, SqlExpression::Null) => Ok(Trilean::from_bool( - cell_value.is_none() || matches!(cell_value, Some(DataValue::Null)), - )), - "IS NOT" if matches!(right, SqlExpression::Null) => Ok(Trilean::from_bool( - cell_value.is_some() && !matches!(cell_value, Some(DataValue::Null)), - )), - // Standard comparison operators - use centralized logic _ => { let table_value = cell_value.unwrap_or(DataValue::Null); @@ -1162,10 +1072,13 @@ fn reject_unlifted_window(expr: &SqlExpression) -> Result<()> { } } -/// The comparison operators the value evaluator implements, and so the ones a -/// WHERE comparison delegates. Expects the operator already upper-cased. -fn is_comparison_operator(op_upper: &str) -> bool { - matches!(op_upper, "=" | "!=" | "<>" | "<" | "<=" | ">" | ">=") +/// The binary predicates WHERE hands whole to the value evaluator: the +/// comparisons and the NULL tests. Expects the operator already upper-cased. +fn delegates_to_value_evaluator(op_upper: &str) -> bool { + matches!( + op_upper, + "=" | "!=" | "<>" | "<" | "<=" | ">" | ">=" | "IS NULL" | "IS NOT NULL" | "LIKE" + ) } /// Operators whose result is a truth value, handled by `evaluate_binary_op`'s @@ -1173,18 +1086,7 @@ fn is_comparison_operator(op_upper: &str) -> bool { fn is_predicate_operator(op_upper: &str) -> bool { matches!( op_upper, - "=" | "==" - | "!=" - | "<>" - | "<" - | "<=" - | ">" - | ">=" - | "LIKE" - | "IS" - | "IS NOT" - | "IS NULL" - | "IS NOT NULL" + "=" | "!=" | "<>" | "<" | "<=" | ">" | ">=" | "LIKE" | "IS NULL" | "IS NOT NULL" ) } diff --git a/tests/comparison/corpus/02_where.toml b/tests/comparison/corpus/02_where.toml index b62be2c1..bd06ad1a 100644 --- a/tests/comparison/corpus/02_where.toml +++ b/tests/comparison/corpus/02_where.toml @@ -297,3 +297,28 @@ sql = "SELECT id, name FROM big_ids WHERE id IN (9007199254740993, 12) ORDER BY # The control for the case above. Already AGREE: R13 slice 4 part 1 delegated # IN to the value evaluator, which fixed it for IN and BETWEEN without anyone # noticing - no case held an integer this large. + +# --- P55: LIKE patterns were compiled as regexes, unescaped --- +# Fixed by R13 slice 4: WHERE's LIKE delegates to `sql_like`, which treats +# only % and _ as special. The comments below describe the old behaviour. + +[[case]] +id = "where_like_dot_is_literal" +data = "pattern_matching.csv" +sql = "SELECT id, s FROM pattern_matching WHERE s LIKE 'a.c' ORDER BY id" +# P55. WHERE turned `%` into `.*` and `_` into `.` and passed every other +# character to the regex engine as-is, so `.` matched any character: `abc` +# came back alongside `a.c`. + +[[case]] +id = "where_like_bracket_is_literal" +data = "pattern_matching.csv" +sql = "SELECT id, s FROM pattern_matching WHERE s LIKE '[a]bc' ORDER BY id" +# P55. `[a]` was a regex character class: matched `abc`, missed `[a]bc` itself. + +[[case]] +id = "where_like_paren_is_literal" +data = "pattern_matching.csv" +sql = "SELECT id, s FROM pattern_matching WHERE s LIKE 'a(b%' ORDER BY id" +# P55. `(` opened a regex group, so the query failed: "Invalid LIKE pattern +# 'a(b%': regex parse error ... unclosed group". diff --git a/tests/evaluator_matrix_tests.rs b/tests/evaluator_matrix_tests.rs index eecd4fc6..221ac617 100644 --- a/tests/evaluator_matrix_tests.rs +++ b/tests/evaluator_matrix_tests.rs @@ -231,6 +231,77 @@ const KNOWN_OPERANDS: &[Known] = &[ }, ]; +/// LIKE (R13 slice 4). WHERE and the value evaluator each have their own +/// matcher - WHERE a regex built from the pattern, the value evaluator a +/// character walk - so these rows aim at what separates the two: pattern +/// characters that mean something to a regex, and text across a newline. +/// +/// | row | s | +/// |-----|-----------------| +/// | 0 | 'abc' | +/// | 1 | 'a.c' | +/// | 2 | 'a(bc' | +/// | 3 | 'line1\nline2' | +/// | 4 | NULL | +/// | 5 | 'ABC' | +/// | 6 | '[a]bc' | +fn like_table() -> DataTable { + let mut t = DataTable::new("t"); + t.add_column(DataColumn::new("s")); + for s in [ + Some("abc"), + Some("a.c"), + Some("a(bc"), + Some("line1\nline2"), + None, + Some("ABC"), + Some("[a]bc"), + ] { + let v = s.map_or(DataValue::Null, |s| DataValue::String(s.to_string())); + t.add_row(DataRow::new(vec![v])).unwrap(); + } + t +} + +/// Generated by DuckDB from the same table (`s VARCHAR`), as for `EXPECTED`. +const EXPECTED_LIKE: &[(&str, &str)] = &[ + // Characters that are syntax to a regex are literal in a LIKE pattern. + ("s LIKE 'a.c'", "FTFFUFF"), + ("s LIKE 'a(b%'", "FFTFUFF"), + ("s LIKE '[a]bc'", "FFFFUFT"), + // `%` and `_` are the only wildcards, and `%` spans a newline. + ("s LIKE 'a_c'", "TTFFUFF"), + ("s LIKE '%'", "TTTTUTT"), + ("s LIKE 'line1%'", "FFFTUFF"), + ("s LIKE '%2'", "FFFTUFF"), + // Anchored at both ends, and case-sensitive outside --case-insensitive. + ("s LIKE 'abc'", "TFFFUFF"), + ("s LIKE 'A%'", "FFFFUTF"), + ("s LIKE ''", "FFFFUFF"), +]; + +/// LIKE over a number, on the operand table. DuckDB refuses these outright +/// (no `LIKE(DOUBLE, VARCHAR)`), so the expected answers are **not** DuckDB's: +/// they are the deliberate choice recorded as D2 in docs/SQL_PARITY.md - a +/// number is matched as the text the tool displays for it. `5.0` displays as +/// `5`, so it matches `'5%'` but not `'5.0'`. +const EXPECTED_NUMBER_LIKE: &[(&str, &str)] = &[ + ("n LIKE '5%'", "TUF"), + ("n LIKE '5.0'", "FUF"), + ("n LIKE '7._'", "FUT"), + ("big LIKE '9007%'", "TTU"), + ("big LIKE '%993'", "TFU"), +]; + +// R13 slice 4 recorded four WHERE entries here - WHERE's LIKE answered FALSE +// for every non-text operand. All four went FIXED when LIKE delegated. +const KNOWN_NUMBER_LIKE: &[Known] = &[]; + +// R13 slice 4 recorded six WHERE entries here (P55: the pattern compiled to a +// regex, unescaped - `.`, `[a]` and `(` were live, and `%` stopped at a +// newline). All six went FIXED when LIKE delegated to `sql_like`. +const KNOWN_LIKE: &[Known] = &[]; + fn trilean_char(t: Trilean) -> char { match t { Trilean::True => 'T', @@ -297,6 +368,21 @@ fn every_operand_reading_through_both_evaluators() { check_matrix(&operand_table(), EXPECTED_OPERANDS, KNOWN_OPERANDS, false); } +#[test] +fn every_like_pattern_through_both_evaluators() { + check_matrix(&like_table(), EXPECTED_LIKE, KNOWN_LIKE, false); +} + +#[test] +fn like_over_a_number_through_both_evaluators() { + check_matrix( + &operand_table(), + EXPECTED_NUMBER_LIKE, + KNOWN_NUMBER_LIKE, + false, + ); +} + fn check_matrix( t: &DataTable, expected_answers: &[(&str, &str)],