Add Support for modulus operation in substrait
#13108
Conversation
LatrecheYasser
changed the title
mod with the standard substrait name modulusmod with the standard substrait name modulus
LatrecheYasser
force-pushed
the
replace_mode_with_modulus
branch
from
af82001 to
0d500a9
Compare
LatrecheYasser
changed the title
mod with the standard substrait name modulusmodulus operation
| "subtract" => Some(Operator::Minus), | ||
| "multiply" => Some(Operator::Multiply), | ||
| "divide" => Some(Operator::Divide), | ||
| "mod" => Some(Operator::Modulo), |
There was a problem hiding this comment.
because mod is the same as modulus we need to mark it as deprecated somehow, but how?
There was a problem hiding this comment.
I'm not sure how to produce a deprecation warning for this programatically.
However, we can provide an easy migration path if we also update the producer code to use modulus so we can release a version of DataFusion that consumes mod and modulus, and produces only modulus. Then the version after than we remove mod and announce it as a breaking change in the release notes.
LatrecheYasser
changed the title
modulus operation modulus operation
alamb
left a comment
alamb
left a comment
There was a problem hiding this comment.
Thank you @LatrecheYasser
cc @Blizzara @westonpace and @vbarua
alamb
changed the title
modulus operation modulus operation in substrait
alamb
left a comment
alamb
left a comment
There was a problem hiding this comment.
This change seems good to me -- thank you @LatrecheYasser and @vbarua
My only concern is that it isn't tested (and no tests needed to be updated either).
But maybe that is ok, as this PR doesn't improve / make the testing situation worse
|
LGTM. Tests don’t need updating as we mainly test the roundtrips (I didn’t check if we test even that for this specific function, but even if we did, it still would just keep passing.) I dunno if it makes sense to manually test all functions, but I think nowdays the substrait-rs might include the Substrait default function definition yamls so we could maybe do something to compare the produced functions against them (which would have caught the ”mod” before).. |
|
Thank you @LatrecheYasser and @Blizzara I am sorry for the delay in merging this |
4 participants
Which issue does this PR close?
This PR replaces adds the operation name
moduluswhich is the standard Substrait operation name formodRationale for this change
What changes are included in this PR?
Are these changes tested?
Are there any user-facing changes?