From 720342948865d56a687ae2d5de94d26093c8aa05 Mon Sep 17 00:00:00 2001 From: Arash M <27716912+am357@users.noreply.github.com> Date: Wed, 16 Aug 2023 12:10:09 -0700 Subject: [PATCH 1/5] Add Catalog to NameResolver During the work for adding a steel thread for PartiQL Typing (Recent PR #389) and after merging #410 to `feat-type-plan-poc` it is realized that we need to refactor the code to remove `DynamicLocalup` `VarExpr` with the assumption that we work based off of Typing and Value Environment from the Catalog. We have a Typing Environment in the Catalog at the moment and we are going to add the Variable Environment as well. In preparation for such task, we need to make the `NameResolver` Catalog aware. In that regard this commit adds the `Catalog` to `NameResolver` Expecting subsequent PR(s) for the name resolving using the Catalog. --- partiql-ast-passes/src/name_resolver.rs | 31 +++++++++++++++++++++---- partiql-logical-planner/src/lib.rs | 5 ++-- 2 files changed, 30 insertions(+), 6 deletions(-) diff --git a/partiql-ast-passes/src/name_resolver.rs b/partiql-ast-passes/src/name_resolver.rs index 993c11c1..3cacbf4c 100644 --- a/partiql-ast-passes/src/name_resolver.rs +++ b/partiql-ast-passes/src/name_resolver.rs @@ -4,6 +4,7 @@ use indexmap::{IndexMap, IndexSet}; use partiql_ast::ast; use partiql_ast::ast::{GroupByExpr, GroupKey}; use partiql_ast::visit::{Traverse, Visit, Visitor}; +use partiql_catalog::Catalog; use std::sync::atomic::{AtomicU32, Ordering}; type FnvIndexSet = IndexSet; @@ -82,8 +83,8 @@ enum EnclosingClause { /// Resolves which clauses in a query produce & consume variable references by walking the /// AST and collecting variable references. Also partially infers alias if no `AS` alias /// was provided in the query. -#[derive(Default, Debug)] -pub struct NameResolver { +#[derive(Debug)] +pub struct NameResolver<'c> { // environment stack tracking id_path_to_root: Vec, id_child_stack: Vec>, @@ -99,9 +100,31 @@ pub struct NameResolver { // errors that occur during name resolution errors: Vec, + catalog: &'c dyn Catalog, } -impl NameResolver { +impl<'c> NameResolver<'c> { + pub fn new(catalog: &'c dyn Catalog) -> Self { + NameResolver { + // environment stack tracking + id_path_to_root: Default::default(), + id_child_stack: Default::default(), + keyref_stack: Default::default(), + lateral_stack: Default::default(), + id_gen: Default::default(), + + // data flow tracking + enclosing_clause: Default::default(), + in_scope: Default::default(), + schema: Default::default(), + aliases: Default::default(), + + // errors that occur during name resolution + errors: Default::default(), + catalog, + } + } + pub fn resolve( &mut self, query: &ast::AstNode, @@ -188,7 +211,7 @@ impl NameResolver { } } -impl<'ast> Visitor<'ast> for NameResolver { +impl<'ast, 'c> Visitor<'ast> for NameResolver<'c> { fn enter_ast_node(&mut self, id: ast::NodeId) -> Traverse { self.id_path_to_root.push(id); if let Some(children) = self.id_child_stack.last_mut() { diff --git a/partiql-logical-planner/src/lib.rs b/partiql-logical-planner/src/lib.rs index 73ce018c..e1f58630 100644 --- a/partiql-logical-planner/src/lib.rs +++ b/partiql-logical-planner/src/lib.rs @@ -5,7 +5,7 @@ use partiql_ast_passes::name_resolver::NameResolver; use partiql_logical as logical; use partiql_parser::Parsed; -use partiql_catalog::Catalog; +use partiql_catalog::{Catalog, PartiqlCatalog}; mod builtins; mod lower; @@ -25,7 +25,8 @@ impl<'c> LogicalPlanner<'c> { parsed: &Parsed, ) -> Result, AstTransformationError> { let q = &parsed.ast; - let mut resolver = NameResolver::default(); + let catalog = PartiqlCatalog::default(); + let mut resolver = NameResolver::new(&catalog); let registry = resolver.resolve(q)?; let planner = AstToLogical::new(self.catalog, registry); planner.lower_query(q) From 2c33fdf4aa58f1267020725dddc04c66f11433c8 Mon Sep 17 00:00:00 2001 From: Arash M <27716912+am357@users.noreply.github.com> Date: Wed, 16 Aug 2023 15:18:47 -0700 Subject: [PATCH 2/5] Read variable name from Catalog in NameResolver --- partiql-ast-passes/src/name_resolver.rs | 40 ++++++++++++++++++++----- partiql-logical-planner/Cargo.toml | 1 + partiql-logical-planner/src/lib.rs | 8 +++-- partiql-logical-planner/src/lower.rs | 2 -- 4 files changed, 40 insertions(+), 11 deletions(-) diff --git a/partiql-ast-passes/src/name_resolver.rs b/partiql-ast-passes/src/name_resolver.rs index 3cacbf4c..cfbd2826 100644 --- a/partiql-ast-passes/src/name_resolver.rs +++ b/partiql-ast-passes/src/name_resolver.rs @@ -2,7 +2,7 @@ use crate::error::{AstTransformError, AstTransformationError}; use fnv::FnvBuildHasher; use indexmap::{IndexMap, IndexSet}; use partiql_ast::ast; -use partiql_ast::ast::{GroupByExpr, GroupKey}; +use partiql_ast::ast::{GroupByExpr, GroupKey, SymbolPrimitive}; use partiql_ast::visit::{Traverse, Visit, Visitor}; use partiql_catalog::Catalog; use std::sync::atomic::{AtomicU32, Ordering}; @@ -209,6 +209,19 @@ impl<'c> NameResolver<'c> { fn push_consume_name(&mut self, name: NameRef) { self.keyref_stack.last_mut().unwrap().consume.insert(name); } + + #[inline] + fn catalog_resolvable(&mut self, name: &SymbolPrimitive) -> bool { + // TODO fix the name look up based on case sensitivity: + // https://github.com/partiql/partiql-lang-rust/issues/426 + dbg!(self.catalog); + + if let Some(_) = self.catalog.resolve_type(name.value.as_str()) { + true + } else { + false + } + } } impl<'ast, 'c> Visitor<'ast> for NameResolver<'c> { @@ -358,19 +371,32 @@ impl<'ast, 'c> Visitor<'ast> for NameResolver<'c> { // in a From path, a prefix `@` means to look locally before globally Cf. specification section 10 let name = if is_from_path { match &var_ref.qualifier { - ast::ScopeQualifier::Unqualified => NameRef { - sym: var_ref.name.clone(), - lookup: vec![NameLookup::Global, NameLookup::Local], - }, + ast::ScopeQualifier::Unqualified => { + if self.catalog_resolvable(&var_ref.name) { + NameRef { + sym: var_ref.name.clone(), + lookup: vec![NameLookup::Global], + } + } else { + self.errors.push(AstTransformError::IllegalState(format!( + "No schema found for [{:?}] in the Catalog", + &var_ref.name + ))); + NameRef { + sym: var_ref.name.clone(), + lookup: vec![NameLookup::Global], + } + } + } ast::ScopeQualifier::Qualified => NameRef { sym: var_ref.name.clone(), - lookup: vec![NameLookup::Local, NameLookup::Global], + lookup: vec![NameLookup::Local], }, } } else { NameRef { sym: var_ref.name.clone(), - lookup: vec![NameLookup::Local, NameLookup::Global], + lookup: vec![NameLookup::Local], } }; diff --git a/partiql-logical-planner/Cargo.toml b/partiql-logical-planner/Cargo.toml index 36724e5b..83c0127b 100644 --- a/partiql-logical-planner/Cargo.toml +++ b/partiql-logical-planner/Cargo.toml @@ -43,3 +43,4 @@ thiserror = "1.0" [dev-dependencies] partiql-eval = { path = "../partiql-eval", version = "0.5.*" } +partiql-types = { path = "../partiql-types", version = "0.5.*"} diff --git a/partiql-logical-planner/src/lib.rs b/partiql-logical-planner/src/lib.rs index e1f58630..bcd7cca8 100644 --- a/partiql-logical-planner/src/lib.rs +++ b/partiql-logical-planner/src/lib.rs @@ -37,7 +37,9 @@ impl<'c> LogicalPlanner<'c> { mod tests { use assert_matches::assert_matches; use partiql_ast_passes::error::AstTransformationError; - use partiql_catalog::PartiqlCatalog; + use partiql_catalog::{PartiqlCatalog, TypeEnvEntry}; + use partiql_types::any; + use partiql_catalog::Catalog; use partiql_eval::env::basic::MapBindings; @@ -59,7 +61,8 @@ mod tests { fn lower( parsed: &Parsed, ) -> Result, AstTransformationError> { - let catalog = PartiqlCatalog::default(); + let mut catalog = PartiqlCatalog::default(); + let _oid = catalog.add_type_entry(TypeEnvEntry::new("customer", &[], any!())); let planner = LogicalPlanner::new(&catalog); planner.lower(parsed) } @@ -67,6 +70,7 @@ mod tests { #[track_caller] fn evaluate(logical: LogicalPlan, bindings: MapBindings) -> Value { let catalog = PartiqlCatalog::default(); + let mut planner = plan::EvaluatorPlanner::new(EvaluationMode::Permissive, &catalog); let mut plan = planner.compile(&logical).expect("Expect no plan error"); println!("{}", plan.to_dot_graph()); diff --git a/partiql-logical-planner/src/lower.rs b/partiql-logical-planner/src/lower.rs index e742465e..25f82890 100644 --- a/partiql-logical-planner/src/lower.rs +++ b/partiql-logical-planner/src/lower.rs @@ -418,8 +418,6 @@ impl<'a> AstToLogical<'a> { } } - // TODO in the presence of schema, error if the variable reference doesn't correspond to a data table - // assume global ValueExpr::VarRef(symprim_to_binding(&varref.name), VarRefType::Global) } From 0d67207fab166d3c388c72bd53f3ab40b7b71d66 Mon Sep 17 00:00:00 2001 From: Arash M <27716912+am357@users.noreply.github.com> Date: Wed, 16 Aug 2023 15:19:27 -0700 Subject: [PATCH 3/5] [BugFix][LogicalPlanner] Read Catalog from self and default --- partiql-logical-planner/src/lib.rs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/partiql-logical-planner/src/lib.rs b/partiql-logical-planner/src/lib.rs index bcd7cca8..32381623 100644 --- a/partiql-logical-planner/src/lib.rs +++ b/partiql-logical-planner/src/lib.rs @@ -25,8 +25,7 @@ impl<'c> LogicalPlanner<'c> { parsed: &Parsed, ) -> Result, AstTransformationError> { let q = &parsed.ast; - let catalog = PartiqlCatalog::default(); - let mut resolver = NameResolver::new(&catalog); + let mut resolver = NameResolver::new(self.catalog); let registry = resolver.resolve(q)?; let planner = AstToLogical::new(self.catalog, registry); planner.lower_query(q) From 9df44312b902c441581d522b51c03c37137c607c Mon Sep 17 00:00:00 2001 From: Arash M <27716912+am357@users.noreply.github.com> Date: Wed, 16 Aug 2023 15:19:46 -0700 Subject: [PATCH 4/5] Fix formatting --- partiql-logical-planner/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/partiql-logical-planner/src/lib.rs b/partiql-logical-planner/src/lib.rs index 32381623..d581d097 100644 --- a/partiql-logical-planner/src/lib.rs +++ b/partiql-logical-planner/src/lib.rs @@ -36,9 +36,9 @@ impl<'c> LogicalPlanner<'c> { mod tests { use assert_matches::assert_matches; use partiql_ast_passes::error::AstTransformationError; + use partiql_catalog::Catalog; use partiql_catalog::{PartiqlCatalog, TypeEnvEntry}; use partiql_types::any; - use partiql_catalog::Catalog; use partiql_eval::env::basic::MapBindings; From cef234361e37815f0d14c7496690564228ff607b Mon Sep 17 00:00:00 2001 From: Arash M <27716912+am357@users.noreply.github.com> Date: Thu, 17 Aug 2023 09:00:44 -0700 Subject: [PATCH 5/5] Experiment with the removal of error in name_resolver --- partiql-ast-passes/src/name_resolver.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/partiql-ast-passes/src/name_resolver.rs b/partiql-ast-passes/src/name_resolver.rs index cfbd2826..57a00de7 100644 --- a/partiql-ast-passes/src/name_resolver.rs +++ b/partiql-ast-passes/src/name_resolver.rs @@ -378,10 +378,10 @@ impl<'ast, 'c> Visitor<'ast> for NameResolver<'c> { lookup: vec![NameLookup::Global], } } else { - self.errors.push(AstTransformError::IllegalState(format!( - "No schema found for [{:?}] in the Catalog", - &var_ref.name - ))); + // self.errors.push(AstTransformError::IllegalState(format!( + // "No schema found for [{:?}] in the Catalog", + // &var_ref.name + // ))); NameRef { sym: var_ref.name.clone(), lookup: vec![NameLookup::Global],