jayzhan211 commented on code in PR #26723:
URL: https://github.com/apache/datafusion/pull/26723#discussion_r4388551145
##########
datafusion/optimizer/src/scalar_subquery_to_join.rs:
##########
@@ -362,8 +362,41 @@ fn build_join(
.alias(subquery_alias.to_string())?
.build()?;
+ // A HAVING-like condition recorded from a filter above the aggregate can
+ // leave the join ON only when every empty-input default gets a
+ // compensation CASE below: an aggregate whose default is NULL has no CASE
+ // to check such a condition in, so for those it keeps filtering in the
+ // ON as before.
+ let having_in_compensation = match &collected_count_expr_map {
Review Comment:
@siddubakka thanks, all three earlier points are fixed and the results match
Postgres. One case still returns `0`. My earlier caveat about NULL-default
aggregates was too vague: it only needs to apply to the subquery's output
column. `expr_map` also contains aggregates used only in `HAVING`, so a `max`
there makes this gate false and the condition stays in the `ON`:
```sql
SELECT x.a, x.d,
(SELECT count(*) FROM (VALUES (1)) AS y(c) WHERE y.c = x.d HAVING
max(y.c) < x.a) AS v
FROM (VALUES (1, 1), (2, 1), (1, 9)) AS x(a, d)
ORDER BY x.d, x.a;
-- Postgres: NULL, 1, NULL. This branch (and main): 0, 1, 0
```
Only the subquery's output column needs a compensation `CASE`, so could the
gate look at just that column? With this change the query above matches
Postgres, `HAVING max(y.c) IS NULL OR count(*) < x.a` gives `NULL, 1, 0` like
Postgres, and the optimizer tests and `subquery.slt` still pass. Could you also
add the query above to `subquery.slt`?
```diff
- let having_in_compensation = match &collected_count_expr_map {
- Some(expr_map) if !expr_map.is_empty() =>
expr_map.values().try_fold(
- true,
- |all_compensated, default_expr| -> Result<bool> {
- Ok(all_compensated
- && !evaluates_to_null(
- default_expr.clone(),
- default_expr.column_refs(),
- )?)
- },
- )?,
- _ => false,
+ let output_name = subquery_plan.schema().field(0).name();
+ let having_in_compensation = match collected_count_expr_map
+ .as_ref()
+ .and_then(|expr_map| expr_map.get(output_name))
+ {
+ Some(default_expr) => {
+ !evaluates_to_null(default_expr.clone(),
default_expr.column_refs())?
+ }
+ None => false,
};
```
--
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]
