Fix subquery global binding lookup and error propagation (#454)

CHANGELOG.md

@@ -11,6 +11,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 ### Added
 
 ### Fixed
+- partiql-eval: Fixed propagation of errors in subqueries to outer query
+- partiql-eval: Fixed handling of nested binding environments in subqueries
 
 ## [0.7.0] - 2024-03-12
 ### Changed

partiql-catalog/src/context.rs

@@ -2,7 +2,10 @@ use partiql_value::{BindingsName, DateTime, Tuple, Value};
 use std::any::Any;
 use std::fmt::Debug;
 
-pub trait Bindings<T>: Debug {
+pub trait Bindings<T>: Debug
+where
+    T: Debug,
+{
     fn get(&self, name: &BindingsName<'_>) -> Option<&T>;
 }
 

partiql-eval/src/env.rs

@@ -96,6 +96,33 @@ pub mod basic {
             }
         }
     }
+
+    #[derive(Debug)]
+    pub struct NestedBindings<'a, T>
+    where
+        T: Debug,
+    {
+        bindings: MapBindings<T>,
+        parent: &'a dyn Bindings<T>,
+    }
+
+    impl<'a, T> NestedBindings<'a, T>
+    where
+        T: Debug,
+    {
+        pub fn new(bindings: MapBindings<T>, parent: &'a dyn Bindings<T>) -> Self {
+            Self { bindings, parent }
+        }
+    }
+
+    impl<'a, T> Bindings<T> for NestedBindings<'a, T>
+    where
+        T: Debug,
+    {
+        fn get(&self, name: &BindingsName<'_>) -> Option<&T> {
+            self.bindings.get(name).or_else(|| self.parent.get(name))
+        }
+    }
 }
 
 #[cfg(test)]

partiql-eval/src/eval/evaluable.rs

@@ -1321,10 +1321,15 @@ impl EvalExpr for EvalSubQueryExpr {
             let nested_ctx: NestedContext<'_, '_> = NestedContext::new(bindings, ctx);
 
             let mut plan = self.plan.borrow_mut();
-            if let Ok(evaluated) = plan.execute_mut(&nested_ctx) {
-                evaluated.result
-            } else {
-                Missing
+
+            match plan.execute_mut(&nested_ctx) {
+                Ok(evaluated) => evaluated.result,
+                Err(err) => {
+                    for e in err.errors {
+                        ctx.add_error(e);
+                    }
+                    Missing
+                }
             }
         };
         Cow::Owned(value)

partiql-eval/src/eval/mod.rs

@@ -13,7 +13,7 @@ use petgraph::{Directed, Outgoing};
 
 use partiql_value::Value;
 
-use crate::env::basic::MapBindings;
+use crate::env::basic::{MapBindings, NestedBindings};
 
 use petgraph::graph::NodeIndex;
 
@@ -181,6 +181,7 @@ impl<'a> BasicContext<'a> {
         }
     }
 }
+
 impl<'a> SessionContext<'a> for BasicContext<'a> {
     fn bindings(&self) -> &dyn Bindings<Value> {
         &self.bindings
@@ -195,6 +196,7 @@ impl<'a> SessionContext<'a> for BasicContext<'a> {
         self.user.get(&key).copied()
     }
 }
+
 impl<'a> EvalContext<'a> for BasicContext<'a> {
     fn as_session(&'a self) -> &'a dyn SessionContext<'a> {
         self
@@ -215,12 +217,13 @@ impl<'a> EvalContext<'a> for BasicContext<'a> {
 
 #[derive(Debug)]
 pub struct NestedContext<'a, 'c> {
-    pub bindings: MapBindings<Value>,
+    pub bindings: NestedBindings<'a, Value>,
     pub parent: &'a dyn EvalContext<'c>,
 }
 
 impl<'a, 'c> NestedContext<'a, 'c> {
     pub fn new(bindings: MapBindings<Value>, parent: &'a dyn EvalContext<'c>) -> Self {
+        let bindings = NestedBindings::new(bindings, parent.bindings());
         NestedContext { bindings, parent }
     }
 }

partiql/src/lib.rs

@@ -1,6 +1,8 @@
 #![deny(rust_2018_idioms)]
 #![deny(clippy::all)]
 
+mod subquery_tests;
+
 #[cfg(test)]
 mod tests {
     use partiql_ast_passes::error::AstTransformationError;

partiql/src/subquery_tests.rs

@@ -0,0 +1,155 @@
+#![deny(rust_2018_idioms)]
+#![deny(clippy::all)]
+#![warn(clippy::pedantic)]
+
+#[cfg(test)]
+mod tests {
+    use partiql_catalog::context::SystemContext;
+    use partiql_catalog::{Catalog, PartiqlCatalog};
+    use partiql_eval::env::basic::MapBindings;
+    use partiql_eval::error::EvalErr;
+    use partiql_eval::eval::BasicContext;
+    use partiql_eval::plan::EvaluationMode;
+    use partiql_logical::{LogicalPlan, ProjectValue, VarRefType};
+    use partiql_value::{bag, tuple, Bag, BindingsName, DateTime, Value};
+    use std::any::Any;
+
+    #[track_caller]
+    #[inline]
+    pub(crate) fn evaluate(
+        catalog: &dyn Catalog,
+        logical: partiql_logical::LogicalPlan<partiql_logical::BindingsOp>,
+        bindings: MapBindings<Value>,
+        ctx_vals: &[(String, &(dyn Any))],
+    ) -> Result<Value, EvalErr> {
+        let mut planner =
+            partiql_eval::plan::EvaluatorPlanner::new(EvaluationMode::Strict, 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).map(|out| out.result)
+    }
+
+    #[test]
+    fn locals_in_subqueries() {
+        //  `SELECT VALUE _1 from (SELECT VALUE foo from <<{'a': 'b'}>> AS foo) AS _1;`
+        let mut sub_query = partiql_logical::LogicalPlan::new();
+        let scan_op_id =
+            sub_query.add_operator(partiql_logical::BindingsOp::Scan(partiql_logical::Scan {
+                expr: partiql_logical::ValueExpr::Lit(Box::new(Value::Bag(Box::new(Bag::from(
+                    vec![tuple![("a", "b")].into()],
+                ))))),
+                as_key: "foo".into(),
+                at_key: None,
+            }));
+        let project_value_op_id = sub_query.add_operator(
+            partiql_logical::BindingsOp::ProjectValue(partiql_logical::ProjectValue {
+                expr: partiql_logical::ValueExpr::VarRef(
+                    BindingsName::CaseSensitive("foo".into()),
+                    VarRefType::Local,
+                ),
+            }),
+        );
+        sub_query.add_flow(scan_op_id, project_value_op_id);
+
+        let sink_op_id = sub_query.add_operator(partiql_logical::BindingsOp::Sink);
+        sub_query.add_flow(project_value_op_id, sink_op_id);
+
+        let mut plan = LogicalPlan::new();
+        let scan_op_id =
+            plan.add_operator(partiql_logical::BindingsOp::Scan(partiql_logical::Scan {
+                expr: partiql_logical::ValueExpr::SubQueryExpr(partiql_logical::SubQueryExpr {
+                    plan: sub_query,
+                }),
+                as_key: "_1".into(),
+                at_key: None,
+            }));
+
+        let project_value_op_id =
+            plan.add_operator(partiql_logical::BindingsOp::ProjectValue(ProjectValue {
+                expr: partiql_logical::ValueExpr::VarRef(
+                    BindingsName::CaseSensitive("_1".into()),
+                    VarRefType::Local,
+                ),
+            }));
+        plan.add_flow(scan_op_id, project_value_op_id);
+
+        let sink_op_id = plan.add_operator(partiql_logical::BindingsOp::Sink);
+        plan.add_flow(project_value_op_id, sink_op_id);
+
+        let catalog = PartiqlCatalog::default();
+        let bindings = MapBindings::default();
+        let res = evaluate(&catalog, plan, bindings, &[]).expect("should eval correctly");
+        dbg!(&res);
+        assert!(res != Value::Missing);
+        assert_eq!(res, Value::from(bag![tuple![("a", "b")]]))
+    }
+
+    #[test]
+    fn globals_in_subqueries() {
+        //  `foo` is defined in global environment as `<<{'a': 'b'}>>`
+        //  `SELECT VALUE _1 FROM (SELECT VALUE foo FROM foo AS foo) AS _1;`
+        let mut sub_query = partiql_logical::LogicalPlan::new();
+        let scan_op_id =
+            sub_query.add_operator(partiql_logical::BindingsOp::Scan(partiql_logical::Scan {
+                expr: partiql_logical::ValueExpr::VarRef(
+                    BindingsName::CaseSensitive("foo".into()),
+                    VarRefType::Global,
+                ),
+                as_key: "foo".into(),
+                at_key: None,
+            }));
+        let project_value_op_id = sub_query.add_operator(
+            partiql_logical::BindingsOp::ProjectValue(partiql_logical::ProjectValue {
+                expr: partiql_logical::ValueExpr::VarRef(
+                    BindingsName::CaseSensitive("foo".into()),
+                    VarRefType::Local,
+                ),
+            }),
+        );
+        sub_query.add_flow(scan_op_id, project_value_op_id);
+
+        let sink_op_id = sub_query.add_operator(partiql_logical::BindingsOp::Sink);
+        sub_query.add_flow(project_value_op_id, sink_op_id);
+
+        let mut plan = LogicalPlan::new();
+        let scan_op_id =
+            plan.add_operator(partiql_logical::BindingsOp::Scan(partiql_logical::Scan {
+                expr: partiql_logical::ValueExpr::SubQueryExpr(partiql_logical::SubQueryExpr {
+                    plan: sub_query,
+                }),
+                as_key: "_1".into(),
+                at_key: None,
+            }));
+
+        let project_value_op_id =
+            plan.add_operator(partiql_logical::BindingsOp::ProjectValue(ProjectValue {
+                expr: partiql_logical::ValueExpr::VarRef(
+                    BindingsName::CaseSensitive("_1".into()),
+                    VarRefType::Local,
+                ),
+            }));
+        plan.add_flow(scan_op_id, project_value_op_id);
+
+        let sink_op_id = plan.add_operator(partiql_logical::BindingsOp::Sink);
+        plan.add_flow(project_value_op_id, sink_op_id);
+
+        let catalog = PartiqlCatalog::default();
+        let mut bindings = MapBindings::default();
+        bindings.insert(
+            "foo",
+            Value::Bag(Box::new(Bag::from(vec![tuple![("a", "b")].into()]))),
+        );
+        let res = evaluate(&catalog, plan, bindings, &[]).expect("should eval correctly");
+        dbg!(&res);
+        assert!(res != Value::Missing);
+        assert_eq!(res, Value::from(bag![tuple![("a", "b")]]))
+    }
+}