fix: evaluate a volatile BETWEEN value one time

[adriangb]

Which issue does this PR close?

• Closes BETWEEN evaluates its operand two times, so a volatile operand returns wrong rows #25457.

Part of the leaf-pushdown EPIC: #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.

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:

+----------+
| 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:

> 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:

+----------+
| 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:

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:

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);

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

[github-actions]

Thank you for opening this pull request!

Reviewer note: 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

     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

[codecov-commenter]

Codecov Report

❌ Patch coverage is 43.18937% with 171 lines in your changes missing coverage. Please review.
✅ Project coverage is 82.32%. Comparing base (0e292dc) to head (536e5a6).
⚠️ Report is 2 commits behind head on main.

Files with missing lines	Patch %	Lines
datafusion/proto-models/src/generated/pbjson.rs	0.00%	96 Missing ⚠️
...atafusion/physical-expr/src/expressions/between.rs	63.36%	40 Missing and 34 partials ⚠️
datafusion/proto/src/physical_plan/from_proto.rs	0.00%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            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     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[jayzhan211]

@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 🤔