kosiew commented on code in PR #24103: URL: https://github.com/apache/datafusion/pull/24103#discussion_r3835184192
########## datafusion/sqllogictest/test_files/functional_dependencies.slt: ########## @@ -229,15 +229,15 @@ query II rowsort SELECT x, y FROM t_uniq GROUP BY x; Review Comment: Could we avoid asserting the specific value returned by `ANY_VALUE` here? Since `ANY_VALUE(y)` is intentionally arbitrary, both `NULL 1` and `NULL 2` should be valid results for the NULL group. I think it would be more robust to keep the EXPLAIN assertion, but test the query invariant separately, for example by counting the grouped rows and asserting that the result is `2`. That would verify the bug fix without making the test depend on aggregate input order. ########## datafusion/common/src/functional_dependencies.rs: ########## Review Comment: I think we need to apply the `duplicate_nulls` restriction to the other functional dependency consumers as well. Right now it is excluded by `get_target_functional_dependencies`, but GROUP BY reduction and ORDER BY reduction can still use this dependency as a key, and the DISTINCT elimination path can still receive its `Dependency::Single` mode. For example, `SELECT x FROM t_uniq GROUP BY x, y` can still be reduced to `GROUP BY x`, which merges the two NULL groups and loses a row. Similarly, the ORDER BY case can drop `y` as a tie-breaker, and DISTINCT can be removed even though nullable UNIQUE does not guarantee uniqueness across NULLs. The new field documentation already says these uses are not valid when `duplicate_nulls` is true. Could we make that eligibility check central, or otherwise apply it consistently to GROUP BY reduction, ORDER BY reduction, and DISTINCT elimination? I think the corresponding 1.2, 2.2, and 3.2 regression cases should also be updated to assert the corrected results and plans. -- 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]
