theirix commented on code in PR #24419:
URL: https://github.com/apache/datafusion/pull/24419#discussion_r3825999987
##########
datafusion/functions-aggregate/src/median.rs:
##########
@@ -91,6 +68,7 @@ make_udaf_expr_and_func!(
#[derive(PartialEq, Eq, Hash, Debug)]
pub struct Median {
signature: Signature,
+ percentile_cont: PercentileCont,
Review Comment:
Well, I've tried, but the simplifier has a flaw with the window aggregation
functions (median(x) OVER ) - it is unable to call a simplifier and just falls
back to a normal UDAF accumulator, making the rewrite irrelevant.
This could be a reason why `approx_percentile_cont` doesn't do rewrites
either. I can try fixing these simplifier bugs later.
```
4. query failed: DataFusion error: Internal error: median accumulator should
have been simplified to standard percentile_cont.
This issue was likely caused by a bug in DataFusion's code. Please help us
to resolve this by filing a bug report in our issue tracker:
https://github.com/apache/datafusion/issues
[SQL] SELECT
timestamp,
tags,
value,
median(DISTINCT value) OVER (
PARTITION BY tags
ORDER BY timestamp
ROWS BETWEEN UNBOUNDED PRECEDING AND CURRENT ROW
) AS distinct_median
FROM median_window_test
ORDER BY tags, timestamp;
at
/Users/irix/projects/third-party/datafusion/datafusion/sqllogictest/test_files/aggregate.slt:1080
```
</details>
With median distinct, it is also failing with some conflicting optimisation
with group by - the plan omits grouping entirely
```
[SQL] explain select median(distinct c) from t;
[Diff] (-expected|+actual)
logical_plan
- 01)Projection: median(alias1) AS median(DISTINCT t.c)
- 02)--Aggregate: groupBy=[[]], aggr=[[median(alias1)]]
- 03)----Aggregate: groupBy=[[CAST(t.c AS Float64) AS alias1]], aggr=[[]]
- 04)------TableScan: t projection=[c]
+ 01)Aggregate: groupBy=[[]], aggr=[[percentile_cont(DISTINCT CAST(t.c AS
Float64), Float64(0.5)) AS median(DISTINCT t.c)]]
+ 02)--TableScan: t projection=[c]
physical_plan
- 01)ProjectionExec: expr=[median(alias1)@0 as median(DISTINCT t.c)]
- 02)--AggregateExec: mode=Final, gby=[], aggr=[median(alias1)]
- 03)----CoalescePartitionsExec
- 04)------AggregateExec: mode=Partial, gby=[], aggr=[median(alias1)]
- 05)--------AggregateExec: mode=FinalPartitioned, gby=[alias1@0 as
alias1], aggr=[]
- 06)----------RepartitionExec: partitioning=Hash([alias1@0], 4),
input_partitions=1
- 07)------------AggregateExec: mode=Partial, gby=[CAST(c@0 AS Float64) as
alias1], aggr=[]
- 08)--------------DataSourceExec: partitions=1, partition_sizes=[1]
+ 01)AggregateExec: mode=Single, gby=[], aggr=[percentile_cont(DISTINCT
t.c, 0.5) as median(DISTINCT t.c)]
+ 02)--DataSourceExec: partitions=1, partition_sizes=[1]
at
/Users/irix/projects/third-party/datafusion/datafusion/sqllogictest/test_files/aggregate.slt:1502
```
So, in the middle ground, I refactored the code a bit to avoid schema
twiddling in median - now we just create accumulators explicitly, and it looks
more concise.
--
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]
