fix: do not prune ORDER BY keys using nullable UNIQUE constraints by buraksenn · Pull Request #23918 · apache/datafusion

buraksenn · 2026-07-27T08:18:18Z

Which issue does this PR close?

Closes Incorrect results: ORDER BY drops a sort key based on a nullable UNIQUE constraint #23818.

Rationale for this change

Please check the issue for details, but the main idea is that a nullable UNIQUE column permits multiple NULL rows, so it does not determine later sort keys across them and they must not be pruned.

What changes are included in this PR?

FunctionalDependence gets a null_equality field that records the NULL semantics under which the dependency holds. The default is NullEqualsNothing.
Sort-key pruning only uses a dependency that holds when NULLs are treated as equal (for example a GROUP BY key), has a non-nullable determinant, or whose source columns are all NOT NULL.
A primary key that is downgraded on the NULL-padded side of an outer join keeps NullEqualsNull, because padded rows are NULL in every column on that side.
The dependency for the whole GROUP BY key is no longer skipped because of a narrower dependency that doesn't hold across NULLs.

Are these changes tested?

Yes. Regression tests are added in functional_dependencies.slt for nullable UNIQUE, NOT NULL UNIQUE, LEFT JOIN with a primary key, and GROUP BY over a nullable UNIQUE column.

Are there any user-facing changes?

Yes: incorrect results are fixed. There is also an API change: the public FunctionalDependence struct has a new null_equality field, so code that builds it with a struct literal must switch to FunctionalDependence::new(..). This is documented in the 56.0.0 upgrade guide.

codecov-commenter · 2026-07-27T09:07:21Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 82.58%. Comparing base (0af6818) to head (ca574fd).
⚠️ Report is 7 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #23918      +/-   ##
==========================================
- Coverage   82.58%   82.58%   -0.01%     
==========================================
  Files        1142     1142              
  Lines      440930   440985      +55     
  Branches   440930   440985      +55     
==========================================
+ Hits       364123   364168      +45     
+ Misses      54804    54803       -1     
- Partials    22003    22014      +11

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

neilconway · 2026-07-27T14:51:09Z

There are a bunch of related bugs / issues here:

As well as the issue this PR targets, #23818. Based on #23636 (review) , I would guess there might also be an issue with window queries.

Probably makes sense to look at this whole area holistically. I see that @alamb and @LDM-A have also been working on related tickets, so we should probably make sure we coordinate.

buraksenn · 2026-07-27T16:13:15Z

There are a bunch of related bugs / issues here:

Incorrect results with unique constraints #23634

Incorrect results: GROUP BY column dropped based on a nullable UNIQUE constraint #23819

Incorrect results: GROUP BY returns two rows for the NULL group with a nullable UNIQUE constraint #23820

As well as the issue this PR targets, #23818. Based on #23636 (review) , I would guess there might also be an issue with window queries.

Probably makes sense to look at this whole area holistically. I see that @alamb and @LDM-A have also been working on related tickets, so we should probably make sure we coordinate.

Thanks for the heads up. I thought with the tests added in #23821 I can implement a fix but did not see the discussion in #23820. I'll follow it and can also close this PR if it will implemented in a combined fashion

LDM-A · 2026-07-27T17:16:07Z

There are a bunch of related bugs / issues here:

Incorrect results with unique constraints #23634

Incorrect results: GROUP BY column dropped based on a nullable UNIQUE constraint #23819

Incorrect results: GROUP BY returns two rows for the NULL group with a nullable UNIQUE constraint #23820

As well as the issue this PR targets, #23818. Based on #23636 (review) , I would guess there might also be an issue with window queries.
Probably makes sense to look at this whole area holistically. I see that @alamb and @LDM-A have also been working on related tickets, so we should probably make sure we coordinate.

Thanks for the heads up. I thought with the tests added in #23821 I can implement a fix but did not see the discussion in #23820. I'll follow it and can also close this PR if it will implemented in a combined fashion

I have not had too much chance to look into the #23820 ticket after my initial weekend. However @alamb mentioned a refactor of the functional dependency file to take care off all the free functions in it. Maybe since there is multiple PRs coming making changes we can do a refactor then add in the changes. Rather than merging some now then refactoring those with the refactor and finishing some off

LDM-A · 2026-07-27T17:43:52Z

            .strip_backtrace();

-        insta::assert_snapshot!(e, @r#"Error during planning: Extension planner for NoOp created an ExecutionPlan with mismatched schema. LogicalPlan schema: DFSchema { inner: Schema { fields: [Field { name: "a", data_type: Int32 }], metadata: {} }, field_qualifiers: [None], functional_dependencies: FunctionalDependencies { deps: [] } }, ExecutionPlan schema: Schema { fields: [Field { name: "b", data_type: Int32 }], metadata: {} }"#);
+        insta::assert_snapshot!(e, @r#"Error during planning: Extension planner for NoOp created an ExecutionPlan with mismatched schema. LogicalPlan schema: DFSchema { inner: Schema { fields: [Field { name: "a", data_type: Int32 }], metadata: {} }, field_qualifiers: [None], functional_dependencies: FunctionalDependencies { deps: [], null_equalities: [] } }, ExecutionPlan schema: Schema { fields: [Field { name: "b", data_type: Int32 }], metadata: {} }"#);


For this as well if we make NullEquality part of FunctionalDependence instead of FunctionalDependencies it would create an easier to read error in my opinion

I've moved it int other FunctionalDependence struct which also simplified the PR imo

buraksenn · 2026-07-30T21:17:24Z

@alamb can you take a look into this when you've time? I think there needs to be choice of incorporating NullEquality field into FunctionalDependence struct or keeping it as a separate vectors (this PR does this way). If we decide on that I think these 3 issues can be implemented without blocking each other. If this approach is not ok I can explore alternatives and share it as well

edit: just moved field into the FunctionalDependence struct

github-actions · 2026-08-04T15:47:26Z

Thank you for opening this pull request!

Reviewer note: cargo-semver-checks reported the current version number is not SemVer-compatible with the changes in this pull request (compared against the base branch).

Details

     Cloning apache/main
    Building datafusion-common v55.1.0 (current)
       Built [  22.815s] (current)
     Parsing datafusion-common v55.1.0 (current)
      Parsed [   0.045s] (current)
    Building datafusion-common v55.1.0 (baseline)
       Built [  24.573s] (baseline)
     Parsing datafusion-common v55.1.0 (baseline)
      Parsed [   0.048s] (baseline)
    Checking datafusion-common v55.1.0 -> v55.1.0 (no change; assume patch)
     Checked [   0.522s] 223 checks: 222 pass, 1 fail, 0 warn, 31 skip

--- failure constructible_struct_adds_field: struct exhaustively constructible through public API adds field ---

Description:
A pub struct that could be exhaustively constructed with a literal using only public API has a new pub field, breaking existing exhaustive literals.
        ref: https://doc.rust-lang.org/reference/expressions/struct-expr.html
       impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.50.0/src/lints/constructible_struct_adds_field.ron

Failed in:
  field FunctionalDependence.null_equality in /home/runner/work/datafusion/datafusion/datafusion/common/src/functional_dependencies.rs:153

     Summary semver requires new major version: 1 major and 0 minor checks failed
    Finished [  49.747s] datafusion-common
    Building datafusion-expr v55.1.0 (current)
       Built [  22.383s] (current)
     Parsing datafusion-expr v55.1.0 (current)
      Parsed [   0.059s] (current)
    Building datafusion-expr v55.1.0 (baseline)
       Built [  22.591s] (baseline)
     Parsing datafusion-expr v55.1.0 (baseline)
      Parsed [   0.062s] (baseline)
    Checking datafusion-expr v55.1.0 -> v55.1.0 (no change; assume patch)
     Checked [   0.905s] 223 checks: 223 pass, 31 skip
     Summary no semver update required
    Finished [  46.970s] datafusion-expr
    Building datafusion-sqllogictest v55.1.0 (current)
       Built [  70.683s] (current)
     Parsing datafusion-sqllogictest v55.1.0 (current)
      Parsed [   0.016s] (current)
    Building datafusion-sqllogictest v55.1.0 (baseline)
       Built [  64.926s] (baseline)
     Parsing datafusion-sqllogictest v55.1.0 (baseline)
      Parsed [   0.017s] (baseline)
    Checking datafusion-sqllogictest v55.1.0 -> v55.1.0 (no change; assume patch)
     Checked [   0.063s] 223 checks: 223 pass, 31 skip
     Summary no semver update required
    Finished [ 138.311s] datafusion-sqllogictest

jayzhan211

Thanks @buraksenn , here are some suggestions

jayzhan211 · 2026-09-25T14:07:21Z

+    /// 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,


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:

+### `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.

jayzhan211 · 2026-09-25T14:07:52Z

+        // 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;


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.

- // 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.

jayzhan211 · 2026-09-25T14:08:39Z


            // The following simple comparison is working well because
            // GROUP BY expressions come here as a prefix.
            item.source_indices.iter().all(|idx| idx < &count)


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.

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.

buraksenn · 2026-09-29T14:04:43Z

Thanks for the review @jayzhan211. I've applied the reviews and adjusted PR description. I can't add api-change label since I dont have permission though

jayzhan211

Thanks @buraksenn !

fix: do not prune ORDER BY keys using nullable UNIQUE constraints

3f37107

buraksenn commented Jul 27, 2026

View reviewed changes

Comment thread datafusion/common/src/functional_dependencies.rs Outdated

github-actions Bot added core Core DataFusion crate sqllogictest SQL Logic Tests (.slt) common Related to common crate labels Jul 27, 2026

LDM-A reviewed Jul 27, 2026

View reviewed changes

move null_equality to FunctionalDependence struct

f94b82f

github-actions Bot removed the core Core DataFusion crate label Aug 4, 2026

github-actions Bot added the auto detected api change Auto detected API change label Aug 4, 2026

buraksenn added 3 commits August 13, 2026 20:48

Merge remote-tracking branch 'upstream/main' into omp/pr-sync-23918

390c083

Merge branch 'main' into fix-23818-nullable-unique-order-by

92fd688

Merge branch 'main' into fix-23818-nullable-unique-order-by

a9c7494

jayzhan211 reviewed Sep 25, 2026

View reviewed changes

buraksenn added 2 commits September 29, 2026 16:30

Merge branch 'main' into fix-23818-nullable-unique-order-by

5a5af92

apply reviews

1727d2a

github-actions Bot added the documentation Improvements or additions to documentation label Sep 29, 2026

fix missed failing test

ca574fd

github-actions Bot added the logical-expr Logical plan and expressions label Sep 29, 2026

jayzhan211 approved these changes Sep 30, 2026

View reviewed changes

jayzhan211 added this pull request to the merge queue Sep 30, 2026

Merged via the queue into apache:main with commit de9d1f9 Sep 30, 2026
43 checks passed

github-actions Bot added the v56.0.0 label Sep 30, 2026

Conversation

buraksenn commented Jul 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

Uh oh!

Uh oh!

codecov-commenter commented Jul 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

neilconway commented Jul 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

buraksenn commented Jul 27, 2026

Uh oh!

LDM-A commented Jul 27, 2026

Uh oh!

LDM-A Jul 27, 2026

Choose a reason for hiding this comment

Uh oh!

buraksenn Aug 4, 2026

Choose a reason for hiding this comment

Uh oh!

buraksenn commented Jul 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions Bot commented Aug 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jayzhan211 left a comment

Choose a reason for hiding this comment

Uh oh!

jayzhan211 Sep 25, 2026

Choose a reason for hiding this comment

Uh oh!

jayzhan211 Sep 25, 2026

Choose a reason for hiding this comment

Uh oh!

jayzhan211 Sep 25, 2026

Choose a reason for hiding this comment

Uh oh!

buraksenn commented Sep 29, 2026

Uh oh!

jayzhan211 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

buraksenn commented Jul 27, 2026 •

edited

Loading

codecov-commenter commented Jul 27, 2026 •

edited

Loading

neilconway commented Jul 27, 2026 •

edited

Loading

buraksenn commented Jul 30, 2026 •

edited

Loading

github-actions Bot commented Aug 4, 2026 •

edited

Loading