From 64ce42a8ae5da0e018dde971734bb65d123122ae Mon Sep 17 00:00:00 2001 From: Cole Gentry Date: Thu, 16 Jul 2026 12:23:43 -0400 Subject: [PATCH 1/3] refactor(computed): replace meval with built-in expression engine meval has been unmaintained since 2017 and its transitive nom 1.2.4 will be rejected by a future version of Rust. Evaluated evalexpr, fasteval, and exmex as replacements; each silently changes the value or validity of formulas meval accepts today (evalexpr: integer division and math::-namespaced functions; fasteval: itself unmaintained, missing sqrt/exp/ln/atan2; exmex: -2^2 == +4 and left-associative ^). Instead, src/expression/engine.rs is a self-contained tokenizer + shunting-yard + RPN evaluator that mirrors meval's grammar exactly, verified with 200k differentially-fuzzed expressions against meval (0 mismatches, including accept/reject agreement on malformed input) plus an embedded golden-value corpus. Formulas now compile once and evaluate per record against a value slice (previously a fresh meval Context was rebuilt for every record). On the 223k-record Haltech example log: 15-31x faster for normal formulas, and z-score formulas drop from 14 s to 6.6 ms because channel statistics resolve to constants at compile time. Time-shift syntax (RPM[-1], RPM@-0.1s) and the public expression API are unchanged. Also: MCP/IPC formula evaluation now computes channel statistics when a formula uses _mean_/_stdev_/_min_/_max_/_range_ variables (previously it silently produced all zeros); log2/log10/trunc/fract/pow, tau, and phi now work (their names were already reserved); added the ignored benchmark test tests/expression_bench.rs for before/after comparisons. --- CLAUDE.md | 16 +- Cargo.lock | 27 +- Cargo.toml | 3 - src/expression/engine.rs | 750 +++++++++++++++++++++++ src/{expression.rs => expression/mod.rs} | 260 +++++--- src/ipc/handler.rs | 20 +- src/ui/computed_channels_manager.rs | 8 +- tests/computed_channels_tests.rs | 3 +- tests/core/computed_channels_tests.rs | 3 +- tests/expression_bench.rs | 78 +++ 10 files changed, 1054 insertions(+), 114 deletions(-) create mode 100644 src/expression/engine.rs rename src/{expression.rs => expression/mod.rs} (68%) create mode 100644 tests/expression_bench.rs diff --git a/CLAUDE.md b/CLAUDE.md index 2c80027..1a994db 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -58,7 +58,9 @@ src/ ├── units.rs # Unit preference types and conversions ├── normalize.rs # Field name normalization system ├── computed.rs # Computed channels data types and library -├── expression.rs # Formula parsing and evaluation engine +├── expression/ +│ ├── mod.rs # Formula parsing, channel refs, time shifts, evaluation +│ └── engine.rs # Built-in expression compiler/evaluator (replaced meval) ├── updater.rs # Auto-update functionality ├── analytics.rs # Privacy-respecting analytics ├── i18n.rs # rust-i18n language/locale selection @@ -160,11 +162,12 @@ src/ - `ComputedChannelLibrary` - Global persistent library stored as JSON - Support for time-shifting: index offsets (e.g., `RPM[-1]`) and time offsets (e.g., `RPM@-0.1s`) -- **`expression.rs`** - Formula evaluation engine: - - Parses mathematical expressions using meval - - Extracts channel references with time-shift syntax +- **`expression/`** - Formula evaluation engine: + - `engine.rs` - Self-contained expression compiler/evaluator (zero deps; replaced the unmaintained meval crate while preserving its exact grammar: `+ - * / % ^` with standard precedence, right-assoc `^`, `-2^2 == -4`, variadic `min`/`max`, `pi`/`e`/`tau`/`phi` constants) + - Formulas compile once to an RPN instruction list; per-record evaluation fills variable slots from log data with no parsing, hashing, or allocation in the loop + - Extracts channel references with time-shift syntax (regex preprocessing, unchanged) - Validates formulas against available channels - - Evaluates formulas across all log records with proper time-shifting + - Statistical variables (`_mean_X`, `_stdev_X`, ...) resolve to constants at compile time — callers must pass statistics via `evaluate_all_records_with_stats` when `formula_uses_statistics()` is true (a formula using them without statistics is now an error instead of silent zeros) - **`updater.rs`** - Auto-update system: - Checks GitHub releases for new versions @@ -399,7 +402,6 @@ Handled in `UltraLogApp::handle_keyboard_shortcuts` (`src/app.rs`); ignored whil - **open** (5) - Cross-platform URL/email opening - **strum** (0.28) - Enum string conversion for channel types - **regex** (1.12) - Log file parsing -- **meval** (0.2) - Mathematical expression evaluation for computed channels - **ureq** (3.3) - HTTP client for auto-updates and OpenECU Alliance API - **semver** (1.0) - Version comparison - **serde_yml** (0.0.12) - YAML parsing for adapter/protocol specs (replaces deprecated `serde_yaml`) @@ -420,6 +422,8 @@ Handled in `UltraLogApp::handle_keyboard_shortcuts` (`src/app.rs`); ignored whil - **thiserror** / **anyhow** - Error handling - **tracing** / **tracing-subscriber** - Structured logging +Computed-channel formula evaluation is handled by the built-in `src/expression/engine.rs` (no external crate — do not re-add meval or an expression-evaluator dependency; meval was removed because it was unmaintained and its `nom` 1.2.4 transitive dep will be rejected by future Rust). + ## Test Data Example log files are in `exampleLogs/` organized by ECU type: diff --git a/Cargo.lock b/Cargo.lock index c85a7a1..4a0a5f6 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1429,12 +1429,6 @@ dependencies = [ "zlib-rs", ] -[[package]] -name = "fnv" -version = "1.0.7" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "3f9eec918d3f24069decb9af1554cad7c880e2da24a9afd88aca000531ab82c1" - [[package]] name = "foldhash" version = "0.2.0" @@ -2475,7 +2469,7 @@ dependencies = [ "itoa 1.0.18", "log", "md-5", - "nom 8.0.0", + "nom", "nom_locate", "rand 0.9.5", "rangemap", @@ -2568,16 +2562,6 @@ dependencies = [ "autocfg", ] -[[package]] -name = "meval" -version = "0.2.0" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "f79496a5651c8d57cd033c5add8ca7ee4e3d5f7587a4777484640d9cb60392d9" -dependencies = [ - "fnv", - "nom 1.2.4", -] - [[package]] name = "mime" version = "0.3.17" @@ -2710,12 +2694,6 @@ version = "0.2.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "2bf50223579dc7cdcfb3bfcacf7069ff68243f8c363f62ffa99cf000a6b9c451" -[[package]] -name = "nom" -version = "1.2.4" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "a5b8c256fd9471521bcb84c3cdba98921497f1a331cbc15b8030fc63b82050ce" - [[package]] name = "nom" version = "8.0.0" @@ -2733,7 +2711,7 @@ checksum = "0b577e2d69827c4740cba2b52efaad1c4cc7c73042860b199710b3575c68438d" dependencies = [ "bytecount", "memchr", - "nom 8.0.0", + "nom", ] [[package]] @@ -5096,7 +5074,6 @@ dependencies = [ "flate2", "image", "memmap2", - "meval", "objc2 0.6.4", "objc2-foundation 0.3.2", "open", diff --git a/Cargo.toml b/Cargo.toml index 1ced0ad..a727b0c 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -41,9 +41,6 @@ regex = "1.12" strum = { version = "0.28", features = ["derive"] } rayon = "1.11" # Parallel iteration for parsing -# Expression evaluation (for computed channels) -meval = "0.2" - # Platform-specific directories dirs = "6.0" diff --git a/src/expression/engine.rs b/src/expression/engine.rs new file mode 100644 index 0000000..353a70b --- /dev/null +++ b/src/expression/engine.rs @@ -0,0 +1,750 @@ +//! Self-contained mathematical expression compiler and evaluator for computed +//! channel formulas. +//! +//! This replaces the unmaintained `meval` crate (last release 2017, depends on +//! `nom` 1.2.4 which will be rejected by a future version of Rust) while +//! preserving meval's exact grammar and semantics: +//! +//! - Operators: `+ - * / % ^` with meval's precedence table +//! (`+ -` = 1 left, `* / %` = 2 left, unary `+ -` = 3, `^` = 4 **right**) +//! so `-2^2 == -4` and `2^3^2 == 512`, matching standard math convention. +//! - Numbers: `2`, `2.`, `2.5`, `0.125e9`, `20.5E-3` (no leading-dot floats). +//! - Identifiers: start with a letter or `_`, continue with letters, digits +//! or `_`. Unicode letters are accepted (a superset of meval, which was +//! ASCII-only) so sanitized international channel names work. +//! - Functions: meval's built-in set, plus `log2`, `log10`, `trunc`, `fract` +//! and 2-arg `pow` (all names were already reserved in formulas). `min` and +//! `max` are variadic with at least one argument. `log` is intentionally +//! not defined (it was not defined in meval either, and its base would be +//! ambiguous); the error suggests `ln`/`log10`. +//! - Constants: `pi` and `e` (meval), plus `tau` and `phi` (already reserved). +//! +//! Formulas are compiled once to an RPN instruction list with variables +//! resolved to slots, then evaluated per record against a value slice with a +//! reusable stack — no per-record parsing, hashing or allocation. + +/// Binary operators, with meval's precedence and associativity. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +enum BinOp { + Add, + Sub, + Mul, + Div, + Rem, + Pow, +} + +impl BinOp { + fn prec(self) -> u32 { + match self { + BinOp::Add | BinOp::Sub => 1, + BinOp::Mul | BinOp::Div | BinOp::Rem => 2, + BinOp::Pow => 4, + } + } + + fn right_assoc(self) -> bool { + matches!(self, BinOp::Pow) + } + + fn apply(self, a: f64, b: f64) -> f64 { + match self { + BinOp::Add => a + b, + BinOp::Sub => a - b, + BinOp::Mul => a * b, + BinOp::Div => a / b, + BinOp::Rem => a % b, + BinOp::Pow => a.powf(b), + } + } +} + +/// Unary operator precedence: binds tighter than `* /` but looser than `^`, +/// exactly like meval (so `-2^2` is `-(2^2)`). +const UNARY_PREC: u32 = 3; + +#[derive(Debug, Clone, PartialEq)] +enum Token { + Number(f64), + Var(String), + /// Function name; the argument count is filled in during RPN conversion. + Func(String, usize), + UnaryPlus, + UnaryMinus, + Binary(BinOp), + LParen, + RParen, + Comma, +} + +/// A compiled instruction operating on the evaluation stack. +#[derive(Debug, Clone)] +enum Instr { + Const(f64), + /// Push the value of variable slot `n`. + Var(usize), + Bin(BinOp), + Neg, + Func1(fn(f64) -> f64), + Func2(fn(f64, f64) -> f64), + /// Fold the top `n` stack values with `f64::min`. + MinN(usize), + /// Fold the top `n` stack values with `f64::max`. + MaxN(usize), +} + +/// A formula compiled to RPN with variables resolved to slot indices. +#[derive(Debug, Clone)] +pub struct CompiledExpr { + instrs: Vec, + var_names: Vec, + max_stack: usize, +} + +fn is_ident_start(c: char) -> bool { + c.is_alphabetic() || c == '_' +} + +fn is_ident_continue(c: char) -> bool { + c.is_alphanumeric() || c == '_' +} + +/// Length in bytes of the identifier at the start of `s`. +fn ident_len(s: &str) -> usize { + let mut chars = s.char_indices(); + match chars.next() { + Some((_, c)) if is_ident_start(c) => {} + _ => return 0, + } + for (i, c) in chars { + if !is_ident_continue(c) { + return i; + } + } + s.len() +} + +/// Parse a number literal at the start of `s`, returning (value, length). +/// +/// Grammar (same as meval): `digit+ ('.' digit*)? ([eE] [+-]? digit+)?`. +/// A trailing `e`/`E` without a valid exponent makes the whole literal +/// invalid (meval behaves the same way). +fn number_len(s: &str) -> Result { + let bytes = s.as_bytes(); + let mut i = 0; + while i < bytes.len() && bytes[i].is_ascii_digit() { + i += 1; + } + debug_assert!(i > 0, "number_len called on non-digit start"); + if i < bytes.len() && bytes[i] == b'.' { + i += 1; + while i < bytes.len() && bytes[i].is_ascii_digit() { + i += 1; + } + } + if i < bytes.len() && (bytes[i] == b'e' || bytes[i] == b'E') { + let mut j = i + 1; + if j < bytes.len() && (bytes[j] == b'+' || bytes[j] == b'-') { + j += 1; + } + let exp_digits_start = j; + while j < bytes.len() && bytes[j].is_ascii_digit() { + j += 1; + } + if j == exp_digits_start { + return Err(format!( + "invalid number literal '{}'", + &s[..(j).min(s.len())] + )); + } + i = j; + } + Ok(i) +} + +#[derive(Debug, Clone, Copy, PartialEq)] +enum TokState { + /// Expecting an operand: number, var, function, unary op or '('. + LExpr, + /// Expecting an operator: binary op, ')' or ','. + AfterRExpr, +} + +#[derive(Debug, Clone, Copy, PartialEq)] +enum ParenKind { + Subexpr, + Func, +} + +/// Tokenize a formula using the same state machine as meval's tokenizer. +fn tokenize(input: &str) -> Result, String> { + let mut state = TokState::LExpr; + let mut paren_stack: Vec = Vec::new(); + let mut tokens = Vec::new(); + + let mut rest = input.trim_start(); + while !rest.is_empty() { + let c = rest.chars().next().unwrap(); + let (token, len) = match state { + TokState::LExpr => { + if c.is_ascii_digit() { + let len = number_len(rest)?; + let value: f64 = rest[..len] + .parse() + .map_err(|_| format!("invalid number literal '{}'", &rest[..len]))?; + (Token::Number(value), len) + } else if is_ident_start(c) { + let len = ident_len(rest); + let name = &rest[..len]; + // A '(' after optional whitespace makes this a function call. + let after = rest[len..].trim_start(); + if let Some(stripped) = after.strip_prefix('(') { + let consumed = rest.len() - stripped.len(); + (Token::Func(name.to_string(), 0), consumed) + } else { + (Token::Var(name.to_string()), len) + } + } else { + match c { + '+' => (Token::UnaryPlus, 1), + '-' => (Token::UnaryMinus, 1), + '(' => (Token::LParen, 1), + _ => return Err(format!("unexpected character '{c}'")), + } + } + } + TokState::AfterRExpr => match c { + '+' => (Token::Binary(BinOp::Add), 1), + '-' => (Token::Binary(BinOp::Sub), 1), + '*' => (Token::Binary(BinOp::Mul), 1), + '/' => (Token::Binary(BinOp::Div), 1), + '%' => (Token::Binary(BinOp::Rem), 1), + '^' => (Token::Binary(BinOp::Pow), 1), + ')' if !paren_stack.is_empty() => (Token::RParen, 1), + ',' if paren_stack.last() == Some(&ParenKind::Func) => (Token::Comma, 1), + _ => return Err(format!("unexpected character '{c}'")), + }, + }; + + match token { + Token::LParen => paren_stack.push(ParenKind::Subexpr), + Token::Func(..) => paren_stack.push(ParenKind::Func), + Token::RParen => { + paren_stack.pop(); + state = TokState::AfterRExpr; + } + Token::Var(_) | Token::Number(_) => state = TokState::AfterRExpr, + Token::Binary(_) | Token::Comma => state = TokState::LExpr, + Token::UnaryPlus | Token::UnaryMinus => {} + } + + tokens.push(token); + rest = rest[len..].trim_start(); + } + + if state == TokState::LExpr { + return Err("missing operand".to_string()); + } + if !paren_stack.is_empty() { + return Err("missing closing parenthesis".to_string()); + } + Ok(tokens) +} + +/// Convert infix tokens to RPN using meval's shunting-yard rules. +fn to_rpn(tokens: Vec) -> Result, String> { + let mut output: Vec = Vec::with_capacity(tokens.len()); + let mut stack: Vec = Vec::new(); + + // Precedence of a stacked operator token (operands never end up here). + fn stack_prec(token: &Token) -> Option { + match token { + Token::Binary(op) => Some(op.prec()), + Token::UnaryPlus | Token::UnaryMinus => Some(UNARY_PREC), + _ => None, + } + } + + for token in tokens { + match token { + Token::Number(_) | Token::Var(_) => output.push(token), + Token::UnaryPlus | Token::UnaryMinus => stack.push(token), + Token::Binary(op) => { + while let Some(top) = stack.last() { + let Some(top_prec) = stack_prec(top) else { + break; + }; + let pops = if op.right_assoc() { + op.prec() < top_prec + } else { + op.prec() <= top_prec + }; + if pops { + output.push(stack.pop().unwrap()); + } else { + break; + } + } + stack.push(token); + } + Token::LParen | Token::Func(..) => stack.push(token), + Token::RParen => { + let mut found = false; + while let Some(top) = stack.pop() { + match top { + Token::LParen => { + found = true; + break; + } + Token::Func(name, nargs) => { + found = true; + output.push(Token::Func(name, nargs + 1)); + break; + } + other => output.push(other), + } + } + if !found { + return Err("mismatched closing parenthesis".to_string()); + } + } + Token::Comma => { + let mut found = false; + while let Some(top) = stack.pop() { + match top { + Token::LParen => return Err("unexpected comma".to_string()), + Token::Func(name, nargs) => { + found = true; + stack.push(Token::Func(name, nargs + 1)); + break; + } + other => output.push(other), + } + } + if !found { + return Err("unexpected comma".to_string()); + } + } + } + } + + while let Some(top) = stack.pop() { + match top { + Token::Binary(_) | Token::UnaryPlus | Token::UnaryMinus => output.push(top), + _ => return Err("missing closing parenthesis".to_string()), + } + } + + Ok(output) +} + +fn func1(name: &str) -> Option f64> { + Some(match name { + "sqrt" => f64::sqrt, + "exp" => f64::exp, + "ln" => f64::ln, + "log2" => f64::log2, + "log10" => f64::log10, + "abs" => f64::abs, + "sin" => f64::sin, + "cos" => f64::cos, + "tan" => f64::tan, + "asin" => f64::asin, + "acos" => f64::acos, + "atan" => f64::atan, + "sinh" => f64::sinh, + "cosh" => f64::cosh, + "tanh" => f64::tanh, + "asinh" => f64::asinh, + "acosh" => f64::acosh, + "atanh" => f64::atanh, + "floor" => f64::floor, + "ceil" => f64::ceil, + "round" => f64::round, + "trunc" => f64::trunc, + "fract" => f64::fract, + "signum" => f64::signum, + _ => return None, + }) +} + +fn func2(name: &str) -> Option f64> { + Some(match name { + "atan2" => f64::atan2, + "pow" => f64::powf, + _ => return None, + }) +} + +fn constant(name: &str) -> Option { + Some(match name { + "pi" => std::f64::consts::PI, + "e" => std::f64::consts::E, + "tau" => std::f64::consts::TAU, + "phi" => 1.618_033_988_749_895_f64, + _ => return None, + }) +} + +impl CompiledExpr { + /// Parse and compile a formula. Every identifier that is not a function + /// or constant becomes a variable slot in `var_names` (first-appearance + /// order). + pub fn parse(input: &str) -> Result { + let rpn = to_rpn(tokenize(input)?)?; + + let mut instrs = Vec::with_capacity(rpn.len()); + let mut var_names: Vec = Vec::new(); + + for token in rpn { + let instr = match token { + Token::Number(value) => Instr::Const(value), + Token::Var(name) => { + if let Some(value) = constant(&name) { + Instr::Const(value) + } else { + let slot = var_names + .iter() + .position(|v| *v == name) + .unwrap_or_else(|| { + var_names.push(name); + var_names.len() - 1 + }); + Instr::Var(slot) + } + } + Token::Func(name, nargs) => match name.as_str() { + "min" => { + if nargs == 0 { + return Err("function 'min' needs at least 1 argument".to_string()); + } + Instr::MinN(nargs) + } + "max" => { + if nargs == 0 { + return Err("function 'max' needs at least 1 argument".to_string()); + } + Instr::MaxN(nargs) + } + _ => { + if let Some(f) = func1(&name) { + if nargs != 1 { + return Err(format!( + "function '{name}' expects 1 argument, got {nargs}" + )); + } + Instr::Func1(f) + } else if let Some(f) = func2(&name) { + if nargs != 2 { + return Err(format!( + "function '{name}' expects 2 arguments, got {nargs}" + )); + } + Instr::Func2(f) + } else if name == "log" { + return Err( + "unknown function 'log' (use 'ln' for natural log or 'log10')" + .to_string(), + ); + } else { + return Err(format!("unknown function '{name}'")); + } + } + }, + Token::Binary(op) => Instr::Bin(op), + Token::UnaryMinus => Instr::Neg, + // Unary plus is the identity; drop it. + Token::UnaryPlus => continue, + Token::LParen | Token::RParen | Token::Comma => { + unreachable!("parens/commas never appear in RPN output") + } + }; + instrs.push(instr); + } + + // Verify stack balance and compute the maximum stack depth so that + // `eval` can pre-reserve and never reallocate. + let mut depth: isize = 0; + let mut max_stack: isize = 0; + for instr in &instrs { + let delta = match instr { + Instr::Const(_) | Instr::Var(_) => 1, + Instr::Neg | Instr::Func1(_) => 0, + Instr::Bin(_) | Instr::Func2(_) => -1, + Instr::MinN(n) | Instr::MaxN(n) => 1 - (*n as isize), + }; + depth += delta; + if depth <= 0 { + return Err("missing operand".to_string()); + } + max_stack = max_stack.max(depth); + } + if depth != 1 { + return Err("too many operands".to_string()); + } + + Ok(CompiledExpr { + instrs, + var_names, + max_stack: max_stack as usize, + }) + } + + /// Variable slot names in slot order; `eval` expects values in this order. + pub fn var_names(&self) -> &[String] { + &self.var_names + } + + /// Evaluate against variable slot values, reusing `stack` as scratch + /// space so repeated evaluation does not allocate. + pub fn eval_with_stack(&self, vars: &[f64], stack: &mut Vec) -> f64 { + debug_assert_eq!(vars.len(), self.var_names.len()); + stack.clear(); + stack.reserve(self.max_stack); + for instr in &self.instrs { + match instr { + Instr::Const(value) => stack.push(*value), + Instr::Var(slot) => stack.push(vars.get(*slot).copied().unwrap_or(0.0)), + Instr::Neg => { + let top = stack.last_mut().expect("validated at compile time"); + *top = -*top; + } + Instr::Bin(op) => { + let b = stack.pop().expect("validated at compile time"); + let a = stack.last_mut().expect("validated at compile time"); + *a = op.apply(*a, b); + } + Instr::Func1(f) => { + let top = stack.last_mut().expect("validated at compile time"); + *top = f(*top); + } + Instr::Func2(f) => { + let b = stack.pop().expect("validated at compile time"); + let a = stack.last_mut().expect("validated at compile time"); + *a = f(*a, b); + } + Instr::MinN(n) => { + let base = stack.len() - n; + let folded = stack[base..].iter().copied().fold(f64::INFINITY, f64::min); + stack.truncate(base); + stack.push(folded); + } + Instr::MaxN(n) => { + let base = stack.len() - n; + let folded = stack[base..] + .iter() + .copied() + .fold(f64::NEG_INFINITY, f64::max); + stack.truncate(base); + stack.push(folded); + } + } + } + stack.pop().expect("validated at compile time") + } + + /// Convenience single-shot evaluation (allocates a fresh stack). + #[cfg(test)] + pub fn eval(&self, vars: &[f64]) -> f64 { + let mut stack = Vec::with_capacity(self.max_stack); + self.eval_with_stack(vars, &mut stack) + } +} + +#[cfg(test)] +mod tests { + use super::*; + + fn eval(src: &str) -> f64 { + CompiledExpr::parse(src).unwrap().eval(&[]) + } + + fn eval_vars(src: &str, vars: &[(&str, f64)]) -> f64 { + let compiled = CompiledExpr::parse(src).unwrap(); + let vals: Vec = compiled + .var_names() + .iter() + .map(|n| { + vars.iter() + .find(|(name, _)| name == n) + .map(|(_, v)| *v) + .unwrap_or_else(|| panic!("no value for variable '{n}'")) + }) + .collect(); + compiled.eval(&vals) + } + + /// Golden values computed with meval 0.2 before it was removed, pinning + /// this engine to meval's exact semantics (precedence, associativity, + /// unary binding, `%`, variadic min/max, constants, number formats). + /// The engine was additionally verified against meval with 200k + /// differentially-fuzzed random expressions at replacement time. + #[test] + fn test_meval_golden_values() { + let vars = [("A", 3.5_f64), ("B", -2.25_f64), ("C", 0.75_f64)]; + let cases: &[(&str, f64)] = &[ + ("-2^2", -4.0), + ("2^3^2", 512.0), + ("-(2+1)^2", -9.0), + ("2^-2", 0.25), + ("A - -B", 1.25), + ("-A^2 + B", -14.5), + ("A * -B^2", -17.71875), + ("A % B * C", 0.9375), + ("A / B / C", -2.074074074074074), + ("A - B - C", 5.0), + ("2 ^ A ^ C", 5.892527391914566), + ("min(A, B, C, 2)", -2.25), + ("max(A, min(B, C), abs(B))", 3.5), + ("atan2(A, B) + atan2(B, A)", 1.5707963267948966), + ("sin(A)^2 + cos(A)^2", 1.0), + ("sqrt(abs(B)) * exp(C) - ln(A)", 1.9227370564236441), + ("tanh(C) + asinh(A) - atan(B)", 3.7534414212526066), + ("floor(A) + ceil(B) + round(C) + signum(B)", 1.0), + ("(A + B) * (A - B) / (C + 1)", 4.107142857142857), + ("pi * A^2 + e^C", 40.60151002308764), + ("1e2 + 2.5E-1 * A", 100.875), + ("17.", 17.0), + ("0.125e1 ^ 2", 1.5625), + ("--A + +B", 1.25), + ("A*(B+C)^2-4/C%3", 5.541666666666667), + ("min(max(A, B), max(B, C), 1.5)", 0.75), + ("acosh(A) + sinh(B) % cosh(C)", 1.1177288483706174), + ("asin(C) * acos(C) / atan(A)", 0.4742167032429436), + ]; + for (src, expected) in cases { + let got = eval_vars(src, &vars); + assert!( + (got - expected).abs() <= 1e-12 * expected.abs().max(1.0), + "'{src}': got {got}, expected {expected}" + ); + } + } + + #[test] + fn test_basic_arithmetic() { + assert_eq!(eval("1 + 2 * 3"), 7.0); + assert_eq!(eval("(1 + 2) * 3"), 9.0); + assert_eq!(eval("10 / 4"), 2.5); + assert_eq!(eval("7 % 3"), 1.0); + assert_eq!(eval("2 ^ 10"), 1024.0); + } + + #[test] + fn test_meval_precedence_semantics() { + // Unary minus binds looser than `^` (standard math convention). + assert_eq!(eval("-2^2"), -4.0); + assert_eq!(eval("-(2+1)^2"), -9.0); + // `^` is right-associative. + assert_eq!(eval("2^3^2"), 512.0); + // Unary minus binds tighter than `*` and `/`. + assert_eq!(eval("-2 * 3"), -6.0); + assert_eq!(eval("2 * -3"), -6.0); + assert_eq!(eval("2^-2"), 0.25); + assert_eq!(eval("1 - -2"), 3.0); + assert_eq!(eval("--2"), 2.0); + assert_eq!(eval("+2"), 2.0); + } + + #[test] + fn test_number_formats() { + assert_eq!(eval("2."), 2.0); + assert_eq!(eval("2.5"), 2.5); + assert_eq!(eval("0.125e9"), 0.125e9); + assert_eq!(eval("20.5E-3"), 20.5e-3); + assert_eq!(eval("123e+2"), 12300.0); + } + + #[test] + fn test_invalid_numbers() { + assert!(CompiledExpr::parse(".5").is_err()); // no leading-dot floats + assert!(CompiledExpr::parse("1e").is_err()); + assert!(CompiledExpr::parse("1e+").is_err()); + } + + #[test] + fn test_functions() { + assert!((eval("sqrt(16)") - 4.0).abs() < 1e-12); + assert!((eval("sin(0)") - 0.0).abs() < 1e-12); + assert!((eval("cos(0)") - 1.0).abs() < 1e-12); + assert!((eval("abs(-3)") - 3.0).abs() < 1e-12); + assert!((eval("ln(e)") - 1.0).abs() < 1e-12); + assert!((eval("log10(100)") - 2.0).abs() < 1e-12); + assert!((eval("log2(8)") - 3.0).abs() < 1e-12); + assert!((eval("atan2(1, 1)") - std::f64::consts::FRAC_PI_4).abs() < 1e-12); + assert!((eval("pow(2, 10)") - 1024.0).abs() < 1e-12); + assert!((eval("signum(-5)") - -1.0).abs() < 1e-12); + assert!((eval("trunc(2.7)") - 2.0).abs() < 1e-12); + assert!((eval("fract(2.75)") - 0.75).abs() < 1e-12); + // Function name followed by whitespace then '(' is still a call. + assert!((eval("sqrt (16)") - 4.0).abs() < 1e-12); + } + + #[test] + fn test_variadic_min_max() { + assert_eq!(eval("min(3)"), 3.0); + assert_eq!(eval("min(3, 1, 2)"), 1.0); + assert_eq!(eval("max(3, 1, 2, 9, 4)"), 9.0); + assert_eq!(eval("max(min(3, 1), 2)"), 2.0); + } + + #[test] + fn test_constants() { + assert_eq!(eval("pi"), std::f64::consts::PI); + assert_eq!(eval("e"), std::f64::consts::E); + assert_eq!(eval("tau"), std::f64::consts::TAU); + assert!((eval("phi") - 1.618033988749895).abs() < 1e-12); + } + + #[test] + fn test_variables() { + assert_eq!(eval_vars("X * 2", &[("X", 21.0)]), 42.0); + assert_eq!( + eval_vars( + "RPM_lb__neg_1_rb_ + _mean_RPM", + &[("RPM_lb__neg_1_rb_", 5.0), ("_mean_RPM", 2.0)] + ), + 7.0 + ); + // Repeated variables share one slot. + let compiled = CompiledExpr::parse("X + X * X").unwrap(); + assert_eq!(compiled.var_names(), ["X"]); + assert_eq!(compiled.eval(&[3.0]), 12.0); + } + + #[test] + fn test_var_slot_order_is_first_appearance() { + let compiled = CompiledExpr::parse("B + A").unwrap(); + assert_eq!(compiled.var_names(), ["B", "A"]); + assert_eq!(compiled.eval(&[1.0, 10.0]), 11.0); + } + + #[test] + fn test_parse_errors() { + assert!(CompiledExpr::parse("").is_err()); + assert!(CompiledExpr::parse(" ").is_err()); + assert!(CompiledExpr::parse("1 +").is_err()); + assert!(CompiledExpr::parse("+ + +").is_err()); + assert!(CompiledExpr::parse("(1").is_err()); + assert!(CompiledExpr::parse("1)").is_err()); + assert!(CompiledExpr::parse("min()").is_err()); + assert!(CompiledExpr::parse("1, 2").is_err()); // comma outside function + assert!(CompiledExpr::parse("2 3").is_err()); // no implicit multiplication + assert!(CompiledExpr::parse("2 (3)").is_err()); + assert!(CompiledExpr::parse("sqrt(1, 2)").is_err()); // wrong arity + assert!(CompiledExpr::parse("atan2(1)").is_err()); + assert!(CompiledExpr::parse("unknown_func(1)").is_err()); + // 'log' is intentionally undefined; the error should point at ln/log10. + let err = CompiledExpr::parse("log(10)").unwrap_err(); + assert!(err.contains("ln"), "unexpected error: {err}"); + } + + #[test] + fn test_float_edge_semantics() { + assert!(eval("1 / 0").is_infinite()); + assert!(eval("0 / 0").is_nan()); + assert!(eval("sqrt(-1)").is_nan()); + // min/max fold from +/-infinity, ignoring NaN (same as meval). + assert_eq!(eval("max(0 / 0, 1)"), 1.0); + } +} diff --git a/src/expression.rs b/src/expression/mod.rs similarity index 68% rename from src/expression.rs rename to src/expression/mod.rs index 81d3c59..5a4763e 100644 --- a/src/expression.rs +++ b/src/expression/mod.rs @@ -3,10 +3,17 @@ //! This module handles parsing mathematical formulas that reference channel data, //! including support for time-shifted values (both index-based and time-based), //! and pre-computed channel statistics for anomaly detection. +//! +//! Formulas are compiled once by the built-in [`engine`] (which replaced the +//! unmaintained `meval` crate while preserving its grammar exactly) and then +//! evaluated per record against a slice of variable values, so evaluating a +//! formula across a large log does no per-record parsing or allocation. + +mod engine; use crate::computed::{ChannelReference, TimeShift}; use crate::parsers::types::Value; -use meval::{Context, Expr}; +use engine::CompiledExpr; use regex::Regex; use std::collections::HashMap; use std::sync::LazyLock; @@ -89,16 +96,25 @@ static UNQUOTED_CHANNEL_REGEX: LazyLock = LazyLock::new(|| { .expect("Invalid regex pattern") }); -/// Known meval functions and constants that should not be treated as channel names +/// Function and constant names reserved by the expression engine; these are +/// never treated as channel names. (Channels with these exact names can still +/// be referenced by quoting them.) const RESERVED_NAMES: &[&str] = &[ "sin", "cos", "tan", "asin", "acos", "atan", "atan2", "sinh", "cosh", "tanh", "asinh", "acosh", "atanh", "sqrt", "abs", "exp", "ln", "log", "log2", "log10", "floor", "ceil", "round", "trunc", - "fract", "signum", "max", "min", "pi", "e", "tau", "phi", + "fract", "signum", "max", "min", "pow", "pi", "e", "tau", "phi", ]; /// Prefixes for statistical variables that should not be treated as channel names const STATS_PREFIXES: &[&str] = &["_mean_", "_stdev_", "_min_", "_max_", "_range_"]; +/// Whether a formula references statistical variables +/// (`_mean_*`, `_stdev_*`, `_min_*`, `_max_*`, `_range_*`) and therefore needs +/// pre-computed channel statistics to evaluate. +pub fn formula_uses_statistics(formula: &str) -> bool { + STATS_PREFIXES.iter().any(|p| formula.contains(p)) +} + /// Extract all channel references from a formula pub fn extract_channel_references(formula: &str) -> Vec { let mut references = Vec::new(); @@ -126,7 +142,7 @@ pub fn extract_channel_references(formula: &str) -> Vec { let time_shift_str = caps.get(3).map(|m| m.as_str()); let full_match = caps.get(0).unwrap().as_str().to_string(); - // Skip reserved names (meval functions/constants) + // Skip reserved names (engine functions/constants) if RESERVED_NAMES.contains(&name.to_lowercase().as_str()) { continue; } @@ -208,40 +224,44 @@ pub fn validate_formula(formula: &str, available_channels: &[String]) -> Result< return Err(format!("Unknown channels: {}", missing.join(", "))); } - // Try to parse the formula with meval (using dummy variables) - let test_formula = prepare_formula_for_meval(formula, &refs); + // Compile the formula and check that every variable it uses resolves to + // either a channel reference or a statistical variable. + let compiled = compile_formula(formula, &refs)?; - // Create a context with all variables set to 1.0 - let mut ctx = Context::new(); - for r in &refs { - let var_name = sanitize_var_name(&r.full_match); - ctx.var(&var_name, 1.0); - } + let ref_names: std::collections::HashSet = refs + .iter() + .map(|r| sanitize_var_name(&r.full_match)) + .collect(); - // Also set dummy values for statistical variables (for anomaly detection formulas) + // Dummy statistics for every available channel (mirrors evaluation-time + // injection so stats-based formulas validate). + let mut stat_names = std::collections::HashSet::new(); for channel in available_channels { let safe_name = sanitize_var_name(channel); - ctx.var(format!("_mean_{}", safe_name), 1.0); - ctx.var(format!("_stdev_{}", safe_name), 1.0); - ctx.var(format!("_min_{}", safe_name), 0.0); - ctx.var(format!("_max_{}", safe_name), 2.0); - ctx.var(format!("_range_{}", safe_name), 2.0); - } - - match test_formula.parse::() { - Ok(expr) => { - // Try to evaluate with dummy values - match expr.eval_with_context(&ctx) { - Ok(_) => Ok(()), - Err(e) => Err(format!("Evaluation error: {}", e)), - } + for prefix in STATS_PREFIXES { + stat_names.insert(format!("{prefix}{safe_name}")); } - Err(e) => Err(format!("Parse error: {}", e)), } + + for var_name in compiled.var_names() { + if !ref_names.contains(var_name) && !stat_names.contains(var_name) { + return Err(format!("Evaluation error: unknown variable '{var_name}'")); + } + } + + Ok(()) } -/// Prepare a formula for meval by replacing channel references with sanitized variable names -fn prepare_formula_for_meval(formula: &str, refs: &[ChannelReference]) -> String { +/// Compile a formula: replace channel references with sanitized variable +/// names, then parse with the expression engine. +fn compile_formula(formula: &str, refs: &[ChannelReference]) -> Result { + let prepared = prepare_formula(formula, refs); + CompiledExpr::parse(&prepared).map_err(|e| format!("Parse error: {e}")) +} + +/// Prepare a formula for the engine by replacing channel references with +/// sanitized variable names +fn prepare_formula(formula: &str, refs: &[ChannelReference]) -> String { let mut result = formula.to_string(); // Sort refs by length (longest first) to avoid partial replacements @@ -256,7 +276,7 @@ fn prepare_formula_for_meval(formula: &str, refs: &[ChannelReference]) -> String result } -/// Sanitize a channel reference into a valid meval variable name. +/// Sanitize a channel reference into a valid engine variable name. /// Encodes special characters distinctly to avoid collisions /// (e.g., `RPM[-1]` vs `RPM[+1]` must produce different names). fn sanitize_var_name(full_match: &str) -> String { @@ -311,10 +331,66 @@ pub fn build_channel_bindings( Ok(bindings) } +/// Where a compiled variable slot gets its value from during evaluation. +enum SlotSource<'a> { + /// A channel reference: read from the log with an optional time shift. + Channel { + channel_index: usize, + time_shift: &'a TimeShift, + }, + /// A constant for the whole evaluation (statistical variables). + Constant(f64), +} + +/// Resolve every variable slot of a compiled formula to its value source. +fn resolve_slots<'a>( + compiled: &CompiledExpr, + refs: &'a [ChannelReference], + bindings: &HashMap, + stat_values: &HashMap, +) -> Result>, String> { + let ref_by_var: HashMap = refs + .iter() + .map(|r| (sanitize_var_name(&r.full_match), r)) + .collect(); + + compiled + .var_names() + .iter() + .map(|var_name| { + if let Some(r) = ref_by_var.get(var_name) { + Ok(SlotSource::Channel { + channel_index: bindings.get(&r.name).copied().unwrap_or(0), + time_shift: &r.time_shift, + }) + } else if let Some(value) = stat_values.get(var_name) { + Ok(SlotSource::Constant(*value)) + } else { + Err(format!("Evaluation error: unknown variable '{var_name}'")) + } + }) + .collect() +} + +/// Build the map of statistical variable names to their values +/// (`_mean_ChannelName`, `_stdev_ChannelName`, ...). +fn build_stat_values(statistics: &HashMap) -> HashMap { + let mut values = HashMap::with_capacity(statistics.len() * STATS_PREFIXES.len()); + for (channel_name, channel_stats) in statistics { + let safe_name = sanitize_var_name(channel_name); + values.insert(format!("_mean_{safe_name}"), channel_stats.mean); + values.insert(format!("_stdev_{safe_name}"), channel_stats.stdev); + values.insert(format!("_min_{safe_name}"), channel_stats.min); + values.insert(format!("_max_{safe_name}"), channel_stats.max); + values.insert(format!("_range_{safe_name}"), channel_stats.range); + } + values +} + /// Evaluate a formula for all records in the log /// -/// If `statistics` is provided, injects statistical variables for each channel: -/// - `_mean_ChannelName`, `_stdev_ChannelName`, `_min_ChannelName`, `_max_ChannelName`, `_range_ChannelName` +/// If the formula uses statistical variables (`_mean_ChannelName`, ...), use +/// [`evaluate_all_records_with_stats`] and provide the statistics. pub fn evaluate_all_records( formula: &str, bindings: &HashMap, @@ -340,51 +416,41 @@ pub fn evaluate_all_records_with_stats( } let refs = extract_channel_references(formula); - let prepared_formula = prepare_formula_for_meval(formula, &refs); - // Parse the formula once - let expr: Expr = prepared_formula - .parse() - .map_err(|e| format!("Parse error: {}", e))?; + // Compile once; per-record evaluation only fills the variable slots. + let compiled = compile_formula(formula, &refs)?; + let stat_values = statistics.map(build_stat_values).unwrap_or_default(); + let slots = resolve_slots(&compiled, &refs, bindings, &stat_values)?; + + let mut values = vec![0.0_f64; slots.len()]; + // Statistical variables are constant across records; fill them once. + for (slot, value) in slots.iter().zip(values.iter_mut()) { + if let SlotSource::Constant(c) = slot { + *value = *c; + } + } let num_records = log_data.len(); let mut results = Vec::with_capacity(num_records); + let mut stack = Vec::new(); for record_idx in 0..num_records { - let mut ctx = Context::new(); - - // Set each channel variable to its value at the appropriate record - for r in &refs { - let channel_idx = bindings.get(&r.name).copied().unwrap_or(0); - let value = get_shifted_value(record_idx, &r.time_shift, channel_idx, log_data, times); - let var_name = sanitize_var_name(&r.full_match); - ctx.var(&var_name, value); - } - - // Inject statistics variables if provided - if let Some(stats) = statistics { - for (channel_name, channel_stats) in stats { - let safe_name = sanitize_var_name(channel_name); - ctx.var(format!("_mean_{}", safe_name), channel_stats.mean); - ctx.var(format!("_stdev_{}", safe_name), channel_stats.stdev); - ctx.var(format!("_min_{}", safe_name), channel_stats.min); - ctx.var(format!("_max_{}", safe_name), channel_stats.max); - ctx.var(format!("_range_{}", safe_name), channel_stats.range); + for (slot, value) in slots.iter().zip(values.iter_mut()) { + if let SlotSource::Channel { + channel_index, + time_shift, + } = slot + { + *value = get_shifted_value(record_idx, time_shift, *channel_index, log_data, times); } } - match expr.eval_with_context(&ctx) { - Ok(value) => { - // Handle NaN and infinity - if value.is_nan() || value.is_infinite() { - results.push(0.0); - } else { - results.push(value); - } - } - Err(_) => { - results.push(0.0); - } + let result = compiled.eval_with_stack(&values, &mut stack); + // Handle NaN and infinity + if result.is_finite() { + results.push(result); + } else { + results.push(0.0); } } @@ -542,6 +608,20 @@ mod tests { assert!(result.is_err()); } + #[test] + fn test_validate_stats_formula() { + let channels = vec!["RPM".to_string()]; + let result = validate_formula("(RPM - _mean_RPM) / _stdev_RPM", &channels); + assert!(result.is_ok()); + } + + #[test] + fn test_validate_variadic_min_max() { + let channels = vec!["RPM".to_string(), "Boost".to_string(), "TPS".to_string()]; + assert!(validate_formula("min(RPM, Boost, TPS)", &channels).is_ok()); + assert!(validate_formula("max(RPM, Boost, TPS, 100)", &channels).is_ok()); + } + #[test] fn test_evaluate_simple() { let data = vec![ @@ -583,6 +663,48 @@ mod tests { assert_eq!(result[2], 1000.0); } + #[test] + fn test_evaluate_with_stats() { + let data = vec![ + vec![Value::Float(1.0)], + vec![Value::Float(2.0)], + vec![Value::Float(3.0)], + ]; + let times = vec![0.0, 0.1, 0.2]; + let mut bindings = HashMap::new(); + bindings.insert("RPM".to_string(), 0); + + let channels = vec!["RPM".to_string()]; + let statistics = compute_all_channel_statistics(&channels, &data); + + let result = evaluate_all_records_with_stats( + "RPM - _mean_RPM", + &bindings, + &data, + ×, + Some(&statistics), + ) + .unwrap(); + assert_eq!(result.len(), 3); + assert!((result[0] - (-1.0)).abs() < 1e-12); + assert!((result[1] - 0.0).abs() < 1e-12); + assert!((result[2] - 1.0).abs() < 1e-12); + } + + #[test] + fn test_evaluate_stats_formula_without_statistics_errors() { + let data = vec![vec![Value::Float(1.0)]]; + let times = vec![0.0]; + let mut bindings = HashMap::new(); + bindings.insert("RPM".to_string(), 0); + + // Statistical variables require statistics to be provided; previously + // (with meval) this silently produced all zeros. + let result = evaluate_all_records("RPM - _mean_RPM", &bindings, &data, ×); + assert!(result.is_err()); + assert!(result.unwrap_err().contains("_mean_RPM")); + } + #[test] fn test_find_record_at_time() { let times = vec![0.0, 0.1, 0.2, 0.3, 0.4]; diff --git a/src/ipc/handler.rs b/src/ipc/handler.rs index 3f2baaa..7d02c3b 100644 --- a/src/ipc/handler.rs +++ b/src/ipc/handler.rs @@ -425,12 +425,20 @@ impl UltraLogApp { Ok(bindings) => { computed.channel_bindings = bindings.clone(); - // Evaluate - match expression::evaluate_all_records( + // Evaluate, computing channel statistics first if the + // formula uses statistical variables (z-scores etc.) + let statistics = expression::formula_uses_statistics(&formula).then(|| { + expression::compute_all_channel_statistics( + &available_channels, + &file.log.data, + ) + }); + match expression::evaluate_all_records_with_stats( &formula, &bindings, &file.log.data, file.log.get_times_as_f64(), + statistics.as_ref(), ) { Ok(values) => { computed.cached_data = Some(values); @@ -527,11 +535,17 @@ impl UltraLogApp { Err(e) => return IpcResponse::error(e), }; - let all_values = match expression::evaluate_all_records( + // Compute channel statistics first if the formula uses statistical + // variables (z-scores etc.) + let statistics = expression::formula_uses_statistics(formula).then(|| { + expression::compute_all_channel_statistics(&available_channels, &file.log.data) + }); + let all_values = match expression::evaluate_all_records_with_stats( formula, &bindings, &file.log.data, file.log.get_times_as_f64(), + statistics.as_ref(), ) { Ok(v) => v, Err(e) => return IpcResponse::error(e), diff --git a/src/ui/computed_channels_manager.rs b/src/ui/computed_channels_manager.rs index c274d34..2f23bf1 100644 --- a/src/ui/computed_channels_manager.rs +++ b/src/ui/computed_channels_manager.rs @@ -10,7 +10,7 @@ use crate::app::UltraLogApp; use crate::computed::{ComputedChannel, ComputedChannelTemplate}; use crate::expression::{ build_channel_bindings, compute_all_channel_statistics, evaluate_all_records, - evaluate_all_records_with_stats, extract_channel_references, + evaluate_all_records_with_stats, extract_channel_references, formula_uses_statistics, }; use crate::parsers::types::ComputedChannelInfo; use crate::parsers::Channel; @@ -567,11 +567,7 @@ impl UltraLogApp { }; // Check if formula uses statistical variables (for z-score anomaly detection) - let needs_statistics = template.formula.contains("_mean_") - || template.formula.contains("_stdev_") - || template.formula.contains("_min_") - || template.formula.contains("_max_") - || template.formula.contains("_range_"); + let needs_statistics = formula_uses_statistics(&template.formula); // Evaluate the formula (with or without statistics) let cached_data = if needs_statistics { diff --git a/tests/computed_channels_tests.rs b/tests/computed_channels_tests.rs index 5e484b9..8f52299 100644 --- a/tests/computed_channels_tests.rs +++ b/tests/computed_channels_tests.rs @@ -872,7 +872,8 @@ fn test_formula_with_negative_numbers() { #[test] fn test_formula_with_scientific_notation() { let channels = vec!["X".to_string()]; - // meval may not support scientific notation directly, use regular decimals + // Scientific notation in literals is rejected at validation (the channel + // extractor would treat the exponent as a channel name); use decimals. let result = validate_formula("X * 0.001 + 250", &channels); assert!(result.is_ok()); } diff --git a/tests/core/computed_channels_tests.rs b/tests/core/computed_channels_tests.rs index 5e484b9..8f52299 100644 --- a/tests/core/computed_channels_tests.rs +++ b/tests/core/computed_channels_tests.rs @@ -872,7 +872,8 @@ fn test_formula_with_negative_numbers() { #[test] fn test_formula_with_scientific_notation() { let channels = vec!["X".to_string()]; - // meval may not support scientific notation directly, use regular decimals + // Scientific notation in literals is rejected at validation (the channel + // extractor would treat the exponent as a channel name); use decimals. let result = validate_formula("X * 0.001 + 250", &channels); assert!(result.is_ok()); } diff --git a/tests/expression_bench.rs b/tests/expression_bench.rs new file mode 100644 index 0000000..8b7b786 --- /dev/null +++ b/tests/expression_bench.rs @@ -0,0 +1,78 @@ +//! Benchmark for computed-channel formula evaluation over a large log. +//! +//! Ignored by default because it parses an 84 MB example log. Run with: +//! +//! ```bash +//! cargo test --release --test expression_bench -- --ignored --nocapture +//! ``` + +use std::time::Instant; +use ultralog::expression::{ + build_channel_bindings, compute_all_channel_statistics, evaluate_all_records, + evaluate_all_records_with_stats, extract_channel_references, +}; +use ultralog::parsers::haltech::Haltech; +use ultralog::parsers::types::Parseable; + +const LOG_PATH: &str = "exampleLogs/haltech/2025-03-06_0937pm_Logs658to874.csv"; + +#[test] +#[ignore = "benchmark: parses an 84 MB log; run explicitly with --ignored"] +fn bench_evaluate_large_log() { + let content = std::fs::read_to_string(LOG_PATH) + .unwrap_or_else(|e| panic!("failed to read '{LOG_PATH}': {e}")); + let log = Haltech.parse(&content).expect("should parse Haltech log"); + let available_channels: Vec = log.channels.iter().map(|c| c.name()).collect(); + let num_records = log.data.len(); + eprintln!( + "parsed {num_records} records, {} channels", + available_channels.len() + ); + + let formulas = [ + ("simple", "RPM * 2 + 1", false), + ("index shift", "RPM - RPM[-1]", false), + ( + "time shift", + "(\"Manifold Pressure\" - \"Manifold Pressure\"@-0.1s) * 10", + false, + ), + ("functions", "sqrt(abs(RPM)) + sin(RPM / 1000)", false), + ("z-score", "(RPM - _mean_RPM) / _stdev_RPM", true), + ]; + + let statistics = compute_all_channel_statistics(&available_channels, &log.data); + + for (label, formula, needs_stats) in formulas { + let refs = extract_channel_references(formula); + let bindings = build_channel_bindings(&refs, &available_channels) + .unwrap_or_else(|e| panic!("bindings for '{formula}': {e}")); + + let mut best = f64::INFINITY; + let mut len = 0; + for _ in 0..3 { + let start = Instant::now(); + let result = if needs_stats { + evaluate_all_records_with_stats( + formula, + &bindings, + &log.data, + &log.times, + Some(&statistics), + ) + } else { + evaluate_all_records(formula, &bindings, &log.data, &log.times) + } + .unwrap_or_else(|e| panic!("evaluate '{formula}': {e}")); + let elapsed = start.elapsed().as_secs_f64(); + best = best.min(elapsed); + len = result.len(); + } + assert_eq!(len, num_records); + eprintln!( + "{label:<12} {formula:<55} best of 3: {:>8.1} ms ({:>6.0} ns/record)", + best * 1000.0, + best * 1e9 / num_records as f64 + ); + } +} From 4f8db51da5407f6ed7d6f0fb26df2b3f3c61202f Mon Sep 17 00:00:00 2001 From: Cole Gentry Date: Thu, 16 Jul 2026 14:30:40 -0400 Subject: [PATCH 2/3] fix: address PR #77 review feedback - Use a HashMap for variable slot lookup during compilation instead of a linear scan (O(n) instead of O(n^2) in distinct variables) - Error on a missing channel binding in resolve_slots instead of silently falling back to channel index 0 - Recognize scientific-notation exponent fragments in the channel extractor so literals like 1e2 validate and evaluate end-to-end (previously the "e2" was treated as an unknown channel at validation) --- src/expression/engine.rs | 14 +++---- src/expression/mod.rs | 54 ++++++++++++++++++++++++++- tests/computed_channels_tests.rs | 7 +++- tests/core/computed_channels_tests.rs | 7 +++- 4 files changed, 69 insertions(+), 13 deletions(-) diff --git a/src/expression/engine.rs b/src/expression/engine.rs index 353a70b..6fdddb2 100644 --- a/src/expression/engine.rs +++ b/src/expression/engine.rs @@ -395,6 +395,8 @@ impl CompiledExpr { let mut instrs = Vec::with_capacity(rpn.len()); let mut var_names: Vec = Vec::new(); + let mut slot_by_name: std::collections::HashMap = + std::collections::HashMap::new(); for token in rpn { let instr = match token { @@ -402,14 +404,12 @@ impl CompiledExpr { Token::Var(name) => { if let Some(value) = constant(&name) { Instr::Const(value) + } else if let Some(&slot) = slot_by_name.get(&name) { + Instr::Var(slot) } else { - let slot = var_names - .iter() - .position(|v| *v == name) - .unwrap_or_else(|| { - var_names.push(name); - var_names.len() - 1 - }); + let slot = var_names.len(); + var_names.push(name.clone()); + slot_by_name.insert(name, slot); Instr::Var(slot) } } diff --git a/src/expression/mod.rs b/src/expression/mod.rs index 5a4763e..e3aad0f 100644 --- a/src/expression/mod.rs +++ b/src/expression/mod.rs @@ -152,8 +152,25 @@ pub fn extract_channel_references(formula: &str) -> Vec { continue; } - // Skip if this position is inside a quoted reference let start_pos = caps.get(0).unwrap().start(); + + // Skip the exponent fragment of a scientific-notation literal (the + // "e2" in "1e2"): an identifier of the form e immediately + // preceded by a digit or '.' is part of a number, not a channel. + let is_exponent_fragment = { + let mut name_chars = name.chars(); + matches!(name_chars.next(), Some('e' | 'E')) + && name_chars.all(|c| c.is_ascii_digit()) + && formula[..start_pos] + .chars() + .next_back() + .is_some_and(|c| c.is_ascii_digit() || c == '.') + }; + if is_exponent_fragment { + continue; + } + + // Skip if this position is inside a quoted reference let is_inside_quoted = references.iter().any(|r| { if let Some(pos) = formula.find(&r.full_match) { start_pos >= pos && start_pos < pos + r.full_match.len() @@ -359,8 +376,11 @@ fn resolve_slots<'a>( .iter() .map(|var_name| { if let Some(r) = ref_by_var.get(var_name) { + let channel_index = bindings.get(&r.name).copied().ok_or_else(|| { + format!("Evaluation error: no binding for channel '{}'", r.name) + })?; Ok(SlotSource::Channel { - channel_index: bindings.get(&r.name).copied().unwrap_or(0), + channel_index, time_shift: &r.time_shift, }) } else if let Some(value) = stat_values.get(var_name) { @@ -615,6 +635,36 @@ mod tests { assert!(result.is_ok()); } + #[test] + fn test_extract_skips_scientific_notation_exponents() { + // "e2" in "1e2" is an exponent, not a channel. + assert!(extract_channel_references("RPM * 1e2").len() == 1); + assert!(extract_channel_references("2E10 + 1.5e3").is_empty()); + // ...but a genuine channel named like an exponent is still extracted + // when it does not directly follow a digit. + let refs = extract_channel_references("RPM + e2"); + assert!(refs.iter().any(|r| r.name == "e2")); + } + + #[test] + fn test_validate_scientific_notation() { + let channels = vec!["RPM".to_string()]; + assert!(validate_formula("RPM * 1e2", &channels).is_ok()); + assert!(validate_formula("RPM * 1.5E-3 + 2e10", &channels).is_ok()); + } + + #[test] + fn test_evaluate_missing_binding_errors() { + let data = vec![vec![Value::Float(1.0)]]; + let times = vec![0.0]; + // Bindings intentionally empty: the referenced channel is unresolved. + let bindings = HashMap::new(); + + let result = evaluate_all_records("RPM * 2", &bindings, &data, ×); + assert!(result.is_err()); + assert!(result.unwrap_err().contains("no binding")); + } + #[test] fn test_validate_variadic_min_max() { let channels = vec!["RPM".to_string(), "Boost".to_string(), "TPS".to_string()]; diff --git a/tests/computed_channels_tests.rs b/tests/computed_channels_tests.rs index 8f52299..781aebe 100644 --- a/tests/computed_channels_tests.rs +++ b/tests/computed_channels_tests.rs @@ -872,10 +872,13 @@ fn test_formula_with_negative_numbers() { #[test] fn test_formula_with_scientific_notation() { let channels = vec!["X".to_string()]; - // Scientific notation in literals is rejected at validation (the channel - // extractor would treat the exponent as a channel name); use decimals. let result = validate_formula("X * 0.001 + 250", &channels); assert!(result.is_ok()); + + // Scientific-notation literals validate too: the channel extractor + // recognizes exponent fragments ("e2" in "1e2") as part of the number. + assert!(validate_formula("X * 1e2", &channels).is_ok()); + assert!(validate_formula("X * 1.5E-3 + 2e10", &channels).is_ok()); } #[test] diff --git a/tests/core/computed_channels_tests.rs b/tests/core/computed_channels_tests.rs index 8f52299..781aebe 100644 --- a/tests/core/computed_channels_tests.rs +++ b/tests/core/computed_channels_tests.rs @@ -872,10 +872,13 @@ fn test_formula_with_negative_numbers() { #[test] fn test_formula_with_scientific_notation() { let channels = vec!["X".to_string()]; - // Scientific notation in literals is rejected at validation (the channel - // extractor would treat the exponent as a channel name); use decimals. let result = validate_formula("X * 0.001 + 250", &channels); assert!(result.is_ok()); + + // Scientific-notation literals validate too: the channel extractor + // recognizes exponent fragments ("e2" in "1e2") as part of the number. + assert!(validate_formula("X * 1e2", &channels).is_ok()); + assert!(validate_formula("X * 1.5E-3 + 2e10", &channels).is_ok()); } #[test] From 70b4150674a2be73988ac14b2ec058f5f2c130ae Mon Sep 17 00:00:00 2001 From: Cole Gentry Date: Thu, 16 Jul 2026 14:35:11 -0400 Subject: [PATCH 3/3] =?UTF-8?q?fix(docs):=20avoid=20intra-doc=20link=20to?= =?UTF-8?q?=20private=20engine=20module=20=E2=80=94=20rustdoc=20-D=20warni?= =?UTF-8?q?ngs?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- src/expression/mod.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/expression/mod.rs b/src/expression/mod.rs index e3aad0f..77f0c5b 100644 --- a/src/expression/mod.rs +++ b/src/expression/mod.rs @@ -4,8 +4,8 @@ //! including support for time-shifted values (both index-based and time-based), //! and pre-computed channel statistics for anomaly detection. //! -//! Formulas are compiled once by the built-in [`engine`] (which replaced the -//! unmaintained `meval` crate while preserving its grammar exactly) and then +//! Formulas are compiled once by the built-in `engine` module (which replaced +//! the unmaintained `meval` crate while preserving its grammar exactly) and then //! evaluated per record against a slice of variable values, so evaluating a //! formula across a large log does no per-record parsing or allocation.