Regression: `sql_planner` benchmark panic'ing on main

[alamb]

The bug is that

cargo bench --profile dev --bench sql_planner -- physical_plan_tpcds_all

Panic's on main like this:

thread 'main' panicked at datafusion/core/benches/sql_planner.rs:64:14:
called `Result::unwrap()` on an `Err` value: Internal("Physical input schema should be the same as the one converted from logical input schema. Differences: \n\t- field nullability at index 5 [sales_cnt]: (physical) true vs (logical) false\n\t- field nullability at index 6 [sales_amt]: (physical) true vs (logical) false")
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

error: bench failed, to rerun pass `-p datafusion --bench sql_planner`

That is somewhat bad that all the tests pass too 🤔

I expect that the benchmarks are not panicing

Originally posted by @alamb in #17419 (comment)

[alamb]

I ran this on the most recent commit on branch-50 (DataFusion 50.0.0) and it works

I am now running git bisect to find the bad commit

[pepijnve]

I ran cargo bench --profile dev --bench sql_planner -- physical_plan_tpcds_all locally this morning. No panic on 0d60ccae40d0e8f2d22c15fafb01c5d4be8c63a6.

[pepijnve]

🤦‍♂️ internal clone main != upstream main. Running again on 5bbdb7eb114d19e9a45f993af4ab2d2535c0cfbc.

Edit: Double 🤦‍♂️. I shouldn't do this kind of work before my first cup of coffee of the day. First commit hash was even not from the main repo, but from one of the testing submodules. Anyway, bisecting now and will report back here.

[pepijnve]

Bisect points to e5dcc8c

[alamb]

You beat me to it:

(venv) andrewlamb@Andrews-MacBook-Pro-3:~/Software/datafusion$ git bisect bad
e5dcc8c04f9559f8af6efea3c7ff8202f3c1c618 is the first bad commit
commit e5dcc8c04f9559f8af6efea3c7ff8202f3c1c618 (HEAD)
Author: Chen Chongchen <chenkovsky@qq.com>
Date:   Mon Sep 8 18:47:03 2025 +0800

    fix: lazy evaluation for coalesce (#17357)

    * fix: lazy evaluation for coalesce

    * update test

 datafusion/functions/src/core/coalesce.rs     | 88 +++++++++++++++++++++++++++++++---------------------------------------------------------
 datafusion/sqllogictest/test_files/select.slt | 16 +++++++++++-----
 2 files changed, 42 insertions(+), 62 deletions(-)

[pepijnve]

I’ll try to understand what was broken in this change later today.

[2010YOUY01]

Should we test benchmarks in the CI extended test?

[pepijnve]

@2010YOUY01 it would be a good idea to get the extra code coverage in CI, but the runtime is quite long (at least on my laptop). Is there a way to globally get criterion to run fewer iterations? Would be great to just run the bench code for correctness and not for performance measurement.

[pepijnve]

Short update on my investigation so far.

The query that's causing the failure is TPC-DS query 75. This should actually already be covered by the tcpds_planning.rs tests.

In its current state, the test passes though. If I modify the test as follows then it fails with the same error. This was rather surprising since the first thing SessionState::create_physical_plan does is call SessionState::optimize. So I modified create_physical_plan to call optimize twice in a row and then, lo and behold, the test passes again. So it looks like the implicit second optimisation pass is hiding the problematic from the test case.

I ran the entire tpcds_planning suite with this patch. Only query 75 planning fails.

diff --git a/datafusion/core/tests/tpcds_planning.rs b/datafusion/core/tests/tpcds_planning.rs
--- a/datafusion/core/tests/tpcds_planning.rs	(revision e5dcc8c04f9559f8af6efea3c7ff8202f3c1c618)
+++ b/datafusion/core/tests/tpcds_planning.rs	(date 1759050028279)
@@ -1052,9 +1052,10 @@
     for sql in &sql {
         let df = ctx.sql(sql).await?;
         let (state, plan) = df.into_parts();
-        let plan = state.optimize(&plan)?;
         if create_physical {
             let _ = state.create_physical_plan(&plan).await?;
+        } else {
+            let _ = state.optimize(&plan)?;
         }
     }
 

[pepijnve]

There seem to be two things going on here.

First, the expression simplification introduced in the commit we're looking at changes nullability in the schema. For the coalesce we can easily derive that the results is not nullable if at least one of the expressions is not nullable. For the case rewrite result this is feasible in theory, but the necessary analysis to conclude that CASE WHEN x IS NOT NULL THEN x ELSE y END is not nullable if y is not nullable does not seem to be implemented yet.

Secondly, at the logical level something seems to be going wrong at the schema layer. After parsing the SQL, the relevant portion of the logical plan is

Union [..., sales_cnt:Int64, ...]
                    Projection: ..., CAST(catalog_sales.cs_quantity AS Int64) - CASE WHEN __common_expr_7 IS NOT NULL THEN __common_expr_7 ELSE Int64(0) END AS sales_cnt, ..., sales_cnt:Int64;N, ...]

Note that sales_cnt is marked nullable in the projection, but not nullable in the union. My suspicion is that after the expression simplification the schema is not being updated correctly.

After a second optimisation pass we get

Union [..., sales_cnt:Int64;N, ...]
                    Projection: ..., CAST(catalog_sales.cs_quantity AS Int64) - CASE WHEN __common_expr_7 IS NOT NULL THEN __common_expr_7 ELSE Int64(0) END AS sales_cnt, ..., sales_cnt:Int64;N, ...]

In other words, the schema error seems to be getting resolved as a side effect of doing another rewrite pass over the tree.

[pepijnve]

To confirm my hunch I hacked together a quick check that I added to the expression simplifier and that confirms

if transformed.transformed {
    let is_nullable = self.info.nullable(&transformed.data)?;
    if is_nullable != was_nullable {
        return plan_err!(
            "Nullability changed from {} to {} when simplifying {} to {}",
            was_nullable,
            is_nullable,
            original,
            transformed.data
        );
    }
}

results in

Error: Context("Optimizer rule 'simplify_expressions' failed", Plan("Nullability changed from false to true when simplifying coalesce(CAST(catalog_returns.cr_return_quantity AS Int64), Int64(0)) to CASE WHEN CAST(catalog_returns.cr_return_quantity AS Int64) IS NOT NULL THEN CAST(catalog_returns.cr_return_quantity AS Int64) ELSE Int64(0) END"))

Is that supposed to be allowed? In other words, should the simplifier be handling this case or not?

[pepijnve]

Is that supposed to be allowed? In other words, should the simplifier be handling this case or not?

Looking at LogicalPlan::map_expressions I would say no. The code there assumes the original schema can be used for the mapped version.