1fanwang commented on PR #25742: URL: https://github.com/apache/datafusion/pull/25742#issuecomment-5843848188
Thanks for the DuckDB differential run, @kita-renji. Around 10k queries with NULLs, partial NULLs and nested set ops is a much stronger check than the tests I had, and it found real bugs. **1 and 2**: fixed in https://github.com/apache/datafusion/commit/a03258c0de127156aa4380b0fcee040b6dc1986c. The two sides now get different row number names, so they are only requalified when the user's own columns conflict. The join keys come from the requalified schemas, so `t1.x` and `t2.x` on one side become `left.x` and `left.x:1` as they do for plain `INTERSECT`. The final projection then aliases each column back to the left input's qualifier and name. `ORDER BY t.x` and `col("t.x")` resolve now, including when both sides read the same table (distinct `INTERSECT` still reports `left.x` there). **3**: opened #25766 for the window partitioning bug. `negative_zero.slt` now has the min(2, 1) and 2 - 1 cases, recording the current wrong output with a link to the issue, in https://github.com/apache/datafusion/commit/84ef8f7f49b5e179ef85724ea33c46a0a2f7f907. **4**: I haven't changed the plan shape here. Your numbers make the grouped-count version look like the better default when most rows are distinct, but choosing between that and dedicated Intersect/Except nodes is the open question from #12955, and I'd like a maintainer to pick the direction first. I can do it as a follow-up once someone weighs in. **Smaller things**, in https://github.com/apache/datafusion/commit/145bee473463e653c1d137e3881e986e3278c8ef: - `LogicalPlanBuilder::intersect` and `except` now document that `is_all = true` does not preserve duplicate counts and point to `intersect_all` and `except_all`. I didn't add `#[deprecated]`, because the same functions are still the right call with `is_all = false`. Splitting them felt like an API decision for a maintainer. - Removed the dead `is_all` parameter from Substrait `intersect_rel`. - Without `row_number`, planning fails with "INTERSECT ALL and EXCEPT ALL require the row_number window function, which is not registered" (or the `DataFrame::intersect` / `except` equivalent). There's a test for it and an entry in the 56.0.0 upgrade guide. - `intersection.slt` covers min/max counts with NULLs, partial-NULL rows, `a EXCEPT ALL b INTERSECT ALL c` and its parenthesized form, and both regressions (84ef8f7). One side effect: the Substrait `self_referential_intersect_all` / `except_all` round-trip tests no longer compare schemas. The plan's output is now `data.a`, and Substrait, which doesn't carry qualifiers, brings it back as `left.a`. -- 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]
