[Func/Arith] Multiply, Division and Modulus #32
Conversation
| @@ -67,8 +67,6 @@ scalar_functions: | |||
| - name: modulus | |||
| local_name: "%" | |||
| infix: True | |||
| required_options: | |||
There was a problem hiding this comment.
postgres supports modulus with numeric operands, but as per Substrait documentation, it should only work on integer values.
There was a problem hiding this comment.
We'll need to update the substrait docs here too
| @@ -37,8 +37,6 @@ scalar_functions: | |||
| - name: modulus | |||
| local_name: "%" | |||
| infix: True | |||
| required_options: | |||
There was a problem hiding this comment.
SQLite supports two different operations for modulus.
%operator that will not work for non-integer operands.modfunction that shall work for non-integer operands.
There was a problem hiding this comment.
Is it possible to test both?
There was a problem hiding this comment.
Not right now. AFAIK, right now the BFT assumes every dialect has only one implementation of a function. I will create an issue to make it change.
| rounding: TIE_TO_EVEN | ||
| on_division_by_zero: LIMIT | ||
| on_domain_error: NAN | ||
| - name: modulus |
There was a problem hiding this comment.
cudf also supports mod function on floating points, while the substrait documentation states operation on only integer arguments.
There was a problem hiding this comment.
I'll make this change in the substrait spec.
| is 0. This is the default behavior in many systems because it helps to avoid | ||
| bias in rounding. | ||
|
|
||
| #### TIE_AWAY_FROM_ZERO |
There was a problem hiding this comment.
Ideally we would point to these common options instead of duplicating them everywhere.
There was a problem hiding this comment.
@sanjibansg let's use this PR to address the duplication issues
There was a problem hiding this comment.
This will have some framework-level changes. I have thought to make a separate YAML file containing the definition of such options. Functions that follow such options, and do not have any exceptions, should fetch the definition from that YAML, or should have their own definition here. I will make a separate PR for that.
sanjibansg
changed the title
This PR adds the Multiply, Division, and Modulus functions.