jayzhan211 commented on code in PR #26723:
URL: https://github.com/apache/datafusion/pull/26723#discussion_r4373212034
##########
datafusion/optimizer/src/scalar_subquery_to_join.rs:
##########
@@ -417,7 +444,17 @@ fn build_join(
// outer table has a column with the same name as the aggregate.
let value_col = Column::new(Some(subquery_alias), name);
- let mut builder = when(Expr::Column(indicator_col).is_null(),
result);
+ // On an unmatched LEFT JOIN row, a HAVING-like condition means
+ // the subquery produced no row. Return the empty-input default
+ // only when that condition passes on the defaults; otherwise
+ // return NULL.
+ let unmatched_value = match &having_on_default {
Review Comment:
The `HAVING` condition is still part of the join `ON`, so a row whose real
count fails `HAVING` also shows up as unmatched. The fallback then checks
`HAVING` against the default, which can pass, so the issue still reproduces
with `<` or `<>`:
```sql
SELECT x.a, x.d,
(SELECT count(*) FROM (VALUES (1)) AS y(c) WHERE y.c = x.d HAVING
count(*) < x.a) AS v
FROM (VALUES (1, 1), (2, 1), (1, 9)) AS x(a, d)
ORDER BY x.d, x.a;
-- Postgres: (1, 1) -> NULL, (2, 1) -> 1, (1, 9) -> 0. This branch: (1, 1)
-> 0
```
Could these conditions stay out of the `ON` and be checked in the
compensation `CASE` instead, the way `having_arm` already handles a local
`HAVING`? For example `WHEN __always_true IS NULL THEN CASE WHEN H(default)
THEN default END WHEN H(actual) IS NOT TRUE THEN NULL ELSE actual END`. Then
"unmatched" only means "no group", and both directions come out right. One
caveat: aggregates that are NULL on empty input skip compensation, so for those
the condition would have to stay in the `ON`.
##########
datafusion/optimizer/src/scalar_subquery_to_join.rs:
##########
@@ -399,6 +399,33 @@ fn build_join(
// introduced is the untyped `Null` of the HAVING arm. The expressions
// themselves come from a plan `TypeCoercion` has already coerced.
let mut expr_rewrite = TypeCoercionRewriter::new(new_plan.schema());
+ // Evaluate the HAVING-like conditions (recorded during decorrelation
+ // from filters sitting above the aggregate) against the empty-input
+ // aggregate defaults: `count(*)` becomes `0`, and so on.
+ let having_on_default =
+ match conjunction(pull_up.having_like_join_filters.clone()) {
+ Some(combined) => {
+ let requalified = replace_qualified_name(
+ combined,
+ &all_correlated_cols,
+ subquery_alias,
+ )?;
+ let substituted = requalified
+ .transform_up(|e| {
+ if let Expr::Column(Column { name, .. }) = &e
+ && let Some(default_val) = expr_map.get(name)
Review Comment:
This substitution matches columns by bare name, so it also replaces outer
columns that happen to share the aggregate's output name. That turns a result
that is correct today (`0`) into `NULL`:
```sql
SELECT x.n, x.d,
(SELECT s.n FROM (SELECT count(*) AS n FROM (VALUES (1)) AS y(c)
WHERE y.c = x.d) AS s
WHERE s.n < x.n) AS v
FROM (VALUES (5, 9), (0, 9), (5, 1)) AS x(n, d)
ORDER BY x.d, x.n;
-- Postgres and main: (5, 9) -> 0. This branch (rebased on main): (5, 9) ->
NULL
```
Could you limit it to columns qualified with the subquery alias, and add
this query as a test?
```rs
if let Expr::Column(Column {
relation: Some(relation),
name,
..
}) = &e
&& relation.table() == subquery_alias
&& let Some(default_val) = expr_map.get(name)
```
##########
datafusion/optimizer/src/scalar_subquery_to_join.rs:
##########
@@ -399,6 +399,33 @@ fn build_join(
// introduced is the untyped `Null` of the HAVING arm. The expressions
// themselves come from a plan `TypeCoercion` has already coerced.
let mut expr_rewrite = TypeCoercionRewriter::new(new_plan.schema());
+ // Evaluate the HAVING-like conditions (recorded during decorrelation
+ // from filters sitting above the aggregate) against the empty-input
+ // aggregate defaults: `count(*)` becomes `0`, and so on.
+ let having_on_default =
Review Comment:
Could you rebase onto main? Since #26551, main builds the join filter
through `build_join_filter`, so `conjunction`, `replace_qualified_name` and
`all_correlated_cols` no longer resolve here and the branch doesn't compile
after rebasing. Requalifying through the same helper builds on main:
```rs
let having_on_default = match build_join_filter(
pull_up.having_like_join_filters.clone(),
pull_up.correlated_subquery_cols_map.values().flatten(),
subquery_alias,
)? {
Some(requalified) => {
let substituted = requalified
.transform_up(|e| {
if let Expr::Column(Column { name, .. }) = &e
&& let Some(default_val) = expr_map.get(name)
{
return Ok(Transformed::yes(default_val.clone()));
}
Ok(Transformed::no(e))
})
.data()?
.rewrite(&mut expr_rewrite)
.data()?;
Some(substituted)
}
None => None,
};
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
