feat: add aggregate count functions with decimal return type#670
feat: add aggregate count functions with decimal return type#670jacques-n merged 3 commits intosubstrait-io:mainfrom
Conversation
scgkiran
requested review from
EpsilonPrime,
cpcloud,
jacques-n,
vbarua and
westonpace
as code owners
| %YAML 1.2 | ||
| --- | ||
| aggregate_functions: | ||
| - name: "count" |
There was a problem hiding this comment.
overloading functions based only on return type causes problems, see e.g. #606 (comment)
There was a problem hiding this comment.
I have them in a different yaml file. can this still cause problems?
There was a problem hiding this comment.
different files should be ok.
There was a problem hiding this comment.
I believe it'll still break substrait-java, see substrait-io/substrait-java#275
There was a problem hiding this comment.
I think we should fix that in substrait-java then as opposed to holding back the specification. Extensions being independent is key to extensions being extensions 😊
There was a problem hiding this comment.
It's possible for us to work around duplicates in substrait-java by excluding them from the default extensions for now. See substrait-io/substrait-java#275 (comment)
| @@ -0,0 +1,41 @@ | |||
| %YAML 1.2 | |||
| --- | |||
| aggregate_functions: | |||
There was a problem hiding this comment.
the name of this file is weird. in other cases type names in filenames are about input types and this one is about output types. I suggest we rename file to something that is more descriptive. Maybe also add a little text to the functions clarifying how they are different than classic count functions.
vbarua
left a comment
vbarua
left a comment
There was a problem hiding this comment.
Overall these changes look reasonable to me. Just one suggestion about the name count_star.
4 participants
No description provided.