kosiew commented on code in PR #24103: URL: https://github.com/apache/datafusion/pull/24103#discussion_r3782912285
########## datafusion/common/src/functional_dependencies.rs: ########## Review Comment: I think we need to make the `duplicate_nulls` eligibility check more central here. Right now it is excluded only by `get_target_functional_dependencies`, while the GROUP BY and ORDER BY reduction paths can still use these dependencies as keys, and DISTINCT elimination still sees their `Dependency::Single` mode. That leaves the same NULL correctness issue in a few places. For example, the existing `functional_dependencies.slt` 3.2 query, `SELECT x FROM t_uniq GROUP BY x, y`, can still be reduced to `GROUP BY x`, which loses one of the NULL rows. Similarly, 2.2 can drop the `y` sort tie-breaker, and 1.2 can remove DISTINCT. Since the documented invariant for `duplicate_nulls` says these uses are not valid, could we centralize this eligibility check and make sure it is applied to every FD consumer, including DISTINCT elimination? The corresponding regression cases should also be updated to assert the correct results and plans. ########## 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: Small test suggestion: since `ANY_VALUE` is intentionally arbitrary, I don't think we should make the regression depend on it returning exactly `NULL 2`. A valid change to aggregate input or ordering could change that result without introducing a correctness regression. Could we keep the EXPLAIN assertion, but test the SQL invariant separately? For example, we could count the rows produced by the grouped query and assert that the result is `2`. -- 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]
