Regression: error planning TPC-DS query: input schema nullability mismatch

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

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

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

🤦‍♂️ 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.

Bisect points to e5dcc8c

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

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

Should we test benchmarks in the CI extended test?

@2010YOUY01

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

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

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.

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?

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.

The SQLite tests surfaced an implementation error related to case-with-expression nullability in #17813. I've fixed that and added some additional tests. I think the PR is in good enough shape to consider merging it. I was a bit reluctant at first to go with the logical strategy, but I'm starting to warm up to it. The code is not that complicated in the end, easy enough to cover with lots of unit tests, and (I think) an improvement over the current situation.

The SQLite tests surfaced an implementation error related to case-with-expression nullability in #17813.

Thank you -- I will review this carefully today

While reviewing this PR

#17801 Improve nullability reporting of case expressions #17813

I had another idea about how to preserve the improvements made to nullif but avoid this bug. Perhaps instead of simplifying to CASE entirely, we had the the physical function and instead called into the CaseExpr instead in the ExecutionPlan

Let me make a POC to see if that approach is viable

Let me make a POC to see if that approach is viable

The basic idea seems reasonable. However, i got stuck on the fact that the Case implementation is in datafusion_physical_expr which is not available in datafusion-functions 🤔

We could factor it into some common module (datafusion-case 🤔 ) but that is sounding like a much larger change

I made a PR that shows this is a real problem (not just a benchmark problem)

Avoid double optimizing in tpchds_planning tests to avoid masking errors #18536

I had another idea about how to preserve the improvements made to nullif but avoid this bug. Perhaps instead of simplifying to CASE entirely, we had the the physical function and instead called into the CaseExpr instead in the ExecutionPlan

I tried that avenue as well. The problems is that with ScalarUDF at the moment you only get eager evaluation. There's no way to achieve the lazy evaluation coalesce requires. #17997 was an attempt to try and make that possible.

(bringing some comments from #17813 (comment) here).

SInce in my mind this ticket is blocking the 51.0.0 release my plan is:

Revert the two relevant changes in the 51 branch to avoid the regression there
Continue with the real problems for 52

I was thinking about this one some more:

Avoid double optimizing in tpchds_planning tests to avoid masking errors #18536

The reason the issue was masked was a second simplification seems to have cleaned up the expresion
I looked briefly into the simplifier and I am not quite sure why that is. Here is some printouys of

When running

test --test tpcds_planning tpcds_physical_q75 -- --exact

With the code in https://github.com/apache/datafusion/pull/18536/files

https://github.com/apache/datafusion/blob/6852d502a8fac35a21efcb5a59b398208379fe0a/datafusion/optimizer/src/simplify_expressions/expr_simplifier.rs#L241-L240

AAL Starting simplification for expr: Alias(Alias { expr: BinaryExpr(BinaryExpr { left: Cast(Cast { expr: Column(Column { relation: Some(Bare { table: "catalog_sales" }), name: "cs_quantity" }), data_type: Int64 }), op: Minus, right: ScalarFunction(ScalarFunction { func: ScalarUDF { inner: CoalesceFunc { signature: Signature { type_signature: UserDefined, volatility: Immutable, parameter_names: None } } }, args: [Cast(Cast { expr: Column(Column { relation: Some(Bare { table: "catalog_returns" }), name: "cr_return_quantity" }), data_type: Int64 }), Literal(Int64(0), None)] }) }), relation: None, name: "sales_cnt", metadata: None })
AAL Simplifying cycle 0/3, has_transformed=false 'CAST(catalog_sales.cs_quantity AS Int64) - coalesce(CAST(catalog_returns.cr_return_quantity AS Int64), Int64(0)) AS sales_cnt', 
AAL Simplifying cycle 1/3, has_transformed=true 'CAST(catalog_sales.cs_quantity AS Int64) - 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 AS sales_cnt',

[branch-51] Revert rewrite for coalesce, `nvl` and `nvl2` simplificat…

#17801 Improve nullability reporting of case expressions (#17813)

apache#17801 Improve nullability reporting of case expressions (apach…

Navigation Menu

Repository navigation

Regression: error planning TPC-DS query: input schema nullability mismatch #17801

28 remaining items

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Regression: error planning TPC-DS query: input schema nullability mismatch #17801

Description

Activity

alamb commented on Sep 26, 2025

pepijnve commented on Sep 27, 2025

pepijnve commented on Sep 27, 2025

pepijnve commented on Sep 27, 2025

alamb commented on Sep 27, 2025

pepijnve commented on Sep 27, 2025

2010YOUY01 commented on Sep 28, 2025

pepijnve commented on Sep 28, 2025

pepijnve commented on Sep 28, 2025

pepijnve commented on Sep 28, 2025

pepijnve commented on Sep 28, 2025

pepijnve commented on Sep 28, 2025

28 remaining items

pepijnve commented on Nov 7, 2025

alamb commented on Nov 7, 2025

alamb commented on Nov 7, 2025

alamb commented on Nov 7, 2025

alamb commented on Nov 7, 2025

pepijnve commented on Nov 7, 2025

alamb commented on Nov 9, 2025

alamb commented on Nov 9, 2025

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions