From 60c082b61ef626948e1a3404acc8a011ab8187ad Mon Sep 17 00:00:00 2001 From: Richard Newman Date: Mon, 17 Apr 2017 13:14:30 -0700 Subject: [PATCH] Part 4: pass inputs through algebrizing and execution. (#418) This also adds a test that an `UnboundVariables` error is raised if a variable mentioned in the `:in` clause isn't bound. --- query-algebrizer/src/clauses/mod.rs | 57 +++++++++++++++++-------- query-algebrizer/src/clauses/pattern.rs | 14 ++++-- query-algebrizer/src/errors.rs | 7 +++ query-algebrizer/src/lib.rs | 26 ++++++++--- query/src/lib.rs | 4 +- src/conn.rs | 7 +-- src/errors.rs | 7 +++ src/lib.rs | 2 + src/query.rs | 38 +++++++++++------ tests/query.rs | 45 +++++++++++++++++++ 10 files changed, 159 insertions(+), 48 deletions(-) diff --git a/query-algebrizer/src/clauses/mod.rs b/query-algebrizer/src/clauses/mod.rs index e606a24e..c7d0822f 100644 --- a/query-algebrizer/src/clauses/mod.rs +++ b/query-algebrizer/src/clauses/mod.rs @@ -59,6 +59,7 @@ use types::{ TableAlias, }; +mod inputs; mod or; mod pattern; mod predicate; @@ -66,6 +67,8 @@ mod resolve; use validate::validate_or_join; +pub use self::inputs::QueryInputs; + // We do this a lot for errors. trait RcCloned { fn cloned(&self) -> T; @@ -240,6 +243,38 @@ impl ConjoiningClauses { ..Default::default() } } + + pub fn with_inputs(in_variables: BTreeSet, inputs: T) -> ConjoiningClauses + where T: Into> { + ConjoiningClauses::with_inputs_and_alias_counter(in_variables, inputs, RcCounter::new()) + } + + pub fn with_inputs_and_alias_counter(in_variables: BTreeSet, + inputs: T, + alias_counter: RcCounter) -> ConjoiningClauses + where T: Into> { + match inputs.into() { + None => ConjoiningClauses::with_alias_counter(alias_counter), + Some(QueryInputs { mut types, mut values }) => { + // Discard any bindings not mentioned in our :in clause. + types.keep_intersected_keys(&in_variables); + values.keep_intersected_keys(&in_variables); + + let mut cc = ConjoiningClauses { + alias_counter: alias_counter, + input_variables: in_variables, + value_bindings: values, + ..Default::default() + }; + + // Pre-fill our type mappings with the types of the input bindings. + cc.known_types + .extend(types.iter() + .map(|(k, v)| (k.clone(), unit_type_set(*v)))); + cc + }, + } + } } /// Cloning. @@ -271,28 +306,16 @@ impl ConjoiningClauses { } } -impl ConjoiningClauses { - #[allow(dead_code)] - fn with_value_bindings(bindings: BTreeMap) -> ConjoiningClauses { - let mut cc = ConjoiningClauses { - value_bindings: bindings, - ..Default::default() - }; - - // Pre-fill our type mappings with the types of the input bindings. - cc.known_types - .extend(cc.value_bindings - .iter() - .map(|(k, v)| (k.clone(), unit_type_set(v.value_type())))); - cc - } -} - impl ConjoiningClauses { pub fn bound_value(&self, var: &Variable) -> Option { self.value_bindings.get(var).cloned() } + /// Return a set of the variables externally bound to values. + pub fn value_bound_variables(&self) -> BTreeSet { + self.value_bindings.keys().cloned().collect() + } + /// Return a single `ValueType` if the given variable is known to have a precise type. /// Returns `None` if the type of the variable is unknown. /// Returns `None` if the type of the variable is known but not precise -- "double diff --git a/query-algebrizer/src/clauses/pattern.rs b/query-algebrizer/src/clauses/pattern.rs index 99afdbf0..21e98138 100644 --- a/query-algebrizer/src/clauses/pattern.rs +++ b/query-algebrizer/src/clauses/pattern.rs @@ -270,6 +270,7 @@ mod testing { use super::*; use std::collections::BTreeMap; + use std::collections::BTreeSet; use std::rc::Rc; use mentat_core::attribute::Unique; @@ -288,6 +289,7 @@ mod testing { }; use clauses::{ + QueryInputs, add_attribute, associate_ident, ident, @@ -660,7 +662,9 @@ mod testing { let b: BTreeMap = vec![(y.clone(), TypedValue::Boolean(true))].into_iter().collect(); - let mut cc = ConjoiningClauses::with_value_bindings(b); + let inputs = QueryInputs::with_values(b); + let variables: BTreeSet = vec![Variable::from_valid_name("?y")].into_iter().collect(); + let mut cc = ConjoiningClauses::with_inputs(variables, inputs); cc.apply_pattern(&schema, Pattern { source: None, @@ -705,7 +709,9 @@ mod testing { let b: BTreeMap = vec![(y.clone(), TypedValue::Long(42))].into_iter().collect(); - let mut cc = ConjoiningClauses::with_value_bindings(b); + let inputs = QueryInputs::with_values(b); + let variables: BTreeSet = vec![Variable::from_valid_name("?y")].into_iter().collect(); + let mut cc = ConjoiningClauses::with_inputs(variables, inputs); cc.apply_pattern(&schema, Pattern { source: None, @@ -737,7 +743,9 @@ mod testing { let b: BTreeMap = vec![(y.clone(), TypedValue::Long(42))].into_iter().collect(); - let mut cc = ConjoiningClauses::with_value_bindings(b); + let inputs = QueryInputs::with_values(b); + let variables: BTreeSet = vec![Variable::from_valid_name("?y")].into_iter().collect(); + let mut cc = ConjoiningClauses::with_inputs(variables, inputs); cc.apply_pattern(&schema, Pattern { source: None, diff --git a/query-algebrizer/src/errors.rs b/query-algebrizer/src/errors.rs index b699039a..256ee970 100644 --- a/query-algebrizer/src/errors.rs +++ b/query-algebrizer/src/errors.rs @@ -10,6 +10,8 @@ extern crate mentat_query; +use mentat_core::ValueType; + use self::mentat_query::{ PlainSymbol, }; @@ -20,6 +22,11 @@ error_chain! { } errors { + InputTypeDisagreement(var: PlainSymbol, declared: ValueType, provided: ValueType) { + description("input type disagreement") + display("value of type {} provided for var {}, expected {}", provided, var, declared) + } + UnknownFunction(name: PlainSymbol) { description("no such function") display("no function named {}", name) diff --git a/query-algebrizer/src/lib.rs b/query-algebrizer/src/lib.rs index 344ee1d2..29b8ad5f 100644 --- a/query-algebrizer/src/lib.rs +++ b/query-algebrizer/src/lib.rs @@ -15,6 +15,7 @@ extern crate mentat_core; extern crate mentat_query; use std::collections::BTreeSet; +use std::ops::Sub; mod errors; mod types; @@ -41,6 +42,10 @@ pub use errors::{ Result, }; +pub use clauses::{ + QueryInputs, +}; + #[allow(dead_code)] pub struct AlgebraicQuery { default_source: SrcVar, @@ -74,16 +79,20 @@ impl AlgebraicQuery { pub fn is_known_empty(&self) -> bool { self.cc.is_known_empty() } + + /// Return a set of the input variables mentioned in the `:in` clause that have not yet been + /// bound. We do this by looking at the CC. + pub fn unbound_variables(&self) -> BTreeSet { + self.cc.input_variables.sub(&self.cc.value_bound_variables()) + } } pub fn algebrize_with_counter(schema: &Schema, parsed: FindQuery, counter: usize) -> Result { - let alias_counter = RcCounter::with_initial(counter); - let cc = clauses::ConjoiningClauses::with_alias_counter(alias_counter); - algebrize_with_cc(schema, parsed, cc) + algebrize_with_inputs(schema, parsed, counter, QueryInputs::default()) } pub fn algebrize(schema: &Schema, parsed: FindQuery) -> Result { - algebrize_with_cc(schema, parsed, clauses::ConjoiningClauses::default()) + algebrize_with_inputs(schema, parsed, 0, QueryInputs::default()) } /// Take an ordering list. Any variables that aren't fixed by the query are used to produce @@ -122,8 +131,13 @@ fn validate_and_simplify_order(cc: &ConjoiningClauses, order: Option> } } -#[allow(dead_code)] -pub fn algebrize_with_cc(schema: &Schema, parsed: FindQuery, mut cc: ConjoiningClauses) -> Result { +pub fn algebrize_with_inputs(schema: &Schema, + parsed: FindQuery, + counter: usize, + inputs: QueryInputs) -> Result { + let alias_counter = RcCounter::with_initial(counter); + let mut cc = ConjoiningClauses::with_inputs_and_alias_counter(parsed.in_vars, inputs, alias_counter); + // TODO: integrate default source into pattern processing. // TODO: flesh out the rest of find-into-context. let where_clauses = parsed.where_clauses; diff --git a/query/src/lib.rs b/query/src/lib.rs index 3a1c501a..3c267b07 100644 --- a/query/src/lib.rs +++ b/query/src/lib.rs @@ -34,7 +34,6 @@ extern crate edn; extern crate mentat_core; use std::collections::{ - BTreeMap, BTreeSet, }; @@ -46,7 +45,6 @@ pub use edn::{NamespacedKeyword, PlainSymbol}; use mentat_core::{ TypedValue, - ValueType, }; pub type SrcVarName = String; // Do not include the required syntactic '$'. @@ -743,4 +741,4 @@ impl ContainsVariables for Pattern { acc_ref(acc, v) } } -} +} \ No newline at end of file diff --git a/src/conn.rs b/src/conn.rs index 340e0210..88a26b70 100644 --- a/src/conn.rs +++ b/src/conn.rs @@ -10,7 +10,6 @@ #![allow(dead_code)] -use std::collections::HashMap; use std::sync::{Arc, Mutex}; use rusqlite; @@ -19,7 +18,6 @@ use edn; use mentat_core::{ Schema, - TypedValue, }; use mentat_db::db; @@ -29,13 +27,12 @@ use mentat_db::{ TxReport, }; -use mentat_query::Variable; - use mentat_tx_parser; use errors::*; use query::{ q_once, + QueryInputs, QueryResults, }; @@ -118,7 +115,7 @@ impl Conn { query: &str, inputs: T, limit: U) -> Result - where T: Into>>, + where T: Into>, U: Into> { diff --git a/src/errors.rs b/src/errors.rs index b623fd75..c9172ae0 100644 --- a/src/errors.rs +++ b/src/errors.rs @@ -12,6 +12,8 @@ use rusqlite; +use std::collections::BTreeSet; + use edn; use mentat_db; use mentat_query_algebrizer; @@ -40,6 +42,11 @@ error_chain! { } errors { + UnboundVariables(names: BTreeSet) { + description("unbound variables at execution time") + display("variables {:?} unbound at execution time", names) + } + InvalidArgumentName(name: String) { description("invalid argument name") display("invalid argument name: '{}'", name) diff --git a/src/lib.rs b/src/lib.rs index 89b60890..c9686a86 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -54,7 +54,9 @@ pub use mentat_db::{ pub use query::{ NamespacedKeyword, PlainSymbol, + QueryInputs, QueryResults, + Variable, q_once, }; diff --git a/src/query.rs b/src/query.rs index 89515499..5a4f8fb0 100644 --- a/src/query.rs +++ b/src/query.rs @@ -8,17 +8,20 @@ // CONDITIONS OF ANY KIND, either express or implied. See the License for the // specific language governing permissions and limitations under the License. -use std::collections::HashMap; - use rusqlite; use rusqlite::types::ToSql; use mentat_core::{ Schema, - TypedValue, }; -use mentat_query_algebrizer::algebrize; +use mentat_query_algebrizer::{ + algebrize_with_inputs, +}; + +pub use mentat_query_algebrizer::{ + QueryInputs, +}; pub use mentat_query::{ NamespacedKeyword, @@ -42,31 +45,31 @@ pub use mentat_query_projector::{ QueryResults, }; -use errors::Result; +use errors::{ + ErrorKind, + Result, +}; pub type QueryExecutionResult = Result; -/// Take an EDN query string, a reference to a open SQLite connection, a Mentat DB, and an optional -/// collection of input bindings (which should be keyed by `"?varname"`), and execute the query -/// immediately, blocking the current thread. +/// Take an EDN query string, a reference to an open SQLite connection, a Mentat schema, and an +/// optional collection of input bindings (which should be keyed by `"?varname"`), and execute the +/// query immediately, blocking the current thread. /// Returns a structure that corresponds to the kind of input query, populated with `TypedValue` /// instances. -/// The caller is responsible for ensuring that the SQLite connection is in a transaction if +/// The caller is responsible for ensuring that the SQLite connection has an open transaction if /// isolation is required. -#[allow(unused_variables)] pub fn q_once<'sqlite, 'schema, 'query, T, U> (sqlite: &'sqlite rusqlite::Connection, schema: &'schema Schema, query: &'query str, inputs: T, limit: U) -> QueryExecutionResult - where T: Into>>, + where T: Into>, U: Into> { - // TODO: validate inputs. - let parsed = parse_find_string(query)?; - let mut algebrized = algebrize(schema, parsed)?; + let mut algebrized = algebrize_with_inputs(schema, parsed, 0, inputs.into().unwrap_or(QueryInputs::default()))?; if algebrized.is_known_empty() { // We don't need to do any SQL work at all. @@ -75,6 +78,13 @@ pub fn q_once<'sqlite, 'schema, 'query, T, U> algebrized.apply_limit(limit.into()); + // Because this is q_once, we can check that all of our `:in` variables are bound at this point. + // If they aren't, the user has made an error -- perhaps writing the wrong variable in `:in`, or + // not binding in the `QueryInput`. + let unbound = algebrized.unbound_variables(); + if !unbound.is_empty() { + bail!(ErrorKind::UnboundVariables(unbound.into_iter().map(|v| v.to_string()).collect())); + } let select = query_to_select(algebrized); let SQLQuery { sql, args } = select.query.to_sql_query()?; diff --git a/tests/query.rs b/tests/query.rs index 8ad3b582..65c6880d 100644 --- a/tests/query.rs +++ b/tests/query.rs @@ -21,11 +21,18 @@ use mentat_core::{ use mentat::{ NamespacedKeyword, + QueryInputs, QueryResults, + Variable, new_connection, q_once, }; +use mentat::errors::{ + Error, + ErrorKind, +}; + #[test] fn test_rel() { let mut c = new_connection("").expect("Couldn't open conn."); @@ -159,3 +166,41 @@ fn test_coll() { println!("Coll took {}µs", start.to(end).num_microseconds().unwrap()); } +#[test] +fn test_inputs() { + let mut c = new_connection("").expect("Couldn't open conn."); + let db = mentat_db::db::ensure_current_version(&mut c).expect("Couldn't open DB."); + + // entids::DB_INSTALL_VALUE_TYPE = 5. + let ee = (Variable::from_valid_name("?e"), TypedValue::Ref(5)); + let inputs = QueryInputs::with_value_sequence(vec![ee]); + let results = q_once(&c, &db.schema, + "[:find ?i . :in ?e :where [?e :db/ident ?i]]", inputs, None) + .expect("query to succeed"); + + if let QueryResults::Scalar(Some(TypedValue::Keyword(value))) = results { + assert_eq!(value.as_ref(), &NamespacedKeyword::new("db.install", "valueType")); + } else { + panic!("Expected scalar."); + } +} + +/// Ensure that a query won't be run without all of its `:in` variables being bound. +#[test] +fn test_unbound_inputs() { + let mut c = new_connection("").expect("Couldn't open conn."); + let db = mentat_db::db::ensure_current_version(&mut c).expect("Couldn't open DB."); + + // Bind the wrong var by 'mistake'. + let xx = (Variable::from_valid_name("?x"), TypedValue::Ref(5)); + let inputs = QueryInputs::with_value_sequence(vec![xx]); + let results = q_once(&c, &db.schema, + "[:find ?i . :in ?e :where [?e :db/ident ?i]]", inputs, None); + + match results { + Result::Err(Error(ErrorKind::UnboundVariables(vars), _)) => { + assert_eq!(vars, vec!["?e".to_string()].into_iter().collect()); + }, + _ => panic!("Expected unbound variables."), + } +}