You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "alamb (via GitHub)" <gi...@apache.org> on 2023/03/07 21:03:13 UTC
[GitHub] [arrow-datafusion] alamb opened a new pull request, #5511: Minor: simplify simplify test cases, support `^`, `&`, `|`, `<<` and `>>` opertors for building exprs
alamb opened a new pull request, #5511:
URL: https://github.com/apache/arrow-datafusion/pull/5511
# Which issue does this PR close?
Follow on to https://github.com/apache/arrow-datafusion/pull/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?
1. Support `^`, `&`, `|`, `<<` and `>>` opertors for building exprs
2. Simplify expression simply test cases
# Are there any user-facing changes?
You can now create exprs like
```rust
let expr = lit(0) & lit(45)
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] izveigor commented on a diff in pull request #5511: Simplify simplify test cases, support `^`, `&`, `|`, `<<` and `>>` opertors for building exprs
Posted by "izveigor (via GitHub)" <gi...@apache.org>.
izveigor commented on code in PR #5511:
URL: https://github.com/apache/arrow-datafusion/pull/5511#discussion_r1129140359
##########
datafusion/expr/src/operator.rs:
##########
@@ -275,6 +280,60 @@ impl ops::Rem for Expr {
}
}
+/// Support `<expr> & <expr>` fluent style
+impl ops::BitAnd for Expr {
+ type Output = Self;
+
+ fn bitand(self, rhs: Self) -> Self {
+ binary_expr(self, Operator::BitwiseAnd, rhs)
+ }
+}
+
+/// Support `<expr> | <expr>` fluent style
+impl ops::BitOr for Expr {
+ type Output = Self;
+
+ fn bitor(self, rhs: Self) -> Self {
+ binary_expr(self, Operator::BitwiseOr, rhs)
+ }
+}
+
+/// Support `<expr> ^ <expr>` fluent style
+impl ops::BitXor for Expr {
+ type Output = Self;
+
+ fn bitxor(self, rhs: Self) -> Self {
+ binary_expr(self, Operator::BitwiseXor, rhs)
+ }
+}
+
+/// Support `<expr> << <expr>` fluent style
+impl ops::Shl for Expr {
+ type Output = Self;
+
+ fn shl(self, rhs: Self) -> Self::Output {
+ binary_expr(self, Operator::BitwiseShiftLeft, rhs)
+ }
+}
+
+/// Support `<expr> >> <expr>` fluent style
+impl ops::Shr for Expr {
+ type Output = Self;
+
+ fn shr(self, rhs: Self) -> Self::Output {
+ binary_expr(self, Operator::BitwiseShiftRight, rhs)
+ }
+}
+
+/// Support `<expr> - <expr>` fluent style
Review Comment:
`-<expr>`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] ursabot commented on pull request #5511: Simplify simplify test cases, support `^`, `&`, `|`, `<<` and `>>` operators for building exprs
Posted by "ursabot (via GitHub)" <gi...@apache.org>.
ursabot commented on PR #5511:
URL: https://github.com/apache/arrow-datafusion/pull/5511#issuecomment-1464900576
Benchmark runs are scheduled for baseline = 9587339b0fb060f8d153bbb0f8de6a740195ccea and contender = aaa4e1496ba89daa03de47dc1feed7d7e57f62a8. aaa4e1496ba89daa03de47dc1feed7d7e57f62a8 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Skipped :warning: Benchmarking of arrow-datafusion-commits is not supported on ec2-t3-xlarge-us-east-2] [ec2-t3-xlarge-us-east-2](https://conbench.ursa.dev/compare/runs/32b451b9385842bd8127b1385dd8b176...108fe5ded9fa481595c713bbbc0513e3/)
[Skipped :warning: Benchmarking of arrow-datafusion-commits is not supported on test-mac-arm] [test-mac-arm](https://conbench.ursa.dev/compare/runs/1c9e1f4c64524d1ab552cc856e0ace06...03d09d76cb1d47f2810f827c518ec9bc/)
[Skipped :warning: Benchmarking of arrow-datafusion-commits is not supported on ursa-i9-9960x] [ursa-i9-9960x](https://conbench.ursa.dev/compare/runs/fe981c67b8014e5083ce338039f00ad9...28cb16989c2f4b6496a6a116cd473f18/)
[Skipped :warning: Benchmarking of arrow-datafusion-commits is not supported on ursa-thinkcentre-m75q] [ursa-thinkcentre-m75q](https://conbench.ursa.dev/compare/runs/460b4f224e5d411186f83d8e543fd602...a50eb8c37840442baf366a434da84f62/)
Buildkite builds:
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] alamb commented on a diff in pull request #5511: Simplify simplify test cases, support `^`, `&`, `|`, `<<` and `>>` operators for building exprs
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #5511:
URL: https://github.com/apache/arrow-datafusion/pull/5511#discussion_r1131533045
##########
datafusion/expr/src/operator.rs:
##########
@@ -275,6 +280,60 @@ impl ops::Rem for Expr {
}
}
+/// Support `<expr> & <expr>` fluent style
+impl ops::BitAnd for Expr {
+ type Output = Self;
+
+ fn bitand(self, rhs: Self) -> Self {
+ binary_expr(self, Operator::BitwiseAnd, rhs)
+ }
+}
+
+/// Support `<expr> | <expr>` fluent style
+impl ops::BitOr for Expr {
+ type Output = Self;
+
+ fn bitor(self, rhs: Self) -> Self {
+ binary_expr(self, Operator::BitwiseOr, rhs)
+ }
+}
+
+/// Support `<expr> ^ <expr>` fluent style
+impl ops::BitXor for Expr {
+ type Output = Self;
+
+ fn bitxor(self, rhs: Self) -> Self {
+ binary_expr(self, Operator::BitwiseXor, rhs)
+ }
+}
+
+/// Support `<expr> << <expr>` fluent style
+impl ops::Shl for Expr {
+ type Output = Self;
+
+ fn shl(self, rhs: Self) -> Self::Output {
+ binary_expr(self, Operator::BitwiseShiftLeft, rhs)
+ }
+}
+
+/// Support `<expr> >> <expr>` fluent style
+impl ops::Shr for Expr {
+ type Output = Self;
+
+ fn shr(self, rhs: Self) -> Self::Output {
+ binary_expr(self, Operator::BitwiseShiftRight, rhs)
+ }
+}
+
+/// Support `<expr> - <expr>` fluent style
Review Comment:
```suggestion
/// Support `- <expr>` fluent style
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] alamb commented on a diff in pull request #5511: Simplify simplify test cases, support `^`, `&`, `|`, `<<` and `>>` operators for building exprs
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #5511:
URL: https://github.com/apache/arrow-datafusion/pull/5511#discussion_r1131535249
##########
datafusion/optimizer/src/simplify_expressions/expr_simplifier.rs:
##########
@@ -2227,87 +2069,86 @@ mod tests {
#[test]
fn test_simplify_or_and_non_null() {
let l = col("c2_non_null").gt(lit(5));
- let r = binary_expr(
- col("c1_non_null").lt(lit(6)),
- Operator::And,
- col("c2_non_null").gt(lit(5)),
- );
+ let r = and(col("c1_non_null").lt(lit(6)), col("c2_non_null").gt(lit(5)));
// (c2 > 5) OR ((c1 < 6) AND (c2 > 5)) --> c2 > 5
- let expr = binary_expr(l.clone(), Operator::Or, r.clone());
+ let expr = or(l.clone(), r.clone());
// This is only true if `c1 < 6` is not nullable / can not be null.
let expected = col("c2_non_null").gt(lit(5));
assert_eq!(simplify(expr), expected);
// ((c1 < 6) AND (c2 > 5)) OR (c2 > 5) --> c2 > 5
- let expr = binary_expr(l, Operator::Or, r);
+ let expr = or(l, r);
assert_eq!(simplify(expr), expected);
}
#[test]
fn test_simplify_and_or() {
let l = col("c2").gt(lit(5));
- let r = binary_expr(col("c1").lt(lit(6)), Operator::Or, col("c2").gt(lit(5)));
+ let r = or(col("c1").lt(lit(6)), col("c2").gt(lit(5)));
// (c2 > 5) AND ((c1 < 6) OR (c2 > 5)) --> c2 > 5
- let expr = binary_expr(l.clone(), Operator::And, r.clone());
+ let expr = and(l.clone(), r.clone());
// no rewrites if c1 can be null
let expected = expr.clone();
assert_eq!(simplify(expr), expected);
// ((c1 < 6) OR (c2 > 5)) AND (c2 > 5) --> c2 > 5
- let expr = binary_expr(l, Operator::And, r);
+ let expr = and(l, r);
let expected = expr.clone();
assert_eq!(simplify(expr), expected);
}
#[test]
fn test_simplify_and_or_non_null() {
let l = col("c2_non_null").gt(lit(5));
- let r = binary_expr(
- col("c1_non_null").lt(lit(6)),
- Operator::Or,
- col("c2_non_null").gt(lit(5)),
- );
+ let r = or(col("c1_non_null").lt(lit(6)), col("c2_non_null").gt(lit(5)));
// (c2 > 5) AND ((c1 < 6) OR (c2 > 5)) --> c2 > 5
- let expr = binary_expr(l.clone(), Operator::And, r.clone());
+ let expr = and(l.clone(), r.clone());
// This is only true if `c1 < 6` is not nullable / can not be null.
let expected = col("c2_non_null").gt(lit(5));
assert_eq!(simplify(expr), expected);
// ((c1 < 6) OR (c2 > 5)) AND (c2 > 5) --> c2 > 5
- let expr = binary_expr(l, Operator::And, r);
+ let expr = and(l, r);
assert_eq!(simplify(expr), expected);
}
#[test]
fn test_simplify_null_and_false() {
- let expr = binary_expr(lit_bool_null(), Operator::And, lit(false));
+ let expr = and(lit_bool_null(), lit(false));
let expr_eq = lit(false);
assert_eq!(simplify(expr), expr_eq);
}
#[test]
fn test_simplify_divide_null_by_null() {
- let null = Expr::Literal(ScalarValue::Int32(None));
- let expr_plus = binary_expr(null.clone(), Operator::Divide, null.clone());
+ let null = lit(ScalarValue::Int32(None));
+ let expr_plus = null.clone() / null.clone();
let expr_eq = null;
assert_eq!(simplify(expr_plus), expr_eq);
}
#[test]
fn test_simplify_simplify_arithmetic_expr() {
- let expr_plus = binary_expr(lit(1), Operator::Plus, lit(1));
+ let expr_plus = lit(1) + lit(1);
+
+ assert_eq!(simplify(expr_plus), lit(2));
+ }
+
+ #[test]
+ fn test_simplify_simplify_eq_expr() {
Review Comment:
fixed in 1afb57054
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] izveigor commented on a diff in pull request #5511: Simplify simplify test cases, support `^`, `&`, `|`, `<<` and `>>` opertors for building exprs
Posted by "izveigor (via GitHub)" <gi...@apache.org>.
izveigor commented on code in PR #5511:
URL: https://github.com/apache/arrow-datafusion/pull/5511#discussion_r1129140359
##########
datafusion/expr/src/operator.rs:
##########
@@ -275,6 +280,60 @@ impl ops::Rem for Expr {
}
}
+/// Support `<expr> & <expr>` fluent style
+impl ops::BitAnd for Expr {
+ type Output = Self;
+
+ fn bitand(self, rhs: Self) -> Self {
+ binary_expr(self, Operator::BitwiseAnd, rhs)
+ }
+}
+
+/// Support `<expr> | <expr>` fluent style
+impl ops::BitOr for Expr {
+ type Output = Self;
+
+ fn bitor(self, rhs: Self) -> Self {
+ binary_expr(self, Operator::BitwiseOr, rhs)
+ }
+}
+
+/// Support `<expr> ^ <expr>` fluent style
+impl ops::BitXor for Expr {
+ type Output = Self;
+
+ fn bitxor(self, rhs: Self) -> Self {
+ binary_expr(self, Operator::BitwiseXor, rhs)
+ }
+}
+
+/// Support `<expr> << <expr>` fluent style
+impl ops::Shl for Expr {
+ type Output = Self;
+
+ fn shl(self, rhs: Self) -> Self::Output {
+ binary_expr(self, Operator::BitwiseShiftLeft, rhs)
+ }
+}
+
+/// Support `<expr> >> <expr>` fluent style
+impl ops::Shr for Expr {
+ type Output = Self;
+
+ fn shr(self, rhs: Self) -> Self::Output {
+ binary_expr(self, Operator::BitwiseShiftRight, rhs)
+ }
+}
+
+/// Support `<expr> - <expr>` fluent style
Review Comment:
Support `-<expr>` fluent style
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] comphead commented on pull request #5511: Simplify simplify test cases, support `^`, `&`, `|`, `<<` and `>>` opertors for building exprs
Posted by "comphead (via GitHub)" <gi...@apache.org>.
comphead commented on PR #5511:
URL: https://github.com/apache/arrow-datafusion/pull/5511#issuecomment-1460450347
looks great, just wondering should we also preserve old syntax in tests?
like
```
let expr_a = binary_expr(col("c2"), Operator::Multiply, lit(0));
let expr_b = binary_expr(lit(0), Operator::Multiply, col("c2"));
```
If this syntax is not deprecated, it should probably be tested as well?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] alamb commented on a diff in pull request #5511: Simplify simplify test cases, support `^`, `&`, `|`, `<<` and `>>` opertors for building exprs
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #5511:
URL: https://github.com/apache/arrow-datafusion/pull/5511#discussion_r1128587403
##########
datafusion/optimizer/src/simplify_expressions/expr_simplifier.rs:
##########
@@ -1769,79 +1749,43 @@ mod tests {
fn test_simplify_composed_bitwise_and() {
// ((c2 > 5) & (c1 < 6)) & (c2 > 5) --> (c2 > 5) & (c1 < 6)
- let expr = binary_expr(
- binary_expr(
- col("c2").gt(lit(5)),
- Operator::BitwiseAnd,
- col("c1").lt(lit(6)),
- ),
- Operator::BitwiseAnd,
+ let expr = bitwise_and(
Review 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
##########
datafusion/optimizer/src/simplify_expressions/expr_simplifier.rs:
##########
@@ -1470,24 +1470,17 @@ mod tests {
#[test]
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);
Review 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)
##########
datafusion/optimizer/src/simplify_expressions/expr_simplifier.rs:
##########
@@ -2227,87 +2069,86 @@ mod tests {
#[test]
fn test_simplify_or_and_non_null() {
let l = col("c2_non_null").gt(lit(5));
- let r = binary_expr(
- col("c1_non_null").lt(lit(6)),
- Operator::And,
- col("c2_non_null").gt(lit(5)),
- );
+ let r = and(col("c1_non_null").lt(lit(6)), col("c2_non_null").gt(lit(5)));
// (c2 > 5) OR ((c1 < 6) AND (c2 > 5)) --> c2 > 5
- let expr = binary_expr(l.clone(), Operator::Or, r.clone());
+ let expr = or(l.clone(), r.clone());
// This is only true if `c1 < 6` is not nullable / can not be null.
let expected = col("c2_non_null").gt(lit(5));
assert_eq!(simplify(expr), expected);
// ((c1 < 6) AND (c2 > 5)) OR (c2 > 5) --> c2 > 5
- let expr = binary_expr(l, Operator::Or, r);
+ let expr = or(l, r);
assert_eq!(simplify(expr), expected);
}
#[test]
fn test_simplify_and_or() {
let l = col("c2").gt(lit(5));
- let r = binary_expr(col("c1").lt(lit(6)), Operator::Or, col("c2").gt(lit(5)));
+ let r = or(col("c1").lt(lit(6)), col("c2").gt(lit(5)));
// (c2 > 5) AND ((c1 < 6) OR (c2 > 5)) --> c2 > 5
- let expr = binary_expr(l.clone(), Operator::And, r.clone());
+ let expr = and(l.clone(), r.clone());
// no rewrites if c1 can be null
let expected = expr.clone();
assert_eq!(simplify(expr), expected);
// ((c1 < 6) OR (c2 > 5)) AND (c2 > 5) --> c2 > 5
- let expr = binary_expr(l, Operator::And, r);
+ let expr = and(l, r);
let expected = expr.clone();
assert_eq!(simplify(expr), expected);
}
#[test]
fn test_simplify_and_or_non_null() {
let l = col("c2_non_null").gt(lit(5));
- let r = binary_expr(
- col("c1_non_null").lt(lit(6)),
- Operator::Or,
- col("c2_non_null").gt(lit(5)),
- );
+ let r = or(col("c1_non_null").lt(lit(6)), col("c2_non_null").gt(lit(5)));
// (c2 > 5) AND ((c1 < 6) OR (c2 > 5)) --> c2 > 5
- let expr = binary_expr(l.clone(), Operator::And, r.clone());
+ let expr = and(l.clone(), r.clone());
// This is only true if `c1 < 6` is not nullable / can not be null.
let expected = col("c2_non_null").gt(lit(5));
assert_eq!(simplify(expr), expected);
// ((c1 < 6) OR (c2 > 5)) AND (c2 > 5) --> c2 > 5
- let expr = binary_expr(l, Operator::And, r);
+ let expr = and(l, r);
assert_eq!(simplify(expr), expected);
}
#[test]
fn test_simplify_null_and_false() {
- let expr = binary_expr(lit_bool_null(), Operator::And, lit(false));
+ let expr = and(lit_bool_null(), lit(false));
let expr_eq = lit(false);
assert_eq!(simplify(expr), expr_eq);
}
#[test]
fn test_simplify_divide_null_by_null() {
- let null = Expr::Literal(ScalarValue::Int32(None));
- let expr_plus = binary_expr(null.clone(), Operator::Divide, null.clone());
+ let null = lit(ScalarValue::Int32(None));
+ let expr_plus = null.clone() / null.clone();
let expr_eq = null;
assert_eq!(simplify(expr_plus), expr_eq);
}
#[test]
fn test_simplify_simplify_arithmetic_expr() {
- let expr_plus = binary_expr(lit(1), Operator::Plus, lit(1));
+ let expr_plus = lit(1) + lit(1);
+
+ assert_eq!(simplify(expr_plus), lit(2));
+ }
+
+ #[test]
+ fn test_simplify_simplify_eq_expr() {
Review 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] alamb commented on pull request #5511: Simplify simplify test cases, support `^`, `&`, `|`, `<<` and `>>` opertors for building exprs
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #5511:
URL: https://github.com/apache/arrow-datafusion/pull/5511#issuecomment-1462074944
> 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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] izveigor commented on a diff in pull request #5511: Simplify simplify test cases, support `^`, `&`, `|`, `<<` and `>>` opertors for building exprs
Posted by "izveigor (via GitHub)" <gi...@apache.org>.
izveigor commented on code in PR #5511:
URL: https://github.com/apache/arrow-datafusion/pull/5511#discussion_r1129152194
##########
datafusion/optimizer/src/simplify_expressions/expr_simplifier.rs:
##########
@@ -2227,87 +2069,86 @@ mod tests {
#[test]
fn test_simplify_or_and_non_null() {
let l = col("c2_non_null").gt(lit(5));
- let r = binary_expr(
- col("c1_non_null").lt(lit(6)),
- Operator::And,
- col("c2_non_null").gt(lit(5)),
- );
+ let r = and(col("c1_non_null").lt(lit(6)), col("c2_non_null").gt(lit(5)));
// (c2 > 5) OR ((c1 < 6) AND (c2 > 5)) --> c2 > 5
- let expr = binary_expr(l.clone(), Operator::Or, r.clone());
+ let expr = or(l.clone(), r.clone());
// This is only true if `c1 < 6` is not nullable / can not be null.
let expected = col("c2_non_null").gt(lit(5));
assert_eq!(simplify(expr), expected);
// ((c1 < 6) AND (c2 > 5)) OR (c2 > 5) --> c2 > 5
- let expr = binary_expr(l, Operator::Or, r);
+ let expr = or(l, r);
assert_eq!(simplify(expr), expected);
}
#[test]
fn test_simplify_and_or() {
let l = col("c2").gt(lit(5));
- let r = binary_expr(col("c1").lt(lit(6)), Operator::Or, col("c2").gt(lit(5)));
+ let r = or(col("c1").lt(lit(6)), col("c2").gt(lit(5)));
// (c2 > 5) AND ((c1 < 6) OR (c2 > 5)) --> c2 > 5
- let expr = binary_expr(l.clone(), Operator::And, r.clone());
+ let expr = and(l.clone(), r.clone());
// no rewrites if c1 can be null
let expected = expr.clone();
assert_eq!(simplify(expr), expected);
// ((c1 < 6) OR (c2 > 5)) AND (c2 > 5) --> c2 > 5
- let expr = binary_expr(l, Operator::And, r);
+ let expr = and(l, r);
let expected = expr.clone();
assert_eq!(simplify(expr), expected);
}
#[test]
fn test_simplify_and_or_non_null() {
let l = col("c2_non_null").gt(lit(5));
- let r = binary_expr(
- col("c1_non_null").lt(lit(6)),
- Operator::Or,
- col("c2_non_null").gt(lit(5)),
- );
+ let r = or(col("c1_non_null").lt(lit(6)), col("c2_non_null").gt(lit(5)));
// (c2 > 5) AND ((c1 < 6) OR (c2 > 5)) --> c2 > 5
- let expr = binary_expr(l.clone(), Operator::And, r.clone());
+ let expr = and(l.clone(), r.clone());
// This is only true if `c1 < 6` is not nullable / can not be null.
let expected = col("c2_non_null").gt(lit(5));
assert_eq!(simplify(expr), expected);
// ((c1 < 6) OR (c2 > 5)) AND (c2 > 5) --> c2 > 5
- let expr = binary_expr(l, Operator::And, r);
+ let expr = and(l, r);
assert_eq!(simplify(expr), expected);
}
#[test]
fn test_simplify_null_and_false() {
- let expr = binary_expr(lit_bool_null(), Operator::And, lit(false));
+ let expr = and(lit_bool_null(), lit(false));
let expr_eq = lit(false);
assert_eq!(simplify(expr), expr_eq);
}
#[test]
fn test_simplify_divide_null_by_null() {
- let null = Expr::Literal(ScalarValue::Int32(None));
- let expr_plus = binary_expr(null.clone(), Operator::Divide, null.clone());
+ let null = lit(ScalarValue::Int32(None));
+ let expr_plus = null.clone() / null.clone();
let expr_eq = null;
assert_eq!(simplify(expr_plus), expr_eq);
}
#[test]
fn test_simplify_simplify_arithmetic_expr() {
- let expr_plus = binary_expr(lit(1), Operator::Plus, lit(1));
+ let expr_plus = lit(1) + lit(1);
+
+ assert_eq!(simplify(expr_plus), lit(2));
+ }
+
+ #[test]
+ fn test_simplify_simplify_eq_expr() {
Review 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`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] alamb commented on pull request #5511: Simplify simplify test cases, support `^`, `&`, `|`, `<<` and `>>` operators for building exprs
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #5511:
URL: https://github.com/apache/arrow-datafusion/pull/5511#issuecomment-1462706360
> > 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 needed
I reviewed the use of `binary_expr` and I think there is plenty of other coverage (see https://github.com/search?q=repo%3Aapache%2Farrow-datafusion%20binary_expr&type=code)
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
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 `expr_fn.rs` more deliberately or in a more structured way, I think we could do it as a follow on PR / add tests specifically for that coverage, rather than rely on tests for other functionality (simplification) covering somewhat unrelated functionality (expr creation)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] alamb merged pull request #5511: Simplify simplify test cases, support `^`, `&`, `|`, `<<` and `>>` operators for building exprs
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb merged PR #5511:
URL: https://github.com/apache/arrow-datafusion/pull/5511
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org