feat: pass parent to get_node_properties and add support for create aggregate with order by
#72
Conversation
| // 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)); | ||
| } | ||
| } |
There was a problem hiding this comment.
@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...
There was a problem hiding this comment.
its not a big deal, but I hoped that the AST is "clean enough" for this to not be required...
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
psteinroe
changed the title
parent to get_node_properties and add support for create aggregate with order by
|
@cvng we now pass the |
There was a problem hiding this comment.
sure! I think parent will be required for some other "intermediate" nodes as well @psteinroe
What kind of change does this PR introduce?
adds support for
CREATE AGGREGATEWhat is the current behavior?
panics
What is the new behavior?
still panics for aggregates with
ORDER BYAdditional context
follow-up for #69