Simplify simplify test cases, support ^, &, |, << and >> operators for building exprs
#5511
Conversation
github-actions
bot
added
logical-expr
alamb
changed the title
^, &, |, << and >> opertors for building exprs^, &, |, << and >> opertors for building exprs
| } | ||
|
|
||
| #[test] | ||
| fn test_simplify_simplify_eq_expr() { |
There was a problem hiding this comment.
I think check of expr_plus in test_simplify_simplify_eq_expr is redundant because it is already checked in test_simplify_simplify_arithmetic_expr
There was a problem hiding this comment.
Thanks -- I was trying to split up test_simplify_simplify_arithmetic_exprs into two separate tests as it seemed like it was testing two things, However, now that i see this diff It just looks like I added a new one. I will fix it.
|
looks great, just wondering should we also preserve old syntax in tests? like If this syntax is not deprecated, it should probably be tested as well? |
I agree it should be tested. I will review our existing coverage and see if anything else is neede |
| fn test_simplify_multiply_by_one() { | ||
| let expr_a = binary_expr(col("c2"), Operator::Multiply, lit(1)); | ||
| let expr_b = binary_expr(lit(1), Operator::Multiply, col("c2")); | ||
| let expr_a = col("c2") * lit(1); |
There was a problem hiding this comment.
There are no intended logical changes in this file, only simplifying how the tests are expressed (and there is a significant number of lines removed)
| col("c1").lt(lit(6)), | ||
| ), | ||
| Operator::BitwiseAnd, | ||
| let expr = bitwise_and( |
There was a problem hiding this comment.
I also tried writing this using the new & operator, but I thought the structure (precidence) of the resulting expression was significantly harder to see, so I switched to using the bitwise_and type functions to reduce some duplication
| } | ||
|
|
||
| #[test] | ||
| fn test_simplify_simplify_eq_expr() { |
There was a problem hiding this comment.
Thanks -- I was trying to split up test_simplify_simplify_arithmetic_exprs into two separate tests as it seemed like it was testing two things, However, now that i see this diff It just looks like I added a new one. I will fix it.
alamb
changed the title
^, &, |, << and >> opertors for building exprs^, &, |, << and >> operators for building exprs
I reviewed the use of I didn't see much focused testing for the functions defined in in https://github.com/apache/arrow-datafusion/blob/main/datafusion/expr/src/expr_fn.rs Thus I conclude this PR doesn't impact the test coverage for expression creation functions. If we want to cover the functions in |
|
Benchmark runs are scheduled for baseline = 9587339 and contender = aaa4e14. aaa4e14 is a master commit associated with this PR. Results will be available as each benchmark for each run completes. |
4 participants
Which issue does this PR close?
Follow on to #5423
Rationale for this change
as @izveigor pointed out on https://github.com/apache/arrow-datafusion/pull/5423/files#r1125686357, the tests in expr simplifier used a more verbose style that he followed, which makes sense.
However, I would like to keep the codebase easier to maintain and thus use less ceremony for creating Exprs.
What changes are included in this PR?
^,&,|,<<and>>opertors for building exprsAre there any user-facing changes?
You can now create exprs like