Add errors from BaseTableExpr's to the evaluator (#447)

Author: jpschorr

CHANGELOG.md

@@ -10,13 +10,17 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 - Adds quotes to the attributes of PartiQL tuple's debug output so it can be read and transformed using Kotlin `partiql-cli`
 - Adds u8, u16, u32, u64, and u128 support to partiql_value::Value::from(type)
 - [breaking] Changes the interface to `EvalPlan` to accept an `EvalContext`
+- [breaking] Changes `EvaluationError` to not implement `Clone` 
+- [breaking] Changes the structure of `EvalPlan`
 
 ### Added
 - Add `partiql-extension-visualize` for visualizing AST and logical plan
 - Add a `SessionContext` containing both a system-level and a user-level context object usable by expression evaluation 
 
 ### Fixed
 - Fixed `ORDER BY`'s ability to see into projection aliases
+- Fixed errors in `BaseTableExpr`s get added to the evaluation context
+- Fixed certain errors surfacing in Permissive evaluation mode, when they should only be present in Strict mode
 
 ## [0.6.0] - 2023-10-31
 ### Changed

partiql-catalog/src/lib.rs

@@ -46,6 +46,7 @@ pub struct ObjectId {
 }
 
 pub type BaseTableExprResultError = Box<dyn Error>;
+
 pub type BaseTableExprResultValueIter<'a> =
     Box<dyn 'a + Iterator<Item = Result<Value, BaseTableExprResultError>>>;
 pub type BaseTableExprResult<'a> =

partiql-eval/src/error.rs

@@ -1,6 +1,7 @@
 use crate::eval::evaluable::Evaluable;
 use crate::eval::expr::EvalExpr;
 use crate::eval::EvalContext;
+use partiql_catalog::BaseTableExprResultError;
 use partiql_value::{Tuple, Value};
 use std::borrow::Cow;
 use thiserror::Error;
@@ -30,7 +31,7 @@ pub struct EvalErr {
 }
 
 /// An error that can happen during evaluation.
-#[derive(Error, Debug, Clone, PartialEq, Eq, Hash)]
+#[derive(Error, Debug)]
 #[non_exhaustive]
 pub enum EvaluationError {
     /// Internal error that was not due to user input or API violation.
@@ -42,9 +43,13 @@ pub enum EvaluationError {
     /// Feature has not yet been implemented.
     #[error("Not yet implemented: {0}")]
     NotYetImplemented(String),
+
+    /// Error originating in an extension
+    #[error("Base Table Expression Error")]
+    ExtensionResultError(#[from] BaseTableExprResultError),
 }
 
-/// Used when an error occurs during the the logical to eval plan conversion. Allows the conversion
+/// Used when an error occurs during the logical to eval plan conversion. Allows the conversion
 /// to continue in order to report multiple errors.
 #[derive(Debug)]
 pub(crate) struct ErrorNode {}

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

@@ -37,14 +37,14 @@ impl EvalExpr for EvalFnBaseTableExpr {
                 let bag: Result<Bag, _> = it.collect();
                 match bag {
                     Ok(b) => Value::from(b),
-                    Err(_) => {
-                        // TODO hook into pending eval errors
+                    Err(err) => {
+                        ctx.add_error(err.into());
                         Missing
                     }
                 }
             }
-            Err(_) => {
-                // TODO hook into pending eval errors
+            Err(err) => {
+                ctx.add_error(err.into());
                 Missing
             }
         };

partiql-eval/src/eval/mod.rs

@@ -23,6 +23,7 @@ use petgraph::visit::EdgeRef;
 use unicase::UniCase;
 
 use crate::eval::evaluable::{EvalType, Evaluable};
+use crate::plan::EvaluationMode;
 
 pub(crate) mod eval_expr_wrapper;
 pub mod evaluable;
@@ -31,11 +32,14 @@ pub mod expr;
 /// Represents a PartiQL evaluation query plan which is a plan that can be evaluated to produce
 /// a result. The plan uses a directed `petgraph::StableGraph`.
 #[derive(Debug)]
-pub struct EvalPlan(pub StableGraph<Box<dyn Evaluable>, u8, Directed>);
+pub struct EvalPlan {
+    mode: EvaluationMode,
+    plan_graph: StableGraph<Box<dyn Evaluable>, u8, Directed>,
+}
 
 impl Default for EvalPlan {
     fn default() -> Self {
-        Self::new()
+        Self::new(EvaluationMode::Permissive, Default::default())
     }
 }
 
@@ -48,13 +52,16 @@ fn err_illegal_state(msg: impl AsRef<str>) -> EvalErr {
 
 impl EvalPlan {
     /// Creates a new evaluation plan.
-    fn new() -> Self {
-        EvalPlan(StableGraph::<Box<dyn Evaluable>, u8, Directed>::new())
+    pub fn new(
+        mode: EvaluationMode,
+        plan_graph: StableGraph<Box<dyn Evaluable>, u8, Directed>,
+    ) -> Self {
+        EvalPlan { mode, plan_graph }
     }
 
     #[inline]
     fn plan_graph(&mut self) -> &mut StableGraph<Box<dyn Evaluable>, u8> {
-        &mut self.0
+        &mut self.plan_graph
     }
 
     #[inline]
@@ -73,7 +80,7 @@ impl EvalPlan {
         // that all v ∈ V \{v0} are reachable from v0. Note that this is the definition of trees
         // without the condition |E| = |V | − 1. Hence, all trees are DAGs.
         // Reference: https://link.springer.com/article/10.1007/s00450-009-0061-0
-        let ops = toposort(&self.0, None).map_err(|e| EvalErr {
+        let ops = toposort(&self.plan_graph, None).map_err(|e| EvalErr {
             errors: vec![EvaluationError::InvalidEvaluationPlan(format!(
                 "Malformed evaluation plan detected: {e:?}"
             ))],
@@ -101,7 +108,7 @@ impl EvalPlan {
                 result = Some(src.evaluate(ctx));
 
                 // return on first evaluation error
-                if ctx.has_errors() {
+                if ctx.has_errors() && self.mode == EvaluationMode::Strict {
                     return Err(EvalErr {
                         errors: ctx.errors(),
                     });
@@ -127,7 +134,7 @@ impl EvalPlan {
     }
 
     pub fn to_dot_graph(&self) -> String {
-        format!("{:?}", Dot::with_config(&self.0, &[]))
+        format!("{:?}", Dot::with_config(&self.plan_graph, &[]))
     }
 }
 

partiql-eval/src/plan.rs

@@ -49,6 +49,7 @@ macro_rules! correct_num_args_or_err {
     };
 }
 
+#[derive(Debug, Eq, PartialEq)]
 pub enum EvaluationMode {
     Strict,
     Permissive,
@@ -129,22 +130,26 @@ impl<'c> EvaluatorPlanner<'c> {
     fn plan_eval<const STRICT: bool>(&mut self, lg: &LogicalPlan<BindingsOp>) -> EvalPlan {
         let flows = lg.flows();
 
-        let mut graph: StableGraph<_, _> = Default::default();
+        let mut plan_graph: StableGraph<_, _> = Default::default();
         let mut seen = HashMap::new();
 
         for (s, d, branch_num) in flows {
             let mut add_node = |op_id: &OpId| {
                 let logical_op = lg.operator(*op_id).unwrap();
-                *seen
-                    .entry(*op_id)
-                    .or_insert_with(|| graph.add_node(self.get_eval_node::<{ STRICT }>(logical_op)))
+                *seen.entry(*op_id).or_insert_with(|| {
+                    plan_graph.add_node(self.get_eval_node::<{ STRICT }>(logical_op))
+                })
             };
 
             let (s, d) = (add_node(s), add_node(d));
-            graph.add_edge(s, d, *branch_num);
+            plan_graph.add_edge(s, d, *branch_num);
         }
-
-        EvalPlan(graph)
+        let mode = if STRICT {
+            EvaluationMode::Strict
+        } else {
+            EvaluationMode::Permissive
+        };
+        EvalPlan::new(mode, plan_graph)
     }
 
     fn get_eval_node<const STRICT: bool>(&mut self, be: &BindingsOp) -> Box<dyn Evaluable> {

partiql/Cargo.toml

@@ -39,6 +39,8 @@ itertools = "0.12"
 criterion = "0.5"
 rand = "0.8"
 
+assert_matches = "1.5"
+
 [[bench]]
 name = "bench_eval_multi_like"
 harness = false