Handle nulls in approx_percentile_cont #11721
Conversation
Dandandan
changed the title
|
|
||
| if args.ignore_nulls { | ||
| return not_impl_err!( | ||
| "IGNORE NULLS clause not yet supported for APPROX_PERCENTILE_CONT" |
There was a problem hiding this comment.
Effectively this fix actually does ignore nulls, but we don't have a way to disable setting RESPECT NULLS for aggregate functions, I can file a follow-up ticket.
I believe we should also move DataFusion to SELECT percentile_cont(0.5) WITHIN GROUP(order by v) (used by PostgreSQL, others) syntax...
There was a problem hiding this comment.
Maybe we can trigger it if used as a window function PERCENTILE_CONT(...) OVER (PARTITION BY ... IGNORE NULLS) 🤔
There was a problem hiding this comment.
Hm... for now I think it makes sense to ignore the ignore_nulls argument (like we did before) and remove any nulls which makes the most sense. Null handling is broken anyway (resulting in 0 instead of NULL), so no one will rely on percentile_cont on columns with nulls.
I'll file a ticket to discuss standardizing PERCENTILE_CONT and having it behave like PostgreSQL / Spark / Snowflake, etc....
| query I | ||
| SELECT APPROX_PERCENTILE_CONT(v, 0.5) FROM (VALUES (1), (2), (3), (NULL), (NULL), (NULL)) as t (v); | ||
| ---- | ||
| 2 |
There was a problem hiding this comment.
This was 0 before (not even NULL)
2 participants
Which issue does this PR close?
Closes #11720
Rationale for this change
Fix a bug in
approx_percentile_contwhen having nulls in inputWhat changes are included in this PR?
Filters input arrays on nulls if present
Are these changes tested?
Yes, additional sql test
Are there any user-facing changes?
Just the fix