Merge 2761709 into 59a5fb1

.github/workflows/ci_build_test.yml

@@ -6,8 +6,6 @@ env:
   CARGO_TEST_RESULT_NAME: cargo_test_results.json
   CONFORMANCE_REPORT_NAME: cts_report.json
   COMPARISON_REPORT_NAME: cts-comparison-report.md
-  RUST_TEST_TIME_UNIT: 150,5000
-  RUST_TEST_TIME_INTEGRATION: 150,5000
 
 jobs:
   build:
@@ -105,7 +103,7 @@ jobs:
       # to format and use the output.
       - name: Cargo Test of the conformance tests (can fail) and save to json file
         continue-on-error: true
-        run: cargo test --verbose --package partiql-conformance-tests --features "conformance_test" --release -- -Z unstable-options --ensure-time --format json > ${{ env.CARGO_TEST_RESULT_NAME }}
+        run: cargo test --verbose --package partiql-conformance-tests --features "conformance_test" --release -- -Z unstable-options --format json > ${{ env.CARGO_TEST_RESULT_NAME }}
       # Create a conformance report from the `cargo test` json file
       - run: cargo run --features report_tool --bin generate_cts_report ${{ env.CARGO_TEST_RESULT_NAME }} ${GITHUB_SHA} ${{ env.CONFORMANCE_REPORT_NAME }}
       # Upload conformance report for comparison with future runs
@@ -162,7 +160,7 @@ jobs:
         continue-on-error: true
         run: |
           cd ${{ github.event.pull_request.base.sha }}
-          cargo test --verbose --package partiql-conformance-tests --features "conformance_test" --release -- -Z unstable-options --ensure-time --format json > ${{ env.CARGO_TEST_RESULT_NAME }}
+          cargo test --verbose --package partiql-conformance-tests --features "conformance_test" --release -- -Z unstable-options --format json > ${{ env.CARGO_TEST_RESULT_NAME }}
       - name: (If download of target branch conformance report fails) Generate conformance test report for target branch
         if: ${{ steps.download-report.outcome == 'failure' }}
         continue-on-error: true

CHANGELOG.md

@@ -21,6 +21,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 - *BREAKING:* partiql-parser: `Parsed` struct's `ast` field is now an `ast::AstNode<ast::TopLevelQuery>`
 - *BREAKING:* partiql-eval: `Evaluable` trait's `update_input` fn now also takes in an `EvalContext`
 - *BREAKING:* partiql-logical: changed modeling of `Project` `exprs` to be a `Vec<(String, ValueExpr)>` rather than a `HashMap<String, ValueExpr>` to support multiple project items with the same alias
+- *BREAKING:* partiql-logical: changed modeling of `VarRef` to include a `VarRefType` to indicate whether to do a local vs global binding lookup
 
 ### Added
 - Strict mode evaluation partial support added.
@@ -37,6 +38,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 - partiql-parser set quantifier for bag operators fixed to `DISTINCT`
 - partiql-parser set quantifier for bag operators fixed to be `DISTINCT` when unspecified
 - partiql-logical-planner add error for when a `HAVING` is included without `GROUP BY`
+- Fixes variable resolution lookup order and excessive lookups
 
 ## [0.5.0] - 2023-06-06
 ### Changed

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

@@ -207,10 +207,7 @@ mod tests {
 
         let parsed = parse(statement);
         let lowered = lower(&catalog, &parsed.expect("parse"));
-        let bindings = env
-            .as_ref()
-            .map(|e| (e).into())
-            .unwrap_or_else(MapBindings::default);
+        let bindings = env.as_ref().map(|e| (e).into()).unwrap_or_default();
         let out = evaluate(&catalog, lowered, bindings);
 
         assert!(out.is_bag());

partiql-ast-passes/src/name_resolver.rs

@@ -205,7 +205,7 @@ impl<'ast> Visitor<'ast> for NameResolver {
         let id = *self.current_node();
         self.enclosing_clause
             .entry(EnclosingClause::Query)
-            .or_insert_with(Vec::new)
+            .or_default()
             .push(id);
         self.enter_keyref();
         Traverse::Continue
@@ -273,24 +273,18 @@ impl<'ast> Visitor<'ast> for NameResolver {
         let id = *self.current_node();
         self.enclosing_clause
             .entry(EnclosingClause::FromLet)
-            .or_insert_with(Vec::new)
+            .or_default()
             .push(id);
         self.enter_keyref();
 
         // Scopes above this `FROM` in the AST are in-scope to use variables defined by this from
         for in_scope in self.id_path_to_root.iter().rev().skip(1) {
-            self.in_scope
-                .entry(*in_scope)
-                .or_insert_with(Vec::new)
-                .push(id);
+            self.in_scope.entry(*in_scope).or_default().push(id);
         }
 
         // This `FROM` item is in-scope of variables defined by any preceding items in this `FROM` (e.g., lateral joins)
         for in_scope in self.lateral_stack.last().unwrap() {
-            self.in_scope
-                .entry(id)
-                .or_insert_with(Vec::new)
-                .push(*in_scope);
+            self.in_scope.entry(id).or_default().push(*in_scope);
         }
 
         self.lateral_stack.last_mut().unwrap().push(id);
@@ -383,7 +377,49 @@ impl<'ast> Visitor<'ast> for NameResolver {
         Traverse::Continue
     }
 
+    fn enter_group_key(&mut self, _group_key: &'ast GroupKey) -> Traverse {
+        self.enter_keyref();
+        let id = *self.current_node();
+
+        if self
+            .enclosing_clause
+            .get(&EnclosingClause::FromLet)
+            .is_none()
+        {
+            self.errors.push(AstTransformError::IllegalState(
+                "group_key expects a FromLet enclosing clause".to_string(),
+            ))
+        }
+
+        self.enclosing_clause
+            .get(&EnclosingClause::FromLet)
+            .expect("EnclosingClause::FromLet")
+            .iter()
+            .for_each(|enclosing_clause| {
+                self.in_scope.entry(id).or_default().push(*enclosing_clause);
+            });
+
+        self.enclosing_clause
+            .entry(EnclosingClause::Query)
+            .or_default()
+            .push(id);
+        Traverse::Continue
+    }
+
     fn exit_group_key(&mut self, group_key: &'ast GroupKey) -> Traverse {
+        let KeyRefs {
+            consume,
+            produce_required,
+            ..
+        } = match self.exit_keyref() {
+            Ok(kr) => kr,
+            Err(e) => {
+                self.errors.push(e);
+                return Traverse::Stop;
+            }
+        };
+        let mut produce = produce_required;
+
         let id = *self.current_node();
         // get the "as" alias for each `GROUP BY` expr
         // 1. if explicitly given
@@ -397,21 +433,50 @@ impl<'ast> Visitor<'ast> for NameResolver {
             Symbol::Unknown(self.id_gen.next_id())
         };
         self.aliases.insert(id, as_alias.clone());
+        produce.insert(as_alias.clone());
         self.keyref_stack
             .last_mut()
             .unwrap()
             .produce_required
             .insert(as_alias);
+        self.schema.insert(id, KeySchema { consume, produce });
+        Traverse::Continue
+    }
+
+    fn enter_group_by_expr(&mut self, _group_by_expr: &'ast GroupByExpr) -> Traverse {
+        self.enter_keyref();
+        let id = *self.current_node();
+        // Scopes above this `GROUP BY` in the AST are in-scope to use variables defined by this GROUP BY
+        for in_scope in self.id_path_to_root.iter().rev().skip(1) {
+            self.in_scope.entry(*in_scope).or_default().push(id);
+        }
         Traverse::Continue
     }
 
     fn exit_group_by_expr(&mut self, group_by_expr: &'ast GroupByExpr) -> Traverse {
+        let id = *self.current_node();
+        let KeyRefs {
+            consume,
+            produce_required,
+            ..
+        } = match self.exit_keyref() {
+            Ok(kr) => kr,
+            Err(e) => {
+                self.errors.push(e);
+                return Traverse::Stop;
+            }
+        };
+
+        // TODO: delete in_scope for FROM sources in subsequent clauses
+
+        let mut produce: Names = produce_required;
         // add the `GROUP AS` alias
         if let Some(sym) = &group_by_expr.group_as_alias {
-            let id = *self.current_node();
             let as_alias = Symbol::Known(sym.clone());
-            self.aliases.insert(id, as_alias);
+            self.aliases.insert(id, as_alias.clone());
+            produce.insert(as_alias);
         }
+        self.schema.insert(id, KeySchema { consume, produce });
         Traverse::Continue
     }
 }

partiql-conformance-test-generator/src/reader.rs

@@ -291,7 +291,7 @@ impl TryFrom<&Element> for EvaluationModeList {
             IonType::Symbol | IonType::String => Ok(eval_mode(value)?.into()),
             IonType::List => {
                 let list = value.as_sequence().unwrap();
-                let eval_modes: Result<Vec<_>, _> = list.elements().map(|e| eval_mode(e)).collect();
+                let eval_modes: Result<Vec<_>, _> = list.elements().map(eval_mode).collect();
                 Ok(eval_modes?.into())
             }
             _ => Err(miette!(

partiql-conformance-tests/tests/mod.rs

@@ -161,10 +161,7 @@ pub(crate) fn eval<'a>(
 
     let parsed = parse(statement)?;
     let lowered = lower(&catalog, &parsed)?;
-    let bindings = env
-        .as_ref()
-        .map(|e| (&e.value).into())
-        .unwrap_or_else(MapBindings::default);
+    let bindings = env.as_ref().map(|e| (&e.value).into()).unwrap_or_default();
     let plan = compile(mode, &catalog, lowered)?;
     Ok(evaluate(plan, bindings)?)
 }

partiql-eval/benches/bench_eval.rs

@@ -9,7 +9,9 @@ use partiql_eval::plan;
 use partiql_eval::plan::EvaluationMode;
 use partiql_logical as logical;
 use partiql_logical::BindingsOp::{Project, ProjectAll};
-use partiql_logical::{BinaryOp, BindingsOp, JoinKind, LogicalPlan, PathComponent, ValueExpr};
+use partiql_logical::{
+    BinaryOp, BindingsOp, JoinKind, LogicalPlan, PathComponent, ValueExpr, VarRefType,
+};
 use partiql_value::{bag, list, tuple, BindingsName, Value};
 
 fn data() -> MapBindings<Value> {
@@ -68,17 +70,21 @@ fn join_data() -> MapBindings<Value> {
 
 fn scan(name: &str, as_key: &str) -> BindingsOp {
     BindingsOp::Scan(logical::Scan {
-        expr: ValueExpr::VarRef(BindingsName::CaseInsensitive(name.into())),
+        expr: ValueExpr::VarRef(
+            BindingsName::CaseInsensitive(name.into()),
+            VarRefType::Global,
+        ),
         as_key: as_key.to_string(),
         at_key: None,
     })
 }
 
 fn path_var(name: &str, component: &str) -> ValueExpr {
     ValueExpr::Path(
-        Box::new(ValueExpr::VarRef(BindingsName::CaseInsensitive(
-            name.into(),
-        ))),
+        Box::new(ValueExpr::VarRef(
+            BindingsName::CaseInsensitive(name.into()),
+            VarRefType::Local,
+        )),
         vec![PathComponent::Key(BindingsName::CaseInsensitive(
             component.to_string(),
         ))],
@@ -151,9 +157,10 @@ fn eval_bench(c: &mut Criterion) {
 
         let from = logical_plan.add_operator(BindingsOp::Scan(logical::Scan {
             expr: ValueExpr::Path(
-                Box::new(ValueExpr::VarRef(BindingsName::CaseInsensitive(
-                    "hr".to_string(),
-                ))),
+                Box::new(ValueExpr::VarRef(
+                    BindingsName::CaseInsensitive("hr".to_string()),
+                    VarRefType::Local,
+                )),
                 vec![PathComponent::Key(BindingsName::CaseInsensitive(
                     "employeesNestScalars".to_string(),
                 ))],

partiql-eval/src/eval/evaluable.rs

@@ -805,7 +805,7 @@ impl Evaluable for EvalGroupBy {
                     }
                     groups
                         .entry(group)
-                        .or_insert(vec![])
+                        .or_default()
                         .push(Value::Tuple(Box::new(v_as_tuple.clone())));
                 }
 
@@ -1503,8 +1503,7 @@ impl Evaluable for EvalOuterIntersect {
                 let lhs: HashSet<Value> = lhs.collect();
                 Bag::from_iter(
                     rhs.filter(|elem| lhs.contains(elem))
-                        .collect::<HashSet<_>>()
-                        .into_iter(),
+                        .collect::<HashSet<_>>(),
                 )
             }
         };

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

@@ -170,12 +170,12 @@ impl BindEvalExpr for EvalOpBinary {
         match self {
             EvalOpBinary::And => logical!(AndCheck, |lhs, rhs| lhs.and(rhs)),
             EvalOpBinary::Or => logical!(OrCheck, |lhs, rhs| lhs.or(rhs)),
-            EvalOpBinary::Eq => equality!(|lhs, rhs| NullableEq::eq(lhs, rhs)),
-            EvalOpBinary::Neq => equality!(|lhs, rhs| NullableEq::neq(lhs, rhs)),
-            EvalOpBinary::Gt => equality!(|lhs, rhs| NullableOrd::gt(lhs, rhs)),
-            EvalOpBinary::Gteq => equality!(|lhs, rhs| NullableOrd::gteq(lhs, rhs)),
-            EvalOpBinary::Lt => equality!(|lhs, rhs| NullableOrd::lt(lhs, rhs)),
-            EvalOpBinary::Lteq => equality!(|lhs, rhs| NullableOrd::lteq(lhs, rhs)),
+            EvalOpBinary::Eq => equality!(NullableEq::eq),
+            EvalOpBinary::Neq => equality!(NullableEq::neq),
+            EvalOpBinary::Gt => equality!(NullableOrd::gt),
+            EvalOpBinary::Gteq => equality!(NullableOrd::gteq),
+            EvalOpBinary::Lt => equality!(NullableOrd::lt),
+            EvalOpBinary::Lteq => equality!(NullableOrd::lteq),
             EvalOpBinary::Add => math!(|lhs, rhs| lhs + rhs),
             EvalOpBinary::Sub => math!(|lhs, rhs| lhs - rhs),
             EvalOpBinary::Mul => math!(|lhs, rhs| lhs * rhs),

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

@@ -1,6 +1,6 @@
 use crate::env::Bindings;
 
-use crate::eval::expr::EvalExpr;
+use crate::eval::expr::{BindError, BindEvalExpr, EvalExpr};
 use crate::eval::EvalContext;
 
 use partiql_value::Value::Missing;
@@ -94,19 +94,50 @@ impl EvalExpr for EvalDynamicLookup {
     }
 }
 
-/// Represents a variable reference in a (sub)query, e.g. `a` in `SELECT b as a FROM`.
-#[derive(Debug)]
-pub(crate) struct EvalVarRef {
+/// Represents a local variable reference in a (sub)query, e.g. `b` in `SELECT t.b as a FROM T as t`.
+#[derive(Debug, Clone)]
+pub(crate) enum EvalVarRef {
+    Local(BindingsName),
+    Global(BindingsName),
+}
+
+impl BindEvalExpr for EvalVarRef {
+    fn bind<const STRICT: bool>(
+        &self,
+        _: Vec<Box<dyn EvalExpr>>,
+    ) -> Result<Box<dyn EvalExpr>, BindError> {
+        Ok(match self {
+            EvalVarRef::Global(name) => Box::new(EvalGlobalVarRef { name: name.clone() }),
+            EvalVarRef::Local(name) => Box::new(EvalLocalVarRef { name: name.clone() }),
+        })
+    }
+}
+
+#[inline]
+fn borrow_or_missing(value: Option<&Value>) -> Cow<Value> {
+    value.map_or_else(|| Cow::Owned(Missing), Cow::Borrowed)
+}
+
+/// Represents a local variable reference in a (sub)query, e.g. `b` in `SELECT t.b as a FROM T as t`.
+#[derive(Debug, Clone)]
+pub(crate) struct EvalLocalVarRef {
     pub(crate) name: BindingsName,
 }
 
-impl EvalExpr for EvalVarRef {
-    fn evaluate<'a>(&'a self, bindings: &'a Tuple, ctx: &'a dyn EvalContext) -> Cow<'a, Value> {
-        let value = Bindings::get(bindings, &self.name).or_else(|| ctx.bindings().get(&self.name));
+impl EvalExpr for EvalLocalVarRef {
+    fn evaluate<'a>(&'a self, bindings: &'a Tuple, _: &'a dyn EvalContext) -> Cow<'a, Value> {
+        borrow_or_missing(Bindings::get(bindings, &self.name))
+    }
+}
 
-        match value {
-            None => Cow::Owned(Missing),
-            Some(v) => Cow::Borrowed(v),
-        }
+/// Represents a global variable reference in a (sub)query, e.g. `T` in `SELECT t.b as a FROM T as t`.
+#[derive(Debug, Clone)]
+pub(crate) struct EvalGlobalVarRef {
+    pub(crate) name: BindingsName,
+}
+
+impl EvalExpr for EvalGlobalVarRef {
+    fn evaluate<'a>(&'a self, _: &'a Tuple, ctx: &'a dyn EvalContext) -> Cow<'a, Value> {
+        borrow_or_missing(ctx.bindings().get(&self.name))
     }
 }

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

@@ -215,7 +215,7 @@ mod tests {
         assert_eq!(like_to_re_pattern("foo__bar", Some('\\')), r#"^foo..bar$"#);
         assert_eq!(
             like_to_re_pattern("foo_.*?_bar", Some('\\')),
-            r#"^foo.\.\*\?.bar$"#
+            r"^foo.\.\*\?.bar$"
         );
     }