refactor(r13): WHERE BETWEEN / IN delegate to the value evaluator (R13 slice 4, part 1)

docs/ENGINE_REFACTORING.md

@@ -473,7 +473,7 @@ feature work**, and so we can tell the difference between "this is awkward" and
 ### R13 — Two expression evaluators, every boolean operator implemented twice
 - **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
-  next.
+  in progress (`BETWEEN`, `IN`, `NOT IN` delegated 2026-09-15).
   **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 →
@@ -597,6 +597,44 @@ feature work**, and so we can tell the difference between "this is awkward" and
     `CASE x WHEN 'v'` (its own `values_equal`, with its own numeric rules — goes
     with slice 4) and method calls such as `.Contains()` routed to registry
     functions (slice 5).
+- **Slice 4, first part (2026-09-15).** `BETWEEN`, `IN` and `NOT IN` in WHERE
+  now evaluate through the value evaluator and cross back once, in
+  `evaluate_delegated` (`Trilean::from_value`); WHERE's own copies of the
+  AND/OR-over-comparisons rules are deleted. The P15 raw-window-operand check
+  stays on WHERE's side of the delegation (`reject_unlifted_window`) — the value
+  evaluator would evaluate one quietly. Five commits, parity report
+  byte-identical after each, matrix green in both runs:
+  - *Pin, then fix, alias resolution.* Delegating moves column resolution onto
+    the value evaluator, so both resolvers were probed first. Both were wrong
+    for an alias-qualified operand over two columns sharing a name: WHERE's fast
+    path resolved `i.a` to an index through the `ExecutionContext`, turned it
+    back into the bare name and looked *that* up (finding the first `a`), and
+    the inner value evaluator was built with no aliases. Latent — join output
+    disambiguates duplicate names before WHERE sees them, so no SQL probe
+    reached it — but pinned as five recorded divergences, then fixed by using
+    the index directly and handing the context's aliases in.
+  - *Logging was the cost.* The first delegation (BETWEEN) looked like a 6×
+    per-row regression. It was not the evaluator: non-interactive mode records
+    TRACE to a ring buffer and a log file, and `ArithmeticEvaluator::evaluate`
+    formatted the expression with `{:?}` at every node of every row. Removed —
+    and every existing value-evaluator path got faster with it (over
+    `trades_100000.csv`, ~2.4 s load: `WHERE price + 0 > quantity` 5.5 → 3.0 s,
+    `SELECT price * quantity` 4.8 → 2.6 s, `GROUP BY UPPER(trader)` 4.3 →
+    2.6 s). Delegated BETWEEN / IN / NOT IN each ended *faster* than the arm
+    they replaced.
+  - *Still ahead for the comparison arms* — each needs its own pin, since the
+    operand readers are not quite the same: WHERE reads `DATETIME(...)` as a
+    `DateTime` and `TODAY` in **local** time, the value evaluator as a `String`
+    and in **UTC** (the two disagree near midnight); WHERE reads `5.0` as
+    `Integer`, the value evaluator as `Float`; a comparison's left side may be a
+    legacy method call (`Length()`, `IndexOf()`, `Trim*()`) with no registry
+    twin yet (slice 5). `LIKE` has a regex cache in `EvaluationContext` that the
+    value evaluator's matcher does not use. `AND` / `OR` / `NOT` / `CASE` stay
+    recursive until their children can be WHERE-only method calls no longer.
+  - *Noticed, not done:* `WHERE price > quantity` (WHERE's own path) still runs
+    ~1 s over load where the delegated shapes do not — likely more per-row
+    logging in or around the WHERE loop; and window evaluation logs `info!`
+    timings per row (out of R13's scope).
 - **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
@@ -662,7 +700,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–6 (retire WHERE arms)
+R13 one evaluator ─── ACTIVE from 2026-09-13; slices 1 (pin), 2 (3VL), 3 (construction + case mode) DONE; 4 IN PROGRESS (BETWEEN/IN done) → 5–6
 ```
 
 **A note on ordering, from the P18/P19 work being next.** The WHERE evaluator
@@ -703,3 +741,4 @@ AGREE count — which makes it safe to land well before the semantics change.
 | 2026-09-13 | R13 slice 1: evaluator matrix (36 predicates × both evaluators × 5 NULL-bearing rows, DuckDB-derived expectations). WHERE 36/36, value evaluator 4/36. P48 widened; P50–P52 filed; six corpus cases, AGREE unchanged | — |
 | 2026-09-13 | R13 slice 2: value evaluator three-valued; `Trilean::from_value`/`to_value`; `to_bool` removed. Matrix value column 4/36 → 36/36 in one change. Closes P48, P50 — 168 → **174 AGREE** | — |
 | 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 | — |

src/data/arithmetic_evaluator.rs

@@ -119,12 +119,12 @@ impl<'a> ArithmeticEvaluator<'a> {
     }
 
     /// Evaluate an SQL expression to produce a `DataValue`
+    ///
+    /// No logging on this path: it runs once per expression node per row, and
+    /// non-interactive mode records TRACE to a file, so a `debug!` formatting
+    /// the expression here cost more than evaluating it (2.5 s of a 5 s
+    /// `WHERE price + 0 > quantity` over 100k rows).
     pub fn evaluate(&mut self, expr: &SqlExpression, row_index: usize) -> Result<DataValue> {
-        debug!(
-            "ArithmeticEvaluator: evaluating {:?} for row {}",
-            expr, row_index
-        );
-
         match expr {
             SqlExpression::Column(column_ref) => self.evaluate_column_ref(column_ref, row_index),
             SqlExpression::StringLiteral(s) => Ok(DataValue::String(s.clone())),
@@ -225,10 +225,6 @@ impl<'a> ArithmeticEvaluator<'a> {
             let qualified_name = format!("{}.{}", actual_table, column_ref.name);
 
             if let Some(col_idx) = self.table.find_column_by_qualified_name(&qualified_name) {
-                debug!(
-                    "Resolved {}.{} -> '{}' at index {}",
-                    table_prefix, column_ref.name, qualified_name, col_idx
-                );
                 return self
                     .table
                     .get_value(row_index, col_idx)
@@ -238,10 +234,6 @@ impl<'a> ArithmeticEvaluator<'a> {
 
             // Fallback: try unqualified lookup
             if let Some(col_idx) = self.table.get_column_index(&column_ref.name) {
-                debug!(
-                    "Resolved {}.{} -> unqualified '{}' at index {}",
-                    table_prefix, column_ref.name, column_ref.name, col_idx
-                );
                 return self
                     .table
                     .get_value(row_index, col_idx)
@@ -275,10 +267,6 @@ impl<'a> ArithmeticEvaluator<'a> {
 
                 // For now, just use the column name part
                 // In the future, we could validate the table/alias part
-                debug!(
-                    "Resolving qualified column: {} -> {}",
-                    column_name, col_name
-                );
                 col_name.to_string()
             } else {
                 column_name.to_string()
@@ -358,13 +346,6 @@ impl<'a> ArithmeticEvaluator<'a> {
         let left_val = self.evaluate(left, row_index)?;
         let right_val = self.evaluate(right, row_index)?;
 
-        debug!(
-            "ArithmeticEvaluator: {} {} {}",
-            self.format_value(&left_val),
-            op,
-            self.format_value(&right_val)
-        );
-
         match op {
             "+" => self.add_values(&left_val, &right_val),
             "-" => self.subtract_values(&left_val, &right_val),
@@ -486,16 +467,6 @@ impl<'a> ArithmeticEvaluator<'a> {
         }
     }
 
-    /// Format a `DataValue` for debug output
-    fn format_value(&self, value: &DataValue) -> String {
-        match value {
-            DataValue::Integer(i) => i.to_string(),
-            DataValue::Float(f) => f.to_string(),
-            DataValue::String(s) => format!("'{s}'"),
-            _ => format!("{value:?}"),
-        }
-    }
-
     /// Convert DataValue to string for pattern matching
     fn value_to_string(&self, value: &DataValue) -> String {
         match value {
@@ -1431,11 +1402,6 @@ impl<'a> ArithmeticEvaluator<'a> {
 
         // Check if we have this function in the registry
         if self.function_registry.get(function_name).is_some() {
-            debug!(
-                "Proxying method '{}' through function registry as '{}'",
-                method, function_name
-            );
-
             // Prepare arguments: receiver is the first argument, followed by method args
             let mut func_args = vec![value.clone()];
 
@@ -1464,28 +1430,20 @@ impl<'a> ArithmeticEvaluator<'a> {
         else_branch: &Option<Box<SqlExpression>>,
         row_index: usize,
     ) -> Result<DataValue> {
-        debug!(
-            "ArithmeticEvaluator: evaluating CASE expression for row {}",
-            row_index
-        );
-
         // Evaluate each WHEN condition in order
         for branch in when_branches {
             // Evaluate the condition as a boolean
             let condition_result = self.evaluate_condition_as_bool(&branch.condition, row_index)?;
 
             if condition_result {
-                debug!("CASE: WHEN condition matched, evaluating result expression");
                 return self.evaluate(&branch.result, row_index);
             }
         }
 
         // If no WHEN condition matched, evaluate ELSE clause (or return NULL)
         if let Some(else_expr) = else_branch {
-            debug!("CASE: No WHEN matched, evaluating ELSE expression");
             self.evaluate(else_expr, row_index)
         } else {
-            debug!("CASE: No WHEN matched and no ELSE, returning NULL");
             Ok(DataValue::Null)
         }
     }
@@ -1498,14 +1456,8 @@ impl<'a> ArithmeticEvaluator<'a> {
         else_branch: &Option<Box<SqlExpression>>,
         row_index: usize,
     ) -> Result<DataValue> {
-        debug!(
-            "ArithmeticEvaluator: evaluating simple CASE expression for row {}",
-            row_index
-        );
-
         // Evaluate the main expression once
         let case_value = self.evaluate(expr, row_index)?;
-        debug!("Simple CASE: evaluated expression to {:?}", case_value);
 
         // Compare against each WHEN value in order
         for branch in when_branches {
@@ -1514,17 +1466,14 @@ impl<'a> ArithmeticEvaluator<'a> {
 
             // Check for equality
             if self.values_equal(&case_value, &when_value)? {
-                debug!("Simple CASE: WHEN value matched, evaluating result expression");
                 return self.evaluate(&branch.result, row_index);
             }
         }
 
         // If no WHEN value matched, evaluate ELSE clause (or return NULL)
         if let Some(else_expr) = else_branch {
-            debug!("Simple CASE: No WHEN matched, evaluating ELSE expression");
             self.evaluate(else_expr, row_index)
         } else {
-            debug!("Simple CASE: No WHEN matched and no ELSE, returning NULL");
             Ok(DataValue::Null)
         }
     }