#17801 Improve nullability reporting of case expressions #17813

pepijnve · 2025-09-28T15:02:38Z

Which issue does this PR close?

Closes Regression: error planning TPC-DS query: input schema nullability mismatch #17801
Obviates (contains) and thus Closes Revert "Disable failing benchmark query (#17809)" #17833
Obviates (contains) and thus Closes Avoid double optimizing in tpchds_planning tests to avoid masking errors #18536

Rationale for this change

#17357 introduced a change that replaces coalesce function calls with case expressions. In the current implementation these two differ in the way they report their nullability. coalesce is more precise than case all will report itself as not nullable in situations where the equivalent case does report being nullable.

The rest of the codebase currently does not expect the nullability property of an expression to change as a side effect of expression simplification. This PR is a first attempt to align the nullability of coalesce and case.

What changes are included in this PR?

Tweaks to the nullable logic for the logical and physical case expression code to report case as being not nullable in more situations.

For logical case, a best effort const evaluation of 'when' expressions is done to determine 'then' reachability. The code errs on the conservative side wrt nullability.
For physical case, const evaluation of 'when' expressions using a placeholder record batch is attempted to determine 'then' reachability. Again if const evaluation is not possible, the code errs on the conservative side.
The optimizer schema check has been relaxed slightly to allow nullability to be removed by optimizer passes without having to disable the schema check entirely
The panic'ing benchmark has been reenabled

Are these changes tested?

Additional unit tests have been added to test the new logic.

Are there any user-facing changes?

No

alamb

Thank you for this PR @pepijnve --

I am not quite sure about this implementation (I am hoping #17628 might solve the problem too with more sophisticated case folding)

However, I verified it does solve the problem with running the benchmarks so from that perspective I think we should proceed

My only real concern is that the newly added tests cover only the new code, and not the "end to end" behavior you tracked down (namely that the case pattern with coalesce changes the nullability).

Would it be possible to add some of the cases as expr simplification tests too? Somewhere like here?

datafusion/datafusion/optimizer/src/simplify_expressions/expr_simplifier.rs

Line 3881 in 247450d

#[test]

alamb · 2025-09-29T14:22:35Z

datafusion/expr/src/expr_schema.rs

+            when(binary_expr(col("foo"), Operator::Eq, lit(5)), col("foo"))
+                .otherwise(lit(0))?,


Minor: You can probably make this more concise using the eq method, something like this:

Suggested change

when(binary_expr(col("foo"), Operator::Eq, lit(5)), col("foo"))

.otherwise(lit(0))?,

when(col("foo").eq(lit(5))), col("foo")).otherwise(lit(0))?,

likewise there is Expr::and for ands that could be used as well below

However, the current setup of using and as a prefix is pretty clear too, so maybe what you have here is actually more readable.

Ah I missed that. I was looking for prefix versions, and hadn't realised infix ones existed too.

I ended up sticking with prefix notation for the boolean combinators and infix for the rest. Using infix for the boolean made it hard to read. I've also added the SQL equivalent as a comment.

alamb · 2025-09-29T14:28:43Z

datafusion/expr/src/expr_schema.rs

        assert!(expr.nullable(&get_schema(false)).unwrap());
    }

+    fn check_nullability(


I found this a little confusing at first, because it makes an explicit assumption that expr's will never introduce nulls (in order for !expr.nullable(&get_schema(false))?, to be true). So for example, it wouldn't do the right thing with the NULLIF function NULLIF(foo, 25) or something

Maybe some comments would help

Suggested change

fn check_nullability(

/// Verifies that `expr` has `nullable` nullability when the 'foo' column is

/// null.

/// Also assumes and verifies that `expr` is NOT nullable when 'foo' is NOT null

fn check_nullability(

I've reworked the logical plan test cases already to (hopefully) make it more obvious what's going on. I hadn't given this function much thought since it was only a test thing.

alamb · 2025-09-29T14:31:19Z

datafusion/expr/src/expr_schema.rs

+        check_nullability(
+            when(binary_expr(col("foo"), Operator::Eq, lit(5)), col("foo"))
+                .otherwise(lit(0))?,
+            true,


technically this could also be reported as false, given that if foo is null, then the expr resolves to 0 (non null)

> create table t(foo int) as values (0), (NULL), (5); 0 row(s) fetched. Elapsed 0.001 seconds. > select foo, CASE WHEN foo=5 THEN foo ELSE 0 END from t; +------+---------------------------------------------------------+ | foo | CASE WHEN t.foo = Int64(5) THEN t.foo ELSE Int64(0) END | +------+---------------------------------------------------------+ | 0 | 0 | | NULL | 0 | | 5 | 5 | +------+---------------------------------------------------------+ 3 row(s) fetched. Elapsed 0.002 seconds.

However, maybe we can improve that in a follow on PR

Agreed, the const evaluation is far from complete. I tried to do something good enough for the coalesce simplification initially.
I was wondering the whole time if there isn't some existing null analysis logic somewhere in the codebase we could reuse. The best I could come up with is rewriting the full expression by replacing the then expression with literal NULL and then attempting const evaluation. But that got me worrying about planning overhead again.

alamb · 2025-09-29T14:32:17Z

datafusion/expr/src/expr_schema.rs

+            when(
+                or(
+                    is_not_null(col("foo")),
+                    binary_expr(col("foo"), Operator::Eq, lit(5)),


as above, I don't think this expression can everr be true so this overall expression is still non nullable

alamb · 2025-09-29T14:34:33Z

datafusion/expr/src/expr_schema.rs

+                col("foo"),
+            )
+            .otherwise(lit(0))?,
+            true,


Here too -- this expression is not nullabile

> select foo, CASE WHEN foo=5 OR foo IS NOT NULL THEN foo ELSE 0 END from t; +------+------------------------------------------------------------------------------+ | foo | CASE WHEN t.foo = Int64(5) OR t.foo IS NOT NULL THEN t.foo ELSE Int64(0) END | +------+------------------------------------------------------------------------------+ | 0 | 0 | | NULL | 0 | | 5 | 5 | +------+------------------------------------------------------------------------------+ 3 row(s) fetched. Elapsed 0.002 seconds.

FWIW, if you comment out the filter step (i.e. revert to the pre-patch version) all of these cases are reported as being nullable. The scope of this PR is to get at least some cases that are definitely not nullable reported as such, not ensure all cases are reported correctly.

alamb · 2025-09-29T14:35:54Z

datafusion/expr/src/expr_schema.rs

+            .otherwise(lit(0))?,
+            true,
+            get_schema,
+        )?;


Can you also please add a check with is_null in the OR clause (which should be null)

Something like the equivalent to

> select foo, CASE WHEN foo=5 OR foo IS NULL THEN foo ELSE 0 END from t; +------+--------------------------------------------------------------------------+ | foo | CASE WHEN t.foo = Int64(5) OR t.foo IS NULL THEN t.foo ELSE Int64(0) END | +------+--------------------------------------------------------------------------+ | 0 | 0 | | NULL | NULL | | 5 | 5 | +------+--------------------------------------------------------------------------+ 3 row(s) fetched. Elapsed 0.000 seconds.

Like

check_nullability( when( or( binary_expr(col("foo"), Operator::Eq, lit(5)), is_null(col("foo")), ), col("foo"), ) .otherwise(lit(0))?, true, get_schema, )?;

I've added this test case

pepijnve · 2025-09-29T16:43:07Z

I am not quite sure about this implementation (I am hoping #17628 might solve the problem too with more sophisticated case folding)

I warned you it wasn't very elegant. 😄 I don't think #17628 covers the same thing though. What we're trying to do here is get ExprSchemable::nullable to report an accurate value outside of the optimiser. Ideally you would want that to work both before and after case folding.
I agree that there is a lot of overlap in the required logic though. If you were to take the case expression, replace the then expression trees with literal NULL everywhere they occur, then perform case folding, and then perform null analysis then you would get the same result.

My only real concern is that the newly added tests cover only the new code, and not the "end to end" behavior you tracked down (namely that the case pattern with coalesce changes the nullability).

Would it be possible to add some of the cases as expr simplification tests too? Somewhere like here?

I'm not sure what kind of test you have in mind. The end to end case is (admittedly very indirectly) covered by TPC-DS query 75 and the removal of the double optimisation. If you revert the production code change in this PR, but keep the test change you'll see that it fails.

For the simplifier itself, I was wondering if there shouldn't be some internal assertions that verifies that the result of calling ScalarUDF::simplify doesn't change key aspects of the expression that the schema also encodes. Both the data type and the nullability should not change since that causes the expression and the schema to be mismatched.

pepijnve · 2025-09-30T08:44:12Z

@alamb thinking about this a bit more. I'm going to struggle expressing myself sufficiently clearly here, but I'll try to explain the idea behind what I'm doing. Maybe that can help us figure out a better way to express the idea.

What I'm trying to do is improve the accuracy of the predicate is_nullable(expr) specifically for CASE expressions.
In the current code this predicate is implemented by checking (in pseudo code) case_expr.when_then.any?((when, then) -> is_nullable(then)) || is_nullable(case_expr.else). This results in quite a few CASE expressions being reported as nullable even though they're not.

In particular there's one interesting case (pun not intended) which results from the coalesce simplification and that is CASE WHEN x IS NOT NULL THEN x ELSE y. If the expression x is nullable, is_nullable will currently report the entire case expression as nullable. The implementation does not take into account that there is a guard clause preventing the null value of x from being returned.

What I attempted to do in this PR is to look at the more general form WHEN f(x) THEN x where f(x) is some arbitrary predicate that may or may not depend on the value of x. What the code is trying to do is to const evaluate f(NULL). If it can with 100% certainty (i.e. Some is returned) and the evaluated value is false, then predicate f guarantees this particular branch of the case expression from ever returning a NULL and we can ignore the nullability of x.

I tried to implement this in a cheap, but imprecise way. My rationale was that even though it's not perfect, it's an improvement in accuracy over the current code.
A possible alternative would be to rewrite the expression corresponding to f using the binding x = null and then attempting to const evaluate using the existing const evaluation code. That introduces a dependency from the logical expression module to the optimiser though and would probably have a longer run time than the current crude approximation.

pepijnve · 2025-09-30T14:35:02Z

I've massaged the logical plan version of the code a bit further already to hopefully clarify what it's doing. I then ran the test cases with logging output rather than assertions before and after the extra filtering to illustrate what's being changed. After the change all tests pass. Before the patch it reports the following

CASE WHEN x IS NOT NULL THEN x ELSE Int32(0) END nullable? should be false, but was true
CASE WHEN NOT x IS NULL THEN x ELSE Int32(0) END nullable? should be false, but was true
CASE WHEN x IS NOT NULL AND x = Int32(5) THEN x ELSE Int32(0) END nullable? should be false, but was true
CASE WHEN x = Int32(5) AND x IS NOT NULL THEN x ELSE Int32(0) END nullable? should be false, but was true
CASE WHEN x = Int32(5) AND x IS NOT NULL OR x = bar AND x IS NOT NULL THEN x ELSE Int32(0) END nullable? should be false, but was true

pepijnve · 2025-09-30T18:30:53Z

@alamb I've taken the logical expression portion of the PR another step further which ensures correct answers for the expressions you mentioned earlier. I can complete the physical expression portion as well if you like. Unless you tell me this path is a dead end.

alamb · 2025-09-30T18:54:27Z

@alamb I've taken the logical expression portion of the PR another step further which ensures correct answers for the expressions you mentioned earlier. I can complete the physical expression portion as well if you like. Unless you tell me this path is a dead end.

Thank you -- I will try and get to this one asap. Somehow every time i think I am getting the queue of reviews under control there are like 50 new notifications ! It is a good problem to have.

pepijnve · 2025-09-30T19:18:06Z

Thank you -- I will try and get to this one asap. Somehow every time i think I am getting the queue of reviews under control there are like 50 new notifications ! It is a good problem to have.

No pressure from my side. I just write up my notes and move on to the next thing. Async delayed response is fine.

pepijnve · 2025-10-01T14:20:47Z

I experimented a bit with the rewrite + const eval approach on the physical expression side of things. While attractive and simple to implement, the downside is that it's going to be very hard to ensure the logical and physical side agree. Logical needs to work without ExecutionProps so it has less information available to it compared to the PhysicalExpr tree. I don't see a way to resolve that. As a consequence I ended up with an limited ad hoc version of const evaluation on the logical side and would have to do the same for physical which isn't really ideal from a DRY perspective.

alamb · 2025-10-06T15:25:59Z

Than you -- this is on my list of things to review shortly

…ates

alamb

Thank you @pepijnve -- this is looking so close -- I think we should roll back some of the GuaranteeRewriter changes to avoid API churn. If you could break them out into their own PR I think that would be good and I could review them quickly

alamb · 2025-11-19T13:30:30Z

datafusion/expr-common/src/interval_arithmetic.rs

+    /// ```text
+    /// A ∧ B │ F U T
+    /// ──────┼──────
+    ///     F │ F F F


this truth table seems to be missing the values for A (self)

That's the left column. Let me see if I can figure out a compact way to clarify that.

These tables were based on https://en.wikipedia.org/wiki/Three-valued_logic#Kleene_and_Priest_logics

I've tweaked them a bit further to resemble those as closely as possible.

alamb · 2025-11-19T13:30:41Z

datafusion/expr-common/src/interval_arithmetic.rs

+    /// This method uses the following truth table.
+    ///
+    /// ```text
+    /// A ∨ B │ F U T


likewise here this table is missing values for A

alamb · 2025-11-19T13:33:26Z

datafusion/optimizer/src/simplify_expressions/guarantees.rs

-/// See a full example in [`ExprSimplifier::with_guarantees()`].
-///
-/// [`ExprSimplifier::with_guarantees()`]: crate::simplify_expressions::expr_simplifier::ExprSimplifier::with_guarantees
-pub struct GuaranteeRewriter<'a> {


Unless there is a good reason, I think we should avoid removing this API as it will cause unecessary churn on downstream crates

If you find rewrite_with_guarantees easier to work with, maybe you leave GuaranteeRewriter and but implement rewrite_with_guarantees in terms of that

alamb · 2025-11-19T13:36:10Z

datafusion/expr/src/expr_rewriter/guarantees.rs

+use datafusion_common::{DataFusionError, HashMap, Result};
+use datafusion_expr_common::interval_arithmetic::{Interval, NullableInterval};
+
+struct GuaranteeRewriter<'a> {


I think the GuaranteeRewriter is part of the public API, so making this change would potentially cause breaking downstream changes: https://docs.rs/datafusion/latest/datafusion/optimizer/simplify_expressions/struct.GuaranteeRewriter.html

I think we should leave the GuaranteeRewriter API in place (w/ comments etc) and then make rewrite_with_guarantees a method or something

Perhaps

impl GuaranteeRewriter { /// Create new guarantees from an iterator pub fn new( guarantees: impl IntoIterator<Item = &'a (Expr, NullableInterval)>, ) /// Create new gurantees from a map pub fn new( guarantees: &'a HashMap<&'a Expr, &'a NullableInterval>, ) }

🤔

Agreed, it's a breaking change. It's already breaking simply because of the move from one crate to another unless we add a reexport from optimizer.

No objections to restoring public visibility of the struct though. I was just trying to follow the example/style of the order by rewrite sibling on the new module location.

alamb · 2025-11-19T13:39:58Z

datafusion/optimizer/src/simplify_expressions/guarantees.rs

 ///
-/// See a full example in [`ExprSimplifier::with_guarantees()`].
-///
-/// [`ExprSimplifier::with_guarantees()`]: crate::simplify_expressions::expr_simplifier::ExprSimplifier::with_guarantees


You probably removed this doc link b/c the code is now in a different module that doesn't depend on optimizer.

However I think the link still adds value

What we have done in other places where we can't rely on auto links is to use the direct HTML link: https://docs.rs/datafusion/latest/datafusion/optimizer/simplify_expressions/struct.ExprSimplifier.html#method.with_guarantees

Which isn't as good as rustdoc doesn't check that the links don't get broken, but I think it is better than just removing the link totally

Yep, that's why I removed it here. Will restore.

pepijnve · 2025-11-19T14:39:33Z

Thank you @pepijnve -- this is looking so close -- I think we should roll back some of the GuaranteeRewriter changes to avoid API churn. If you could break them out into their own PR I think that would be good and I could review them quickly

I'll make this a separate PR taking the comments you logged so far into account. It'll be easier to track that way.

pepijnve · 2025-11-19T15:14:41Z

@alamb guarantee stuff moved over to #18821

## Which issue does this PR close? - None, break out PR of changes done in #17813 ## Rationale for this change In #17813 `GuaranteeRewriter` is used from the `datafusion_expr` crate. In order to enable this the type needed to be moved from `datafusion_optimizer` to `datafusion_expr`. Additionally, during the development of #17813 some latent bugs were discovered in `GuaranteeRewriter` that have been resolved. ## What changes are included in this PR? - Move `GuaranteeRewriter` to `datafusion_expr` - Fix two bugs where rewrites of 'between' expression would fail - when one of the bounds was untyped null - when the lower bound was greater than the upper bound - Add logic to replace expressions with literal null based on provided guarantees - Split implementation into smaller functions for easier readability ## Are these changes tested? - Existing tests updated - Tests added for bugfixes ## Are there any user-facing changes? No --------- Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org>

# Conflicts: # datafusion/expr/src/expr_rewriter/guarantees.rs # datafusion/expr/src/expr_rewriter/mod.rs # datafusion/optimizer/src/simplify_expressions/mod.rs

pepijnve · 2025-11-19T22:05:26Z

The changes from #18821 have been merged into this PR from main. PR currently reflects only the changes relevant to the original problem description.

alamb · 2025-11-19T22:21:36Z

Thank you @pepijnve -- I plan to give this one a final review tomorrow morning and merge it in

This reverts commit 5cc0be5.

alamb

Looks great -- thank you (again) @pepijnve

I took the liberty of pushing the change to run the benchmark query again so we could really close #17833 and merging up from main

datafusion/core/tests/tpcds_planning.rs

alamb · 2025-11-20T12:00:34Z

datafusion/expr/src/expr_schema.rs

            Expr::Column(c) => input_schema.nullable(c),
            Expr::OuterReferenceColumn(field, _) => Ok(field.is_nullable()),
            Expr::Literal(value, _) => Ok(value.is_null()),
            Expr::Case(case) => {


While re-reading this I can't help but think the logic is quite non trivial - and someone trying to figure out if an expression is nullable on a deeply nested function might end up calling this function many times

Not for this PR, but I think we should consider how to cache or otherwise avoid re-computing the same nullabilty (and DataType) expressions over and over again.

I'll writeup a follow on ticket

That's absolutely correct. Performance overhead concerns were the main reason I had initially avoided rewriting the expression and instead tried to do the rewrite indirectly. Rather than rewriting using a NullableInterval::Null guarantee, I was checking this using a callback function.

It's probably feasible, but non-trivial to cache this result. What would you use as storage location?

See https://github.com/apache/datafusion/pull/17813/files#r2545958309. That already mitigates the additional calculations a little bit.

It's probably feasible, but non-trivial to cache this result. What would you use as storage location?

Yes, I agree it is non trivial. I wrote up some ideas in

Cache the result of Expr::data_type / Expr::nullable / Expr::to_field to speed up planning #18845

I started looking at the possible options here already a bit. I don't immediately see a simple solution.

alamb · 2025-11-20T12:07:02Z

🤖 ./gh_compare_branch_bench.sh Benchmark Script Running
Linux aal-dev 6.14.0-1018-gcp #19~24.04.1-Ubuntu SMP Wed Sep 24 23:23:09 UTC 2025 x86_64 x86_64 x86_64 GNU/Linux
Comparing issue_17801 (ff9a41f) to 7fa2a69 diff
BENCH_NAME=sql_planner
BENCH_COMMAND=cargo bench --bench sql_planner
BENCH_FILTER=
BENCH_BRANCH_NAME=issue_17801
Results will be posted here when complete

pepijnve · 2025-11-20T12:51:11Z

datafusion/expr/src/expr_schema.rs

+                            Some(Ok(()))
+                        }
+                    })
+                    .next();


The change from collect to next().is_some() does mitigate the performance overhead a little bit. As soon as one nullable branch is found the iteration will stop.

alamb · 2025-11-20T13:12:20Z

🤖: Benchmark completed

Details

group                                                 issue_17801                            main
-----                                                 -----------                            ----
logical_aggregate_with_join                           1.00    639.2±2.78µs        ? ?/sec    1.00    638.7±7.94µs        ? ?/sec
logical_select_all_from_1000                          1.02     11.1±0.07ms        ? ?/sec    1.00     11.0±0.04ms        ? ?/sec
logical_select_one_from_700                           1.01    422.1±2.17µs        ? ?/sec    1.00    419.9±1.94µs        ? ?/sec
logical_trivial_join_high_numbered_columns            1.00    379.3±1.42µs        ? ?/sec    1.00    377.6±1.71µs        ? ?/sec
logical_trivial_join_low_numbered_columns             1.01    366.1±1.47µs        ? ?/sec    1.00    363.8±1.98µs        ? ?/sec
physical_intersection                                 1.02    848.8±3.75µs        ? ?/sec    1.00    834.9±3.82µs        ? ?/sec
physical_join_consider_sort                           1.01   1409.6±4.86µs        ? ?/sec    1.00   1394.1±9.18µs        ? ?/sec
physical_join_distinct                                1.01    354.9±1.33µs        ? ?/sec    1.00    352.9±1.48µs        ? ?/sec
physical_many_self_joins                              1.01      9.8±0.04ms        ? ?/sec    1.00      9.7±0.09ms        ? ?/sec
physical_plan_clickbench_all                          1.00    182.8±1.77ms        ? ?/sec    1.00    182.6±1.25ms        ? ?/sec
physical_plan_clickbench_q1                           1.00      2.4±0.02ms        ? ?/sec    1.00      2.4±0.02ms        ? ?/sec
physical_plan_clickbench_q10                          1.00      3.2±0.02ms        ? ?/sec    1.00      3.2±0.03ms        ? ?/sec
physical_plan_clickbench_q11                          1.00      3.4±0.03ms        ? ?/sec    1.00      3.4±0.04ms        ? ?/sec
physical_plan_clickbench_q12                          1.00      3.5±0.03ms        ? ?/sec    1.05      3.7±0.37ms        ? ?/sec
physical_plan_clickbench_q13                          1.00      3.2±0.02ms        ? ?/sec    1.01      3.2±0.03ms        ? ?/sec
physical_plan_clickbench_q14                          1.00      3.4±0.03ms        ? ?/sec    1.00      3.4±0.05ms        ? ?/sec
physical_plan_clickbench_q15                          1.00      3.3±0.03ms        ? ?/sec    1.00      3.3±0.03ms        ? ?/sec
physical_plan_clickbench_q16                          1.00      3.1±0.03ms        ? ?/sec    1.00      3.1±0.02ms        ? ?/sec
physical_plan_clickbench_q17                          1.01      3.2±0.04ms        ? ?/sec    1.00      3.2±0.02ms        ? ?/sec
physical_plan_clickbench_q18                          1.01      2.7±0.02ms        ? ?/sec    1.00      2.7±0.01ms        ? ?/sec
physical_plan_clickbench_q19                          1.00      3.6±0.03ms        ? ?/sec    1.00      3.6±0.03ms        ? ?/sec
physical_plan_clickbench_q2                           1.00      2.7±0.02ms        ? ?/sec    1.03      2.8±0.09ms        ? ?/sec
physical_plan_clickbench_q20                          1.00      2.5±0.02ms        ? ?/sec    1.00      2.5±0.01ms        ? ?/sec
physical_plan_clickbench_q21                          1.00      2.8±0.02ms        ? ?/sec    1.00      2.8±0.02ms        ? ?/sec
physical_plan_clickbench_q22                          1.00      3.4±0.02ms        ? ?/sec    1.00      3.4±0.02ms        ? ?/sec
physical_plan_clickbench_q23                          1.01      3.7±0.03ms        ? ?/sec    1.00      3.6±0.03ms        ? ?/sec
physical_plan_clickbench_q24                          1.01      4.1±0.05ms        ? ?/sec    1.00      4.1±0.03ms        ? ?/sec
physical_plan_clickbench_q25                          1.00      2.9±0.03ms        ? ?/sec    1.00      2.9±0.02ms        ? ?/sec
physical_plan_clickbench_q26                          1.01      2.7±0.03ms        ? ?/sec    1.00      2.7±0.02ms        ? ?/sec
physical_plan_clickbench_q27                          1.00      3.0±0.03ms        ? ?/sec    1.00      3.0±0.02ms        ? ?/sec
physical_plan_clickbench_q28                          1.00      3.7±0.04ms        ? ?/sec    1.00      3.7±0.04ms        ? ?/sec
physical_plan_clickbench_q29                          1.00      4.0±0.03ms        ? ?/sec    1.00      4.0±0.04ms        ? ?/sec
physical_plan_clickbench_q3                           1.00      2.7±0.02ms        ? ?/sec    1.00      2.7±0.03ms        ? ?/sec
physical_plan_clickbench_q30                          1.01     14.9±0.15ms        ? ?/sec    1.00     14.8±0.12ms        ? ?/sec
physical_plan_clickbench_q31                          1.00      3.7±0.03ms        ? ?/sec    1.00      3.7±0.02ms        ? ?/sec
physical_plan_clickbench_q32                          1.00      3.7±0.03ms        ? ?/sec    1.00      3.7±0.03ms        ? ?/sec
physical_plan_clickbench_q33                          1.00      3.2±0.02ms        ? ?/sec    1.01      3.3±0.03ms        ? ?/sec
physical_plan_clickbench_q34                          1.00      2.9±0.02ms        ? ?/sec    1.02      3.0±0.18ms        ? ?/sec
physical_plan_clickbench_q35                          1.00      3.0±0.02ms        ? ?/sec    1.01      3.0±0.04ms        ? ?/sec
physical_plan_clickbench_q36                          1.00      3.7±0.03ms        ? ?/sec    1.00      3.7±0.03ms        ? ?/sec
physical_plan_clickbench_q37                          1.00      3.8±0.04ms        ? ?/sec    1.00      3.9±0.03ms        ? ?/sec
physical_plan_clickbench_q38                          1.00      3.8±0.04ms        ? ?/sec    1.00      3.8±0.05ms        ? ?/sec
physical_plan_clickbench_q39                          1.00      3.7±0.04ms        ? ?/sec    1.00      3.7±0.03ms        ? ?/sec
physical_plan_clickbench_q4                           1.01      2.4±0.02ms        ? ?/sec    1.00      2.4±0.01ms        ? ?/sec
physical_plan_clickbench_q40                          1.02      4.6±0.07ms        ? ?/sec    1.00      4.5±0.03ms        ? ?/sec
physical_plan_clickbench_q41                          1.01      3.9±0.04ms        ? ?/sec    1.00      3.9±0.05ms        ? ?/sec
physical_plan_clickbench_q42                          1.00      3.9±0.04ms        ? ?/sec    1.00      3.9±0.03ms        ? ?/sec
physical_plan_clickbench_q43                          1.00      4.2±0.05ms        ? ?/sec    1.00      4.3±0.03ms        ? ?/sec
physical_plan_clickbench_q44                          1.00      2.6±0.02ms        ? ?/sec    1.00      2.6±0.02ms        ? ?/sec
physical_plan_clickbench_q45                          1.00      2.6±0.02ms        ? ?/sec    1.01      2.6±0.02ms        ? ?/sec
physical_plan_clickbench_q46                          1.00      3.0±0.02ms        ? ?/sec    1.00      3.0±0.02ms        ? ?/sec
physical_plan_clickbench_q47                          1.00      3.6±0.03ms        ? ?/sec    1.01      3.6±0.04ms        ? ?/sec
physical_plan_clickbench_q48                          1.00      4.3±0.03ms        ? ?/sec    1.00      4.3±0.05ms        ? ?/sec
physical_plan_clickbench_q49                          1.00      4.6±0.04ms        ? ?/sec    1.00      4.6±0.03ms        ? ?/sec
physical_plan_clickbench_q5                           1.01      2.7±0.02ms        ? ?/sec    1.00      2.7±0.03ms        ? ?/sec
physical_plan_clickbench_q50                          1.02      4.2±0.12ms        ? ?/sec    1.00      4.1±0.03ms        ? ?/sec
physical_plan_clickbench_q51                          1.00      3.2±0.03ms        ? ?/sec    1.00      3.2±0.03ms        ? ?/sec
physical_plan_clickbench_q6                           1.00      2.7±0.02ms        ? ?/sec    1.00      2.7±0.04ms        ? ?/sec
physical_plan_clickbench_q7                           1.01      2.4±0.02ms        ? ?/sec    1.00      2.4±0.02ms        ? ?/sec
physical_plan_clickbench_q8                           1.00      3.2±0.03ms        ? ?/sec    1.01      3.3±0.03ms        ? ?/sec
physical_plan_clickbench_q9                           1.00      3.1±0.02ms        ? ?/sec    1.00      3.1±0.02ms        ? ?/sec
physical_plan_tpcds_all                               1.04   1080.1±6.37ms        ? ?/sec    1.00   1037.9±4.61ms        ? ?/sec
physical_plan_tpch_all                                1.01     65.0±0.25ms        ? ?/sec    1.00     64.4±0.30ms        ? ?/sec
physical_plan_tpch_q1                                 1.01      2.1±0.01ms        ? ?/sec    1.00      2.1±0.01ms        ? ?/sec
physical_plan_tpch_q10                                1.01      4.0±0.01ms        ? ?/sec    1.00      4.0±0.01ms        ? ?/sec
physical_plan_tpch_q11                                1.00      3.6±0.01ms        ? ?/sec    1.00      3.6±0.02ms        ? ?/sec
physical_plan_tpch_q12                                1.00  1839.3±10.93µs        ? ?/sec    1.00   1832.8±9.39µs        ? ?/sec
physical_plan_tpch_q13                                1.00   1450.0±7.49µs        ? ?/sec    1.00   1444.6±8.59µs        ? ?/sec
physical_plan_tpch_q14                                1.00      2.0±0.01ms        ? ?/sec    1.00      2.0±0.01ms        ? ?/sec
physical_plan_tpch_q16                                1.00      2.5±0.01ms        ? ?/sec    1.00      2.4±0.01ms        ? ?/sec
physical_plan_tpch_q17                                1.00      2.6±0.01ms        ? ?/sec    1.00      2.6±0.01ms        ? ?/sec
physical_plan_tpch_q18                                1.00      2.7±0.01ms        ? ?/sec    1.00      2.7±0.01ms        ? ?/sec
physical_plan_tpch_q19                                1.00      3.2±0.02ms        ? ?/sec    1.00      3.2±0.01ms        ? ?/sec
physical_plan_tpch_q2                                 1.01      5.9±0.05ms        ? ?/sec    1.00      5.9±0.02ms        ? ?/sec
physical_plan_tpch_q20                                1.01      3.2±0.01ms        ? ?/sec    1.00      3.2±0.02ms        ? ?/sec
physical_plan_tpch_q21                                1.01      4.2±0.04ms        ? ?/sec    1.00      4.1±0.01ms        ? ?/sec
physical_plan_tpch_q22                                1.00      2.9±0.01ms        ? ?/sec    1.00      2.9±0.02ms        ? ?/sec
physical_plan_tpch_q3                                 1.01      2.7±0.01ms        ? ?/sec    1.00      2.7±0.01ms        ? ?/sec
physical_plan_tpch_q4                                 1.00  1485.1±10.99µs        ? ?/sec    1.00  1482.4±15.79µs        ? ?/sec
physical_plan_tpch_q5                                 1.00      3.3±0.01ms        ? ?/sec    1.00      3.3±0.03ms        ? ?/sec
physical_plan_tpch_q6                                 1.00    874.2±6.76µs        ? ?/sec    1.00   876.6±11.93µs        ? ?/sec
physical_plan_tpch_q7                                 1.01      4.2±0.02ms        ? ?/sec    1.00      4.2±0.02ms        ? ?/sec
physical_plan_tpch_q8                                 1.01      5.5±0.02ms        ? ?/sec    1.00      5.5±0.03ms        ? ?/sec
physical_plan_tpch_q9                                 1.01      4.1±0.02ms        ? ?/sec    1.00      4.1±0.05ms        ? ?/sec
physical_select_aggregates_from_200                   1.00     16.9±0.08ms        ? ?/sec    1.00     16.8±0.07ms        ? ?/sec
physical_select_all_from_1000                         1.02     24.2±0.11ms        ? ?/sec    1.00     23.8±0.09ms        ? ?/sec
physical_select_one_from_700                          1.02  1106.9±10.45µs        ? ?/sec    1.00   1087.9±3.62µs        ? ?/sec
physical_sorted_union_order_by_10_int64               1.00      6.0±0.02ms        ? ?/sec    1.00      6.0±0.06ms        ? ?/sec
physical_sorted_union_order_by_10_uint64              1.01     12.9±0.07ms        ? ?/sec    1.00     12.8±0.05ms        ? ?/sec
physical_sorted_union_order_by_50_int64               1.00    162.3±0.91ms        ? ?/sec    1.00    162.5±1.04ms        ? ?/sec
physical_sorted_union_order_by_50_uint64              1.01    376.1±2.45ms        ? ?/sec    1.00    373.6±2.63ms        ? ?/sec
physical_theta_join_consider_sort                     1.01   1776.7±7.55µs        ? ?/sec    1.00  1754.5±17.61µs        ? ?/sec
physical_unnest_to_join                               1.00  1852.3±16.93µs        ? ?/sec    1.00  1843.8±12.45µs        ? ?/sec
physical_window_function_partition_by_12_on_values    1.00  1110.8±10.07µs        ? ?/sec    1.00  1110.9±13.58µs        ? ?/sec
physical_window_function_partition_by_30_on_values    1.00      2.3±0.01ms        ? ?/sec    1.00      2.3±0.01ms        ? ?/sec
physical_window_function_partition_by_4_on_values     1.02    682.4±4.17µs        ? ?/sec    1.00    670.8±4.37µs        ? ?/sec
physical_window_function_partition_by_7_on_values     1.01    834.0±5.28µs        ? ?/sec    1.00    827.6±5.78µs        ? ?/sec
physical_window_function_partition_by_8_on_values     1.01    893.6±8.56µs        ? ?/sec    1.00    886.8±5.06µs        ? ?/sec
with_param_values_many_columns                        1.01    654.3±5.30µs        ? ?/sec    1.00    650.4±3.87µs        ? ?/sec

pepijnve · 2025-11-20T13:40:56Z

@alamb the variant of the code that did not use GuaranteeRewrite is only a small delta wrt the state of this PR now. Shall I make a new branch with that variant (keeping everything else equal) so we can compare planning performance between the two?

I went ahead and made that change at pepijnve#2

alamb · 2025-11-20T14:42:48Z

Sounds good -- will run

alamb · 2025-11-20T15:36:17Z

Since this PR has been outstanding for so long and it fixes a bug and I desperately want to close it down (so we can move on) I am going to merge it as is.

@pepijnve would you be willing to make a real PR with the change from

Avoid the need to rewrite expressions when evaluating logical case nullability pepijnve/datafusion#2
?

pepijnve · 2025-11-20T15:38:39Z

Since this PR has been outstanding for so long and it fixes a bug and I desperately want to close it down (so we can move on) I am going to merge it as is.

My browser just let out a sigh of relief. GitHub's UI was struggling with this one.

@pepijnve would you be willing to make a real PR with the change from

Certainly

alamb · 2025-11-20T15:40:18Z

Since this PR has been outstanding for so long and it fixes a bug and I desperately want to close it down (so we can move on) I am going to merge it as is.

My browser just let out a sigh of relief. GitHub's UI was struggling with this one.

😆 thank you for sticking with it -- I think the code overall (not just case reporting) is significantly better because of your work

@pepijnve would you be willing to make a real PR with the change from

Certainly

🙏

## Which issue does this PR close? - None, break out PR of changes done in apache#17813 ## Rationale for this change In apache#17813 `GuaranteeRewriter` is used from the `datafusion_expr` crate. In order to enable this the type needed to be moved from `datafusion_optimizer` to `datafusion_expr`. Additionally, during the development of apache#17813 some latent bugs were discovered in `GuaranteeRewriter` that have been resolved. ## What changes are included in this PR? - Move `GuaranteeRewriter` to `datafusion_expr` - Fix two bugs where rewrites of 'between' expression would fail - when one of the bounds was untyped null - when the lower bound was greater than the upper bound - Add logic to replace expressions with literal null based on provided guarantees - Split implementation into smaller functions for easier readability ## Are these changes tested? - Existing tests updated - Tests added for bugfixes ## Are there any user-facing changes? No --------- Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org>

…e#17813) ## Which issue does this PR close? - Closes apache#17801 - Obviates (contains) and thus Closes apache#17833 - Obviates (contains) and thus Closes apache#18536 ## Rationale for this change apache#17357 introduced a change that replaces `coalesce` function calls with `case` expressions. In the current implementation these two differ in the way they report their nullability. `coalesce` is more precise than `case` all will report itself as not nullable in situations where the equivalent `case` does report being nullable. The rest of the codebase currently does not expect the nullability property of an expression to change as a side effect of expression simplification. This PR is a first attempt to align the nullability of `coalesce` and `case`. ## What changes are included in this PR? Tweaks to the `nullable` logic for the logical and physical `case` expression code to report `case` as being not nullable in more situations. - For logical `case`, a best effort const evaluation of 'when' expressions is done to determine 'then' reachability. The code errs on the conservative side wrt nullability. - For physical `case`, const evaluation of 'when' expressions using a placeholder record batch is attempted to determine 'then' reachability. Again if const evaluation is not possible, the code errs on the conservative side. - The optimizer schema check has been relaxed slightly to allow nullability to be removed by optimizer passes without having to disable the schema check entirely - The panic'ing benchmark has been reenabled ## Are these changes tested? Additional unit tests have been added to test the new logic. ## Are there any user-facing changes? No --------- Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org>

github-actions bot added logical-expr Logical plan and expressions physical-expr Changes to the physical-expr crates core Core DataFusion crate labels Sep 28, 2025

pepijnve force-pushed the issue_17801 branch from d2e613a to 88a911b Compare September 29, 2025 06:02

pepijnve mentioned this pull request Sep 29, 2025

Temporarily disable failing sql_planner benchmark query #17809

Merged

pepijnve force-pushed the issue_17801 branch from 88a911b to 482d0be Compare September 29, 2025 10:51

pepijnve marked this pull request as ready for review September 29, 2025 10:52

pepijnve force-pushed the issue_17801 branch 3 times, most recently from 4bbaa82 to 7f8d7cf Compare September 29, 2025 12:56

alamb reviewed Sep 29, 2025

View reviewed changes

pepijnve force-pushed the issue_17801 branch from 9516262 to a6ab83a Compare September 29, 2025 17:19

alamb mentioned this pull request Sep 29, 2025

Revert "Disable failing benchmark query (#17809)" #17833

Closed

pepijnve force-pushed the issue_17801 branch from 88dcd11 to 7ad94e1 Compare October 1, 2025 07:35

pepijnve force-pushed the issue_17801 branch from 7ff0810 to a3a07e9 Compare October 6, 2025 14:15

pepijnve added 6 commits October 6, 2025 18:40

apache#17801 Improve nullability reporting of case expressions

408cee1

apache#17801 Clarify logical expression test cases

045fc9c

apache#17801 Attempt to clarify const evaluation logic

de8b780

apache#17801 Extend predicate const evaluation

bbd2949

apache#17801 Correctly report nullability of implicit casts in predic…

2075f4b

…ates

apache#17801 Code formatting

8c87937

pepijnve force-pushed the issue_17801 branch from a3a07e9 to 8c87937 Compare October 6, 2025 16:42

alamb reviewed Nov 19, 2025

View reviewed changes

pepijnve mentioned this pull request Nov 19, 2025

Move GuaranteeRewriter to datafusion_expr #18821

Merged

Merge remote-tracking branch 'upstream/main' into issue_17801

44221c4

# Conflicts: # datafusion/expr/src/expr_rewriter/guarantees.rs # datafusion/expr/src/expr_rewriter/mod.rs # datafusion/optimizer/src/simplify_expressions/mod.rs

github-actions bot removed the optimizer Optimizer rules label Nov 19, 2025

pepijnve added 2 commits November 19, 2025 22:58

Revert changes in Interval::try_new

5115952

Mimic wikipedia 3VL truth tables as well as possible in ascii art

4b15030

alamb added 2 commits November 20, 2025 06:55

Revert "Disable failing benchmark query (apache#17809)"

5d9fc7e

This reverts commit 5cc0be5.

Merge remote-tracking branch 'apache/main' into issue_17801

ff9a41f

alamb approved these changes Nov 20, 2025

View reviewed changes

pepijnve commented Nov 20, 2025

View reviewed changes

alamb mentioned this pull request Nov 20, 2025

Cache the result of Expr::data_type / Expr::nullable / Expr::to_field to speed up planning #18845

Open

Update comments

3e0fd7e

alamb added this pull request to the merge queue Nov 20, 2025

Merged via the queue into apache:main with commit 0bd127f Nov 20, 2025
32 checks passed

pepijnve mentioned this pull request Nov 20, 2025

Avoid the need to rewrite expressions when evaluating logical case nullability #18849

Open

		when(binary_expr(col("foo"), Operator::Eq, lit(5)), col("foo"))
		.otherwise(lit(0))?,

	when(binary_expr(col("foo"), Operator::Eq, lit(5)), col("foo"))
	.otherwise(lit(0))?,
	when(col("foo").eq(lit(5))), col("foo")).otherwise(lit(0))?,

-    fn check_nullability(
+    /// Verifies that `expr` has `nullable` nullability when the 'foo' column is
+    /// null.
+    /// Also assumes and verifies that `expr` is NOT nullable when 'foo' is NOT null
+    fn check_nullability(

#17801 Improve nullability reporting of case expressions #17813

#17801 Improve nullability reporting of case expressions #17813

Uh oh!

Conversation

pepijnve commented Sep 28, 2025 • edited by alamb Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

Uh oh!

alamb left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pepijnve commented Sep 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pepijnve commented Sep 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pepijnve commented Sep 30, 2025

Uh oh!

pepijnve commented Sep 30, 2025

Uh oh!

alamb commented Sep 30, 2025

Uh oh!

pepijnve commented Sep 30, 2025

Uh oh!

pepijnve commented Oct 1, 2025

Uh oh!

alamb commented Oct 6, 2025

Uh oh!

alamb left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pepijnve Nov 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pepijnve commented Sep 28, 2025 •

edited by alamb

Loading

pepijnve commented Sep 29, 2025 •

edited

Loading

pepijnve commented Sep 30, 2025 •

edited

Loading

pepijnve Nov 19, 2025 •

edited

Loading

pepijnve Nov 20, 2025 •

edited

Loading

alamb Nov 20, 2025 •

edited

Loading

pepijnve Nov 20, 2025 •

edited

Loading

pepijnve commented Nov 20, 2025 •

edited

Loading