Update unions with named node & allow more optional paranthesis - #178
Conversation
| choice( | ||
| $._select_statement, | ||
| $.union, | ||
| $.intersection, |
There was a problem hiding this comment.
there's also an except set operation that yields all tuples in a not present in b
There was a problem hiding this comment.
I had merged that with intersection should these all be grouped under a common set_operation node instead? Or swap back to making them choices under the _select_statement?
SELECT 1 UNION SELECT 2;Current Option on `main` branch
(statement
(select
(keyword_select)
(select_expression
(term value: (literal))))
(keyword_union)
(select
(keyword_select)
(select_expression
(term value: (literal))))))
Current Option in the PR
(statement
(union
(select
(keyword_select)
(select_expression
(term value: (literal))))
(keyword_union)
(select
(keyword_select)
(select_expression
(term value: (literal)))))))
Alternative
(statement
(set_operation
(select
(keyword_select)
(select_expression
(term value: (literal))))
operation: (keyword_union)
(select
(keyword_select)
(select_expression
(term value: (literal)))))))
There was a problem hiding this comment.
oh I like having the generic node type discriminable by the operation field in the third!
| $._dml_read, | ||
| ')', | ||
| ), | ||
| optional_parenthesis($._dml_read), |
There was a problem hiding this comment.
okay it's really weird that you can wrap a select but not an insert or a delete
There was a problem hiding this comment.
@dmfay did you tested that? It is really astonishing how many things are not documented in postgres
Update: I tested it, and yes it is not possible to have parens around delete/insert 😲
There was a problem hiding this comment.
yeah even if you use a returning it refuses to parse any parenthesized write statement 🤯
| (identifier) | ||
| (keyword_as) | ||
| (statement | ||
| (statement |
There was a problem hiding this comment.
missing a possible test for cte in parentheses -- this parses in Postgres:
((((with x as ((((select * from ints)))) ((((select * from x))))))));There was a problem hiding this comment.
I updated the CTE's to be allowed to be wrapped in parenthesis & added a tests but an arbitrary number of parenthesis might require a custom scanner.
There was a problem hiding this comment.
dollar quotes (beyond the few common types I added explicitly) need a custom scanner too, but I didn't have much luck with that when I tried a while back
| } | ||
|
|
||
| function optional_parenthesis(node) { | ||
| return prec.right( |
There was a problem hiding this comment.
does it not work without right-precedence?
There was a problem hiding this comment.
Nope, I think it's because there's multiple layers where the parenthesis are allowed to appear so this pushes it "down" the tree.
$ tree-sitter generate
Unresolved conflict for symbol sequence:
'(' union • ')' …
Possible interpretations:
1: '(' (_dml_read union) • ')' …
2: (_dml_read '(' union • ')')
Possible resolutions:
1: Specify a left or right associativity in `_dml_read`
2: Add a conflict for these rules: `_dml_read`
There was a problem hiding this comment.
ah weird, that's probably it. Not a lot to do about it I suppose
matthias-Q
left a comment
matthias-Q
left a comment
There was a problem hiding this comment.
Wow, many changes. LGTM, I hope nothing breaks 😄
Maybe add this extra function that I suggested in a comment.
|
I added a few more commit to address some PR comments I also have an open question here: #178 (comment) I'd appreciate another round of reviews 🎉 |
458d107 to
5b44a8f
Compare
5b44a8f to
5c01247
Compare
matthias-Q
left a comment
matthias-Q
left a comment
There was a problem hiding this comment.
So many changes. Since it is now mostly using the new functions I would so go for it. looks good!
3 participants
What
fixes #176
Commits might be easier to review separately
optional_parenthesis&wrapped_in_parenthesisfunctions to make it easier work with parenthesiscreate_querynode with_dml_readunion&intersectionset_operationnode instead of having custom rules in_select_statement