Add validation for comparison operations (#505)

CHANGELOG.md

@@ -9,6 +9,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 ## [Unreleased]
 ### Changed
 - *BREAKING* partiql-parser: Added a source location to `ParseError::UnexpectedEndOfInput`
+- partiql-eval: Fixed behavior of comparison and `BETWEEN` operations w.r.t. type mismatches
 
 ### Added
 - partiql-value: Pretty-printing of `Value` via `ToPretty` trait

partiql-eval/src/eval/eval_expr_wrapper.rs

@@ -94,13 +94,24 @@ pub(crate) enum ArgCheckControlFlow<B, C, R = B> {
     Propagate(R),
 }
 
+#[derive(Debug, Clone, PartialEq, Eq, Hash)]
+pub(crate) struct ArgValidateError {
+    pub(crate) message: String,
+    pub(crate) propagate: Value,
+}
+
 /// A type which performs argument checking during evaluation.
 pub(crate) trait ArgChecker: Debug {
     /// Check an argument against an expected type.
     fn arg_check<'a>(
         typ: &PartiqlShape,
         arg: Cow<'a, Value>,
     ) -> ArgCheckControlFlow<Value, Cow<'a, Value>>;
+
+    /// Validate all arguments.
+    fn validate_args(args: Vec<Cow<'_, Value>>) -> Result<Vec<Cow<'_, Value>>, ArgValidateError> {
+        Ok(args)
+    }
 }
 
 /// How to handle argument mismatch and `MISSING` propagation
@@ -273,7 +284,12 @@ impl<const STRICT: bool, const N: usize, E: ExecuteEvalExpr<N>, ArgC: ArgChecker
             ControlFlow::Break(Missing)
         };
 
-        match evaluate_args::<{ STRICT }, ArgC, _>(&self.args, |n| &self.types[n], bindings, ctx) {
+        match evaluate_and_validate_args::<{ STRICT }, ArgC, _>(
+            &self.args,
+            |n| &self.types[n],
+            bindings,
+            ctx,
+        ) {
             ControlFlow::Continue(result) => match result.try_into() {
                 Ok(a) => ControlFlow::Continue(a),
                 Err(args) => err_arg_count_mismatch(args),
@@ -283,7 +299,7 @@ impl<const STRICT: bool, const N: usize, E: ExecuteEvalExpr<N>, ArgC: ArgChecker
     }
 }
 
-pub(crate) fn evaluate_args<
+pub(crate) fn evaluate_and_validate_args<
     'a,
     'c,
     't,
@@ -352,7 +368,18 @@ where
         ControlFlow::Break(v)
     } else {
         // If `propagate` is `None`, then return result
-        ControlFlow::Continue(result)
+        match ArgC::validate_args(result) {
+            Ok(result) => ControlFlow::Continue(result),
+            Err(err) => {
+                if STRICT {
+                    ctx.add_error(EvaluationError::IllegalState(format!(
+                        "Arguments failed validation: {}",
+                        err.message
+                    )))
+                }
+                ControlFlow::Break(err.propagate)
+            }
+        }
     }
 }
 

partiql-eval/src/eval/expr/functions.rs

@@ -1,4 +1,6 @@
-use crate::eval::eval_expr_wrapper::{evaluate_args, DefaultArgChecker, PropagateMissing};
+use crate::eval::eval_expr_wrapper::{
+    evaluate_and_validate_args, DefaultArgChecker, PropagateMissing,
+};
 
 use crate::eval::expr::{BindError, BindEvalExpr, EvalExpr};
 use crate::eval::EvalContext;
@@ -41,7 +43,12 @@ impl<const STRICT: bool> EvalExpr for EvalExprFnScalar<STRICT> {
     {
         type Check<const STRICT: bool> = DefaultArgChecker<STRICT, PropagateMissing<true>>;
         let typ = PartiqlShapeBuilder::init_or_get().new_struct(StructType::new_any());
-        match evaluate_args::<{ STRICT }, Check<STRICT>, _>(&self.args, |_| &typ, bindings, ctx) {
+        match evaluate_and_validate_args::<{ STRICT }, Check<STRICT>, _>(
+            &self.args,
+            |_| &typ,
+            bindings,
+            ctx,
+        ) {
             ControlFlow::Break(v) => Cow::Owned(v),
             ControlFlow::Continue(args) => match self.plan.evaluate(&args, ctx.as_session()) {
                 Ok(v) => v,

partiql-eval/src/eval/expr/operators.rs

@@ -1,7 +1,7 @@
 use crate::eval::eval_expr_wrapper::{
-    ArgCheckControlFlow, ArgChecker, ArgShortCircuit, BinaryValueExpr, DefaultArgChecker,
-    ExecuteEvalExpr, NullArgChecker, PropagateMissing, PropagateNull, TernaryValueExpr,
-    UnaryValueExpr,
+    ArgCheckControlFlow, ArgChecker, ArgShortCircuit, ArgValidateError, BinaryValueExpr,
+    DefaultArgChecker, ExecuteEvalExpr, NullArgChecker, PropagateMissing, PropagateNull,
+    TernaryValueExpr, UnaryValueExpr,
 };
 
 use crate::eval::expr::{BindError, BindEvalExpr, EvalExpr};
@@ -12,7 +12,7 @@ use partiql_types::{
     Static, StructType,
 };
 use partiql_value::Value::{Boolean, Missing, Null};
-use partiql_value::{BinaryAnd, EqualityValue, NullableEq, NullableOrd, Tuple, Value};
+use partiql_value::{BinaryAnd, Comparable, EqualityValue, NullableEq, NullableOrd, Tuple, Value};
 
 use std::borrow::{Borrow, Cow};
 use std::fmt::{Debug, Formatter};
@@ -146,6 +146,34 @@ impl<const TARGET: bool, OnMissing: ArgShortCircuit> ArgChecker
     }
 }
 
+#[derive(Debug)]
+pub(crate) struct ComparisonArgChecker<const STRICT: bool, OnMissing: ArgShortCircuit> {
+    check: PhantomData<DefaultArgChecker<STRICT, OnMissing>>,
+}
+
+impl<const STRICT: bool, OnMissing: ArgShortCircuit> ArgChecker
+    for ComparisonArgChecker<STRICT, OnMissing>
+{
+    #[inline]
+    fn arg_check<'a>(
+        typ: &PartiqlShape,
+        arg: Cow<'a, Value>,
+    ) -> ArgCheckControlFlow<Value, Cow<'a, Value>> {
+        DefaultArgChecker::<{ STRICT }, OnMissing>::arg_check(typ, arg)
+    }
+
+    fn validate_args(args: Vec<Cow<'_, Value>>) -> Result<Vec<Cow<'_, Value>>, ArgValidateError> {
+        if args.len() == 2 && args[0].is_comparable_to(&args[1]) {
+            Ok(args)
+        } else {
+            Err(ArgValidateError {
+                message: "data-type mismatch".to_string(),
+                propagate: OnMissing::propagate(),
+            })
+        }
+    }
+}
+
 impl BindEvalExpr for EvalOpBinary {
     #[inline]
     fn bind<const STRICT: bool>(
@@ -157,6 +185,7 @@ impl BindEvalExpr for EvalOpBinary {
         type InCheck<const STRICT: bool> = DefaultArgChecker<STRICT, PropagateNull<false>>;
         type Check<const STRICT: bool> = DefaultArgChecker<STRICT, PropagateMissing<true>>;
         type EqCheck<const STRICT: bool> = DefaultArgChecker<STRICT, PropagateMissing<false>>;
+        type CompCheck<const STRICT: bool> = ComparisonArgChecker<STRICT, PropagateMissing<true>>;
         type MathCheck<const STRICT: bool> = DefaultArgChecker<STRICT, PropagateMissing<true>>;
 
         macro_rules! create {
@@ -177,6 +206,12 @@ impl BindEvalExpr for EvalOpBinary {
             };
         }
 
+        macro_rules! comparison {
+            ($f:expr) => {
+                create!(CompCheck<STRICT>, [type_dynamic!(), type_dynamic!()], $f)
+            };
+        }
+
         macro_rules! math {
             ($f:expr) => {{
                 let nums = PartiqlShapeBuilder::init_or_get().any_of(type_numeric!());
@@ -195,10 +230,10 @@ impl BindEvalExpr for EvalOpBinary {
                 let wrap = EqualityValue::<false, Value>;
                 NullableEq::neq(&wrap(lhs), &wrap(rhs))
             }),
-            EvalOpBinary::Gt => equality!(NullableOrd::gt),
-            EvalOpBinary::Gteq => equality!(NullableOrd::gteq),
-            EvalOpBinary::Lt => equality!(NullableOrd::lt),
-            EvalOpBinary::Lteq => equality!(NullableOrd::lteq),
+            EvalOpBinary::Gt => comparison!(NullableOrd::gt),
+            EvalOpBinary::Gteq => comparison!(NullableOrd::gteq),
+            EvalOpBinary::Lt => comparison!(NullableOrd::lt),
+            EvalOpBinary::Lteq => comparison!(NullableOrd::lteq),
             EvalOpBinary::Add => math!(|lhs, rhs| lhs + rhs),
             EvalOpBinary::Sub => math!(|lhs, rhs| lhs - rhs),
             EvalOpBinary::Mul => math!(|lhs, rhs| lhs * rhs),
@@ -275,6 +310,35 @@ impl BindEvalExpr for EvalOpBinary {
     }
 }
 
+#[derive(Debug)]
+pub(crate) struct BetweenArgChecker<const STRICT: bool> {
+    check: PhantomData<NullArgChecker>,
+}
+
+impl<const STRICT: bool> ArgChecker for BetweenArgChecker<STRICT> {
+    #[inline]
+    fn arg_check<'a>(
+        typ: &PartiqlShape,
+        arg: Cow<'a, Value>,
+    ) -> ArgCheckControlFlow<Value, Cow<'a, Value>> {
+        NullArgChecker::arg_check(typ, arg)
+    }
+
+    fn validate_args(args: Vec<Cow<'_, Value>>) -> Result<Vec<Cow<'_, Value>>, ArgValidateError> {
+        if args.len() == 3
+            && args[0].is_comparable_to(&args[1])
+            && args[0].is_comparable_to(&args[2])
+        {
+            Ok(args)
+        } else {
+            Err(ArgValidateError {
+                message: "data-type mismatch".to_string(),
+                propagate: Value::Missing,
+            })
+        }
+    }
+}
+
 /// Represents an evaluation `PartiQL` `BETWEEN` operator, e.g. `x BETWEEN 10 AND 20`.
 #[derive(Debug, Default, Clone)]
 pub(crate) struct EvalBetweenExpr {}
@@ -285,7 +349,7 @@ impl BindEvalExpr for EvalBetweenExpr {
         args: Vec<Box<dyn EvalExpr>>,
     ) -> Result<Box<dyn EvalExpr>, BindError> {
         let types = [type_dynamic!(), type_dynamic!(), type_dynamic!()];
-        TernaryValueExpr::create_checked::<{ STRICT }, NullArgChecker, _>(
+        TernaryValueExpr::create_checked::<{ STRICT }, BetweenArgChecker<{ STRICT }>, _>(
             types,
             args,
             |value, from, to| value.gteq(from).and(&value.lteq(to)),

partiql-eval/src/lib.rs

@@ -741,6 +741,9 @@ mod tests {
         // left part of AND evaluates to false
         eval_between_op(Value::from(1), Value::from(2), Null, Value::from(false));
         eval_between_op(Value::from(1), Value::from(2), Missing, Value::from(false));
+        // right part of AND evaluates to false
+        eval_between_op(Value::from(2), Null, Value::from(1), Value::from(false));
+        eval_between_op(Value::from(2), Missing, Value::from(1), Value::from(false));
     }
 
     #[test]

partiql-value/src/lib.rs

@@ -6,7 +6,7 @@ use std::cmp::Ordering;
 
 use std::borrow::Cow;
 
-use std::fmt::{Debug, Formatter};
+use std::fmt::{Debug, Display, Formatter};
 use std::hash::Hash;
 
 use std::iter::Once;
@@ -27,6 +27,7 @@ pub use list::*;
 pub use pretty::*;
 pub use tuple::*;
 
+use partiql_common::pretty::ToPretty;
 #[cfg(feature = "serde")]
 use serde::{Deserialize, Serialize};
 
@@ -60,6 +61,15 @@ pub enum Value {
     // TODO: add other supported PartiQL values -- sexp
 }
 
+impl Display for Value {
+    fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result {
+        match self.to_pretty_string(f.width().unwrap_or(80)) {
+            Ok(pretty) => f.write_str(&pretty),
+            Err(_) => f.write_str("<internal value error occurred>"),
+        }
+    }
+}
+
 impl ops::Add for &Value {
     type Output = Value;
 

partiql/tests/common.rs

@@ -0,0 +1,115 @@
+use partiql_ast_passes::error::AstTransformationError;
+use partiql_catalog::catalog::{Catalog, PartiqlCatalog};
+use partiql_catalog::context::SystemContext;
+use partiql_eval as eval;
+use partiql_eval::env::basic::MapBindings;
+use partiql_eval::error::{EvalErr, PlanErr};
+use partiql_eval::eval::{BasicContext, EvalPlan, EvalResult, Evaluated};
+use partiql_eval::plan::EvaluationMode;
+use partiql_logical as logical;
+use partiql_parser::{Parsed, ParserError, ParserResult};
+use partiql_value::{DateTime, Value};
+use std::error::Error;
+use thiserror::Error;
+
+#[derive(Error, Debug)]
+pub enum TestError<'a> {
+    #[error("Parse error: {0:?}")]
+    Parse(ParserError<'a>),
+    #[error("Lower error: {0:?}")]
+    Lower(AstTransformationError),
+    #[error("Plan error: {0:?}")]
+    Plan(PlanErr),
+    #[error("Evaluation error: {0:?}")]
+    Eval(EvalErr),
+    #[error("Other: {0:?}")]
+    Other(Box<dyn Error>),
+}
+
+impl<'a> From<ParserError<'a>> for TestError<'a> {
+    fn from(err: ParserError<'a>) -> Self {
+        TestError::Parse(err)
+    }
+}
+
+impl From<AstTransformationError> for TestError<'_> {
+    fn from(err: AstTransformationError) -> Self {
+        TestError::Lower(err)
+    }
+}
+
+impl From<PlanErr> for TestError<'_> {
+    fn from(err: PlanErr) -> Self {
+        TestError::Plan(err)
+    }
+}
+
+impl From<EvalErr> for TestError<'_> {
+    fn from(err: EvalErr) -> Self {
+        TestError::Eval(err)
+    }
+}
+
+impl From<Box<dyn Error>> for TestError<'_> {
+    fn from(err: Box<dyn Error>) -> Self {
+        TestError::Other(err)
+    }
+}
+
+#[track_caller]
+#[inline]
+pub fn parse(statement: &str) -> ParserResult<'_> {
+    partiql_parser::Parser::default().parse(statement)
+}
+
+#[track_caller]
+#[inline]
+pub fn lower(
+    catalog: &dyn Catalog,
+    parsed: &Parsed<'_>,
+) -> Result<logical::LogicalPlan<logical::BindingsOp>, AstTransformationError> {
+    let planner = partiql_logical_planner::LogicalPlanner::new(catalog);
+    planner.lower(parsed)
+}
+
+#[track_caller]
+#[inline]
+pub fn compile(
+    mode: EvaluationMode,
+    catalog: &dyn Catalog,
+    logical: logical::LogicalPlan<logical::BindingsOp>,
+) -> Result<EvalPlan, PlanErr> {
+    let mut planner = eval::plan::EvaluatorPlanner::new(mode, catalog);
+    planner.compile(&logical)
+}
+
+#[track_caller]
+#[inline]
+pub fn evaluate(mut plan: EvalPlan, bindings: MapBindings<Value>) -> EvalResult {
+    let sys = SystemContext {
+        now: DateTime::from_system_now_utc(),
+    };
+    let ctx = BasicContext::new(bindings, sys);
+    plan.execute_mut(&ctx)
+}
+
+#[track_caller]
+#[inline]
+pub fn eval_query_with_catalog<'a>(
+    statement: &'a str,
+    catalog: &dyn Catalog,
+    mode: EvaluationMode,
+) -> Result<Evaluated, TestError<'a>> {
+    let parsed = parse(statement)?;
+    let lowered = lower(catalog, &parsed)?;
+    let bindings = Default::default();
+    let plan = compile(mode, catalog, lowered)?;
+    Ok(evaluate(plan, bindings)?)
+}
+
+#[track_caller]
+#[inline]
+pub fn eval_query(statement: &str, mode: EvaluationMode) -> Result<Evaluated, TestError<'_>> {
+    let catalog = PartiqlCatalog::default();
+    eval_query_with_catalog(statement, &catalog, mode)
+}