alamb opened a new issue, #23818: URL: https://github.com/apache/datafusion/issues/23818
### Is your feature request related to a problem or challenge? Follow-up to #23634: the same nullable-`UNIQUE` confusion also affects `ORDER BY`. A trailing sort key is dropped when an earlier key functionally determines it. That is valid for a `PRIMARY KEY`, but not for a `UNIQUE` column, which permits multiple `NULL` rows and therefore does not determine anything across them. ```sql CREATE TABLE t (x INT UNIQUE, y INT) AS VALUES (NULL, 2), (NULL, 1), (1, 3); SELECT x, y FROM t ORDER BY x NULLS LAST, y; -- returns: -- 1 3 -- NULL 2 -- NULL 1 -- -- should be: -- 1 3 -- NULL 1 -- NULL 2 ``` The plan shows the `y` sort key is gone entirely: ```sql EXPLAIN SELECT x, y FROM t ORDER BY x NULLS LAST, y; -- logical_plan -- 01)Sort: t.x ASC NULLS LAST <-- `y` dropped -- 02)--TableScan: t projection=[x, y] -- physical_plan -- 01)SortExec: expr=[x@0 ASC NULLS LAST], preserve_partitioning=[false] -- 02)--DataSourceExec: partitions=1, partition_sizes=[1] ``` Verified against duckdb: ``` ❯ duckdb -c "create table t (x int unique, y int); insert into t values (null,2),(null,1),(1,3); select x, y from t order by x nulls last, y;" ┌───────┬───────┐ │ x │ y │ │ int32 │ int32 │ ├───────┼───────┤ │ 1 │ 3 │ │ NULL │ 1 │ │ NULL │ 2 │ └───────┴───────┘ ``` Note the two `NULL` rows are stored with `y` descending on purpose — with them in ascending order the answer comes out right by accident, even though the plan is equally wrong. ### Describe the solution you'd like Fix the bug. The pruning happens in [`get_required_sort_exprs_indices`](https://github.com/apache/datafusion/blob/main/datafusion/common/src/functional_dependencies.rs), called from the `eliminate_duplicated_expr` rule. Its `removable` check looks at `target_indices` and `source_indices` but never at `FunctionalDependence::nullable`: ```rust let removable = dependencies.deps.iter().any(|dependency| { dependency.target_indices.contains(&field_idx) && dependency .source_indices .iter() .all(|source_idx| known_field_indices.contains(source_idx)) }); ``` A dependency derived from a `UNIQUE` constraint carries `nullable == true`, and that needs to disqualify it here when a source column really can be `NULL` — the same guard `Filter::is_scalar` already applies, and the one being added to `ReplaceDistinctWithAggregate` in #23636. ### Describe alternatives you've considered ### Additional context Related: - #23634 - #23548 - #23636 -- 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]
