0xdeafbeef opened a new pull request, #26898: URL: https://github.com/apache/datafusion/pull/26898
## Which issue does this PR close? - Closes #16965. ## Rationale for this change On DataFusion 53.1.0 and 55.2.0 with `target_partitions = 1`, this query returns its rows in the wrong order: ```sql SELECT a, b FROM (VALUES (1, 1), (2, 2)) AS t (a, b) WHERE a = b GROUP BY GROUPING SETS ((a, b), (a)) ORDER BY a, b NULLS FIRST; ``` DataFusion returns `(1,1), (2,2), (1,NULL), (2,NULL)`. PostgreSQL returns `(1,NULL), (1,1), (2,NULL), (2,2)`. With `row_number() OVER (ORDER BY a, b NULLS FIRST)` added, the same versions attach wrong `row_number` values to rows. With 16 partitions, the query happened to return the right order. I found this by running PostgreSQL's regression suite against a DataFusion-based engine and comparing answers with PostgreSQL. This query and its `row_number()` variant come from that suite's `groupingsets.sql`. The filter `a = b` gives an equivalence `a = b`. The grouping-set `AggregateExec` projected input equivalence properties to its output. The `(a)` grouping set outputs `b` as NULL, so `a = b` is false on those rows. `EnforceSorting` used the false equivalence and rewrote the sort `[a ASC NULLS LAST, b ASC NULLS FIRST]` to `[b ASC NULLS LAST]`. A constant from the filter fails the same way: ```sql SELECT a, b FROM (VALUES (1, 1), (2, 1)) AS t (a, b) WHERE b = 1 GROUP BY GROUPING SETS ((a, b), (a)) ORDER BY a, b NULLS FIRST; ``` 53.1.0 and 55.2.0 return `(1,1), (1,NULL), (2,1), (2,NULL)`, and the correct answer is `(1,NULL), (1,1), (2,NULL), (2,1)`. I looked for a fix and found #24422 on main. It builds the properties of an aggregate with grouping sets from empty `EquivalenceProperties`, and it will ship in 56.0.0. Its tests check that grouping sets do not pick ordered aggregation, and none of them covers equivalences or constants, so I am sending tests for that part. The query from #16965 returns 3 rows (`id`, `NULL`, `id`) instead of 2 on 53.1.0 and 55.2.0 with `target_partitions = 1`. On main, #24422 and #25019 each fix it independently. ## What changes are included in this PR? New cases in `datafusion/sqllogictest/test_files/grouping.slt`, in the existing `target_partitions = 1` block after the #24422 tests: the `EXPLAIN` and results of the first query, the `row_number()` variant, the constant query, and the #16965 query. There are no Rust changes. ## What is the testing strategy for this PR? All new cases pass on main (e4f9b030e). As a negative control, I made grouping-set aggregates use `compute_properties` instead of `compute_grouping_set_properties`. The `EXPLAIN` case and the results of the first query, the `row_number()` variant and the constant query then fail. The first query's sort becomes `[b ASC NULLS LAST]`, and the constant query's becomes `[a ASC NULLS LAST]`. The existing `grouping.slt` and `grouping_set_repartition.slt` tests all pass under that change. The #16965 case fails only when that change is combined with reverting the constant-key exclusion from #25019. It passes with either change alone, and with #24422's clustering-mode change reverted. ## Are there any user-facing changes? No. cc @2010YOUY01 -- 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]
