Review comments for simple aggregation.
This commit is contained in:
parent
3eb898566b
commit
971e166779
8 changed files with 46 additions and 40 deletions
|
@ -367,6 +367,12 @@ impl ::std::iter::Extend<ValueType> 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<ValueTypeTag>;
|
||||
fn has_unique_type_code(&self) -> bool;
|
||||
|
|
|
@ -376,7 +376,7 @@ impl ConjoiningClauses {
|
|||
|
||||
/// Return a set of the variables externally bound to values.
|
||||
pub fn value_bound_variable_set(&self) -> BTreeSet<Variable> {
|
||||
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.
|
||||
|
|
|
@ -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<Variable>,
|
||||
|
||||
/// 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<Variable>,
|
||||
pub order: Option<Vec<OrderBy>>,
|
||||
pub limit: Limit,
|
||||
|
|
|
@ -272,7 +272,7 @@ def_parser!(Query, func, (QueryFunction, Vec<FnArg>), {
|
|||
});
|
||||
|
||||
def_parser!(Query, aggregate, Aggregate, {
|
||||
seq().of_exactly((Query::query_function(), Query::arguments()))
|
||||
seq().of_exactly(Query::func())
|
||||
.map(|(func, args)| Aggregate {
|
||||
func, args,
|
||||
})
|
||||
|
|
|
@ -421,7 +421,7 @@ impl SimpleAggregation for Aggregate {
|
|||
struct ProjectedElements {
|
||||
sql_projection: Projection,
|
||||
templates: Vec<TypedIndex>,
|
||||
group_by: Option<Vec<GroupBy>>,
|
||||
group_by: Vec<GroupBy>,
|
||||
}
|
||||
|
||||
/// Walk an iterator of `Element`s, collecting projector templates and columns.
|
||||
|
@ -554,7 +554,7 @@ fn project_elements<'a, I: IntoIterator<Item = &'a Element>>(
|
|||
return Ok(ProjectedElements {
|
||||
sql_projection: Projection::Columns(cols),
|
||||
templates,
|
||||
group_by: None,
|
||||
group_by: vec![],
|
||||
});
|
||||
}
|
||||
|
||||
|
@ -572,10 +572,7 @@ fn project_elements<'a, I: IntoIterator<Item = &'a Element>>(
|
|||
// 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);
|
||||
|
@ -587,13 +584,11 @@ fn project_elements<'a, I: IntoIterator<Item = &'a Element>>(
|
|||
.cloned()
|
||||
.map(GroupBy::QueryColumn)
|
||||
.ok_or_else(|| ErrorKind::NoTypeAvailableForVariable(var.name().clone()))?;
|
||||
group_cols.push(type_col);
|
||||
group_by.push(type_col);
|
||||
}
|
||||
let val_col = cc_column(&query.cc, &var).map(GroupBy::QueryColumn)?;
|
||||
group_cols.push(val_col);
|
||||
group_by.push(val_col);
|
||||
}
|
||||
Some(group_cols)
|
||||
};
|
||||
|
||||
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<Vec<GroupBy>>,
|
||||
// A list of column names to use as a GROUP BY clause.
|
||||
pub group_by_cols: Vec<GroupBy>,
|
||||
}
|
||||
|
||||
impl CombinedProjection {
|
||||
|
@ -840,7 +834,7 @@ pub fn query_projection(query: &AlgebraicQuery) -> Result<CombinedProjection> {
|
|||
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 {
|
||||
|
|
|
@ -210,7 +210,7 @@ pub struct SelectQuery {
|
|||
pub projection: Projection,
|
||||
pub from: FromClause,
|
||||
pub constraints: Vec<Constraint>,
|
||||
pub group_by: Option<Vec<GroupBy>>,
|
||||
pub group_by: Vec<GroupBy>,
|
||||
pub order: Vec<OrderBy>,
|
||||
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,
|
||||
};
|
||||
|
|
|
@ -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<Vec<GroupBy>>,
|
||||
group_by: Vec<GroupBy>,
|
||||
order: Option<Vec<OrderBy>>,
|
||||
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)
|
||||
}
|
||||
}
|
||||
|
||||
|
|
|
@ -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
|
||||
|
|
Loading…
Reference in a new issue