Simplify binary comparison

bustubx/src/common/scalar.rs

@@ -1,7 +1,8 @@
 use crate::catalog::DataType;
 use crate::{BustubxError, BustubxResult};
+use std::cmp::Ordering;
 
-#[derive(Debug, Clone, PartialEq, Eq)]
+#[derive(Debug, Clone)]
 pub enum ScalarValue {
     Boolean(Option<bool>),
     Int8(Option<i8>),
@@ -59,36 +60,6 @@ impl ScalarValue {
         }
     }
 
-    // TODO compare value with different data type
-    pub fn compare(&self, other: &Self) -> std::cmp::Ordering {
-        match self {
-            Self::Boolean(v1) => match other {
-                Self::Boolean(v2) => v1.cmp(v2),
-                _ => panic!("Not implemented"),
-            },
-            Self::Int8(v1) => match other {
-                Self::Int8(v2) => v1.cmp(v2),
-                _ => panic!("Not implemented"),
-            },
-            Self::Int16(v1) => match other {
-                Self::Int16(v2) => v1.cmp(v2),
-                _ => panic!("Not implemented"),
-            },
-            Self::Int32(v1) => match other {
-                Self::Int32(v2) => v1.cmp(v2),
-                _ => panic!("Not implemented"),
-            },
-            Self::Int64(v1) => match other {
-                Self::Int64(v2) => v1.cmp(v2),
-                _ => panic!("Not implemented"),
-            },
-            Self::UInt64(v1) => match other {
-                Self::UInt64(v2) => v1.cmp(v2),
-                _ => panic!("Not implemented"),
-            },
-        }
-    }
-
     pub fn boolean_from_bytes(bytes: &[u8]) -> bool {
         bytes[0] != 0
     }
@@ -145,6 +116,56 @@ impl ScalarValue {
             ))),
         }
     }
+
+    pub fn wrapping_add(&self, other: Self) -> BustubxResult<Self> {
+        todo!()
+    }
+
+    pub fn wrapping_sub(&self, other: Self) -> BustubxResult<Self> {
+        todo!()
+    }
+}
+
+impl PartialEq for ScalarValue {
+    fn eq(&self, other: &Self) -> bool {
+        use ScalarValue::*;
+        match (self, other) {
+            (Boolean(v1), Boolean(v2)) => v1.eq(v2),
+            (Boolean(_), _) => false,
+            (Int8(v1), Int8(v2)) => v1.eq(v2),
+            (Int8(_), _) => false,
+            (Int16(v1), Int16(v2)) => v1.eq(v2),
+            (Int16(_), _) => false,
+            (Int32(v1), Int32(v2)) => v1.eq(v2),
+            (Int32(_), _) => false,
+            (Int64(v1), Int64(v2)) => v1.eq(v2),
+            (Int64(_), _) => false,
+            (UInt64(v1), UInt64(v2)) => v1.eq(v2),
+            (UInt64(_), _) => false,
+        }
+    }
+}
+
+impl Eq for ScalarValue {}
+
+impl PartialOrd for ScalarValue {
+    fn partial_cmp(&self, other: &Self) -> Option<Ordering> {
+        use ScalarValue::*;
+        match (self, other) {
+            (Boolean(v1), Boolean(v2)) => v1.partial_cmp(v2),
+            (Boolean(_), _) => None,
+            (Int8(v1), Int8(v2)) => v1.partial_cmp(v2),
+            (Int8(_), _) => None,
+            (Int16(v1), Int16(v2)) => v1.partial_cmp(v2),
+            (Int16(_), _) => None,
+            (Int32(v1), Int32(v2)) => v1.partial_cmp(v2),
+            (Int32(_), _) => None,
+            (Int64(v1), Int64(v2)) => v1.partial_cmp(v2),
+            (Int64(_), _) => None,
+            (UInt64(v1), UInt64(v2)) => v1.partial_cmp(v2),
+            (UInt64(_), _) => None,
+        }
+    }
 }
 
 impl std::fmt::Display for ScalarValue {

bustubx/src/error.rs

@@ -10,12 +10,15 @@ pub enum BustubxError {
     #[error("Internal error: {0}")]
     Internal(String),
 
-    #[error("Plan error: {0}")]
-    Plan(String),
-
     #[error("IO error: {0}")]
     Io(#[from] std::io::Error),
 
     #[error("Parser error: {0}")]
     Parser(#[from] sqlparser::parser::ParserError),
+
+    #[error("Plan error: {0}")]
+    Plan(String),
+
+    #[error("Execution error: {0}")]
+    Execution(String),
 }

bustubx/src/execution/physical_plan/sort.rs

@@ -7,7 +7,7 @@ use crate::planner::logical_plan::OrderByExpr;
 use crate::{
     execution::{ExecutionContext, VolcanoExecutor},
     storage::Tuple,
-    BustubxResult,
+    BustubxError, BustubxResult,
 };
 
 use super::PhysicalPlan;
@@ -45,6 +45,7 @@ impl VolcanoExecutor for PhysicalSort {
             all_tuples.push(next_tuple.unwrap());
         }
 
+        // TODO handle error during sorting
         // sort all tuples
         all_tuples.sort_by(|a, b| {
             let mut ordering = std::cmp::Ordering::Equal;
@@ -53,10 +54,15 @@ impl VolcanoExecutor for PhysicalSort {
                 let a_value = self.order_bys[index].expr.evaluate(a).unwrap();
                 let b_value = self.order_bys[index].expr.evaluate(b).unwrap();
                 ordering = if self.order_bys[index].asc {
-                    a_value.compare(&b_value)
+                    a_value.partial_cmp(&b_value)
                 } else {
-                    b_value.compare(&a_value)
-                };
+                    b_value.partial_cmp(&a_value)
+                }
+                .ok_or(BustubxError::Execution(format!(
+                    "Can not compare {} and {}",
+                    a_value, b_value
+                )))
+                .unwrap();
                 index += 1;
             }
             ordering

bustubx/src/expression/binary.rs

@@ -5,6 +5,7 @@ use crate::error::BustubxResult;
 use crate::expression::{Expr, ExprTrait};
 use crate::storage::Tuple;
 use crate::BustubxError;
+use std::cmp::Ordering;
 
 /// Binary expression
 #[derive(Clone, PartialEq, Eq, Debug)]
@@ -45,42 +46,12 @@ impl ExprTrait for BinaryExpr {
         let l = self.left.evaluate(tuple)?;
         let r = self.right.evaluate(tuple)?;
         match self.op {
-            BinaryOp::Gt => {
-                let order = l.compare(&r);
-                Ok(ScalarValue::Boolean(Some(
-                    order == std::cmp::Ordering::Greater,
-                )))
-            }
-            BinaryOp::Lt => {
-                let order = l.compare(&r);
-                Ok(ScalarValue::Boolean(Some(
-                    order == std::cmp::Ordering::Less,
-                )))
-            }
-            BinaryOp::GtEq => {
-                let order = l.compare(&r);
-                Ok(ScalarValue::Boolean(Some(
-                    order == std::cmp::Ordering::Greater || order == std::cmp::Ordering::Equal,
-                )))
-            }
-            BinaryOp::LtEq => {
-                let order = l.compare(&r);
-                Ok(ScalarValue::Boolean(Some(
-                    order == std::cmp::Ordering::Less || order == std::cmp::Ordering::Equal,
-                )))
-            }
-            BinaryOp::Eq => {
-                let order = l.compare(&r);
-                Ok(ScalarValue::Boolean(Some(
-                    order == std::cmp::Ordering::Equal,
-                )))
-            }
-            BinaryOp::NotEq => {
-                let order = l.compare(&r);
-                Ok(ScalarValue::Boolean(Some(
-                    order != std::cmp::Ordering::Equal,
-                )))
-            }
+            BinaryOp::Gt => evaluate_comparison(l, r, &vec![Ordering::Greater]),
+            BinaryOp::Lt => evaluate_comparison(l, r, &vec![Ordering::Less]),
+            BinaryOp::GtEq => evaluate_comparison(l, r, &vec![Ordering::Greater, Ordering::Equal]),
+            BinaryOp::LtEq => evaluate_comparison(l, r, &vec![Ordering::Less, Ordering::Equal]),
+            BinaryOp::Eq => evaluate_comparison(l, r, &vec![Ordering::Equal]),
+            BinaryOp::NotEq => evaluate_comparison(l, r, &vec![Ordering::Greater, Ordering::Less]),
             _ => Err(BustubxError::NotSupport(format!(
                 "binary operator {:?} not support evaluating yet",
                 self.op
@@ -96,6 +67,22 @@ impl ExprTrait for BinaryExpr {
     }
 }
 
+fn evaluate_comparison(
+    left: ScalarValue,
+    right: ScalarValue,
+    accepted_orderings: &[Ordering],
+) -> BustubxResult<ScalarValue> {
+    let order = left
+        .partial_cmp(&right)
+        .ok_or(BustubxError::Execution(format!(
+            "Can not compare {} and {}",
+            left, right
+        )))?;
+    Ok(ScalarValue::Boolean(Some(
+        accepted_orderings.contains(&order),
+    )))
+}
+
 #[derive(Debug, Copy, Clone, PartialEq, Eq, PartialOrd, Hash)]
 pub enum BinaryOp {
     Plus,

bustubx/src/storage/tuple.rs

@@ -90,7 +90,8 @@ impl Tuple {
             let compare_res = self
                 .value(column_index)
                 .unwrap()
-                .compare(&other.value(column_index).unwrap());
+                .partial_cmp(&other.value(column_index).unwrap())
+                .unwrap();
             if compare_res == std::cmp::Ordering::Equal {
                 continue;
             }

tests/sqllogictest/slt/select.slt

@@ -19,6 +19,6 @@ select b from t1
 4
 
 query I
-select 1
+select 1 as a
 ----
 1