-
Notifications
You must be signed in to change notification settings - Fork 166
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: add aggregate function min/max support #219
Conversation
extensions/functions_arithmetic.yaml
Outdated
- value: i8 | ||
nullability: DECLARED_OUTPUT | ||
decomposable: MANY | ||
intermediate: i8 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems both the intermediate
and return
type should be i8?
, indicating that a nullable i8.
Same applied to other impls.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it. BTW do we need to define another function definication that the return
type is not nullable ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it. BTW do we need to define another function definication that the return
type is not nullable ? The specification shows that the functoin signature (which is the key in the yaml file) doesn't include the return type.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
extensions/functions_arithmetic.yaml
Outdated
description: Min a set of values. | ||
impls: | ||
- args: | ||
- options: [ SILENT, SATURATE, ERROR ] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see the purpose of multiple overflow behaviors for min. How would min ever overflow?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just got the meaning of the options
which indicates the behavior of a function overflow. So will remove it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
It looks like your commit doesn't match the expected commit pattern. This is a new feature. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Thanks @weijietong !
No description provided.