Skip to content

Commit

Permalink
Remove panics, todos, update CHANGELOG
Browse files Browse the repository at this point in the history
  • Loading branch information
@alancai98
alancai98 committed Jul 10, 2023
1 parent ad387ce commit 64b07d4
Show file tree
Hide file tree
Showing 2 changed files with 79 additions and 76 deletions.
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
- Add `partiql_ast_passes::static_typer` for type annotating the AST.
- Add ability to parse `ORDER BY`, `LIMIT`, `OFFSET` in children of set operators
- Add `OUTER` bag operator (`OUTER UNION`, `OUTER INTERSECT`, `OUTER EXCEPT`) implementation
- Add ast to logical plan lowering for subqueries

### Fixes
- Fixes parsing of multiple consecutive path wildcards (e.g. `a[*][*][*]`), unpivot (e.g. `a.*.*.*`), and path expressions (e.g. `a[1 + 2][3 + 4][5 + 6]`)—previously these would not parse correctly.
Expand Down
154 changes: 78 additions & 76 deletions partiql-logical-planner/src/lower.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,20 @@ macro_rules! not_yet_implemented_err {
};
}

#[macro_export]
macro_rules! cur_plan_or_fault {
($self:ident) => {{
let plan = $self.plan_stack.last_mut();
if plan.is_none() {
$self.errors.push(AstTransformError::IllegalState(
"current plan is None".to_string(),
));
return partiql_ast::visit::Traverse::Stop;
}
plan.unwrap()
}};
}

#[derive(Copy, Clone, Debug)]
enum QueryContext {
FromLet,
Expand Down Expand Up @@ -255,7 +269,13 @@ impl<'a> AstToLogical<'a> {
errors: self.errors,
});
}
assert_eq!(self.plan_stack.len(), 1);
if self.plan_stack.len() != 1 {
return Err(AstTransformationError {
errors: vec![AstTransformError::IllegalState(
"plan_stack.len() != 1".to_string(),
)],
});
}
Ok(self.plan_stack.pop().expect("plan"))
}

Expand Down Expand Up @@ -490,34 +510,6 @@ impl<'a> AstToLogical<'a> {
fn push_sort_spec(&mut self, spec: logical::SortSpec) {
self.sort_stack.last_mut().unwrap().push(spec);
}

#[inline]
fn plan_add_operator(&mut self, op: BindingsOp) -> OpId {
self.plan_stack.last_mut().unwrap().add_operator(op)
}

#[inline]
fn plan_add_flow(&mut self, src: OpId, dst: OpId) {
self.plan_stack.last_mut().unwrap().add_flow(src, dst)
}

#[inline]
fn plan_add_flow_with_branch_num(&mut self, src: OpId, dst: OpId, branch_num: u8) {
self.plan_stack
.last_mut()
.unwrap()
.add_flow_with_branch_num(src, dst, branch_num)
}

#[inline]
fn plan_operator(&mut self, id: OpId) -> Option<&BindingsOp> {
self.plan_stack.last_mut().unwrap().operator(id)
}

#[inline]
fn plan_operator_as_mut(&mut self, id: OpId) -> Option<&mut BindingsOp> {
self.plan_stack.last_mut().unwrap().operator_as_mut(id)
}
}

// SQL (and therefore PartiQL) text (and therefore AST) is not lexically-scoped as is the
Expand Down Expand Up @@ -618,8 +610,9 @@ impl<'a, 'ast> Visitor<'ast> for AstToLogical<'a> {
let mut benv = self.exit_benv();
eq_or_fault!(self, benv.len(), 1, "Expect benv.len() == 1");
let out = benv.pop().unwrap();
let sink_id = self.plan_add_operator(BindingsOp::Sink);
self.plan_add_flow(out, sink_id);
let plan = cur_plan_or_fault!(self);
let sink_id = plan.add_operator(BindingsOp::Sink);
plan.add_flow(out, sink_id);
Traverse::Continue
}

Expand All @@ -640,16 +633,18 @@ impl<'a, 'ast> Visitor<'ast> for AstToLogical<'a> {
match query.set.node {
QuerySet::Select(_) => {
let clauses = self.exit_q();
let plan = cur_plan_or_fault!(self);
let mut clauses = clauses.evaluation_order().into_iter();
if let Some(mut src_id) = clauses.next() {
for dst_id in clauses {
self.plan_add_flow(src_id, dst_id);
plan.add_flow(src_id, dst_id);
src_id = dst_id;
}
if !self.q_stack.is_empty() {
// this is a subquery
true_or_fault!(self, !self.plan_stack.is_empty(), "plan_stack is empty");
let plan = self.plan_stack.pop().unwrap();
assert!(!self.vexpr_stack.is_empty());
true_or_fault!(self, !self.vexpr_stack.is_empty(), "vexpr_stack is empty");
self.push_vexpr(logical::ValueExpr::SubQueryExpr(SubQueryExpr { plan }));
// don't need bexpr for sink; will use outer query's bexpr
} else {
Expand All @@ -664,8 +659,9 @@ impl<'a, 'ast> Visitor<'ast> for AstToLogical<'a> {
"benv.len() is not between 1 and 3"
);
let mut out = *benv.first().unwrap();
let plan = cur_plan_or_fault!(self);
benv.into_iter().skip(1).for_each(|op| {
self.plan_add_flow(out, op);
plan.add_flow(out, op);
out = op;
});
self.push_bexpr(out);
Expand Down Expand Up @@ -695,6 +691,7 @@ impl<'a, 'ast> Visitor<'ast> for AstToLogical<'a> {
fn exit_query_set(&mut self, query_set: &'ast QuerySet) -> Traverse {
let env = self.exit_env();
let mut benv = self.exit_benv();
let plan = cur_plan_or_fault!(self);

match query_set {
QuerySet::BagOp(bag_op) => {
Expand All @@ -715,20 +712,20 @@ impl<'a, 'ast> Visitor<'ast> for AstToLogical<'a> {
SetQuantifier::Distinct => logical::SetQuantifier::Distinct,
};

let id = self.plan_add_operator(BindingsOp::BagOp(BagOp {
let id = plan.add_operator(BindingsOp::BagOp(BagOp {
bag_op: bag_operator,
setq,
}));
self.plan_add_flow_with_branch_num(lid, id, 0);
self.plan_add_flow_with_branch_num(rid, id, 1);
plan.add_flow_with_branch_num(lid, id, 0);
plan.add_flow_with_branch_num(rid, id, 1);
self.push_bexpr(id);
}
QuerySet::Select(_) => {}
QuerySet::Expr(_) => {
eq_or_fault!(self, env.len(), 1, "env.len() != 1");
let expr = env.into_iter().next().unwrap();
let op = BindingsOp::ExprQuery(logical::ExprQuery { expr });
let id = self.plan_add_operator(op);
let id = plan.add_operator(op);
self.push_bexpr(id);
}
QuerySet::Values(_) => {
Expand Down Expand Up @@ -770,8 +767,10 @@ impl<'a, 'ast> Visitor<'ast> for AstToLogical<'a> {
let env = self.exit_env();
eq_or_fault!(self, env.len(), 0, "env.len() != 0");

let plan = cur_plan_or_fault!(self);

if let Some(SetQuantifier::Distinct) = _projection.setq {
let id = self.plan_add_operator(BindingsOp::Distinct);
let id = plan.add_operator(BindingsOp::Distinct);
self.current_clauses_mut().distinct.replace(id);
}
Traverse::Continue
Expand All @@ -784,10 +783,7 @@ impl<'a, 'ast> Visitor<'ast> for AstToLogical<'a> {
}

fn exit_projection_kind(&mut self, _projection_kind: &'ast ProjectionKind) -> Traverse {
let benv = self.exit_benv();
if !benv.is_empty() {
not_yet_implemented_fault!(self, "Subquery within project".to_string());
}
self.exit_benv();
let env = self.exit_env();

let select: BindingsOp = match _projection_kind {
Expand Down Expand Up @@ -837,7 +833,8 @@ impl<'a, 'ast> Visitor<'ast> for AstToLogical<'a> {
logical::BindingsOp::ProjectValue(logical::ProjectValue { expr })
}
};
let id = self.plan_add_operator(select);
let plan = cur_plan_or_fault!(self);
let id = plan.add_operator(select);
self.current_clauses_mut().select_clause.replace(id);
Traverse::Continue
}
Expand Down Expand Up @@ -1223,7 +1220,8 @@ impl<'a, 'ast> Visitor<'ast> for AstToLogical<'a> {
aggregate_exprs: self.aggregate_exprs.clone(),
group_as_alias: None,
});
let id = self.plan_add_operator(group_by);
let plan = cur_plan_or_fault!(self);
let id = plan.add_operator(group_by);
self.current_clauses_mut().group_by_clause.replace(id);
}
Traverse::Continue
Expand Down Expand Up @@ -1381,7 +1379,8 @@ impl<'a, 'ast> Visitor<'ast> for AstToLogical<'a> {
at_key,
}),
};
let id = self.plan_add_operator(bexpr);
let plan = cur_plan_or_fault!(self);
let id = plan.add_operator(bexpr);
self.push_bexpr(id);
Traverse::Continue
}
Expand Down Expand Up @@ -1417,17 +1416,18 @@ impl<'a, 'ast> Visitor<'ast> for AstToLogical<'a> {

let rid = benv.pop().unwrap();
let lid = benv.pop().unwrap();
let left = Box::new(self.plan_operator(lid).unwrap().clone());
let right = Box::new(self.plan_operator(rid).unwrap().clone());
let plan = cur_plan_or_fault!(self);
let left = Box::new(plan.operator(lid).unwrap().clone());
let right = Box::new(plan.operator(rid).unwrap().clone());
let join = logical::BindingsOp::Join(logical::Join {
kind,
on,
left,
right,
});
let join = self.plan_add_operator(join);
self.plan_add_flow_with_branch_num(lid, join, 0);
self.plan_add_flow_with_branch_num(rid, join, 1);
let join = plan.add_operator(join);
plan.add_flow_with_branch_num(lid, join, 0);
plan.add_flow_with_branch_num(rid, join, 1);
self.push_bexpr(join);
Traverse::Continue
}
Expand Down Expand Up @@ -1459,7 +1459,8 @@ impl<'a, 'ast> Visitor<'ast> for AstToLogical<'a> {
let filter = logical::BindingsOp::Filter(logical::Filter {
expr: env.pop().unwrap(),
});
let id = self.plan_add_operator(filter);
let plan = cur_plan_or_fault!(self);
let id = plan.add_operator(filter);

self.current_clauses_mut().where_clause.replace(id);
Traverse::Continue
Expand All @@ -1477,7 +1478,8 @@ impl<'a, 'ast> Visitor<'ast> for AstToLogical<'a> {
let having = BindingsOp::Having(logical::Having {
expr: env.pop().unwrap(),
});
let id = self.plan_add_operator(having);
let plan = cur_plan_or_fault!(self);
let id = plan.add_operator(having);

self.current_clauses_mut().having_clause.replace(id);
Traverse::Continue
Expand All @@ -1491,12 +1493,7 @@ impl<'a, 'ast> Visitor<'ast> for AstToLogical<'a> {

fn exit_group_by_expr(&mut self, _group_by_expr: &'ast GroupByExpr) -> Traverse {
let aggregate_exprs = self.aggregate_exprs.clone();
let benv = self.exit_benv();
if !benv.is_empty() {
{
not_yet_implemented_fault!(self, "Subquery in group by".to_string());
}
}
self.exit_benv();
let env = self.exit_env();
true_or_fault!(self, env.len().is_even(), "env.len() is not even");

Expand All @@ -1510,6 +1507,7 @@ impl<'a, 'ast> Visitor<'ast> for AstToLogical<'a> {
GroupingStrategy::GroupPartial => logical::GroupingStrategy::GroupPartial,
};

let mut binding = HashMap::new();
// What follows is an approach to implement section 11.2.1 of the PartiQL spec
// (https://partiql.org/assets/PartiQL-Specification.pdf#subsubsection.11.2.1)
// "Grouping Attributes and Direct Use of Grouping Expressions"
Expand All @@ -1526,11 +1524,16 @@ impl<'a, 'ast> Visitor<'ast> for AstToLogical<'a> {
));
return Traverse::Stop;
}
let select_clause = self
.plan_operator_as_mut(select_clause_op_id.expect("select_clause_op_id not None"))
.unwrap();
let mut binding = HashMap::new();
let select_clause_exprs = match select_clause {
let plan = cur_plan_or_fault!(self);
let select_clause =
plan.operator_as_mut(select_clause_op_id.expect("select_clause_op_id not None"));
if select_clause.is_none() {
self.errors.push(AstTransformError::IllegalState(
"select_clause in plan not None".to_string(),
));
return Traverse::Stop;
}
let select_clause_exprs = match select_clause.expect("select_clause is not None") {
BindingsOp::Project(ref mut project) => &mut project.exprs,
BindingsOp::ProjectAll => &mut binding,
BindingsOp::ProjectValue(_) => &mut binding, // TODO: replacement of SELECT VALUE expressions
Expand All @@ -1553,18 +1556,16 @@ impl<'a, 'ast> Visitor<'ast> for AstToLogical<'a> {
Value::String(s) => (*s).clone(),
_ => {
// Report error but allow visitor to continue
// self.errors.push(AstTransformError::IllegalState(
// "Unexpected literal type".to_string(),
// ));
//todo!("Add back error");
self.errors.push(AstTransformError::IllegalState(
"Unexpected literal type".to_string(),
));
"".to_string()
}
},
_ => {
//todo!("Add back error");
// self.errors.push(AstTransformError::IllegalState(
// "Unexpected alias type".to_string(),
// ));
self.errors.push(AstTransformError::IllegalState(
"Unexpected alias type".to_string(),
));
return Traverse::Stop;
}
};
Expand All @@ -1588,8 +1589,7 @@ impl<'a, 'ast> Visitor<'ast> for AstToLogical<'a> {
aggregate_exprs,
group_as_alias,
});

let id = self.plan_add_operator(group_by);
let id = plan.add_operator(group_by);
self.current_clauses_mut().group_by_clause.replace(id);
Traverse::Continue
}
Expand Down Expand Up @@ -1617,7 +1617,8 @@ impl<'a, 'ast> Visitor<'ast> for AstToLogical<'a> {
fn exit_order_by_expr(&mut self, _order_by_expr: &'ast OrderByExpr) -> Traverse {
let specs = self.exit_sort();
let order_by = logical::BindingsOp::OrderBy(logical::OrderBy { specs });
let id = self.plan_add_operator(order_by);
let plan = cur_plan_or_fault!(self);
let id = plan.add_operator(order_by);
if matches!(self.current_ctx(), Some(QueryContext::Query)) {
self.current_clauses_mut().order_by_clause.replace(id);
} else {
Expand Down Expand Up @@ -1690,7 +1691,8 @@ impl<'a, 'ast> Visitor<'ast> for AstToLogical<'a> {
};

let limit_offset = logical::BindingsOp::LimitOffset(logical::LimitOffset { limit, offset });
let id = self.plan_add_operator(limit_offset);
let plan = cur_plan_or_fault!(self);
let id = plan.add_operator(limit_offset);
if matches!(self.current_ctx(), Some(QueryContext::Query)) {
self.current_clauses_mut().limit_offset_clause.replace(id);
} else {
Expand Down

0 comments on commit 64b07d4

Please sign in to comment.