-
Notifications
You must be signed in to change notification settings - Fork 146
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
Add ANSI support for Subtract #535 #593
base: main
Are you sure you want to change the base?
Conversation
@@ -219,6 +219,7 @@ message Subtract { | |||
Expr right = 2; | |||
bool fail_on_error = 3; | |||
DataType return_type = 4; | |||
EvalMode eval_mode = 5; |
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.
Just curious, if a Substrait plan was to send one of the options how it would map to this new functionality? How does ANSI map to an option?
https://substrait.io/extensions/functions_arithmetic/#subtract
@@ -219,6 +219,7 @@ message Subtract { | |||
Expr right = 2; | |||
bool fail_on_error = 3; |
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.
Instead, set this fail_on_error
to true when ANSI enabled.
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.
Okay. It makes sense & i will refactor, earlier i was referecing Abs
.
I am planning to implement my solution as a extension to work done in #616 . Will resume once that PR is merged |
Which issue does this PR close?
Closes #535
Rationale for this change
Part of #313
What changes are included in this PR?
How are these changes tested?