jayzhan211 commented on code in PR #23918:
URL: https://github.com/apache/datafusion/pull/23918#discussion_r4105313196
##########
datafusion/common/src/functional_dependencies.rs:
##########
@@ -144,6 +144,13 @@ pub struct FunctionalDependence {
/// such as after LEFT JOIN or RIGHT JOIN operations, this property may
/// change.
pub nullable: bool,
+ /// The NULL-comparison semantics under which this dependency holds. The
+ /// conservative default, [`NullEquality::NullEqualsNothing`], means it
+ /// holds only across rows whose determinant contains no NULLs; e.g. a
+ /// nullable `UNIQUE` constraint permits multiple NULL rows that may
differ.
+ /// [`NullEquality::NullEqualsNull`] means it also holds when NULL
+ /// determinant values are treated as equal; e.g. a `GROUP BY` key.
+ pub null_equality: NullEquality,
Review Comment:
`FunctionalDependence` is a public struct with all-public fields, so adding
`pub null_equality` breaks downstream struct literals and exhaustive
destructuring. You had to change the patterns in this file for the same reason.
The PR description says "No API changes": please add the `api change` label and
a short note in `docs/source/library-user-guide/upgrading/56.0.0.md`, for
example:
```diff
+### `FunctionalDependence` has a new `null_equality` field
+
+`FunctionalDependence` now records the NULL semantics its dependency holds
under.
+Build it with `FunctionalDependence::new(..)` and, when needed,
+`.with_null_equality(NullEquality::NullEqualsNull)` instead of a struct
literal.
```
##########
datafusion/common/src/functional_dependencies.rs:
##########
Review Comment:
The whole-GROUP-BY-key dep is skipped whenever some dep's source is a subset
of the key, even when that dep can't be used across NULLs. With `u(x INT
UNIQUE, y)`, `SELECT x, y, c FROM (SELECT x, y, count(*) c FROM u GROUP BY x,
y) ORDER BY x, y, c` now keeps `c`, even though `(x, y)` is unique after
grouping. Fine to handle in a follow-up.
```diff
item.source_indices.iter().all(|idx| idx < &count)
+ && item.is_valid_across_nulls(aggr_schema)
}) {
```
I tested both fixes locally before reverting them. With them,
functional_dependencies.slt passes, the plans become Sort: p.a and Sort: x, y,
and the results with DESC tie-breakers are still correct.
##########
datafusion/common/src/functional_dependencies.rs:
##########
@@ -386,7 +416,11 @@ impl FunctionalDependencies {
fn downgrade_dependencies(&mut self) {
// Delete nullable dependencies, since they are no longer valid:
self.deps.retain(|item| !item.nullable);
- self.deps.iter_mut().for_each(|item| item.nullable = true);
+ // Survivors become nullable, and the new NULLs are not equal to one
another:
+ self.deps.iter_mut().for_each(|item| {
+ item.nullable = true;
+ item.null_equality = NullEquality::NullEqualsNothing;
Review Comment:
Only `nullable = false` deps survive `downgrade_dependencies`. For those,
any new NULL in the source comes from a padded row, and a padded row is NULL in
every column on that side, so the targets match. Resetting to
`NullEqualsNothing` stops sort pruning that `main` did correctly. `SELECT l.k,
p.a, p.b FROM l LEFT JOIN p ON l.k = p.a ORDER BY p.a, p.b` (`p.a` PRIMARY KEY)
now keeps `p.b`, where `main` plans `Sort: p.a`.
```diff
- // Survivors become nullable, and the new NULLs are not equal to
one another:
+ // Survivors had a non-null determinant, so every new NULL comes
from a
+ // padded row whose targets are all NULL too:
self.deps.iter_mut().for_each(|item| {
item.nullable = true;
- item.null_equality = NullEquality::NullEqualsNothing;
+ item.null_equality = NullEquality::NullEqualsNull;
});
```
Please add a LEFT JOIN + PK case to `functional_dependencies.slt`.
--
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]
