mustafasrepo commented on code in PR #9686:
URL: https://github.com/apache/arrow-datafusion/pull/9686#discussion_r1529805006
##########
datafusion/physical-plan/src/windows/mod.rs:
##########
@@ -174,20 +174,15 @@ fn create_built_in_window_expr(
name: String,
ignore_nulls: bool,
) -> Result<Arc<dyn BuiltInWindowFunctionExpr>> {
- // need to get the types into an owned vec for some reason
- let input_types: Vec<_> = args
- .iter()
- .map(|arg| arg.data_type(input_schema))
- .collect::<Result<_>>()?;
+ // derive the output datatype from incoming schema
+ let out_data_type: &DataType =
input_schema.field_with_name(&name)?.data_type();
Review Comment:
Actually, I think with these changes parameter `input_schema: &Schema,` name
is misleading in the `create_built_in_window_expr` function. Similarly,
`logical_input_schema: &DFSchema,` parameter for the `create_window_expr` is
misleading. Now these parameters are the schema of the window function (not
input schema). Hence, I think it is guaranteed to find name of the window
function in the schema (As long as window function name calculations are
consistent). However, we should change these parameters name I think. I
checked, how `LogicalPlan::Window` calculates its schema.
It call `exprlist_to_fields` util to calculate fields of window expressions.
- Which in turn uses `.to_field` method on `input_schema`
- Which in turn uses `Expr::get_type` method on `input_schema`
- which calculates output type using `fun.return_type(&data_types)`.
In this sense, I think this PR behaves same with current version, and
removes duplication. However, we should change parameter/variable names to
reflect which data it actually uses.
--
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]
