mohammadnaqvi04 commented on PR #25008:
URL: https://github.com/apache/datafusion/pull/25008#issuecomment-5671574104

   @AdamGS Thanks for the second pass. I worked through the faulty queries and 
am re-scoping what this PR is and isn't claiming to fix.
   
   ```sql
   # HAVING evaluates to NULL for the matching group and must discard its row.
   query I
   SELECT o.k
   FROM outer_rows o
   WHERE EXISTS (
     SELECT count(*) FROM inner_rows i WHERE i.k = o.k
     HAVING count(*) = 0 OR sum(i.v) > 0
   );
   ----
   
   # The same NULL-valued HAVING makes the subquery empty, so NOT IN is true.
   query I
   SELECT o.k
   FROM outer_rows o
   WHERE 1 NOT IN (
     SELECT count(*) FROM inner_rows i WHERE i.k = o.k
     HAVING count(*) = 0 OR sum(i.v) > 0
   );
   ----
   1
   ```
   
   These two cover the same three-valued bug where a HAVING predicate can 
itself be `NULL`. Both are now fixed.
   
   The `LIMIT 0` case and the CAST-based duplicate-match case are gaps in what 
count-bug compensation is capable of tracking. LIMIT-truncated rows aren't 
tracked the way HAVING-failed rows are, and the compensation join can fan out 
multiple rows per outer row under a non-injective (one-to-many from outer row 
to inner) correlation key. I gave this some thought, and think both are big 
enough that they don't belong in this PR.
   
   I've decided to narrow this PR's scope to `EXISTS`/`NOT EXISTS`/`IN`/`NOT 
IN` over groupless aggregates with `HAVING`, including `NULL`-valued and 
compound `HAVING` predicates. I'll open LIMIT-truncation and join fan-out as 
separate follow-ups.
   
   Also, in the process of more adversarial review targeting the `HAVING` 
mechanics here, I discovered three more. I've included fixes and test cases for 
all three:
   
   1. A second filter stacked on an already-compensated aggregate fell back to 
silently dropping the HAVING clause.
   2. The unmatched-row default for that stacked-filter case skipped the 
combined predicate on the `IN`/`NOT IN` side.
   3. The unmatched-row default for that stacked-filter case skipped the 
combined predicate on the `EXISTS` side.
   
   I've also just now run and compared every new query I added in 
`subquery.slt` from my branch against latest DuckDB and DataFusion `main` 
builds.


-- 
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]

Reply via email to