Feat: [datafusion-spark] Migrate avg from comet to datafusion-spark and add tests. #17871
Conversation
|
CC : @andygrove |
| Self { | ||
| name: name.into(), | ||
| signature: Signature::user_defined(Immutable), | ||
| input_data_type: data_type, |
There was a problem hiding this comment.
Can input data type vary? Seems to be only Float64 right now, will there be more options in the future? Same for return data type
There was a problem hiding this comment.
I still think it is confusing to require the input & result data types as inputs here; considering input type should be controlled by signature/coerce_types() only, and result data type should be same as return_type() which apparently uses avg_return_type()
There was a problem hiding this comment.
It is unclear to me why we do this - I think we can either add a patch to be more direct or merge it with Datafusion avg, whichever route is decided upon in the future
codetyri0n
changed the title
There was a problem hiding this comment.
I do wonder if it is possible to merge this code with DataFusion avg, perhaps using generics to control the count type and bool flag for ansi mode in the future, to reduce duplication? Or would it be not worth the effort or are there more differences than just those two?
| Self { | ||
| name: name.into(), | ||
| signature: Signature::user_defined(Immutable), | ||
| input_data_type: data_type, |
There was a problem hiding this comment.
I still think it is confusing to require the input & result data types as inputs here; considering input type should be controlled by signature/coerce_types() only, and result data type should be same as return_type() which apparently uses avg_return_type()
I suggest that initially we accept there is a second Perhaps it would be good to file a ticket to track the idea of consolidation |
|
Thank you @Jefffrey and @codetyri0n |
Which issue does this PR close?
datafusion-sparkSpark Compatible Functions #15914.Rationale for this change
What changes are included in this PR?
Are these changes tested?
Are there any user-facing changes?