You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "adriangb (via GitHub)" <gi...@apache.org> on 2026/09/18 13:47:58 UTC
[PR] fix: evaluate a volatile `BETWEEN` value one time [datafusion]
adriangb opened a new pull request, #25476:
URL: https://github.com/apache/datafusion/pull/25476
## Which issue does this PR close?
- Closes https://github.com/apache/datafusion/issues/25457.
Part of the leaf-pushdown EPIC: https://github.com/apache/datafusion/issues/25459
## Rationale for this change
`x BETWEEN low AND high` evaluated `x` two times. A volatile `x` gives a different value each evaluation, so the query returned the wrong rows.
```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;
```
Observed on `main` at `0e292dcbfd`:
```text
+----------+
| count(*) |
+----------+
| 36022 |
+----------+
```
Expected about 20000. One draw of `random()` keeps the rows with `0.4 <= r <= 0.6`. The observed 36000 is `P(r1 >= 0.4) * P(r2 <= 0.6)`, which is `0.6 * 0.6`. So each row drew two values.
The plan on `main` shows the duplication:
```text
> EXPLAIN SELECT count(*) FROM v WHERE random() BETWEEN 0.4 AND 0.6;
logical_plan
03)----Filter: random() >= Float64(0.4) AND random() <= Float64(0.6)
physical_plan
05)--------FilterExec: random() >= 0.4 AND random() <= 0.6
```
With this PR:
```text
+----------+
| count(*) |
+----------+
| 20129 |
+----------+
> EXPLAIN SELECT count(*) FROM v WHERE random() BETWEEN 0.4 AND 0.6;
logical_plan
03)----Filter: random() BETWEEN Float64(0.4) AND Float64(0.6)
physical_plan
05)--------FilterExec: random() BETWEEN 0.4 AND 0.6
```
`NOT BETWEEN` had the same fault. It returned about 64000 rows and now returns about 80000.
PostgreSQL evaluates `random() BETWEEN 0.4 AND 0.6` one time. This is unverified here, because no PostgreSQL server was available.
### Where the value was duplicated
Two places expanded `BETWEEN` and each one cloned the value:
1. `SimplifyExpressions` rewrote `Expr::Between` into `a >= low AND a <= high` in `datafusion/optimizer/src/simplify_expressions/expr_simplifier.rs`.
2. The physical planner did the same rewrite for any `Expr::Between` that reached it, in `datafusion/physical-expr/src/planner.rs`. It reused one `Arc<dyn PhysicalExpr>` for both sides, but `evaluate` still ran two times.
### Other shapes that were checked
These shapes were checked with a volatile operand. Only `COALESCE` duplicates the operand in the final physical plan. The other shapes evaluate the operand one time.
| Shape | Duplicates a volatile operand |
| --- | --- |
| `x BETWEEN low AND high` | yes, fixed here |
| `x NOT BETWEEN low AND high` | yes, fixed here |
| `COALESCE(x, y)` | yes, see the follow-up below |
| `x IN (a, b, c)` | no, `InListExpr` holds one value expression |
| `CASE x WHEN a THEN ... END` | no, `CaseExpr` evaluates the base one time |
| `x IS DISTINCT FROM y` | no, a plain binary comparison |
| `nullif(x, y)`, `greatest(x, y)` | no, scalar function arguments |
## What changes are included in this PR?
Three commits.
1. A new `BetweenExpr` physical expression in `datafusion/physical-expr/src/expressions/between.rs`. It evaluates the value one time and compares that single result against both bounds. It also gets protobuf support, so a plan that holds it still serializes.
2. `SimplifyExpressions` keeps `BETWEEN` together when the value is volatile, and `create_physical_expr` lowers that shape to `BetweenExpr`.
3. The tests.
### Why this approach
Three options were considered.
- Keep `BETWEEN` unexpanded for a volatile value and give the physical planner a node that evaluates the value one time. This is the option in this PR.
- Rewrite the volatile shape into some other expression that already evaluates one time. The expression language has no such form without a projection.
- Add a pass that hoists a repeated volatile subexpression into a projection column, like `CommonSubexprEliminate` does for other expressions. That pass skips volatile expressions on purpose, because it cannot tell one syntactic occurrence from another. Changing it is a much larger job and it does not remove the duplication that the physical planner adds on its own.
A value that is not volatile keeps the rewrite into two comparisons. The optimizer, the interval analysis and the pruning predicates all understand plain binary comparisons but not `BetweenExpr`. `create_physical_expr` is also public, and a caller that builds a filter for pruning without running the logical optimizer must keep the form that pruning understands. So the plans for every `BETWEEN` that is not volatile are unchanged.
### New public API
`BetweenExpr` and the `between` builder in `datafusion_physical_expr::expressions`, plus the `PhysicalBetweenNode` protobuf message. A physical expression type is unavoidable here, because nothing in the physical expression set evaluates one value and uses it two times. The type is kept minimal: four accessors, `evaluate`, and the protobuf hooks.
## What is the testing strategy for this PR?
- Unit tests in `datafusion/physical-expr/src/expressions/between.rs` for inclusive bounds, `NOT BETWEEN`, nulls, a scalar value, and the display forms.
- A protobuf roundtrip test in `datafusion/proto/tests/cases/plans/filters.rs`.
- `between_evaluates_a_volatile_value_one_time_per_row` in `datafusion/core/tests/user_defined/user_defined_scalar_functions.rs`. A volatile UDF counts the rows it produces values for. Over 100 rows the count must be 100. Without the fix the test fails with `evaluated the volatile value 200 times for 100 rows`.
- sqllogictest cases in `datafusion/sqllogictest/test_files/expr.slt`. The row count of `random() BETWEEN 0.4 AND 0.6` over 100000 rows must be between 18000 and 22000. Two evaluations give about 36000, so the bound separates the two behaviours. `NOT BETWEEN` must be between 78000 and 82000. Two `EXPLAIN` cases pin the plan of a volatile `BETWEEN` and of a plain `BETWEEN`.
Commands and results:
```text
cargo test --profile ci -p datafusion-physical-expr -p datafusion-optimizer -p datafusion-proto -p datafusion-proto-models
882, 26, 1755, 17, 264, 6, 5, 13, 4 passed; 0 failed
cargo test --profile ci -p datafusion --test user_defined_integration --test core_integration
1170 passed; 0 failed
91 passed; 0 failed
cargo test --profile ci -p datafusion-sqllogictest --test sqllogictests
520/520 files completed, 0 failures
cargo clippy --profile ci --all-targets --workspace --features "avro,integration-tests,extended_tests" -- -D warnings
clean
```
No snapshot outside the new `expr.slt` block changed.
## Follow-ups
`COALESCE` has the same fault through a different rewrite. `coalesce.rs::simplify` turns `coalesce(a, b)` into `CASE WHEN a IS NOT NULL THEN a ELSE b END`, which names `a` two times. The fix is not the same, because `coalesce` has no runtime kernel. `invoke_with_args` returns `internal_err!("coalesce should have been simplified to case")`, so the rewrite cannot simply be skipped.
Reproduction on `main` at `0e292dcbfd`:
```sql
CREATE TABLE v AS SELECT value AS a FROM generate_series(1, 100000);
SELECT count(*), count(c)
FROM (SELECT coalesce(nullif(floor(random() * 2), 0), -1) AS c FROM v);
```
```text
Arrow error: Invalid argument error: Column 'c' is declared as non-nullable but contains null values
```
`coalesce` returns a non-null value when its last argument is a non-null literal, so the column is declared non-nullable. The null test and the returned value are two different draws, so the result can be null.
## Are there any user-facing changes?
Yes. `BETWEEN` and `NOT BETWEEN` over a volatile value now return the right rows. The `EXPLAIN` output for that shape shows `BETWEEN` instead of two comparisons. `BetweenExpr` and the `between` builder are new public API.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: evaluate a volatile `BETWEEN` value one time [datafusion]
Posted by "jayzhan211 (via GitHub)" <gi...@apache.org>.
jayzhan211 commented on PR #25476:
URL: https://github.com/apache/datafusion/pull/25476#issuecomment-5738863331
@adriangb One small concern: is it worth adding this complexity for a kind of query that probably isn't common in production? What do you think 🤔
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: evaluate a volatile `BETWEEN` value one time [datafusion]
Posted by "adriangb (via GitHub)" <gi...@apache.org>.
adriangb commented on PR #25476:
URL: https://github.com/apache/datafusion/pull/25476#issuecomment-6511658020
Agreed, closing
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: evaluate a volatile `BETWEEN` value one time [datafusion]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #25476:
URL: https://github.com/apache/datafusion/pull/25476#issuecomment-5731332579
## [Codecov](https://app.codecov.io/gh/apache/datafusion/pull/25476?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:x: Patch coverage is `43.18937%` with `171 lines` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 82.32%. Comparing base ([`0e292dc`](https://app.codecov.io/gh/apache/datafusion/commit/0e292dcbfdabc41a1297c57df9a1e0368b53e64d?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`536e5a6`](https://app.codecov.io/gh/apache/datafusion/commit/536e5a6979ef7fc5418405c3cfe72aa0c58e3136?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 2 commits behind head on main.
| [Files with missing lines](https://app.codecov.io/gh/apache/datafusion/pull/25476?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [datafusion/proto-models/src/generated/pbjson.rs](https://app.codecov.io/gh/apache/datafusion/pull/25476?src=pr&el=tree&filepath=datafusion%2Fproto-models%2Fsrc%2Fgenerated%2Fpbjson.rs&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-ZGF0YWZ1c2lvbi9wcm90by1tb2RlbHMvc3JjL2dlbmVyYXRlZC9wYmpzb24ucnM=) | 0.00% | [96 Missing :warning: ](https://app.codecov.io/gh/apache/datafusion/pull/25476?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...atafusion/physical-expr/src/expressions/between.rs](https://app.codecov.io/gh/apache/datafusion/pull/25476?src=pr&el=tree&filepath=datafusion%2Fphysical-expr%2Fsrc%2Fexpressions%2Fbetween.rs&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-ZGF0YWZ1c2lvbi9waHlzaWNhbC1leHByL3NyYy9leHByZXNzaW9ucy9iZXR3ZWVuLnJz) | 63.36% | [40 Missing and 34 partials :warning: ](https://app.codecov.io/gh/apache/datafusion/pull/25476?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [datafusion/proto/src/physical\_plan/from\_proto.rs](https://app.codecov.io/gh/apache/datafusion/pull/25476?src=pr&el=tree&filepath=datafusion%2Fproto%2Fsrc%2Fphysical_plan%2Ffrom_proto.rs&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-ZGF0YWZ1c2lvbi9wcm90by9zcmMvcGh5c2ljYWxfcGxhbi9mcm9tX3Byb3RvLnJz) | 0.00% | [0 Missing and 1 partial :warning: ](https://app.codecov.io/gh/apache/datafusion/pull/25476?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## main #25476 +/- ##
==========================================
- Coverage 82.35% 82.32% -0.03%
==========================================
Files 1137 1138 +1
Lines 432746 433047 +301
Branches 432746 433047 +301
==========================================
+ Hits 356375 356493 +118
- Misses 54843 54988 +145
- Partials 21528 21566 +38
```
</details>
[:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/datafusion/pull/25476?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: evaluate a volatile `BETWEEN` value one time [datafusion]
Posted by "adriangb (via GitHub)" <gi...@apache.org>.
adriangb commented on PR #25476:
URL: https://github.com/apache/datafusion/pull/25476#issuecomment-6462419679
Hmm @jayzhan211 I'm not sure. I don't see a simpler way of solving the problem. But should we solve it is a valid question. Maybe lets check what Postgres and DuckDB do?
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: evaluate a volatile `BETWEEN` value one time [datafusion]
Posted by "jayzhan211 (via GitHub)" <gi...@apache.org>.
jayzhan211 commented on PR #25476:
URL: https://github.com/apache/datafusion/pull/25476#issuecomment-6496900283
@adriangb I checked both. Postgres has the same behavior as DataFusion today: it expands `BETWEEN` in the parser and evaluates the value twice (35953 of 100000 rows for your query on 14.17). The source has known about it for years, with an "XXX this is really wrong since it results in multiple runtime evaluations of what may be volatile expressions" comment in `transformAExprBetween` that is still on master. DuckDB evaluates it once (19938), but that comes from its general `BETWEEN` node for any value that isn't a plain column, not from a volatile special case.
Since Postgres has accepted this for many years and the case is rare, I think it's fine to leave it as it is. Anyone who needs one evaluation can use a subquery, e.g. `SELECT ... FROM (SELECT random() AS r ...) WHERE r BETWEEN 0.4 AND 0.6`, which already evaluates `random()` once. Would you be okay closing this PR and the issue?
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: evaluate a volatile `BETWEEN` value one time [datafusion]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #25476:
URL: https://github.com/apache/datafusion/pull/25476#issuecomment-5731072327
<!-- semver-check-comment -->
Thank you for opening this pull request!
Reviewer note: [cargo-semver-checks](https://github.com/obi1kenobi/cargo-semver-checks) reported the current version number is not SemVer-compatible with the changes in this pull request (compared against the base branch).
<details>
<summary>Details</summary>
```
Cloning apache/main
Building datafusion v55.1.0 (current)
Built [ 60.924s] (current)
Parsing datafusion v55.1.0 (current)
Parsed [ 0.031s] (current)
Building datafusion v55.1.0 (baseline)
Built [ 50.861s] (baseline)
Parsing datafusion v55.1.0 (baseline)
Parsed [ 0.031s] (baseline)
Checking datafusion v55.1.0 -> v55.1.0 (no change; assume patch)
Checked [ 0.748s] 223 checks: 223 pass, 31 skip
Summary no semver update required
Finished [ 114.896s] datafusion
Building datafusion-optimizer v55.1.0 (current)
Built [ 22.451s] (current)
Parsing datafusion-optimizer v55.1.0 (current)
Parsed [ 0.028s] (current)
Building datafusion-optimizer v55.1.0 (baseline)
Built [ 23.521s] (baseline)
Parsing datafusion-optimizer v55.1.0 (baseline)
Parsed [ 0.029s] (baseline)
Checking datafusion-optimizer v55.1.0 -> v55.1.0 (no change; assume patch)
Checked [ 0.181s] 223 checks: 223 pass, 31 skip
Summary no semver update required
Finished [ 47.019s] datafusion-optimizer
Building datafusion-physical-expr v55.1.0 (current)
Built [ 25.140s] (current)
Parsing datafusion-physical-expr v55.1.0 (current)
Parsed [ 0.042s] (current)
Building datafusion-physical-expr v55.1.0 (baseline)
Built [ 25.433s] (baseline)
Parsing datafusion-physical-expr v55.1.0 (baseline)
Parsed [ 0.043s] (baseline)
Checking datafusion-physical-expr v55.1.0 -> v55.1.0 (no change; assume patch)
Checked [ 0.406s] 223 checks: 223 pass, 31 skip
Summary no semver update required
Finished [ 52.603s] datafusion-physical-expr
Building datafusion-proto v55.1.0 (current)
Built [ 48.585s] (current)
Parsing datafusion-proto v55.1.0 (current)
Parsed [ 0.015s] (current)
Building datafusion-proto v55.1.0 (baseline)
Built [ 47.210s] (baseline)
Parsing datafusion-proto v55.1.0 (baseline)
Parsed [ 0.015s] (baseline)
Checking datafusion-proto v55.1.0 -> v55.1.0 (no change; assume patch)
Checked [ 0.117s] 223 checks: 223 pass, 31 skip
Summary no semver update required
Finished [ 97.337s] datafusion-proto
Building datafusion-proto-models v55.1.0 (current)
Built [ 21.554s] (current)
Parsing datafusion-proto-models v55.1.0 (current)
Parsed [ 0.113s] (current)
Building datafusion-proto-models v55.1.0 (baseline)
Built [ 21.650s] (baseline)
Parsing datafusion-proto-models v55.1.0 (baseline)
Parsed [ 0.116s] (baseline)
Checking datafusion-proto-models v55.1.0 -> v55.1.0 (no change; assume patch)
Checked [ 2.078s] 223 checks: 222 pass, 1 fail, 0 warn, 31 skip
--- failure enum_variant_added: enum variant added on exhaustive enum ---
Description:
A publicly-visible enum without #[non_exhaustive] has a new variant.
ref: https://doc.rust-lang.org/cargo/reference/semver.html#enum-variant-new
impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.50.0/src/lints/enum_variant_added.ron
Failed in:
variant ExprType:Between in /home/runner/work/datafusion/datafusion/datafusion/proto-models/src/generated/prost.rs:1686
variant ExprType:Between in /home/runner/work/datafusion/datafusion/datafusion/proto-models/src/generated/prost.rs:1686
Summary semver requires new major version: 1 major and 0 minor checks failed
Finished [ 46.590s] datafusion-proto-models
Building datafusion-sqllogictest v55.1.0 (current)
Built [ 84.456s] (current)
Parsing datafusion-sqllogictest v55.1.0 (current)
Parsed [ 0.018s] (current)
Building datafusion-sqllogictest v55.1.0 (baseline)
Built [ 82.664s] (baseline)
Parsing datafusion-sqllogictest v55.1.0 (baseline)
Parsed [ 0.020s] (baseline)
Checking datafusion-sqllogictest v55.1.0 -> v55.1.0 (no change; assume patch)
Checked [ 0.096s] 223 checks: 223 pass, 31 skip
Summary no semver update required
Finished [ 169.447s] datafusion-sqllogictest
```
</details>
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: evaluate a volatile `BETWEEN` value one time [datafusion]
Posted by "adriangb (via GitHub)" <gi...@apache.org>.
adriangb closed pull request #25476: fix: evaluate a volatile `BETWEEN` value one time
URL: https://github.com/apache/datafusion/pull/25476
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org