refactored LogOp, LogOpType, and LogOpExpr out. merged it with BinOp*

ci.sh

@@ -1,6 +1,9 @@
 #!/bin/bash
 # runs the stuff in CI.yaml locally
 # unfortunately this needs to be updated manually. just update it if you get annoyed by GHAs failing
+
+set -ex
+
 cargo fmt --all -- --check
 cargo clippy --workspace --all-targets --all-features --locked -- -D warnings
 cargo test --no-fail-fast --workspace --all-features --locked

optd-datafusion-bridge/src/from_optd.rs

@@ -23,10 +23,10 @@ use datafusion::{
 use optd_datafusion_repr::{
     plan_nodes::{
         BetweenExpr, BinOpExpr, BinOpType, CastExpr, ColumnRefExpr, ConstantExpr, ConstantType,
-        Expr, ExprList, FuncExpr, FuncType, JoinType, LogOpExpr, LogOpType, OptRelNode,
-        OptRelNodeRef, OptRelNodeTyp, PhysicalAgg, PhysicalEmptyRelation, PhysicalFilter,
-        PhysicalHashJoin, PhysicalLimit, PhysicalNestedLoopJoin, PhysicalProjection, PhysicalScan,
-        PhysicalSort, PlanNode, SortOrderExpr, SortOrderType,
+        Expr, FuncExpr, FuncType, JoinType, OptRelNode, OptRelNodeRef, OptRelNodeTyp, PhysicalAgg,
+        PhysicalEmptyRelation, PhysicalFilter, PhysicalHashJoin, PhysicalLimit,
+        PhysicalNestedLoopJoin, PhysicalProjection, PhysicalScan, PhysicalSort, PlanNode,
+        SortOrderExpr, SortOrderType,
     },
     properties::schema::Schema as OptdSchema,
     PhysicalCollector, Value,
@@ -189,23 +189,6 @@ impl OptdPlanContext<'_> {
                 }
             }
             OptRelNodeTyp::Sort => unreachable!(),
-            OptRelNodeTyp::LogOp(typ) => {
-                let expr = LogOpExpr::from_rel_node(expr.into_rel_node()).unwrap();
-                let mut children = expr.children().to_vec().into_iter();
-                let first_expr = Self::conv_from_optd_expr(children.next().unwrap(), context)?;
-                let op = match typ {
-                    LogOpType::And => datafusion::logical_expr::Operator::And,
-                    LogOpType::Or => datafusion::logical_expr::Operator::Or,
-                };
-                children.try_fold(first_expr, |acc, expr| {
-                    let expr = Self::conv_from_optd_expr(expr, context)?;
-                    Ok(
-                        Arc::new(datafusion::physical_plan::expressions::BinaryExpr::new(
-                            acc, op, expr,
-                        )) as Arc<dyn PhysicalExpr>,
-                    )
-                })
-            }
             OptRelNodeTyp::BinOp(op) => {
                 let expr = BinOpExpr::from_rel_node(expr.into_rel_node()).unwrap();
                 let left = Self::conv_from_optd_expr(expr.left_child(), context)?;
@@ -232,12 +215,10 @@ impl OptdPlanContext<'_> {
                 // TODO: should we just convert between to x <= c1 and x >= c2?
                 let expr = BetweenExpr::from_rel_node(expr.into_rel_node()).unwrap();
                 Self::conv_from_optd_expr(
-                    LogOpExpr::new(
-                        LogOpType::And,
-                        ExprList::new(vec![
-                            BinOpExpr::new(expr.child(), expr.lower(), BinOpType::Geq).into_expr(),
-                            BinOpExpr::new(expr.child(), expr.upper(), BinOpType::Leq).into_expr(),
-                        ]),
+                    BinOpExpr::new(
+                        BinOpExpr::new(expr.child(), expr.lower(), BinOpType::Geq).into_expr(),
+                        BinOpExpr::new(expr.child(), expr.upper(), BinOpType::Leq).into_expr(),
+                        BinOpType::And,
                     )
                     .into_expr(),
                     context,

optd-datafusion-bridge/src/into_optd.rs

@@ -9,9 +9,9 @@ use optd_core::rel_node::RelNode;
 use optd_datafusion_repr::{
     plan_nodes::{
         BetweenExpr, BinOpExpr, BinOpType, CastExpr, ColumnRefExpr, ConstantExpr, Expr, ExprList,
-        FuncExpr, FuncType, JoinType, LogOpExpr, LogOpType, LogicalAgg, LogicalEmptyRelation,
-        LogicalFilter, LogicalJoin, LogicalLimit, LogicalProjection, LogicalScan, LogicalSort,
-        OptRelNode, OptRelNodeRef, OptRelNodeTyp, PlanNode, SortOrderExpr, SortOrderType,
+        FuncExpr, FuncType, JoinType, LogicalAgg, LogicalEmptyRelation, LogicalFilter, LogicalJoin,
+        LogicalLimit, LogicalProjection, LogicalScan, LogicalSort, OptRelNode, OptRelNodeRef,
+        OptRelNodeTyp, PlanNode, SortOrderExpr, SortOrderType,
     },
     Value,
 };
@@ -294,13 +294,19 @@ impl OptdPlanContext<'_> {
         } else if log_ops.len() == 1 {
             Ok(LogicalJoin::new(left, right, log_ops.remove(0), join_type))
         } else {
-            let expr_list = ExprList::new(log_ops);
-            Ok(LogicalJoin::new(
-                left,
-                right,
-                LogOpExpr::new(LogOpType::And, expr_list).into_expr(),
-                join_type,
-            ))
+            // Build a left-deep tree from log_ops
+            // I wanted to pop from the left instead of the right to maintain the order, even if it's slower
+            // you can obv change log_ops to a Deque to avoid this issue but I didn't bother since I don't wanna
+            // do premature optimization
+            let left_nonlog_op = log_ops.remove(0);
+            let right_nonlog_op = log_ops.remove(0);
+            let mut cond =
+                BinOpExpr::new(left_nonlog_op, right_nonlog_op, BinOpType::And).into_expr();
+            while !log_ops.is_empty() {
+                cond = BinOpExpr::new(cond, log_ops.remove(0), BinOpType::And).into_expr();
+            }
+
+            Ok(LogicalJoin::new(left, right, cond, join_type))
         }
     }
 

optd-datafusion-repr/src/cost/base_cost.rs

@@ -285,7 +285,7 @@ impl OptCostModel {
                     } else {
                         INVALID_SELECTIVITY
                     }
-                } else if bin_op_typ.is_arithmetic() || bin_op_typ.is_logical() {
+                } else if bin_op_typ.is_numerical() || bin_op_typ.is_logical() {
                     INVALID_SELECTIVITY
                 } else {
                     unreachable!("all BinOpTypes should be true for at least one is_*() function")

optd-datafusion-repr/src/plan_nodes.rs

@@ -24,8 +24,7 @@ pub use apply::{ApplyType, LogicalApply};
 pub use empty_relation::{LogicalEmptyRelation, PhysicalEmptyRelation};
 pub use expr::{
     BetweenExpr, BinOpExpr, BinOpType, CastExpr, ColumnRefExpr, ConstantExpr, ConstantType,
-    ExprList, FuncExpr, FuncType, LogOpExpr, LogOpType, SortOrderExpr, SortOrderType, UnOpExpr,
-    UnOpType,
+    ExprList, FuncExpr, FuncType, SortOrderExpr, SortOrderType, UnOpExpr, UnOpType,
 };
 pub use filter::{LogicalFilter, PhysicalFilter};
 pub use join::{JoinType, LogicalJoin, PhysicalHashJoin, PhysicalNestedLoopJoin};
@@ -71,7 +70,6 @@ pub enum OptRelNodeTyp {
     ColumnRef,
     UnOp(UnOpType),
     BinOp(BinOpType),
-    LogOp(LogOpType),
     Func(FuncType),
     SortOrder(SortOrderType),
     Between,
@@ -113,7 +111,6 @@ impl OptRelNodeTyp {
                 | Self::BinOp(_)
                 | Self::Func(_)
                 | Self::SortOrder(_)
-                | Self::LogOp(_)
                 | Self::Between
                 | Self::Cast
         )
@@ -364,9 +361,6 @@ pub fn explain(rel_node: OptRelNodeRef) -> Pretty<'static> {
         OptRelNodeTyp::SortOrder(_) => SortOrderExpr::from_rel_node(rel_node)
             .unwrap()
             .dispatch_explain(),
-        OptRelNodeTyp::LogOp(_) => LogOpExpr::from_rel_node(rel_node)
-            .unwrap()
-            .dispatch_explain(),
         OptRelNodeTyp::PhysicalCollector(_) => PhysicalCollector::from_rel_node(rel_node)
             .unwrap()
             .dispatch_explain(),

optd-datafusion-repr/src/plan_nodes/expr.rs

@@ -309,7 +309,6 @@ pub enum BinOpType {
     Leq,
     And,
     Or,
-    Xor,
 }
 
 impl Display for BinOpType {
@@ -318,8 +317,10 @@ impl Display for BinOpType {
     }
 }
 
+// The pattern of storing numerical, comparison, and logical operators in the same type with is_*() functions
+//     to distinguish between them matches how datafusion::logical_expr::Operator does things
 impl BinOpType {
-    pub fn is_arithmetic(&self) -> bool {
+    pub fn is_numerical(&self) -> bool {
         matches!(
             self,
             Self::Add | Self::Sub | Self::Mul | Self::Div | Self::Mod
@@ -334,7 +335,7 @@ impl BinOpType {
     }
 
     pub fn is_logical(&self) -> bool {
-        matches!(self, Self::And | Self::Or | Self::Xor)
+        matches!(self, Self::And | Self::Or)
     }
 }
 
@@ -531,71 +532,6 @@ impl OptRelNode for SortOrderExpr {
     }
 }
 
-#[derive(Copy, Clone, PartialEq, Eq, Hash, Debug)]
-pub enum LogOpType {
-    And,
-    Or,
-}
-
-impl Display for LogOpType {
-    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
-        write!(f, "{:?}", self)
-    }
-}
-
-#[derive(Clone, Debug)]
-pub struct LogOpExpr(pub Expr);
-
-impl LogOpExpr {
-    pub fn new(op_type: LogOpType, expr_list: ExprList) -> Self {
-        LogOpExpr(Expr(
-            RelNode {
-                typ: OptRelNodeTyp::LogOp(op_type),
-                children: vec![expr_list.into_rel_node()],
-                data: None,
-            }
-            .into(),
-        ))
-    }
-
-    pub fn children(&self) -> ExprList {
-        ExprList::from_rel_node(self.0.child(0)).unwrap()
-    }
-
-    pub fn child(&self, idx: usize) -> Expr {
-        self.children().child(idx)
-    }
-
-    pub fn op_type(&self) -> LogOpType {
-        if let OptRelNodeTyp::LogOp(op_type) = self.clone().into_rel_node().typ {
-            op_type
-        } else {
-            panic!("not a log op")
-        }
-    }
-}
-
-impl OptRelNode for LogOpExpr {
-    fn into_rel_node(self) -> OptRelNodeRef {
-        self.0.into_rel_node()
-    }
-
-    fn from_rel_node(rel_node: OptRelNodeRef) -> Option<Self> {
-        if !matches!(rel_node.typ, OptRelNodeTyp::LogOp(_)) {
-            return None;
-        }
-        Expr::from_rel_node(rel_node).map(Self)
-    }
-
-    fn dispatch_explain(&self) -> Pretty<'static> {
-        Pretty::simple_record(
-            self.op_type().to_string(),
-            vec![],
-            vec![self.children().explain()],
-        )
-    }
-}
-
 #[derive(Clone, Debug)]
 pub struct BetweenExpr(pub Expr);