From 7666bb6766c0e751ff90092f2d2fd7c362d76c67 Mon Sep 17 00:00:00 2001
From: Fokko Driesprong <fokko@apache.org>
Date: Fri, 19 Apr 2024 16:43:46 +0200
Subject: [PATCH] Simplify expression when doing `{and,or}` operations (#339)

This will make sure that we nicely reduce the expression in
the inclusive projection visitor:

https://github.com/apache/iceberg-rust/blob/de80a2436bb2fbbd5b4ec6bcafd0bd041b263595/crates/iceberg/src/expr/visitors/inclusive_projection.rs#L73
---
 crates/iceberg/src/expr/predicate.rs | 53 ++++++++++++++++++++++++++--
 1 file changed, 51 insertions(+), 2 deletions(-)

diff --git a/crates/iceberg/src/expr/predicate.rs b/crates/iceberg/src/expr/predicate.rs
index 1163f3bf0..6cdf4fc6b 100644
--- a/crates/iceberg/src/expr/predicate.rs
+++ b/crates/iceberg/src/expr/predicate.rs
@@ -456,7 +456,13 @@ impl Predicate {
     /// assert_eq!(&format!("{expr}"), "(a < 10) AND (b < 20)");
     /// ```
     pub fn and(self, other: Predicate) -> Predicate {
-        Predicate::And(LogicalExpression::new([Box::new(self), Box::new(other)]))
+        match (self, other) {
+            (Predicate::AlwaysFalse, _) => Predicate::AlwaysFalse,
+            (_, Predicate::AlwaysFalse) => Predicate::AlwaysFalse,
+            (Predicate::AlwaysTrue, rhs) => rhs,
+            (lhs, Predicate::AlwaysTrue) => lhs,
+            (lhs, rhs) => Predicate::And(LogicalExpression::new([Box::new(lhs), Box::new(rhs)])),
+        }
     }
 
     /// Combines two predicates with `OR`.
@@ -477,7 +483,13 @@ impl Predicate {
     /// assert_eq!(&format!("{expr}"), "(a < 10) OR (b < 20)");
     /// ```
     pub fn or(self, other: Predicate) -> Predicate {
-        Predicate::Or(LogicalExpression::new([Box::new(self), Box::new(other)]))
+        match (self, other) {
+            (Predicate::AlwaysTrue, _) => Predicate::AlwaysTrue,
+            (_, Predicate::AlwaysTrue) => Predicate::AlwaysTrue,
+            (Predicate::AlwaysFalse, rhs) => rhs,
+            (lhs, Predicate::AlwaysFalse) => lhs,
+            (lhs, rhs) => Predicate::Or(LogicalExpression::new([Box::new(lhs), Box::new(rhs)])),
+        }
     }
 
     /// Returns a predicate representing the negation ('NOT') of this one,
@@ -658,6 +670,7 @@ mod tests {
     use std::sync::Arc;
 
     use crate::expr::Bind;
+    use crate::expr::Predicate::{AlwaysFalse, AlwaysTrue};
     use crate::expr::Reference;
     use crate::spec::Datum;
     use crate::spec::{NestedField, PrimitiveType, Schema, SchemaRef, Type};
@@ -729,6 +742,42 @@ mod tests {
         assert_eq!(result, expected);
     }
 
+    #[test]
+    fn test_predicate_and_reduce_always_true_false() {
+        let true_or_expr = AlwaysTrue.and(Reference::new("b").less_than(Datum::long(5)));
+        assert_eq!(&format!("{true_or_expr}"), "b < 5");
+
+        let expr_or_true = Reference::new("b")
+            .less_than(Datum::long(5))
+            .and(AlwaysTrue);
+        assert_eq!(&format!("{expr_or_true}"), "b < 5");
+
+        let false_or_expr = AlwaysFalse.and(Reference::new("b").less_than(Datum::long(5)));
+        assert_eq!(&format!("{false_or_expr}"), "FALSE");
+
+        let expr_or_false = Reference::new("b")
+            .less_than(Datum::long(5))
+            .and(AlwaysFalse);
+        assert_eq!(&format!("{expr_or_false}"), "FALSE");
+    }
+
+    #[test]
+    fn test_predicate_or_reduce_always_true_false() {
+        let true_or_expr = AlwaysTrue.or(Reference::new("b").less_than(Datum::long(5)));
+        assert_eq!(&format!("{true_or_expr}"), "TRUE");
+
+        let expr_or_true = Reference::new("b").less_than(Datum::long(5)).or(AlwaysTrue);
+        assert_eq!(&format!("{expr_or_true}"), "TRUE");
+
+        let false_or_expr = AlwaysFalse.or(Reference::new("b").less_than(Datum::long(5)));
+        assert_eq!(&format!("{false_or_expr}"), "b < 5");
+
+        let expr_or_false = Reference::new("b")
+            .less_than(Datum::long(5))
+            .or(AlwaysFalse);
+        assert_eq!(&format!("{expr_or_false}"), "b < 5");
+    }
+
     #[test]
     fn test_predicate_negate_and() {
         let expression = Reference::new("b")