minor: refactor simplify negate #3213
Conversation
github-actions
bot
added
logical-expr
alamb
left a comment
alamb
left a comment
There was a problem hiding this comment.
This looks like a great improvement to me -- thank you @jackwener . I had one style suggestion but I also think this PR could be merged as is
| impl Operator { | ||
| /// If the operator can be negated, return the negated operator | ||
| /// otherwise return None | ||
| pub fn negate(&self) -> Option<Operator> { |
datafusion/expr/src/operator.rs
Outdated
| Operator::NotLike => Some(Operator::Like), | ||
| Operator::IsDistinctFrom => Some(Operator::IsNotDistinctFrom), | ||
| Operator::IsNotDistinctFrom => Some(Operator::IsDistinctFrom), | ||
| _ => None, |
There was a problem hiding this comment.
I would like to suggest replacing _ with an an explicit enumeration of all the other operators, like
Operator::LeftShift | OperatiorRightShift => NoneThe reason is so that as new operators are added, the compiler will point us at this code and we will get another chance to avoid bugs like https://github.com/apache/arrow-datafusion/pull/3167/files#r946145318
Codecov Report
@@ Coverage Diff @@
## master #3213 +/- ##
==========================================
- Coverage 85.84% 85.84% -0.01%
==========================================
Files 291 291
Lines 52860 52909 +49
==========================================
+ Hits 45380 45419 +39
- Misses 7480 7490 +10
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
jimexist
left a comment
jimexist
left a comment
There was a problem hiding this comment.
please do not use catch all (_)
| Operator::NotLike => Some(Operator::Like), | ||
| Operator::IsDistinctFrom => Some(Operator::IsNotDistinctFrom), | ||
| Operator::IsNotDistinctFrom => Some(Operator::IsDistinctFrom), | ||
| Operator::Plus |
|
Benchmark runs are scheduled for baseline = c8d61d8 and contender = 72487cf. 72487cf is a master commit associated with this PR. Results will be available as each benchmark for each run completes. |
5 participants
Which issue does this PR close?
Closes #.
Rationale for this change
Simplify the code, which make we can reuse the negete opeator.
What changes are included in this PR?
refactor simplify negate
Are there any user-facing changes?