refactor: generalize aggregation to handle 0,1, or 2 inputs #360
Conversation
52c78a5 to
21e7445
Compare
| ) -> ibis_types.Value: | ||
| return compile_agg(op, input) | ||
| if isinstance(aggregate, ex.UnaryAggregation): |
There was a problem hiding this comment.
Note to self: Is there another class for 0 argument aggregations (e.g. row count) that we need to handle here?
There was a problem hiding this comment.
CL title is a lie, I haven't actually defined the 0-arg aggregation ops yet. They still pretend to be 1-arg aggregations. Will convert 0-arg ops (rank is another one) in a follow-up pr.
| @@ -382,6 +418,19 @@ def _( | |||
| ) | |||
|
|
|||
|
|
|||
| @compile_binary_agg.register | |||
| def _( | |||
There was a problem hiding this comment.
Not a change introduced in this PR, but it's a bit odd to see functions with name _ outside of a closure function. Even though the name isn't really relevant, as we're relying on type-based dispatching, I'd expect more than a 1-character name.
https://google.github.io/styleguide/pyguide.html#3161-names-to-avoid
There was a problem hiding this comment.
The code examples for @singledispatch in https://docs.python.org/3/library/functools.html use the _ naming for overrides. Also the linter wants unique name for each implementation if it is not _. I can change though if this goes against our style guidelines.
Thank you for opening a Pull Request! Before submitting your PR, there are a few things you can do to make sure it goes smoothly:
Fixes #<issue_number_goes_here> 🦕