alamb commented on code in PR #4115:
URL: https://github.com/apache/arrow-datafusion/pull/4115#discussion_r1015834100
##########
datafusion/proto/src/bytes/mod.rs:
##########
@@ -321,6 +321,98 @@ mod test {
Expr::from_bytes(&bytes).unwrap();
}
+ fn roundtrip_expr(expr: &Expr) -> Expr {
+ let bytes = expr.to_bytes().unwrap();
+ Expr::from_bytes(&bytes).unwrap()
+ }
+
+ #[test]
+ fn exact_roundtrip_linearized_binary_expr() {
+ // (((A AND B) AND C) AND D)
+ let expr_ordered = col("A").and(col("B")).and(col("C")).and(col("D"));
+ assert_eq!(expr_ordered, roundtrip_expr(&expr_ordered));
+
+ // Ensure that no other variation becomes equal
Review Comment:
❤️
##########
datafusion/proto/src/bytes/mod.rs:
##########
@@ -321,6 +321,98 @@ mod test {
Expr::from_bytes(&bytes).unwrap();
}
+ fn roundtrip_expr(expr: &Expr) -> Expr {
+ let bytes = expr.to_bytes().unwrap();
+ Expr::from_bytes(&bytes).unwrap()
+ }
+
+ #[test]
+ fn exact_roundtrip_linearized_binary_expr() {
+ // (((A AND B) AND C) AND D)
+ let expr_ordered = col("A").and(col("B")).and(col("C")).and(col("D"));
+ assert_eq!(expr_ordered, roundtrip_expr(&expr_ordered));
+
+ // Ensure that no other variation becomes equal
+ let other_variants = vec![
+ // (((B AND A) AND C) AND D)
+ col("B").and(col("A")).and(col("C")).and(col("D")),
+ // (((A AND C) AND B) AND D)
+ col("A").and(col("C")).and(col("B")).and(col("D")),
+ // (((A AND B) AND D) AND C)
+ col("A").and(col("B")).and(col("D")).and(col("C")),
+ // A AND (B AND (C AND D)))
+ col("A").and(col("B").and(col("C").and(col("D")))),
+ ];
+ for case in other_variants {
+ // Each variant is still equal to itself
+ assert_eq!(case, roundtrip_expr(&case));
+
+ // But non of them is equal to the original
+ assert_ne!(expr_ordered, roundtrip_expr(&case));
+ assert_ne!(roundtrip_expr(&expr_ordered), roundtrip_expr(&case));
+ }
+ }
+
+ #[test]
+ fn roundtrip_deeply_nested_binary_expr() {
+ // We need more stack space so this doesn't overflow in dev builds
+ std::thread::Builder::new()
+ .stack_size(10_000_000)
+ .spawn(|| {
+ let n = 100;
+ // a < 5
+ let basic_expr = col("a").lt(lit(5i32));
+ // (a < 5) OR (a < 5) OR (a < 5) OR ...
+ let or_chain = (0..n)
+ .fold(basic_expr.clone(), |expr, _|
expr.or(basic_expr.clone()));
+ // (a < 5) OR (a < 5) AND (a < 5) OR (a < 5) AND (a < 5) AND
(a < 5) OR ...
+ let expr =
Review Comment:
nice
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
