From 971e1667790f7b468a3c032e8d7babdfa444016d Mon Sep 17 00:00:00 2001 From: Richard Newman Date: Wed, 28 Jun 2017 14:51:37 -0700 Subject: [PATCH] Review comments for simple aggregation. --- core/src/lib.rs | 6 ++++ query-algebrizer/src/clauses/mod.rs | 2 +- query-algebrizer/src/lib.rs | 12 ++++++-- query-parser/src/parse.rs | 2 +- query-projector/src/lib.rs | 46 +++++++++++++---------------- query-sql/src/lib.rs | 8 ++--- query-translator/src/translate.rs | 8 ++--- query-translator/tests/translate.rs | 2 +- 8 files changed, 46 insertions(+), 40 deletions(-) diff --git a/core/src/lib.rs b/core/src/lib.rs index 73e28858..b436c713 100644 --- a/core/src/lib.rs +++ b/core/src/lib.rs @@ -367,6 +367,12 @@ impl ::std::iter::Extend for ValueTypeSet { } } +/// We have an enum of types, `ValueType`. It can be collected into a set, `ValueTypeSet`. Each type +/// is associated with a type tag, which is how a type is represented in, e.g., SQL storage. Types +/// can share type tags, because backing SQL storage is able to differentiate between some types +/// (e.g., longs and doubles), and so distinct tags aren't necessary. That association is defined by +/// `SQLValueType`. That trait similarly extends to `ValueTypeSet`, which maps a collection of types +/// into a collection of tags. pub trait SQLValueTypeSet { fn value_type_tags(&self) -> BTreeSet; fn has_unique_type_code(&self) -> bool; diff --git a/query-algebrizer/src/clauses/mod.rs b/query-algebrizer/src/clauses/mod.rs index 2e26a07f..b066be62 100644 --- a/query-algebrizer/src/clauses/mod.rs +++ b/query-algebrizer/src/clauses/mod.rs @@ -376,7 +376,7 @@ impl ConjoiningClauses { /// Return a set of the variables externally bound to values. pub fn value_bound_variable_set(&self) -> BTreeSet { - self.value_bindings.keys().cloned().collect() + self.value_bound_variables().cloned().collect() } /// Return a single `ValueType` if the given variable is known to have a precise type. diff --git a/query-algebrizer/src/lib.rs b/query-algebrizer/src/lib.rs index 412f1943..af107a50 100644 --- a/query-algebrizer/src/lib.rs +++ b/query-algebrizer/src/lib.rs @@ -63,11 +63,17 @@ pub struct AlgebraicQuery { default_source: SrcVar, pub find_spec: FindSpec, - /// A set of variables that the caller wishes to be used for grouping when aggregating. + /// The set of variables that the caller wishes to be used for grouping when aggregating. + /// These are specified in the query input, as `:with`, and are then chewed up during projection. + /// If no variables are supplied, then no additional grouping is necessary beyond the + /// non-aggregated projection list. pub with: BTreeSet, - /// A set of variables that must be projected in order for query features such as ordering - /// to work correctly. + /// Some query features, such as ordering, are implemented by implicit reference to SQL columns. + /// In order for these references to be 'live', those columns must be projected. + /// This is the set of variables that must be so projected. + /// This is not necessarily every variable that will be so required -- some variables + /// will already be in the projection list. pub named_projection: BTreeSet, pub order: Option>, pub limit: Limit, diff --git a/query-parser/src/parse.rs b/query-parser/src/parse.rs index 1e59c8d7..3a529920 100644 --- a/query-parser/src/parse.rs +++ b/query-parser/src/parse.rs @@ -272,7 +272,7 @@ def_parser!(Query, func, (QueryFunction, Vec), { }); def_parser!(Query, aggregate, Aggregate, { - seq().of_exactly((Query::query_function(), Query::arguments())) + seq().of_exactly(Query::func()) .map(|(func, args)| Aggregate { func, args, }) diff --git a/query-projector/src/lib.rs b/query-projector/src/lib.rs index 398f56de..714dd6db 100644 --- a/query-projector/src/lib.rs +++ b/query-projector/src/lib.rs @@ -421,7 +421,7 @@ impl SimpleAggregation for Aggregate { struct ProjectedElements { sql_projection: Projection, templates: Vec, - group_by: Option>, + group_by: Vec, } /// Walk an iterator of `Element`s, collecting projector templates and columns. @@ -554,7 +554,7 @@ fn project_elements<'a, I: IntoIterator>( return Ok(ProjectedElements { sql_projection: Projection::Columns(cols), templates, - group_by: None, + group_by: vec![], }); } @@ -572,28 +572,23 @@ fn project_elements<'a, I: IntoIterator>( // Turn this collection of vars into a collection of columns from the query. // Right now we don't allow grouping on anything but a variable bound in the query. // TODO: also group by type tag. - let group_by = if group_by_vars.is_empty() { - None - } else { - let mut group_cols = Vec::with_capacity(2 * group_by_vars.len()); + let mut group_by = Vec::with_capacity(2 * group_by_vars.len()); - for var in group_by_vars.into_iter() { - let types = query.cc.known_type_set(&var); - if !types.has_unique_type_code() { - // Group by type then SQL value. - let type_col = query.cc - .extracted_types - .get(&var) - .cloned() - .map(GroupBy::QueryColumn) - .ok_or_else(|| ErrorKind::NoTypeAvailableForVariable(var.name().clone()))?; - group_cols.push(type_col); - } - let val_col = cc_column(&query.cc, &var).map(GroupBy::QueryColumn)?; - group_cols.push(val_col); + for var in group_by_vars.into_iter() { + let types = query.cc.known_type_set(&var); + if !types.has_unique_type_code() { + // Group by type then SQL value. + let type_col = query.cc + .extracted_types + .get(&var) + .cloned() + .map(GroupBy::QueryColumn) + .ok_or_else(|| ErrorKind::NoTypeAvailableForVariable(var.name().clone()))?; + group_by.push(type_col); } - Some(group_cols) - }; + let val_col = cc_column(&query.cc, &var).map(GroupBy::QueryColumn)?; + group_by.push(val_col); + } Ok(ProjectedElements { sql_projection: Projection::Columns(cols), @@ -807,9 +802,8 @@ pub struct CombinedProjection { /// True if this query requires the SQL query to include DISTINCT. pub distinct: bool, - // An optional list of column names to use as a GROUP BY clause. - // Right now these are all `Name`s, and present in the `Projection`. - pub group_by_cols: Option>, + // A list of column names to use as a GROUP BY clause. + pub group_by_cols: Vec, } impl CombinedProjection { @@ -840,7 +834,7 @@ pub fn query_projection(query: &AlgebraicQuery) -> Result { sql_projection: Projection::One, datalog_projector: Box::new(constant_projector), distinct: false, - group_by_cols: None, // TODO + group_by_cols: vec![], }) } else { match query.find_spec { diff --git a/query-sql/src/lib.rs b/query-sql/src/lib.rs index 8312436f..a4e01582 100644 --- a/query-sql/src/lib.rs +++ b/query-sql/src/lib.rs @@ -210,7 +210,7 @@ pub struct SelectQuery { pub projection: Projection, pub from: FromClause, pub constraints: Vec, - pub group_by: Option>, + pub group_by: Vec, pub order: Vec, pub limit: Limit, } @@ -575,8 +575,8 @@ impl QueryFragment for SelectQuery { { out.push_sql(" AND ") }); } - match self.group_by { - Some(ref group_by) if !group_by.is_empty() => { + match &self.group_by { + group_by if !group_by.is_empty() => { out.push_sql(" GROUP BY "); interpose!(group, group_by, { group.push_sql(out)? }, @@ -785,7 +785,7 @@ mod tests { right: ColumnOrExpression::Entid(65536), }, ], - group_by: None, + group_by: vec![], order: vec![], limit: Limit::None, }; diff --git a/query-translator/src/translate.rs b/query-translator/src/translate.rs index b6fe11f2..0cc90359 100644 --- a/query-translator/src/translate.rs +++ b/query-translator/src/translate.rs @@ -250,7 +250,7 @@ fn table_for_computed(computed: ComputedTable, alias: TableAlias) -> TableOrSubq // Each arm simply turns into a subquery. // The SQL translation will stuff "UNION" between each arm. let projection = Projection::Columns(columns); - cc_to_select_query(projection, cc, false, None, None, Limit::None) + cc_to_select_query(projection, cc, false, vec![], None, Limit::None) }).collect(), alias) }, @@ -272,7 +272,7 @@ fn table_for_computed(computed: ComputedTable, alias: TableAlias) -> TableOrSubq fn cc_to_select_query(projection: Projection, cc: ConjoiningClauses, distinct: bool, - group_by: Option>, + group_by: Vec, order: Option>, limit: Limit) -> SelectQuery { let from = if cc.from.is_empty() { @@ -327,13 +327,13 @@ pub fn cc_to_exists(cc: ConjoiningClauses) -> SelectQuery { distinct: false, projection: Projection::One, from: FromClause::Nothing, - group_by: None, + group_by: vec![], constraints: vec![], order: vec![], limit: Limit::None, } } else { - cc_to_select_query(Projection::One, cc, false, None, None, Limit::None) + cc_to_select_query(Projection::One, cc, false, vec![], None, Limit::None) } } diff --git a/query-translator/tests/translate.rs b/query-translator/tests/translate.rs index d3402b66..ccb9e5e6 100644 --- a/query-translator/tests/translate.rs +++ b/query-translator/tests/translate.rs @@ -191,7 +191,7 @@ fn test_bound_variable_limit_affects_types() { assert_eq!(Some(ValueType::Long), algebrized.cc.known_type(&Variable::from_valid_name("?limit"))); - let select = query_to_select(algebrized).expect("query to translate"); + let select = query_to_select(algebrized).expect("query to successfully translate"); let SQLQuery { sql, args } = select.query.to_sql_query().unwrap(); // TODO: this query isn't actually correct -- we don't yet algebrize for variables that are