Replace custom CEL evaluator with cel-rust#718
Conversation
| } | ||
| values => Err(ExecutionError::invalid_argument_count(2, values.len())), | ||
| } | ||
| } |
There was a problem hiding this comment.
Decimal arithmetic and ordering operators lost with opaque wrapping
Medium Severity
Wrapping Decimal as Value::Opaque removes support for native +, -, * operators and <, <=, >, >= comparisons that the old evaluator explicitly provided. Only decimal.Add is re-implemented as a named function. There are no equivalents for subtraction or multiplication, and cel-rust cannot apply arithmetic or ordering operators to opaque types. In a financial ledger system, this regression in decimal capabilities is concerning even if current expressions happen to use decimal.Add() rather than the + operator.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit d9dd2cd. Configure here.
📊 Performance ReportCommit: 34548db Cala Performance Benchmark Results (non-representative)Criterion Benchmark Results (single-threaded)
Load Testing Results (parallel-execution)
Note: Performance results may vary based on system resources and database state. Last updated by commit 34548db |
d9dd2cd to
7063f15
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
There are 2 total unresolved issues (including 1 from previous review).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 7063f15. Configure here.
7063f15 to
34548db
Compare


Summary
Verification
Performance
Compared
main(69006ee9) against this branch (34548dbb).Compile time
maincargo check -p cala-cel-interpreter --features fail-on-warningsSQLX_OFFLINE=true cargo check --workspacePure CEL microbenchmark
1.32us->5.80us)context.vars.account.metadata.check_velocity: ~49% slowercontext.vars.entry.units == decimal('100'): ~27% slowerParse cost may matter less for normal runtime if expressions are parsed once and reused, but context construction and velocity-style evaluation are on the hot path.
DB-backed
cala-perfvelocity benchmarksRan with a temporary local Postgres cluster and reset DB state before each branch:
mainmeanpost_simple_transaction_with_velocitypost_simple_transaction_with_skipped_velocitypost_simple_transaction_with_hit_velocityOverall: compile impact is small/mixed, but CEL-heavy runtime paths regress by roughly 10-22% in the DB-backed velocity benchmarks, and isolated CEL parsing/context construction are substantially slower.
Not run
Note
High Risk
High risk because it replaces the core CEL parsing/execution engine and rewires type/function handling, which can change evaluation semantics and error behavior across templates/params.
Overview
Switches
cala-cel-interpreterfrom the in-repocala-cel-parser+ custom AST evaluator tocel::Program/cel::Contextexecution, compiling expressions once and executing them against a registered runtime context.Reimplements Cala builtins on top of
cel(e.g.date,uuid,decimal,decimal.Add, timestampformat) and adds bidirectional conversions between CalaCelValue/CelKeyandcel::objects::Valueusing opaque wrappers for Decimal/Uuid.Removes the retired
cala-cel-parsercrate (LALRPOP grammar/build scripts and related context “package” plumbing), updates workspace deps/lockfile accordingly (including newceltransitive deps and dualthiserrorversions), and adjusts/coarsens some tests and date coercion to align with the new timestamp-backed representation.Reviewed by Cursor Bugbot for commit 34548db. Bugbot is set up for automated code reviews on this repo. Configure here.