From 4727abf44c9dd1da9c132cf3fbedeff3099738cb Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 14 Aug 2026 00:47:19 +0000 Subject: [PATCH 1/3] Fix exponential stats rewrite for nested OR predicates `binary_falsify` recursively falsified both children of an `Or` node before checking `EMIT_UNGUARDED_REWRITES`, so every registered `Binary` rewrite rule repeated the full recursive rewrite of the subtree and then discarded all but one copy. That makes the falsify rewrite O(2^depth) for left-deep OR chains such as `a = 1 OR a = 2 OR ...` and O(n^2) for balanced OR trees: a 24-term chain took ~137s to rewrite and ~16x longer for every 4 additional terms. Check the guard before recursing, mirroring the `And` arm (semantics are unchanged: the guarded rule always produced `None` for `Or`). The same 24-term chain now rewrites in ~120us and a 1000-term chain in ~8ms. Adds a regression test that counts `Binary` rule visits to pin the rewrite to one visit per node, and a falsifier shape test for `Or`. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_014a8epAuP9LG1R5tHPL3qpi Signed-off-by: Claude --- vortex-array/src/stats/rewrite/builtins.rs | 75 ++++++++++++++++++++-- 1 file changed, 71 insertions(+), 4 deletions(-) diff --git a/vortex-array/src/stats/rewrite/builtins.rs b/vortex-array/src/stats/rewrite/builtins.rs index 571c8b5ff84..c3d03f68a42 100644 --- a/vortex-array/src/stats/rewrite/builtins.rs +++ b/vortex-array/src/stats/rewrite/builtins.rs @@ -168,10 +168,19 @@ fn binary_falsify( let rhs_falsifier = ctx.falsify(rhs)?; or_collect(lhs_falsifier.into_iter().chain(rhs_falsifier)) } - Operator::Or => match (ctx.falsify(lhs)?, ctx.falsify(rhs)?) { - (Some(lhs), Some(rhs)) if P::EMIT_UNGUARDED_REWRITES => Some(and(lhs, rhs)), - _ => None, - }, + Operator::Or => { + // Check before recursing: falsifying the children first would repeat the + // whole recursive rewrite once per registered `Binary` rule, which is + // exponential in `Or`-nesting depth for chains like `a = 1 OR a = 2 OR ...`. + if !P::EMIT_UNGUARDED_REWRITES { + return Ok(None); + } + + match (ctx.falsify(lhs)?, ctx.falsify(rhs)?) { + (Some(lhs), Some(rhs)) => Some(and(lhs, rhs)), + _ => None, + } + } Operator::Add | Operator::Sub | Operator::Mul | Operator::Div => None, }) } @@ -704,6 +713,8 @@ fn stat_fn(expr: BoundExpression, aggregate_fn: AggregateFnRef) -> BoundExpressi mod tests { use std::sync::Arc; use std::sync::LazyLock; + use std::sync::atomic::AtomicUsize; + use std::sync::atomic::Ordering; use vortex_error::VortexResult; use vortex_session::VortexSession; @@ -737,15 +748,21 @@ mod tests { use crate::expr::stats::Stat; use crate::scalar::Scalar; use crate::scalar_fn::EmptyOptions; + use crate::scalar_fn::ScalarFnId; + use crate::scalar_fn::ScalarFnVTable; use crate::scalar_fn::ScalarFnVTableExt; use crate::scalar_fn::fns::between::BetweenOptions; use crate::scalar_fn::fns::between::StrictComparison; + use crate::scalar_fn::fns::binary::Binary; use crate::scalar_fn::fns::dynamic::DynamicComparison; use crate::scalar_fn::fns::dynamic::DynamicComparisonExpr; use crate::scalar_fn::fns::operators::CompareOperator; use crate::scalar_fn::internal::row_count::RowCount; use crate::stats::expr::StatFn; use crate::stats::expr::StatOptions; + use crate::stats::rewrite::StatsRewriteCtx; + use crate::stats::rewrite::StatsRewriteRule; + use crate::stats::session::StatsSessionExt; static SESSION: LazyLock = LazyLock::new(crate::array_session); @@ -856,6 +873,56 @@ mod tests { gt_eq(stat(col("a"), Stat::Min), lit(50)), )) ); + + let expr = or(gt(col("a"), lit(10)), lt(col("a"), lit(5))); + assert_rewrite_eq!( + falsify(&expr)?, + Some(and( + lt_eq(stat(col("a"), Stat::Max), lit(10)), + gt_eq(stat(col("a"), Stat::Min), lit(5)), + )) + ); + Ok(()) + } + + /// Counts how many times the stats rewrite visits a `Binary` node. + #[derive(Debug)] + struct BinaryVisitCounter(Arc); + + impl StatsRewriteRule for BinaryVisitCounter { + fn scalar_fn_id(&self) -> ScalarFnId { + Binary.id() + } + + fn falsify( + &self, + _expr: &BoundExpression, + _ctx: &StatsRewriteCtx<'_>, + ) -> VortexResult> { + self.0.fetch_add(1, Ordering::Relaxed); + Ok(None) + } + } + + #[test] + fn or_chain_falsify_visits_each_node_once() -> VortexResult<()> { + // Regression test: the `Or` falsifier used to recurse into both children once per + // registered `Binary` rule, making the rewrite exponential in `Or`-nesting depth + // for chains like `a = 0 OR a = 1 OR ...`. + let session = crate::array_session(); + let visits = Arc::new(AtomicUsize::new(0)); + session + .stats() + .register_rewrite(BinaryVisitCounter(Arc::clone(&visits))); + + let expr = (1..16).fold(eq(col("a"), lit(0)), |chain, i| { + or(chain, eq(col("a"), lit(i))) + }); + let falsifier = expr.bind(&test_scope())?.falsify(&session)?; + assert!(falsifier.is_some()); + + // One visit per `Binary` node: 16 comparisons plus 15 `or`s. + assert_eq!(visits.load(Ordering::Relaxed), 31); Ok(()) } From 52e032aedba9f79fb0a1ecf9b709b71a3a810646 Mon Sep 17 00:00:00 2001 From: Robert Kruszewski Date: Fri, 14 Aug 2026 10:47:02 +0100 Subject: [PATCH 2/3] Update builtins.rs Signed-off-by: Robert Kruszewski --- vortex-array/src/stats/rewrite/builtins.rs | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/vortex-array/src/stats/rewrite/builtins.rs b/vortex-array/src/stats/rewrite/builtins.rs index c3d03f68a42..c73672b8bea 100644 --- a/vortex-array/src/stats/rewrite/builtins.rs +++ b/vortex-array/src/stats/rewrite/builtins.rs @@ -906,12 +906,8 @@ mod tests { #[test] fn or_chain_falsify_visits_each_node_once() -> VortexResult<()> { - // Regression test: the `Or` falsifier used to recurse into both children once per - // registered `Binary` rule, making the rewrite exponential in `Or`-nesting depth - // for chains like `a = 0 OR a = 1 OR ...`. - let session = crate::array_session(); let visits = Arc::new(AtomicUsize::new(0)); - session + SESSION .stats() .register_rewrite(BinaryVisitCounter(Arc::clone(&visits))); From 0cc4ea7e251dbef8c78681180a3317ddbc18fe1a Mon Sep 17 00:00:00 2001 From: Robert Kruszewski Date: Fri, 14 Aug 2026 10:48:28 +0100 Subject: [PATCH 3/3] less Signed-off-by: Robert Kruszewski --- vortex-array/src/stats/rewrite/builtins.rs | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/vortex-array/src/stats/rewrite/builtins.rs b/vortex-array/src/stats/rewrite/builtins.rs index c73672b8bea..3cb5fdb06df 100644 --- a/vortex-array/src/stats/rewrite/builtins.rs +++ b/vortex-array/src/stats/rewrite/builtins.rs @@ -723,6 +723,7 @@ mod tests { use crate::aggregate_fn::AggregateFnVTableExt; use crate::aggregate_fn::EmptyOptions as AggregateEmptyOptions; use crate::aggregate_fn::fns::all_non_nan::AllNonNan; + use crate::array_session; use crate::dtype::DType; use crate::dtype::Nullability; use crate::dtype::PType; @@ -764,7 +765,7 @@ mod tests { use crate::stats::rewrite::StatsRewriteRule; use crate::stats::session::StatsSessionExt; - static SESSION: LazyLock = LazyLock::new(crate::array_session); + static SESSION: LazyLock = LazyLock::new(array_session); fn stat(expr: Expression, stat: Stat) -> Expression { let aggregate_fn = stat.aggregate_fn().expect("stat should have aggregate fn"); @@ -906,8 +907,9 @@ mod tests { #[test] fn or_chain_falsify_visits_each_node_once() -> VortexResult<()> { + let session = array_session(); let visits = Arc::new(AtomicUsize::new(0)); - SESSION + session .stats() .register_rewrite(BinaryVisitCounter(Arc::clone(&visits)));