fix: UNIQUE constraint with NULLs incorrectly collapses GROUP BY groups

[xiedeyantu]

Which issue does this PR close?

• Closes Incorrect query results for GROUP BY with UNIQUE constraint #21507

Rationale for this change

DataFusion was deriving functional dependencies too aggressively in two cases:

1. UNIQUE constraints were being treated as functional dependencies even when one or more key columns were nullable. Under SQL semantics, UNIQUE allows multiple NULL values, so such constraints do not guarantee row-level uniqueness and cannot be used to eliminate GROUP BY expressions safely.

2. Recursive CTEs could inherit functional dependencies from the static term (anchor) even though the recursive term may produce rows that violate those dependencies. This could cause downstream optimizer rules to make incorrect uniqueness-based rewrites.

What changes are included in this PR?

This PR tightens functional-dependency derivation and propagation:

• FunctionalDependencies::new_from_constraints now takes per-field nullability and only derives a functional dependency from a UNIQUE constraint when all determinant columns are non-nullable.
• Nullable UNIQUE constraints are omitted entirely instead of being recorded as nullable functional dependencies.
• Aggregate output functional dependencies for GROUP BY are preserved so they can still be used safely by later planning stages.
• Recursive CTE plans now clear inherited functional dependencies from the anchor schema, so outer consumers do not assume uniqueness that the recursive term may break.
• Added regression coverage for:
  • GROUP BY over nullable UNIQUE columns with multiple NULL values
  • join cases where non-null unique keys can still support expansion, but nullable unique keys cannot
  • recursive CTEs where anchor-term uniqueness must not leak into the full recursive result
• Added an upgrade note for the FunctionalDependencies::new_from_constraints API change.

Are these changes tested?

Yes. This PR adds regression coverage for nullable-UNIQUE GROUP BY cases, recursive CTE functional-dependency propagation, and join cases that distinguish nullable from non-null unique keys.

Are there any user-facing changes?

Yes. Queries that rely on grouping or distinctness analysis will no longer assume uniqueness from nullable UNIQUE constraints, and recursive CTE outputs will no longer inherit invalid functional dependencies from the anchor term. This prevents incorrect optimizer rewrites and fixes wrong results in affected queries.

[nuno-faria]

Thanks @xiedeyantu. While this fixes the original issue, it now treats all nulls as different when aggregating, which I don't think is correct. For example:

CREATE TABLE t(a INT, b INT, c INT, UNIQUE(a));
INSERT INTO t VALUES (1, 10, 100), (NULL, 20, 200), (NULL, 30, 300);
SELECT a, SUM(c) AS total FROM t GROUP BY a;
+------+-------+
| a    | total |
+------+-------+
| 1    | 100   |
| NULL | 200   |
| NULL | 300   |
+------+-------+

[xiedeyantu]

Thanks @xiedeyantu. While this fixes the original issue, it now treats all nulls as different when aggregating, which I don't think is correct. For example:

CREATE TABLE t(a INT, b INT, c INT, UNIQUE(a));
INSERT INTO t VALUES (1, 10, 100), (NULL, 20, 200), (NULL, 30, 300);
SELECT a, SUM(c) AS total FROM t GROUP BY a;
+------+-------+
| a    | total |
+------+-------+
| 1    | 100   |
| NULL | 200   |
| NULL | 300   |
+------+-------+

@nuno-faria Thank you for your comprehensive analysis. I have refactored the code and added the test cases you provided; please review it again.

[nuno-faria]

Thanks @xiedeyantu, I have some extra comments.
cc: @neilconway perhaps you would like to review this as well.

[neilconway]

What about taking a different approach here: I'd argue a UNIQUE constraint on a nullable column should not be an FD in the first place. Rather than producing such constraints and then filtering them out in the various places that use the FDs, why not change new_from_constraints to not return an FD in this circumstance to begin with?

[xiedeyantu]

What about taking a different approach here: I'd argue a UNIQUE constraint on a nullable column should not be an FD in the first place. Rather than producing such constraints and then filtering them out in the various places that use the FDs, why not change new_from_constraints to not return an FD in this circumstance to begin with?

I've tried making the modifications based on your suggestions; please check if they meet your requirements.

[nuno-faria]

Thanks @xiedeyantu, LGTM. I leave just some minor requests below.

[xiedeyantu]

This error should be unrelated to PR.

Run cargo test --profile ci -p datafusion-ffi --lib --tests --features integration-tests
  cargo test --profile ci -p datafusion-ffi --lib --tests --features integration-tests
  shell: /bin/bash --noprofile --norc -e -o pipefail {0}
  env:
    CACHE_ON_FAILURE: false
    CARGO_INCREMENTAL: 0
    RUST_BACKTRACE: 1
    RUSTFLAGS: -C debuginfo=line-tables-only -C incremental=false
error: error: unexpected argument 'test' found

Usage: rustup-init[EXE] [OPTIONS]

For more information, try '--help'.


Stack backtrace:
   0: <std::backtrace::Backtrace>::capture
   1: anyhow::kind::Adhoc::new
   2: rustup::cli::setup_mode::main::{{closure}}::{{closure}}
   3: rustup_init::run_rustup_inner::{{closure}}::{{closure}}
   4: rustup_init::run_rustup::{{closure}}::{{closure}}
   5: rustup_init::main::{{closure}}
   6: tokio::runtime::runtime::Runtime::block_on
   7: rustup_init::main
   8: std::sys::backtrace::__rust_begin_short_backtrace
   9: _main

[xiedeyantu]

@nuno-faria Please let me know if there’s anything else I need to do.

[nuno-faria]

@nuno-faria Please let me know if there’s anything else I need to do.

LGTM. @neilconway would you mind reviewing again?

[xiedeyantu]

@nuno-faria Please let me know if there’s anything else I need to do.

LGTM. @neilconway would you mind reviewing again?

Thanks for your review @nuno-faria ! Let's wait for @neilconway 's comments.

[neilconway]

After this PR, do we still need the concept of a functional dependency with nullable as true? I wonder if we can get eliminate that concept entirely.

[neilconway]

Seems a bit fragile to leave FDs on plan.schema() but only strip them out here when they are being used. Should there be FDs on plan.schema() in the first place?

BTW this might be affected by / related to #22037

[neilconway]

I wonder if it would be better to mark the UNIQUE column as NOT NULL, so we don't lose the intent of the original test. (Here and below)

[xiedeyantu]

After this PR, do we still need the concept of a functional dependency with nullable as true? I wonder if we can get eliminate that concept entirely.

@neilconway To be honest, my initial goal was to fix this with minimal changes. Removing  nullable  would likely require a much wider refactor. I've addressed your other comments, but I'm unsure how to proceed here. Would it be better to leave this for a future cleanup?

[neilconway]

@xiedeyantu Yeah, I think it's fine to defer that cleanup to a future PR. Can you resolve the merge conflicts?

[xiedeyantu]

@xiedeyantu Yeah, I think it's fine to defer that cleanup to a future PR. Can you resolve the merge conflicts?

I have resolved the conflict.