Skip to content

feat: pass parent to get_node_properties and add support for create aggregate with order by #72

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Dec 17, 2023
Merged

feat: pass parent to get_node_properties and add support for create aggregate with order by #72

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
79226b7
feat: add support for create aggregate with order by
psteinroe Dec 14, 2023
0753912
feat: pass parent to and finally fix define aggregate
psteinroe Dec 17, 2023
File filter

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 42 additions & 0 deletions crates/codegen/src/get_node_properties.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -481,6 +481,48 @@ fn custom_handlers(node: &Node) -> TokenStream {
tokens.push(TokenProperty::from(Token::As));
}
},
"DefineStmt" => quote! {
tokens.push(TokenProperty::from(Token::Create));
if n.replace {
tokens.push(TokenProperty::from(Token::Or));
tokens.push(TokenProperty::from(Token::Replace));
}
match n.kind() {
protobuf::ObjectType::ObjectAggregate => {
tokens.push(TokenProperty::from(Token::Aggregate));

// n.args is always an array with two nodes
assert_eq!(n.args.len(), 2, "DefineStmt of type ObjectAggregate does not have exactly 2 args");
// the first is either a List or a Node { node: None }

if let Some(node) = &n.args.first() {
if node.node.is_none() {
// if first element is a Node { node: None }, then it's "*"
tokens.push(TokenProperty::from(Token::Ascii42));
} else if let Some(node) = &node.node {
if let NodeEnum::List(_) = node {
// there *seems* to be an integer node in the last position of args that
// defines whether the list contains an order by statement
let integer = n.args.last()
.and_then(|node| node.node.as_ref())
.and_then(|node| if let NodeEnum::Integer(n) = node { Some(n.ival) } else { None });
if integer.is_none() {
panic!("DefineStmt of type ObjectAggregate has no integer node in last position of args");
}
// if the integer is 1, then there is an order by statement
// BUT: the order by tokens should be part of the List or maybe
// even the last FunctionParameter node in the list
if integer.unwrap() == 1 {
tokens.push(TokenProperty::from(Token::Order));
tokens.push(TokenProperty::from(Token::By));
}
}
Copy link
Collaborator Author

@psteinroe psteinroe Dec 14, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cvng I tried it myself and even though I figured how to know what tokens to add, it does still fail because the ORDER BY tokens must be part of either the List node or the last FunctionParameter node.

as of now, I don't see a way around this other than passing the node_graph and figuring out the nodes for the FunctionParameter from its parents...
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

its not a big deal, but I hoped that the AST is "clean enough" for this to not be required...
Copy link
Collaborator Author

@psteinroe psteinroe Dec 14, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

other valuable option: skip the List node entirely in the parser and put its children directly beneath the DefineStmt node. Could get messy though since the List node is also used elsewhere, so this has to be done only for children of DefineStmt.
Copy link
Contributor

@cvng cvng Dec 14, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@psteinroe I'm not confortable enough with the AST to know what a proper solution should be. It the test in #69 passes with the same output?

As of my understanding, I would go with solution 3 (direct children to DefineStmt) - based on the docs - aggregate with order by is kind of a special case

The syntax with ORDER BY in the parameter list creates a special type of aggregate called an ordered-set aggregate;

(also, I'm not sure this syntax exists elsewhere)

I let you close the previous PR if you think this is a better approach
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just by reading this, one could think that the order by is before / parent (same thing) of the list?

aggr_args: | '(' aggr_args_list ORDER BY aggr_args_list ')'

https://github.com/postgres/postgres/blob/REL_15_STABLE/src/backend/parser/gram.y#L8355
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes it should be part of the List node. we should then pass the parent AST node somehow, and for a List node, check if the parent is a DefineStmt, and do the same checks as I implemented for the DefineStmt. passing the node_graph will not be sufficient since we need the sibling token to be resolved first which cannot be guaranteed. we could also just always pass the entire DefineStmt / root node.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

keeping around a notion of context / parent node for some children is what you suggest? this looks like the approach taken by other parsers working with the Postgres AST - if I fully understand

also is it reasonable to have order by direct child of DefineStmt this case?
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

keeping around a notion of context / parent node for some children is what you suggest?

yes, at least within get_node_properties. I will add a pr early next week for this.

also is it reasonable to have order by direct child of DefineStmt this case?

no, because its between two second-level children. we would have to close and re-open the list node which is not a valid. and ignoring the List is also not a great solution.
}
}
},
_ => panic!("Unknown DefineStmt {:#?}", n.kind()),
}
},
"CreateSchemaStmt" => quote! {
tokens.push(TokenProperty::from(Token::Create));
tokens.push(TokenProperty::from(Token::Schema));
Expand Down
1 change: 1 addition & 0 deletions crates/parser/src/parse/libpg_query_node.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,7 @@ impl<'p> LibpgQueryNodeParser<'p> {
) -> LibpgQueryNodeParser<'p> {
let current_depth = parser.depth.clone();
debug!("Parsing node {:#?}", node);
println!("Parsing node {:#?}", node);
Self {
parser,
token_range,
Expand Down
10 changes: 10 additions & 0 deletions crates/parser/tests/data/statements/valid/0039.sql
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
CREATE AGGREGATE aggregate1 (int4) (sfunc = sfunc1, stype = stype1);
CREATE AGGREGATE aggregate1 (int4, bool) (sfunc = sfunc1, stype = stype1);
CREATE AGGREGATE aggregate1 (*) (sfunc = sfunc1, stype = stype1);
CREATE AGGREGATE aggregate1 (int4) (sfunc = sfunc1, stype = stype1, finalfunc_extra, mfinalfuncextra);
CREATE AGGREGATE aggregate1 (int4) (sfunc = sfunc1, stype = stype1, finalfunc_modify = read_only, parallel = restricted);
CREATE AGGREGATE percentile_disc (float8 ORDER BY anyelement) (sfunc = ordered_set_transition, stype = internal, finalfunc = percentile_disc_final, finalfunc_extra);
CREATE AGGREGATE custom_aggregate (float8 ORDER BY column1, column2) (sfunc = sfunc1, stype = stype1);