kosiew commented on code in PR #24103:
URL: https://github.com/apache/datafusion/pull/24103#discussion_r3802589440
##########
datafusion/common/src/functional_dependencies.rs:
##########
@@ -144,6 +144,16 @@ pub struct FunctionalDependence {
/// such as after LEFT JOIN or RIGHT JOIN operations, this property may
/// change.
pub nullable: bool,
+ /// Whether the source key permits multiple NULL rows with inconsistent
+ /// dependent values. This is `true` only for dependencies derived from
+ /// `UNIQUE` constraints (which allow duplicate NULLs). It is `false` for
+ /// PRIMARY KEY constraints, downgraded PKs (where join padding produces
+ /// consistent NULL dependents), and GROUP BY derived keys (where at most
+ /// one NULL group exists).
+ ///
+ /// When `true`, the dependency must NOT be used for GROUP BY expansion,
+ /// GROUP BY/ORDER BY reduction, or DISTINCT removal.
+ pub duplicate_nulls: bool,
Review Comment:
One compatibility concern here: `FunctionalDependence` is public and can be
constructed with a struct literal downstream. Adding the public
`duplicate_nulls` field therefore breaks existing downstream code, which
matches the unresolved `cargo-semver-checks` failure.
Could we keep this metadata in a non-public representation instead, or make
the corresponding release-version change if the public API change is
intentional?
##########
datafusion/sqllogictest/test_files/functional_dependencies.slt:
##########
@@ -229,15 +229,15 @@ query II rowsort
SELECT x, y FROM t_uniq GROUP BY x;
----
1 3
-NULL 1
NULL 2
Review Comment:
I would avoid asserting the exact value returned by `ANY_VALUE` here.
`ANY_VALUE(y)` is intentionally arbitrary, so both `NULL 2` and `NULL 1` can be
valid outcomes.
Could we keep the EXPLAIN assertion, but test the grouped-query invariant
independently, for example by asserting that the result count is 2 rather than
pinning the selected value?
##########
datafusion/common/src/functional_dependencies.rs:
##########
@@ -522,9 +541,15 @@ pub fn get_target_functional_dependencies(
for FunctionalDependence {
source_indices,
target_indices,
+ duplicate_nulls,
..
} in &dependencies.deps
{
+ // A dependency that allows duplicate NULLs (from a UNIQUE constraint)
+ // does not guarantee determination across NULL keys, so skip it.
+ if *duplicate_nulls {
Review Comment:
I think this eligibility check still needs to be centralized or applied
consistently across all FD consumers. Right now it only filters
`get_target_functional_dependencies`.
`get_required_group_by_exprs_indices` still removes `y` from `GROUP BY x,
y`, `get_required_sort_exprs_indices` still drops `y` from `ORDER BY x, y`, and
`ReplaceDistinctWithAggregate` still treats a nullable-UNIQUE
`Dependency::Single` FD as sufficient proof that `DISTINCT` is redundant.
That means the three correctness issues from the original review are still
present, and the SLT currently records those incorrect plans/results. Could we
make the eligibility predicate part of the FD-consumer API, or otherwise apply
`!duplicate_nulls` consistently everywhere an FD is used? After that, 1.2, 2.2,
and 3.2 should be updated to assert the corrected results/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]
