Skip to content

parser: fix select precedence for trailing clauses #544

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
kodiakhq merged 2 commits into from
Jun 17, 2025
Merged

parser: fix select precedence for trailing clauses #544

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a612d1b
parser: fix select precedence for trailing clauses
sbdchd Jun 17, 2025
791dbd9
fix
sbdchd Jun 17, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
77 changes: 57 additions & 20 deletions crates/squawk_parser/src/grammar.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,11 @@ fn array_expr(p: &mut Parser<'_>, m: Option<Marker>) -> CompletedMarker {
R_BRACK
};
while !p.at(EOF) && !p.at(closing) {
if p.at_ts(SELECT_FIRST) && (select(p, None).is_none() || p.at(EOF) || p.at(closing)) {
if p.at_ts(SELECT_FIRST)
&& (select(p, None, &SelectRestrictions::default()).is_none()
|| p.at(EOF)
|| p.at(closing))
{
break;
}
if expr(p).is_none() {
Expand All @@ -73,6 +77,18 @@ fn array_expr(p: &mut Parser<'_>, m: Option<Marker>) -> CompletedMarker {
m.complete(p, ARRAY_EXPR)
}

struct SelectRestrictions {
trailing_clauses: bool,
}

impl Default for SelectRestrictions {
fn default() -> Self {
Self {
trailing_clauses: true,
}
}
}

fn opt_paren_select(p: &mut Parser<'_>) -> Option<CompletedMarker> {
let m = p.start();
if !p.eat(L_PAREN) {
Expand All @@ -83,7 +99,11 @@ fn opt_paren_select(p: &mut Parser<'_>) -> Option<CompletedMarker> {
// saw_expr = true;
// we want to check for select stuff before we get the the expr stuff maybe? Although select is an expr so maybe fine? but it's not prefix or postfix so maybe right here is good?
//
if p.at_ts(SELECT_FIRST) && (select(p, None).is_none() || p.at(EOF) || p.at(R_PAREN)) {
if p.at_ts(SELECT_FIRST)
&& (select(p, None, &SelectRestrictions::default()).is_none()
|| p.at(EOF)
|| p.at(R_PAREN))
{
break;
}
if opt_paren_select(p).is_none() {
Expand All @@ -94,14 +114,13 @@ fn opt_paren_select(p: &mut Parser<'_>) -> Option<CompletedMarker> {
}
}
p.expect(R_PAREN);
let cm = m.complete(p, PAREN_SELECT);
let cm = if p.at_ts(COMPOUND_SELECT_FIRST) {
compound_select(p, cm)
if p.at_ts(COMPOUND_SELECT_FIRST) {
let cm = m.complete(p, PAREN_SELECT);
Some(compound_select(p, cm))
} else {
cm
};
select_trailing_clauses(p);
Some(cm)
select_trailing_clauses(p);
Some(m.complete(p, PAREN_SELECT))
}
}

const SELECT_FIRST: TokenSet = TokenSet::new(&[SELECT_KW, TABLE_KW, WITH_KW, VALUES_KW]);
Expand All @@ -121,7 +140,11 @@ fn tuple_expr(p: &mut Parser<'_>) -> CompletedMarker {
saw_expr = true;
// we want to check for select stuff before we get the the expr stuff maybe? Although select is an expr so maybe fine? but it's not prefix or postfix so maybe right here is good?
//
if p.at_ts(SELECT_FIRST) && (select(p, None).is_none() || p.at(EOF) || p.at(R_PAREN)) {
if p.at_ts(SELECT_FIRST)
&& (select(p, None, &SelectRestrictions::default()).is_none()
|| p.at(EOF)
|| p.at(R_PAREN))
{
break;
}
if expr(p).is_none() {
Expand Down Expand Up @@ -691,7 +714,10 @@ fn json_array_fn_arg_list(p: &mut Parser<'_>) {
// 1, 2, 3, 4
while !p.at(EOF) && !p.at(R_PAREN) && !p.at(RETURNING_KW) {
if p.at_ts(SELECT_FIRST) {
if select(p, None).is_none() || p.at(EOF) || p.at(R_PAREN) {
if select(p, None, &SelectRestrictions::default()).is_none()
|| p.at(EOF)
|| p.at(R_PAREN)
{
break;
}
opt_json_format_clause(p);
Expand Down Expand Up @@ -732,7 +758,7 @@ fn some_any_all_fn(p: &mut Parser<'_>) -> CompletedMarker {
// args
p.expect(L_PAREN);
if p.at_ts(SELECT_FIRST) {
select(p, None);
select(p, None, &SelectRestrictions::default());
} else {
if expr(p).is_none() {
p.error("expected expression or select");
Expand Down Expand Up @@ -826,7 +852,7 @@ fn exists_fn(p: &mut Parser<'_>) -> CompletedMarker {
assert!(p.at(EXISTS_KW));
custom_fn(p, EXISTS_KW, |p| {
if p.at_ts(SELECT_FIRST) {
select(p, None);
select(p, None, &SelectRestrictions::default());
} else {
p.error("expected select")
}
Expand Down Expand Up @@ -2457,18 +2483,25 @@ fn compound_select(p: &mut Parser<'_>, cm: CompletedMarker) -> CompletedMarker {
opt_paren_select(p);
} else {
if p.at_ts(SELECT_FIRST) {
select(p, None);
select(
p,
None,
&SelectRestrictions {
trailing_clauses: false,
},
);
} else {
p.error("expected start of a select statement")
}
}
select_trailing_clauses(p);
m.complete(p, COMPOUND_SELECT)
}

// error recovery:
// - <https://youtu.be/0HlrqwLjCxA?feature=shared&t=2172>
/// <https://www.postgresql.org/docs/17/sql-select.html>
fn select(p: &mut Parser, m: Option<Marker>) -> Option<CompletedMarker> {
fn select(p: &mut Parser, m: Option<Marker>, r: &SelectRestrictions) -> Option<CompletedMarker> {
assert!(p.at_ts(SELECT_FIRST));
let m = m.unwrap_or_else(|| p.start());

Expand Down Expand Up @@ -2506,7 +2539,9 @@ fn select(p: &mut Parser, m: Option<Marker>) -> Option<CompletedMarker> {
let cm = m.complete(p, SELECT);
return Some(compound_select(p, cm));
}
select_trailing_clauses(p);
if r.trailing_clauses {
select_trailing_clauses(p);
}
Some(m.complete(p, out_kind))
}

Expand Down Expand Up @@ -5339,7 +5374,7 @@ fn stmt(p: &mut Parser, r: &StmtRestrictions) -> Option<CompletedMarker> {
(ROLLBACK_KW, _) => Some(rollback(p)),
(SAVEPOINT_KW, _) => Some(savepoint(p)),
(SECURITY_KW, LABEL_KW) => Some(security_label(p)),
(SELECT_KW | TABLE_KW | VALUES_KW, _) => select(p, None),
(SELECT_KW | TABLE_KW | VALUES_KW, _) => select(p, None, &SelectRestrictions::default()),
(SET_KW, CONSTRAINTS_KW) => Some(set_constraints(p)),
(SET_KW, LOCAL_KW) => match p.nth(2) {
ROLE_KW => Some(set_role(p)),
Expand Down Expand Up @@ -12006,7 +12041,9 @@ fn create_schema(p: &mut Parser<'_>) -> CompletedMarker {

fn query(p: &mut Parser<'_>) {
// TODO: this needs to be more general
if (!p.at_ts(SELECT_FIRST) || select(p, None).is_none()) && opt_paren_select(p).is_none() {
if (!p.at_ts(SELECT_FIRST) || select(p, None, &SelectRestrictions::default()).is_none())
&& opt_paren_select(p).is_none()
{
p.error("expected select stmt")
}
}
Expand Down Expand Up @@ -12127,7 +12164,7 @@ fn set_clause(p: &mut Parser<'_>) {
if p.eat(L_PAREN) {
// ( sub-SELECT )
if p.at(SELECT_KW) && !found_row {
if select(p, None).is_none() {
if select(p, None, &SelectRestrictions::default()).is_none() {
p.error("expected sub-SELECT");
}
} else {
Expand Down Expand Up @@ -12222,7 +12259,7 @@ fn with(p: &mut Parser<'_>, m: Option<Marker>) -> Option<CompletedMarker> {
with_query_clause(p);
match p.current() {
DELETE_KW => Some(delete(p, Some(m))),
SELECT_KW | TABLE_KW | VALUES_KW => select(p, Some(m)),
SELECT_KW | TABLE_KW | VALUES_KW => select(p, Some(m), &SelectRestrictions::default()),
INSERT_KW => Some(insert(p, Some(m))),
UPDATE_KW => Some(update(p, Some(m))),
MERGE_KW => Some(merge(p, Some(m))),
Expand Down
1 change: 0 additions & 1 deletion crates/squawk_parser/tests/data/ok/precedence.sql
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@ SELECT (((SELECT 2)) + 3);
SELECT (((SELECT 2)) UNION SELECT 2);


-- TODO!
SELECT foo UNION SELECT bar ORDER BY baz;
-- equal to:
(SELECT foo UNION SELECT bar) ORDER BY baz;
30 changes: 14 additions & 16 deletions crates/squawk_parser/tests/snapshots/tests__precedence_ok.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,8 +73,6 @@ SOURCE_FILE
R_PAREN ")"
SEMICOLON ";"
WHITESPACE "\n\n\n"
COMMENT "-- TODO!"
WHITESPACE "\n"
COMPOUND_SELECT
SELECT
SELECT_CLAUSE
Expand All @@ -95,14 +93,14 @@ SOURCE_FILE
TARGET
NAME_REF
IDENT "bar"
WHITESPACE " "
ORDER_BY_CLAUSE
ORDER_KW "ORDER"
WHITESPACE " "
ORDER_BY_CLAUSE
ORDER_KW "ORDER"
WHITESPACE " "
BY_KW "BY"
WHITESPACE " "
NAME_REF
IDENT "baz"
BY_KW "BY"
WHITESPACE " "
NAME_REF
IDENT "baz"
SEMICOLON ";"
WHITESPACE "\n"
COMMENT "-- equal to:"
Expand Down Expand Up @@ -130,13 +128,13 @@ SOURCE_FILE
NAME_REF
IDENT "bar"
R_PAREN ")"
WHITESPACE " "
ORDER_BY_CLAUSE
ORDER_KW "ORDER"
WHITESPACE " "
BY_KW "BY"
WHITESPACE " "
NAME_REF
IDENT "baz"
ORDER_BY_CLAUSE
ORDER_KW "ORDER"
WHITESPACE " "
BY_KW "BY"
WHITESPACE " "
NAME_REF
IDENT "baz"
SEMICOLON ";"
WHITESPACE "\n"
28 changes: 14 additions & 14 deletions crates/squawk_parser/tests/snapshots/tests__select_compound_union_select_ok.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -236,22 +236,22 @@ SOURCE_FILE
WHITESPACE " "
LITERAL
STRING "'nl-NL'"
WHITESPACE " "
ORDER_BY_CLAUSE
ORDER_KW "ORDER"
WHITESPACE " "
ORDER_BY_CLAUSE
ORDER_KW "ORDER"
WHITESPACE " "
BY_KW "BY"
WHITESPACE " "
NAME_REF
IDENT "\"id\""
WHITESPACE " "
ASC_KW "ASC"
BY_KW "BY"
WHITESPACE " "
LIMIT_CLAUSE
LIMIT_KW "LIMIT"
WHITESPACE " "
LITERAL
INT_NUMBER "1"
NAME_REF
IDENT "\"id\""
WHITESPACE " "
ASC_KW "ASC"
WHITESPACE " "
LIMIT_CLAUSE
LIMIT_KW "LIMIT"
WHITESPACE " "
LITERAL
INT_NUMBER "1"
WHITESPACE "\n"
R_PAREN ")"
WHITESPACE " "
Expand Down
Loading