`ARRAY_AGG .. ORDER BY` fails on `NULL` typed columns

[sadboy]

Describe the bug

This appears to have regressed somewhere between v48 and v49.

To Reproduce

DataFusion CLI v50.3.0
> create table source as
select null as col
union all
select null
union all
select null
;

0 row(s) fetched. 
Elapsed 0.009 seconds.

> select
    array_agg(distinct col) within group (order by col)
from source
group by all
;

Arrow error: Invalid argument error: The data type type Null has no natural order

Expected behavior

DataFusion CLI v48.0.1
> create table source as
select null as col
union all
select null
union all
select null
;

0 row(s) fetched. 
Elapsed 0.011 seconds.

> select
    array_agg(distinct col) within group (order by col)
from source
group by all
;

+--------------------------------+
| array_agg(DISTINCT source.col) |
+--------------------------------+
| [NULL]                         |
+--------------------------------+
1 row(s) fetched. 
Elapsed 0.019 seconds.

Additional context

The exact semantics of a Null typed column is perhaps debatable, but if this behavior is intentional, we should probably catch this at planning time instead of letting it err at execution time.

[VedantSawant616]

this case should be validated during query planning, since the planner already knows the column is of type Null and that ORDER BY isnt valid for it.
the older behavior ([NULL]) was permissive, but the new one fails at execution time. the right fix is to add a planner level check so invalid sort operations on Null types are caught early with a clearer error.

[asolimando]

I would expect the create table statement to be failing already.

SQL standard requires null values to be typed (implicitly via type coercion, or explicitly via cast), does Datafusion behaves differently in this respect?

[VedantSawant616]

that’s a great point, @asolimando. datafusion does handle this a bit differently from the sql standard, possibly influenced by how arrow represents datatype::null as a real type, so when running create table as select null as col, datafusion assigns that null type instead of coercing or rejecting it. this makes the statement valid within its type system, but it also highlights the issue, the planner should recognize that datatype::null isn’t sortable and reject the order by during planning rather than at execution.

[sadboy]

I would expect the create table statement to be failing already.

Yes, disallowing Null-typed columns in CREATE TABLE would be more SQL conformant.

However if DF allowing Null-typed column in tables is a concious decision, then this should be considered a regression and we should revert back to the v48 behavior. Disallowing ORDER BY Null would be problematic as this

DataFusion CLI v50.3.0
> with
        source as (
select null as col
union all
select null
union all
select null
        )

select
    array_agg(distinct col) within group (order by col)
from source
group by all
;

+---------------------------------------------------------------------+
| array_agg(DISTINCT source.col) ORDER BY [source.col ASC NULLS LAST] |
+---------------------------------------------------------------------+
| [NULL]                                                              |
+---------------------------------------------------------------------+
1 row(s) fetched. 
Elapsed 0.010 seconds.

is allowed and works the same in all major SQL engines.

[VedantSawant616]

this finding invalidates the earlier assumption that the query should fail during planning. the query is clearly valid, through successful execution of the cte version.
this points to a regression , the v48 behavior (matching the current cte behavior) appears to be the correct and expected one. the issue likely lies in how the null data type is inferred or handled in the execution plan, depending on whether the data comes from a persisted table or an in-memory cte.