Fix exponential stats rewrite for nested OR predicates - #9408
Conversation
`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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014a8epAuP9LG1R5tHPL3qpi Signed-off-by: Claude <noreply@anthropic.com>
Signed-off-by: Robert Kruszewski <github@robertk.io>
Merging this PR will degrade performance by 0.59%
|
| Mode | Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|---|
| ❌ | Simulation | compress_fsst[(1000, 64, 8)] |
1 ms | 1.2 ms | -12.23% |
| ⚡ | Simulation | cold_misaligned[(64, 256)] |
4.9 ms | 4.4 ms | +12.58% |
Tip
Investigate this regression by commenting @codspeedbot fix this regression on this PR, or directly use the CodSpeed MCP with your agent.
Comparing claude/zonemap-prune-performance-0lqpdu (0cc4ea7) with develop (9a584e4)
Footnotes
-
89 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩
| // 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 { |
There was a problem hiding this comment.
How did we merge this trait:
trait NonNanProof {
const EMIT_UNGUARDED_REWRITES: bool;
fn check(ctx: &StatsRewriteCtx<'_>, expr: &BoundExpression) -> VortexResult<NanCheck>;
}
binary_falsifyrecursively falsified both children of anOrnode before checkingEMIT_UNGUARDED_REWRITES, so every registeredBinaryrewrite 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 asa = 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.