adriangb opened a new issue, #25457: URL: https://github.com/apache/datafusion/issues/25457
### Describe the bug `x BETWEEN low AND high` evaluates `x` two times. When `x` is volatile, the two evaluations give two different values, and the query returns the wrong rows. ### To Reproduce ```sql CREATE TABLE v AS SELECT value AS a FROM generate_series(1, 100000); SELECT count(*) FROM v WHERE random() BETWEEN 0.4 AND 0.6; ``` Result on `main` at 3a647e49dd: ```text +-------+ | count | +-------+ | 35751 | +-------+ ``` One draw of `random()` keeps the rows with `0.4 <= r <= 0.6`, which is about 20000 of 100000. The query keeps about 36000, which is `P(r1 >= 0.4) * P(r2 <= 0.6) = 0.6 * 0.6`. So each row draws two values. The plan shows the duplication: ```text > EXPLAIN SELECT count(*) FROM v WHERE random() BETWEEN 0.4 AND 0.6; logical_plan 01)Projection: count(Int64(1)) AS count(*) 02)--Aggregate: groupBy=[[]], aggr=[[count(Int64(1))]] 03)----Filter: random() >= Float64(0.4) AND random() <= Float64(0.6) 04)------TableScan: v projection=[] physical_plan 05)--------FilterExec: random() >= 0.4 AND random() <= 0.6 ``` ### Expected behavior `random() BETWEEN 0.4 AND 0.6` keeps about 20000 of the 100000 rows. `x` is evaluated one time per row. ### Additional context There are two places that expand the operand: - `simplify_expressions` rewrites `Expr::Between` into `a >= low AND a <= high` and clones `between.expr`. See [`expr_simplifier.rs:1700`](https://github.com/apache/datafusion/blob/main/datafusion/optimizer/src/simplify_expressions/expr_simplifier.rs#L1700). - the physical planner does the same rewrite for any `Expr::Between` that reaches it. See [`planner.rs:446`](https://github.com/apache/datafusion/blob/main/datafusion/physical-expr/src/planner.rs#L446). It reuses one `Arc<dyn PhysicalExpr>` for both sides, but `evaluate` is still called two times, so a volatile function still draws two values. A fix has to cover both places. One option is to keep the operand in a single node, for example a dedicated `BetweenExpr` physical expression, or to let the two comparisons share one evaluated array. The same duplication makes an expensive operand run two times. `SELECT abs(c1) BETWEEN 0 AND log(c1 * 100) FROM t` calls `abs` two times per row. That is a cost problem, not a correctness problem. This was found with a plan invariant that counts the evaluation sites of volatile and `KeepInPlace` calls before and after each optimizer rule. Related guards against the same class: https://github.com/apache/datafusion/issues/24678, https://github.com/apache/datafusion/issues/25415. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
