Changes logical plan to eval plan API to return Result type (#363)

Author: alancai98

CHANGELOG.md

@@ -11,6 +11,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 - *BREAKING:* removed `from_ion` method on `Value`
 - *BREAKING:* partiql-ast: `visit` fn returns a `partiql-ast::Recurse` type to indicate if visitation of children nodes should continue
 - *BREAKING:* partiql-logical-planner: modifies `lower(parsed: &Parsed)` to return a Result type of `Result<logical::LogicalPlan<logical::BindingsOp>, LoweringError>` rather than a `logical::LogicalPlan<logical::BindingsOp>`
+- *BREAKING:* partiql-eval: modifies `compile(&mut self, plan: &LogicalPlan<BindingsOp>)` to return a Result type of `Result<EvalPlan, PlanErr>` rather than an `EvalPlan`
   - This is part of an effort to replace `panic`s with `Result`s
 - *BREAKING:* partiql-logical-planner: Adds a `LogicalPlanner` to encapsulate the `lower` method
 - *BREAKING:* partiql-eval: Adds a `EvaluatorPlanner` now requires a `Catalog` to be supplied at initialization

extension/partiql-extension-ion-functions/src/lib.rs

@@ -183,9 +183,9 @@ mod tests {
         logical: partiql_logical::LogicalPlan<partiql_logical::BindingsOp>,
         bindings: MapBindings<Value>,
     ) -> Value {
-        let planner = partiql_eval::plan::EvaluatorPlanner::new(catalog);
+        let mut planner = partiql_eval::plan::EvaluatorPlanner::new(catalog);
 
-        let mut plan = planner.compile(&logical);
+        let mut plan = planner.compile(&logical).expect("Expect no plan error");
 
         if let Ok(out) = plan.execute_mut(bindings) {
             out.result

partiql-conformance-tests/tests/mod.rs

@@ -1,6 +1,8 @@
 use partiql_catalog::{Catalog, PartiqlCatalog};
 use partiql_eval as eval;
 use partiql_eval::env::basic::MapBindings;
+use partiql_eval::error::PlanErr;
+use partiql_eval::eval::{EvalPlan, EvalResult};
 use partiql_logical as logical;
 use partiql_logical_planner::error::LoweringError;
 use partiql_parser::{Parsed, ParserResult};
@@ -34,20 +36,18 @@ pub(crate) fn lower(
 
 #[track_caller]
 #[inline]
-pub(crate) fn evaluate(
+pub(crate) fn compile(
     catalog: &dyn Catalog,
     logical: logical::LogicalPlan<logical::BindingsOp>,
-    bindings: MapBindings<Value>,
-) -> Value {
-    let planner = eval::plan::EvaluatorPlanner::new(catalog);
-
-    let mut plan = planner.compile(&logical);
+) -> Result<EvalPlan, PlanErr> {
+    let mut planner = eval::plan::EvaluatorPlanner::new(catalog);
+    planner.compile(&logical)
+}
 
-    if let Ok(out) = plan.execute_mut(bindings) {
-        out.result
-    } else {
-        Value::Missing
-    }
+#[track_caller]
+#[inline]
+pub(crate) fn evaluate(mut plan: EvalPlan, bindings: MapBindings<Value>) -> EvalResult {
+    plan.execute_mut(bindings)
 }
 
 #[track_caller]
@@ -111,10 +111,10 @@ pub(crate) fn fail_eval(statement: &str, mode: EvaluationMode, env: &Option<Test
         .as_ref()
         .map(|e| (&e.value).into())
         .unwrap_or_else(MapBindings::default);
-    let out = evaluate(&catalog, lowered, bindings);
+    let plan = compile(&catalog, lowered).expect("compile");
+    let out = evaluate(plan, bindings);
 
-    println!("{:?}", &out);
-    // TODO assert failure
+    assert!(out.is_err());
 }
 
 #[track_caller]
@@ -139,10 +139,13 @@ pub(crate) fn pass_eval(
         .as_ref()
         .map(|e| (&e.value).into())
         .unwrap_or_else(MapBindings::default);
-    let out = evaluate(&catalog, lowered, bindings);
+    let plan = compile(&catalog, lowered).expect("compile");
+    let out = evaluate(plan, bindings);
 
-    println!("{:?}", &out);
-    assert_eq!(out, expected.value);
+    match out {
+        Ok(v) => assert_eq!(v.result, expected.value),
+        Err(err) => panic!("Encountered error: {err:?}"),
+    }
 }
 
 #[allow(dead_code)]

partiql-eval/benches/bench_eval.rs

@@ -122,9 +122,8 @@ fn logical_plan() -> LogicalPlan<BindingsOp> {
 
 fn eval_plan(logical: &LogicalPlan<BindingsOp>) -> EvalPlan {
     let catalog = PartiqlCatalog::default();
-    let planner = plan::EvaluatorPlanner::new(&catalog);
-
-    planner.compile(logical)
+    let mut planner = plan::EvaluatorPlanner::new(&catalog);
+    planner.compile(logical).expect("Expect no plan error")
 }
 
 fn evaluate(mut plan: EvalPlan, bindings: MapBindings<Value>) -> Value {

partiql-eval/src/error.rs

@@ -0,0 +1,66 @@
+use crate::eval::evaluable::Evaluable;
+use crate::eval::expr::EvalExpr;
+use crate::eval::EvalContext;
+use partiql_value::{Tuple, Value};
+use std::borrow::Cow;
+use thiserror::Error;
+
+/// All errors that occurred during [`partiql_logical::LogicalPlan`] to [`eval::EvalPlan`] creation.
+#[derive(Debug)]
+pub struct PlanErr {
+    pub errors: Vec<PlanningError>,
+}
+
+/// An error that can happen during [`partiql_logical::LogicalPlan`] to [`eval::EvalPlan`] creation.
+#[derive(Error, Debug, Clone, PartialEq, Eq, Hash)]
+#[non_exhaustive]
+pub enum PlanningError {
+    /// Feature has not yet been implemented.
+    #[error("Not yet implemented: {0}")]
+    NotYetImplemented(String),
+    /// Internal error that was not due to user input or API violation.
+    #[error("Illegal State: {0}")]
+    IllegalState(String),
+}
+
+/// All errors that occurred during evaluation.
+#[derive(Debug)]
+pub struct EvalErr {
+    pub errors: Vec<EvaluationError>,
+}
+
+/// An error that can happen during evaluation.
+#[derive(Error, Debug, Clone, PartialEq, Eq, Hash)]
+#[non_exhaustive]
+pub enum EvaluationError {
+    /// Malformed evaluation plan with graph containing cycle.
+    #[error("Evaluation Error: invalid evaluation plan detected `{0}`")]
+    InvalidEvaluationPlan(String),
+}
+
+/// Used when an error occurs during the the logical to eval plan conversion. Allows the conversion
+/// to continue in order to report multiple errors.
+#[derive(Debug)]
+pub(crate) struct ErrorNode {}
+
+impl ErrorNode {
+    pub(crate) fn new() -> ErrorNode {
+        ErrorNode {}
+    }
+}
+
+impl Evaluable for ErrorNode {
+    fn evaluate(&mut self, _ctx: &dyn EvalContext) -> Option<Value> {
+        panic!("ErrorNode will not be evaluated")
+    }
+
+    fn update_input(&mut self, _input: Value, _branch_num: u8) {
+        panic!("ErrorNode will not be evaluated")
+    }
+}
+
+impl EvalExpr for ErrorNode {
+    fn evaluate<'a>(&'a self, _bindings: &'a Tuple, _ctx: &'a dyn EvalContext) -> Cow<'a, Value> {
+        panic!("ErrorNode will not be evaluated")
+    }
+}

partiql-eval/src/eval/mod.rs

@@ -2,8 +2,6 @@ use itertools::Itertools;
 
 use std::fmt::Debug;
 
-use thiserror::Error;
-
 use petgraph::algo::toposort;
 use petgraph::dot::{Config, Dot};
 use petgraph::prelude::StableGraph;
@@ -16,6 +14,7 @@ use crate::env::Bindings;
 
 use petgraph::graph::NodeIndex;
 
+use crate::error::{EvalErr, EvaluationError};
 use petgraph::visit::EdgeRef;
 
 use crate::eval::evaluable::Evaluable;
@@ -106,16 +105,6 @@ pub struct Evaluated {
     pub result: Value,
 }
 
-pub struct EvalErr {
-    pub errors: Vec<EvaluationError>,
-}
-
-#[derive(Error, Debug)]
-pub enum EvaluationError {
-    #[error("Evaluation Error: malformed evaluation plan detected `{}`", _0)]
-    InvalidEvaluationPlan(String),
-}
-
 /// Represents an evaluation context that is used during evaluation of a plan.
 pub trait EvalContext {
     fn bindings(&self) -> &dyn Bindings<Value>;

partiql-eval/src/lib.rs

@@ -1,4 +1,5 @@
 pub mod env;
+pub mod error;
 pub mod eval;
 pub mod plan;
 
@@ -27,8 +28,8 @@ mod tests {
 
     fn evaluate(logical: LogicalPlan<BindingsOp>, bindings: MapBindings<Value>) -> Value {
         let catalog = PartiqlCatalog::default();
-        let planner = plan::EvaluatorPlanner::new(&catalog);
-        let mut plan = planner.compile(&logical);
+        let mut planner = plan::EvaluatorPlanner::new(&catalog);
+        let mut plan = planner.compile(&logical).expect("Expect no plan error");
 
         if let Ok(out) = plan.execute_mut(bindings) {
             out.result