Add SQL planner support for ROLLUP and CUBE grouping set expressions
#2446
Conversation
andygrove
changed the title
ROLLUP grouping set expressionsROLLUP and CUBE grouping set expressions
andygrove
changed the title
ROLLUP and CUBE grouping set expressionsROLLUP and CUBE grouping set expressions
| @@ -138,9 +139,33 @@ pub fn create_udaf( | |||
| /// Create field meta-data from an expression, for use in a result set schema | |||
| pub fn exprlist_to_fields<'a>( | |||
| expr: impl IntoIterator<Item = &'a Expr>, | |||
| input_schema: &DFSchema, | |||
| plan: &LogicalPlan, | |||
There was a problem hiding this comment.
note that we pass in a plan now instead of a schema
Thanks @andygrove -- I have put this on my queue for review tomorrow |
| @@ -124,6 +124,10 @@ impl ExprSchemable for Expr { | |||
| "QualifiedWildcard expressions are not valid in a logical query plan" | |||
| .to_owned(), | |||
| )), | |||
| Expr::GroupingSet(_) => { | |||
| // grouping sets do not really have a type and do not appear in projections | |||
| Ok(DataType::Null) | |||
I agree -- it makes the review process much easier ❤️ |
| @@ -249,6 +249,24 @@ pub enum Expr { | |||
| Wildcard, | |||
| /// Represents a reference to all fields in a specific schema. | |||
| QualifiedWildcard { qualifier: String }, | |||
| /// List of grouping set expressions. Only valid in the context of an aggregate | |||
There was a problem hiding this comment.
While reviewing this PR I was wondering how you aim to plan these queries
it seems like a simple way would be to expand them to a set of UNION ALL queries, though then we will likely do the same input query many times which may be unideal.
Anyhow, I am looking forward to the next installment
| @@ -198,6 +202,11 @@ impl ExprSchemable for Expr { | |||
| let data_type = expr.get_type(input_schema)?; | |||
| get_indexed_field(&data_type, key).map(|x| x.is_nullable()) | |||
| } | |||
| Expr::GroupingSet(_) => { | |||
| // grouping sets do not really have the concept of nullable and do not appear | |||
There was a problem hiding this comment.
I wonder if it is worth considering representing the grouping sets in a more specific way (like maybe a specific list on LogicalPlan::Aggregrate) rather than an Expr that can appear anywhere
enum GroupingExpr {
Expr(Expr),
GroupingSet(GroupingSet),
}
/// Aggregates its input based on a set of grouping and aggregate
/// expressions (e.g. SUM).
#[derive(Clone)]
pub struct Aggregate {
/// The incoming logical plan
pub input: Arc<LogicalPlan>,
/// Grouping expressions
pub group_expr: Vec<AggExprs>,
/// Aggregate expressions
pub aggr_expr: Vec<Expr>,
/// The schema description of the aggregate output
pub schema: DFSchemaRef,
}Or something?
|
I think this is good to merge; There are some comments, but we can address them as part of follow on PRs if appropriate. |
|
Nice work @andygrove |
…ons (apache#2446) * Add SQL planner support for ROLLUP and CUBE grouping sets * prep for review * fix more todo comments * code cleanup * clippy * fmt and clippy * revert change * clippy
Which issue does this PR close?
Closes #2378
Rationale for this change
I would like to be able to create a valid logical plan from SQL queries containing
ROLLUPgrouping sets.What changes are included in this PR?
ROLLUPandCUBEgrouping sets in the SQL planner and logical planGROUPING SETSbut I ran into some issues with this so have filed Add support forGROUPING SETSsyntax in SQL planner #2469 to complete this workAre there any user-facing changes?
Yes, we have new
Exprvariants.