alamb commented on code in PR #8920:
URL: https://github.com/apache/arrow-datafusion/pull/8920#discussion_r1460851196
##########
datafusion/physical-expr/src/window/rank.rs:
##########
@@ -89,11 +93,7 @@ impl BuiltInWindowFunctionExpr for Rank {
fn field(&self) -> Result<Field> {
let nullable = false;
- let data_type = match self.rank_type {
- RankType::Basic | RankType::Dense => DataType::UInt64,
- RankType::Percent => DataType::Float64,
- };
- Ok(Field::new(self.name(), data_type, nullable))
+ Ok(Field::new(self.name(), self.data_type.clone(), nullable))
Review Comment:
❤️
##########
datafusion/physical-expr/src/window/cume_dist.rs:
##########
@@ -34,11 +34,15 @@ use std::sync::Arc;
#[derive(Debug)]
pub struct CumeDist {
name: String,
+ data_type: DataType,
Review Comment:
I think it would help to note that this is the output type, not the argument
type
```suggestion
/// Output data type
data_type: DataType,
```
##########
datafusion/core/src/physical_planner.rs:
##########
@@ -719,14 +720,16 @@ impl DefaultPhysicalPlanner {
}
let logical_input_schema = input.schema();
- let physical_input_schema = input_exec.schema();
+ // Extend the schema to include window expression fields
as builtin window functions derives its datatype from incoming schema
Review Comment:
Shouldn't `input.schema()` reflect all the columns that the input produces?
Or does the `WindowAggExec` create new columns "internally" by evaluating
the window expressions?
##########
datafusion/sqllogictest/test_files/window.slt:
##########
@@ -3906,3 +3906,69 @@ ProjectionExec: expr=[sn@0 as sn, ts@1 as ts, currency@2
as currency, amount@3 a
--BoundedWindowAggExec: wdw=[SUM(table_with_pk.amount) ORDER BY
[table_with_pk.sn ASC NULLS LAST] ROWS BETWEEN UNBOUNDED PRECEDING AND CURRENT
ROW: Ok(Field { name: "SUM(table_with_pk.amount) ORDER BY [table_with_pk.sn ASC
NULLS LAST] ROWS BETWEEN UNBOUNDED PRECEDING AND CURRENT ROW", data_type:
Float64, nullable: true, dict_id: 0, dict_is_ordered: false, metadata: {} }),
frame: WindowFrame { units: Rows, start_bound: Preceding(UInt64(NULL)),
end_bound: CurrentRow }], mode=[Sorted]
----SortExec: expr=[sn@0 ASC NULLS LAST]
------MemoryExec: partitions=1, partition_sizes=[1]
+
Review Comment:
These tests all seem to pass for me on main as well (without the changes in
this PR). Is that expected?
--
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]
