jcsherin commented on PR #12374: URL: https://github.com/apache/datafusion/pull/12374#issuecomment-2364187830
> If this is "too late" for this kind of comment, please let me know and I'll delete. I hadn't seen the issue / PR work until today. @Michael-J-Ward Appreciate the extra set of eyes. Thank you, for reviewing the code :raised_hands: > I dug into it because needing to add empty `&str` to all the tests in `datafusion/expr/src/expr.rs` made me think it's not the right abstraction. https://github.com/apache/datafusion/blob/5cc7d06a0dd95dace219c666535630c1c7de3c3c/datafusion/expr/src/expr.rs#L704-L724 The `WindowFunctionDefintion::return_type` is used only for tests. And I believe these unit tests do not provide much value. When `row_number()` was converted to `WindowUDF` the corresponding return type unit test for `row_number()` was removed. And I think this applies to the remaining builtin window methods which are pending conversion to `WindowUDF`. @Michael-J-Ward @jayzhan211 Thank you. -- 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]
