fix: lazy evaluation for coalesce #17357

chenkovsky · 2025-08-30T13:53:14Z

Which issue does this PR close?

Rationale for this change

arguments of coalesce are evaluated eagerly.

What changes are included in this PR?

simplify coalesce to case when expr.

Are these changes tested?

UT

Are there any user-facing changes?

No

nuno-faria

LGTM

nuno-faria · 2025-09-01T18:18:28Z

datafusion/sqllogictest/test_files/select.slt

 # due to the reason describe in https://github.com/apache/datafusion/issues/8927,
 # the following queries will fail


Since the test below now works, I think we could move this comment to the test below this one.

alamb

Thanks @chenkovsky and @nuno-faria -- I think this PR is quite good and probably can be merged. My only potential concern is that we may mess up comet. Let's see if we get any more comments

alamb · 2025-09-05T20:05:08Z

datafusion/sqllogictest/test_files/select.slt

 02)--TableScan: t projection=[x, y]
 physical_plan
-01)ProjectionExec: expr=[coalesce(1, CAST(y@1 / x@0 AS Int64)) as coalesce(Int64(1),t.y / t.x), coalesce(2, CAST(y@1 / x@0 AS Int64)) as coalesce(Int64(2),t.y / t.x)]
+01)ProjectionExec: expr=[CASE WHEN true THEN 1 ELSE CAST(y@1 / x@0 AS Int64) END as coalesce(Int64(1),t.y / t.x), CASE WHEN true THEN 2 ELSE CAST(y@1 / x@0 AS Int64) END as coalesce(Int64(2),t.y / t.x)]


It seems like this should be able to be simplifed even more -- CASE WHEN true THEN ... should go to 1

I filed a ticket to track this idea:

Simplify CASE WHEN true <xx> ... #17448

alamb · 2025-09-05T20:06:18Z

datafusion/functions/src/core/coalesce.rs

+            ));
        }
+
+        let n = args.len();


This is quite a clever implementation.

However, I worry it may cause problems for comet which uses physical evaluation directly

@comphead or @andygrove do you know if comet uses the COALESCE implementation directly?

mbutrovich · 2025-09-05T20:10:32Z

Thanks @chenkovsky and @nuno-faria -- I think this PR is quite good and probably can be merged. My only potential concern is that we may mess up comet. Let's see if we get any more comments

Comet was actually just thinking about coalesce:

apache/datafusion-comet#2270

Maybe @coderfender has some thoughts about this?

alamb · 2025-09-06T10:32:00Z

Thanks @chenkovsky and @nuno-faria -- I think this PR is quite good and probably can be merged. My only potential concern is that we may mess up comet. Let's see if we get any more comments

Comet was actually just thinking about coalesce:

apache/datafusion-comet#2270

Maybe @coderfender has some thoughts about this?

It seems like this PR does the same thing described in the comet PR

fix: implement lazy evaluation in Coalesce function datafusion-comet#2270

Thus it seems like a good thing to merge

Let's wait a while to see if anyone else has comments, otherwise I'll plan to merge this PR

coderfender · 2025-09-06T18:15:24Z

@alamb , @mbutrovich I made changes to comet to fallback to CASE statement to replicate lazy evaluation with coalesce (and then plan to work on this PR). Glad to see that the changes are looking good and once we update DataFusion's version in comet, we should be able to undo my comet changes to leverage DF's coalesce function . I will create a github issue on the comet side for this

alamb · 2025-09-08T10:47:06Z

Thanks everyone!

alamb · 2025-09-09T13:51:19Z

I believe this PR caused the extended tests to fail (as more queries now start woring):

DataFusion extended tests failing on main #17487

This reverts commit e5dcc8c.

@chenkovsky

…ion (#18567) Note this targets the branch-51 release branch ## Which issue does this PR close? - part of #17558 - resolves #17801 in the 51 release branch ## Rationale for this change - We merged some clever rewrites for `coalesce` and `nvl2` to use `CASE` which are faster and more correct (👏 @chenkovsky @kosiew ) - However, these rewrites cause subtle schema mismatches in some cases planning (b/c the CASE simplification nullability logic can't determine the correct nullability in some cases - see #17801) - @pepijnve has some heroic efforts to fix the schema mismatch in #17813 (comment), but it is non trivial and I am worried about merging it so close to the 51 release and introducing new edge cases ## What changes are included in this PR? 1. Revert #17357 / e5dcc8c 3. Revert #17991 / ea83c26 2. Revert #18191 / 22c4214 2. Cherry-pick 6202254, a test that reproduces the schema mismatch issue (from #18536) 3. Cherry-pick 735cacf, a fix for the benchmarks that regressed due to the revert (from #17833) 4. Update datafusion-testing (see separate PR here) for extended tests (see apache/datafusion-testing#15) ## Are these changes tested? Yes I added a new test ## Are there any user-facing changes?

## Which issue does this PR close? - Closes #17801 - Obviates (contains) and thus Closes #17833 - Obviates (contains) and thus Closes #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 --------- 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>

fix: lazy evaluation for coalesce

aedec6a

github-actions bot added sqllogictest SQL Logic Tests (.slt) functions Changes to functions implementation labels Aug 30, 2025

nuno-faria approved these changes Sep 1, 2025

View reviewed changes

update test

6fcd4f8

alamb mentioned this pull request Sep 5, 2025

Simplify CASE WHEN true <xx> ... #17448

Closed

alamb approved these changes Sep 5, 2025

View reviewed changes

alamb merged commit e5dcc8c into apache:main Sep 8, 2025
29 checks passed

alamb mentioned this pull request Sep 9, 2025

DataFusion extended tests failing on main #17487

Closed

alamb mentioned this pull request Sep 9, 2025

Update expected output for coalsece short-circuit apache/datafusion-testing#12

Merged

pepijnve mentioned this pull request Sep 28, 2025

#17801 Improve nullability reporting of case expressions #17813

Merged

This was referenced Oct 8, 2025

Implement lazy evaluation for nvl #17982

Closed

Implement lazy evaluation for nvl2 #17983

Closed

pepijnve mentioned this pull request Oct 11, 2025

Add defensive debug asserts in simplify to ensure schema doesn't change #18001

Open

alamb mentioned this pull request Oct 15, 2025

[EPIC] A collection of items to improve CASE performance #18075

Closed

11 tasks

pepijnve mentioned this pull request Oct 21, 2025

Refactor nvl2 Function to Support Lazy Evaluation and Simplification via CASE Expression #18191

Merged

pepijnve mentioned this pull request Nov 6, 2025

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

Closed

alamb added a commit to alamb/datafusion that referenced this pull request Nov 9, 2025

Revert "fix: lazy evaluation for coalesce (apache#17357)"

395f36c

This reverts commit e5dcc8c.

alamb mentioned this pull request Nov 9, 2025

[branch-51] Revert rewrite for coalesce, nvl and nvl2 simplification #18567

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix: lazy evaluation for coalesce #17357

fix: lazy evaluation for coalesce #17357

Uh oh!

chenkovsky commented Aug 30, 2025 •

edited by alamb

Loading

Uh oh!

nuno-faria left a comment

Uh oh!

nuno-faria Sep 1, 2025

Uh oh!

alamb left a comment

Uh oh!

alamb Sep 5, 2025

Uh oh!

alamb Sep 5, 2025

Uh oh!

mbutrovich commented Sep 5, 2025

Uh oh!

alamb commented Sep 6, 2025

Uh oh!

coderfender commented Sep 6, 2025 •

edited

Loading

Uh oh!

Uh oh!

alamb commented Sep 8, 2025

Uh oh!

alamb commented Sep 9, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

		# due to the reason describe in https://github.com/apache/datafusion/issues/8927,
		# the following queries will fail

fix: lazy evaluation for coalesce #17357

fix: lazy evaluation for coalesce #17357

Uh oh!

Conversation

chenkovsky commented Aug 30, 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!

nuno-faria left a comment

Choose a reason for hiding this comment

Uh oh!

nuno-faria Sep 1, 2025

Choose a reason for hiding this comment

Uh oh!

alamb left a comment

Choose a reason for hiding this comment

Uh oh!

alamb Sep 5, 2025

Choose a reason for hiding this comment

Uh oh!

alamb Sep 5, 2025

Choose a reason for hiding this comment

Uh oh!

mbutrovich commented Sep 5, 2025

Uh oh!

alamb commented Sep 6, 2025

Uh oh!

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

Uh oh!

Uh oh!

Uh oh!

alamb commented Sep 8, 2025

Uh oh!

alamb commented Sep 9, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

chenkovsky commented Aug 30, 2025 •

edited by alamb

Loading

coderfender commented Sep 6, 2025 •

edited

Loading