diff --git a/CHANGELOG.md b/CHANGELOG.md index 7c38c1bb..4f2c51a8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Changed - Adds quotes to the attributes of PartiQL tuple's debug output so it can be read and transformed using Kotlin `partiql-cli` - [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 @@ -16,6 +18,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### 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 diff --git a/partiql-catalog/src/lib.rs b/partiql-catalog/src/lib.rs index 62f92a21..83479cb4 100644 --- a/partiql-catalog/src/lib.rs +++ b/partiql-catalog/src/lib.rs @@ -46,6 +46,7 @@ pub struct ObjectId { } pub type BaseTableExprResultError = Box; + pub type BaseTableExprResultValueIter<'a> = Box>>; pub type BaseTableExprResult<'a> = diff --git a/partiql-eval/src/error.rs b/partiql-eval/src/error.rs index 2bdde323..e310931f 100644 --- a/partiql-eval/src/error.rs +++ b/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 {} diff --git a/partiql-eval/src/eval/expr/base_table.rs b/partiql-eval/src/eval/expr/base_table.rs index 7a9e955b..1d6effc9 100644 --- a/partiql-eval/src/eval/expr/base_table.rs +++ b/partiql-eval/src/eval/expr/base_table.rs @@ -37,14 +37,14 @@ impl EvalExpr for EvalFnBaseTableExpr { let bag: Result = 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 } }; diff --git a/partiql-eval/src/eval/mod.rs b/partiql-eval/src/eval/mod.rs index c12794c8..d5251daf 100644 --- a/partiql-eval/src/eval/mod.rs +++ b/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, u8, Directed>); +pub struct EvalPlan { + mode: EvaluationMode, + plan_graph: StableGraph, 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) -> EvalErr { impl EvalPlan { /// Creates a new evaluation plan. - fn new() -> Self { - EvalPlan(StableGraph::, u8, Directed>::new()) + pub fn new( + mode: EvaluationMode, + plan_graph: StableGraph, u8, Directed>, + ) -> Self { + EvalPlan { mode, plan_graph } } #[inline] fn plan_graph(&mut self) -> &mut StableGraph, 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, &[])) } } diff --git a/partiql-eval/src/plan.rs b/partiql-eval/src/plan.rs index e6fc7996..b269f0c5 100644 --- a/partiql-eval/src/plan.rs +++ b/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(&mut self, lg: &LogicalPlan) -> 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(&mut self, be: &BindingsOp) -> Box { diff --git a/partiql/Cargo.toml b/partiql/Cargo.toml index 9f7bb6de..68257272 100644 --- a/partiql/Cargo.toml +++ b/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 diff --git a/partiql/tests/extension_error.rs b/partiql/tests/extension_error.rs new file mode 100644 index 00000000..6f469fe6 --- /dev/null +++ b/partiql/tests/extension_error.rs @@ -0,0 +1,254 @@ +use std::any::Any; +use std::borrow::Cow; + +use std::error::Error; + +use thiserror::Error; + +use partiql_catalog::call_defs::{CallDef, CallSpec, CallSpecArg}; +use partiql_catalog::context::{SessionContext, SystemContext}; +use partiql_catalog::{ + BaseTableExpr, BaseTableExprResult, BaseTableExprResultError, BaseTableFunctionInfo, Catalog, + Extension, PartiqlCatalog, TableFunction, +}; +use partiql_eval::env::basic::MapBindings; +use partiql_eval::error::{EvalErr, EvaluationError}; +use partiql_eval::eval::{BasicContext, Evaluated}; +use partiql_eval::plan::EvaluationMode; +use partiql_parser::{Parsed, ParserResult}; +use partiql_value::{bag, tuple, DateTime, Value}; + +use partiql_logical as logical; + +#[derive(Debug)] +pub struct UserCtxTestExtension {} + +impl partiql_catalog::Extension for UserCtxTestExtension { + fn name(&self) -> String { + "test_extension".into() + } + + fn load(&self, catalog: &mut dyn Catalog) -> Result<(), Box> { + match catalog + .add_table_function(TableFunction::new(Box::new(TestUserContextFunction::new()))) + { + Ok(_) => Ok(()), + Err(e) => Err(Box::new(e) as Box), + } + } +} + +#[derive(Debug)] +pub(crate) struct TestUserContextFunction { + call_def: CallDef, +} + +impl TestUserContextFunction { + pub fn new() -> Self { + TestUserContextFunction { + call_def: CallDef { + names: vec!["test_user_context"], + overloads: vec![CallSpec { + input: vec![CallSpecArg::Positional], + output: Box::new(|args| { + logical::ValueExpr::Call(logical::CallExpr { + name: logical::CallName::ByName("test_user_context".to_string()), + arguments: args, + }) + }), + }], + }, + } + } +} + +impl BaseTableFunctionInfo for TestUserContextFunction { + fn call_def(&self) -> &CallDef { + &self.call_def + } + + fn plan_eval(&self) -> Box { + Box::new(EvalTestCtxTable {}) + } +} + +#[derive(Error, Debug)] +#[non_exhaustive] +pub enum UserCtxError { + #[error("bad arguments")] + BadArgs, + #[error("runtime error")] + Runtime, +} + +#[derive(Debug)] +pub(crate) struct EvalTestCtxTable {} + +impl BaseTableExpr for EvalTestCtxTable { + fn evaluate<'c>( + &self, + args: &[Cow], + _ctx: &'c dyn SessionContext<'c>, + ) -> BaseTableExprResult<'c> { + if let Some(arg1) = args.first() { + match arg1.as_ref() { + Value::String(_name) => Ok(Box::new(TestDataGen {})), + _ => { + let error = UserCtxError::BadArgs; + Err(Box::new(error) as BaseTableExprResultError) + } + } + } else { + let error = UserCtxError::BadArgs; + Err(Box::new(error) as BaseTableExprResultError) + } + } +} + +struct TestDataGen {} + +impl Iterator for TestDataGen { + type Item = Result; + + fn next(&mut self) -> Option { + Some(Err(Box::new(UserCtxError::Runtime))) + } +} +#[track_caller] +#[inline] +pub(crate) fn parse(statement: &str) -> ParserResult { + partiql_parser::Parser::default().parse(statement) +} + +#[track_caller] +#[inline] +pub(crate) fn lower( + catalog: &dyn Catalog, + parsed: &Parsed, +) -> partiql_logical::LogicalPlan { + let planner = partiql_logical_planner::LogicalPlanner::new(catalog); + planner.lower(parsed).expect("lower") +} + +#[track_caller] +#[inline] +pub(crate) fn evaluate( + mode: EvaluationMode, + catalog: &dyn Catalog, + logical: partiql_logical::LogicalPlan, + bindings: MapBindings, + ctx_vals: &[(String, &(dyn Any))], +) -> Result { + let mut planner = partiql_eval::plan::EvaluatorPlanner::new(mode, catalog); + + let mut plan = planner.compile(&logical).expect("Expect no plan error"); + + let sys = SystemContext { + now: DateTime::from_system_now_utc(), + }; + let mut ctx = BasicContext::new(bindings, sys); + for (k, v) in ctx_vals { + ctx.user.insert(k.as_str().into(), *v); + } + + plan.execute_mut(&ctx) +} + +#[test] +fn test_context_bad_args_permissive() { + let query = "SELECT foo, bar from test_user_context(9) as data"; + + let mut catalog = PartiqlCatalog::default(); + let ext = UserCtxTestExtension {}; + ext.load(&mut catalog).expect("extension load to succeed"); + + let parsed = parse(query); + let lowered = lower(&catalog, &parsed.expect("parse")); + let bindings = Default::default(); + + let ctx: [(String, &dyn Any); 0] = []; + let out = evaluate( + EvaluationMode::Permissive, + &catalog, + lowered, + bindings, + &ctx, + ); + + assert!(out.is_ok()); + assert_eq!(out.unwrap().result, bag!(tuple!()).into()); +} +#[test] +fn test_context_bad_args_strict() { + use assert_matches::assert_matches; + let query = "SELECT foo, bar from test_user_context(9) as data"; + + let mut catalog = PartiqlCatalog::default(); + let ext = UserCtxTestExtension {}; + ext.load(&mut catalog).expect("extension load to succeed"); + + let parsed = parse(query); + let lowered = lower(&catalog, &parsed.expect("parse")); + let bindings = Default::default(); + + let ctx: [(String, &dyn Any); 0] = []; + let out = evaluate(EvaluationMode::Strict, &catalog, lowered, bindings, &ctx); + + assert!(out.is_err()); + let err = out.unwrap_err(); + assert_eq!(err.errors.len(), 1); + let err = &err.errors[0]; + assert_matches!(err, EvaluationError::ExtensionResultError(err) => { + assert_eq!(err.to_string(), "bad arguments") + }); +} + +#[test] +fn test_context_runtime_permissive() { + let query = "SELECT foo, bar from test_user_context('counter') as data"; + + let mut catalog = PartiqlCatalog::default(); + let ext = UserCtxTestExtension {}; + ext.load(&mut catalog).expect("extension load to succeed"); + + let parsed = parse(query); + let lowered = lower(&catalog, &parsed.expect("parse")); + let bindings = Default::default(); + + let ctx: [(String, &dyn Any); 0] = []; + let out = evaluate( + EvaluationMode::Permissive, + &catalog, + lowered, + bindings, + &ctx, + ); + + assert!(out.is_ok()); + assert_eq!(out.unwrap().result, bag!(tuple!()).into()); +} + +#[test] +fn test_context_runtime_strict() { + use assert_matches::assert_matches; + let query = "SELECT foo, bar from test_user_context('counter') as data"; + + let mut catalog = PartiqlCatalog::default(); + let ext = UserCtxTestExtension {}; + ext.load(&mut catalog).expect("extension load to succeed"); + + let parsed = parse(query); + let lowered = lower(&catalog, &parsed.expect("parse")); + let bindings = Default::default(); + + let ctx: [(String, &dyn Any); 0] = []; + let out = evaluate(EvaluationMode::Strict, &catalog, lowered, bindings, &ctx); + + assert!(out.is_err()); + let err = out.unwrap_err(); + assert_eq!(err.errors.len(), 1); + let err = &err.errors[0]; + assert_matches!(err, EvaluationError::ExtensionResultError(err) => { + assert_eq!(err.to_string(), "runtime error") + }); +} diff --git a/partiql/tests/user_context.rs b/partiql/tests/user_context.rs index bd35c776..59564c67 100644 --- a/partiql/tests/user_context.rs +++ b/partiql/tests/user_context.rs @@ -25,7 +25,7 @@ pub struct UserCtxTestExtension {} impl partiql_catalog::Extension for UserCtxTestExtension { fn name(&self) -> String { - "ion".into() + "test_extension".into() } fn load(&self, catalog: &mut dyn Catalog) -> Result<(), Box> {