Skip to content

fix: error instead of dropping a pulled up HAVING in scalar subqueries whose result is NULL on empty input - #26942

Open
abp002 wants to merge 4 commits into
apache:mainfrom
abp002:scalar-subquery-having-error
Open

fix: error instead of dropping a pulled up HAVING in scalar subqueries whose result is NULL on empty input#26942
abp002 wants to merge 4 commits into
apache:mainfrom
abp002:scalar-subquery-having-error

Conversation

@abp002

@abp002 abp002 commented Oct 30, 2026

Copy link
Copy Markdown
Contributor

Which issue does this PR close?

Rationale for this change

A correlated scalar subquery without GROUP BY ignores its HAVING when the HAVING is true for an empty input and the subquery returns an aggregate that is NULL for an empty input (sum, max, min):

SELECT x.a,
       (SELECT sum(y.b) FROM (VALUES (1, 10)) AS y(a, b) WHERE y.a = x.a HAVING sum(y.b) IS NULL) AS v
FROM (VALUES (1), (2)) AS x(a)
-- main: (1, 10), (2, NULL)
-- PostgreSQL: (1, NULL), (2, NULL)

PullUpCorrelatedExpr pulls such a HAVING out of the subquery, and ScalarSubqueryToJoin only applies it again through the count bug compensation CASE. That CASE is not built for expressions that are NULL on empty input, so the HAVING is dropped.

Following #5492 (comment), this makes the query error instead of trying to return the right rows.

What changes are included in this PR?

build_join keeps the subquery correlated when a HAVING was pulled up and the subquery's output is NULL on empty input. The query then fails with Physical plan does not support logical expression ScalarSubquery, the same way #26544 does.

Only the output column is checked. A count(*) subquery whose HAVING reads sum or max still gets the HAVING from the compensation and keeps working.

What is the testing strategy for this PR?

New cases in subquery.slt: the query from the issue, max, and sum with HAVING count(*) = 0 now error. A count(*) subquery with a HAVING on count(*) or on sum, and a HAVING that is false on empty input, still return the same rows as before. All sqllogictest files, the datafusion-optimizer unit tests and the core_integration sql:: tests pass locally.

Are there any user-facing changes?

These queries now return a "not implemented" error instead of wrong results. No API changes.

@github-actions github-actions Bot added optimizer Optimizer rules sqllogictest SQL Logic Tests (.slt) labels Oct 30, 2026
@raghavyadav01

raghavyadav01 commented Oct 31, 2026

Copy link
Copy Markdown
Contributor

Checked this against DuckDB 1.5.6 with a few more shapes (x(a) = 1..4, y(a, b) = (1,10), (1,5), (3,NULL), (4,7)):

  • sum(y.b) with HAVING sum(y.b) IS NULL, also with AS s: main returns 15 and 7 for a = 1 and 4, DuckDB returns NULL. Now errors.
  • sum(y.b) with HAVING count(*) < 2: main returns 15 for a = 1, DuckDB returns NULL. Now errors too. Might be worth adding, since the HAVING reads a different aggregate than the output.
  • count(*), count(*) AS c, count(*) + 1 and count(y.b) outputs with a HAVING that is true on empty input, coalesce(sum(y.b), 0) with HAVING sum(y.b) IS NULL, a HAVING that is false on empty input, and the subquery in WHERE: same rows as main, and they match DuckDB.

So nothing that worked before is rejected now. Looks good to me.

@abp002

abp002 commented Nov 2, 2026

Copy link
Copy Markdown
Contributor Author

Thanks for checking these against DuckDB, @raghavyadav01! I added the sum(y.b) with HAVING count(*) < 2 case with your data. It also covers a HAVING that filters out a non-empty group (a = 1), not just the empty-input one, and it now errors instead of returning 15.

@jayzhan211
jayzhan211 self-requested a review November 8, 2026 03:20
jayzhan211
jayzhan211 requested changes Nov 8, 2026
View reviewed changes

@jayzhan211 jayzhan211 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@abp002 thanks for the fix! In scalar_subquery_to_join.rs around line 403, the count(*) path this PR keeps decorrelated still ignores a HAVING that is NULL for a matched group: the query below returns 1 1, but should return 1 NULL. The arm is WHEN NOT(having) THEN NULL, and NOT NULL falls through to ELSE. Since this is #26505's shape, could you fix it here with IS NOT TRUE and add the query as a test? With that change it returns 1 NULL, and only two EXPLAIN lines in subquery.slt change (!= Int64(0) becomes = Int64(0) IS NOT TRUE).

SELECT x.a, (SELECT count(*) FROM (VALUES (1, CAST(NULL AS INT))) AS y(a, b)
             WHERE y.a = x.a HAVING count(*) = 0 OR sum(y.b) > 5) AS v
FROM (VALUES (1), (2)) AS x(a) ORDER BY x.a;
-- expected: 1 NULL, 2 0
-            .map(|f| (not(f.clone()), lit(ScalarValue::Null)));
+            .map(|f| (f.clone().is_not_true(), lit(ScalarValue::Null)));

Comment thread datafusion/optimizer/src/scalar_subquery_to_join.rs Outdated
// below, which skips the expressions that are NULL on empty input. Keep
// the subquery correlated rather than drop the `HAVING` for its output.
// <https://github.com/apache/datafusion/issues/26505>
if pull_up.pull_up_having_expr.is_some() {

@jayzhan211 jayzhan211 Nov 8, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since #26723 merged, main already computes this lookup in build_join as output_needs_compensation, so after a rebase the same check would be done twice. Could you rebase and gate on that variable instead? It treats a missing output as false, so the else branch here (which no test reaches) is covered too. I ran this on the PR merged with main: subquery.slt and the optimizer unit tests pass, and clippy is clean.

-    if pull_up.pull_up_having_expr.is_some() {
-        let output_name = subquery_plan.schema().field(0).name();
-        let Some(result) = collected_count_expr_map
-            .as_ref()
-            .and_then(|expr_map| expr_map.get(output_name))
-        else {
-            return Ok(None);
-        };
-        if evaluates_to_null(result.clone(), result.column_refs())? {
-            return Ok(None);
-        }
-    }
     ...
     };
+    if pull_up.pull_up_having_expr.is_some() && !output_needs_compensation {
+        return Ok(None);
+    }
     if output_needs_compensation && !pull_up.having_like_join_filters.is_empty() {

@abp002
abp002 force-pushed the scalar-subquery-having-error branch from 06146b8 to 2f7724a Compare November 8, 2026 10:24
@abp002

abp002 commented Nov 8, 2026

Copy link
Copy Markdown
Contributor Author

Thanks for the review, @jayzhan211! I rebased on main and gated on output_needs_compensation as you suggested, so the lookup is only done once. I also changed the HAVING arm to IS NOT TRUE and added your query as a test: it returned 1 1 before the change and 1 NULL now. The only other change is the two EXPLAIN lines in subquery.slt.

@abp002
abp002 force-pushed the scalar-subquery-having-error branch from 2f7724a to b241047 Compare November 14, 2026 13:59
@abp002

abp002 commented Nov 14, 2026

Copy link
Copy Markdown
Contributor Author

Rebased on main again. #26904 added the query from #26505 to subquery_decorrelation.slt with the rows main returns today, so I changed that case to expect the error.

@jayzhan211, could you take another look when you have time?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jayzhan211 jayzhan211 jayzhan211 requested changes

Requested changes must be addressed to merge this pull request.

Assignees

No one assigned

Labels

optimizer Optimizer rules sqllogictest SQL Logic Tests (.slt)

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Correlated scalar subquery ignores HAVING that is true for empty input

3 participants

@abp002 @raghavyadav01 @jayzhan211