Skip to content

Commit 37703c0

Browse files
sbdchdsbdchd
authored
parser: improve error recovery in group by (#695)
1 parent c6e8a1b commit 37703c0

File tree

7 files changed

+447
-56
lines changed

7 files changed

+447
-56
lines changed

‎crates/squawk_parser/src/grammar.rs‎

Lines changed: 39 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -4333,41 +4333,48 @@ fn group_by_list(p: &mut Parser<'_>) {
43334333
// an arbitrary expression formed from input-column values. In case of
43344334
// ambiguity, a GROUP BY name will be interpreted as an input-column name
43354335
// rather than an output column name.
4336+
43364337
while !p.at(EOF) && !p.at(SEMICOLON) {
4337-
group_by_item(p);
4338+
if opt_group_by_item(p).is_none() {
4339+
p.error("expected group by item");
4340+
}
43384341
if !p.eat(COMMA) {
43394342
break;
43404343
}
43414344
}
43424345
}
43434346

4344-
fn group_by_item(p: &mut Parser<'_>) {
4347+
const GROUP_BY_ITEM_FIRST: TokenSet =
4348+
TokenSet::new(&[ROLLUP_KW, CUBE_KW, GROUPING_KW]).union(EXPR_FIRST);
4349+
4350+
fn opt_group_by_item(p: &mut Parser<'_>) -> Option<CompletedMarker> {
4351+
if !p.at_ts(GROUP_BY_ITEM_FIRST) {
4352+
return None;
4353+
}
43454354
let m = p.start();
43464355
let kind = match p.current() {
43474356
ROLLUP_KW => {
43484357
p.bump_any();
4349-
p.expect(L_PAREN);
4350-
if !expr_list(p) {
4351-
p.error("expected expression list");
4352-
};
4353-
p.expect(R_PAREN);
4358+
paren_expr_list(p);
43544359
GROUPING_ROLLUP
43554360
}
43564361
CUBE_KW => {
43574362
p.bump_any();
4358-
p.expect(L_PAREN);
4359-
if !expr_list(p) {
4360-
p.error("expected expression list");
4361-
};
4362-
p.expect(R_PAREN);
4363+
paren_expr_list(p);
43634364
GROUPING_CUBE
43644365
}
43654366
GROUPING_KW if p.nth_at(1, SETS_KW) => {
43664367
p.bump(GROUPING_KW);
43674368
p.bump(SETS_KW);
4368-
p.expect(L_PAREN);
4369-
group_by_list(p);
4370-
p.expect(R_PAREN);
4369+
delimited(
4370+
p,
4371+
L_PAREN,
4372+
R_PAREN,
4373+
COMMA,
4374+
|| "unexpected comma".to_string(),
4375+
GROUP_BY_ITEM_FIRST,
4376+
|p| opt_group_by_item(p).is_some(),
4377+
);
43714378
GROUPING_SETS
43724379
}
43734380
_ => {
@@ -4377,7 +4384,7 @@ fn group_by_item(p: &mut Parser<'_>) {
43774384
GROUPING_EXPR
43784385
}
43794386
};
4380-
m.complete(p, kind);
4387+
Some(m.complete(p, kind))
43814388
}
43824389

43834390
/// <https://www.postgresql.org/docs/17/sql-select.html#SQL-HAVING>
@@ -4575,18 +4582,26 @@ fn opt_all_or_distinct(p: &mut Parser) {
45754582
let m = p.start();
45764583
if p.eat(DISTINCT_KW) {
45774584
if p.eat(ON_KW) {
4578-
p.expect(L_PAREN);
4579-
if !expr_list(p) {
4580-
p.error("expected expression in paren_expr_list");
4581-
}
4582-
p.expect(R_PAREN);
4585+
paren_expr_list(p);
45834586
}
45844587
m.complete(p, DISTINCT_CLAUSE);
45854588
} else {
45864589
m.abandon(p);
45874590
}
45884591
}
45894592

4593+
fn paren_expr_list(p: &mut Parser<'_>) {
4594+
delimited(
4595+
p,
4596+
L_PAREN,
4597+
R_PAREN,
4598+
COMMA,
4599+
|| "unexpected comma".to_string(),
4600+
EXPR_FIRST,
4601+
|p| opt_expr(p).is_some(),
4602+
);
4603+
}
4604+
45904605
/// All keywords
45914606
const COL_LABEL_FIRST: TokenSet = TokenSet::new(&[IDENT])
45924607
.union(UNRESERVED_KEYWORDS)
@@ -4908,25 +4923,13 @@ fn partition_option(p: &mut Parser<'_>) {
49084923
PARTITION_FOR_VALUES_WITH
49094924
// FOR VALUES IN '(' expr_list ')'
49104925
} else if p.eat(IN_KW) {
4911-
p.expect(L_PAREN);
4912-
if !expr_list(p) {
4913-
p.error("expected expr list");
4914-
}
4915-
p.expect(R_PAREN);
4926+
paren_expr_list(p);
49164927
PARTITION_FOR_VALUES_IN
49174928
// FOR VALUES FROM '(' expr_list ')' TO '(' expr_list ')'
49184929
} else if p.eat(FROM_KW) {
4919-
p.expect(L_PAREN);
4920-
if !expr_list(p) {
4921-
p.error("expected expr list");
4922-
}
4923-
p.expect(R_PAREN);
4930+
paren_expr_list(p);
49244931
p.expect(TO_KW);
4925-
p.expect(L_PAREN);
4926-
if !expr_list(p) {
4927-
p.error("expected expr list");
4928-
}
4929-
p.expect(R_PAREN);
4932+
paren_expr_list(p);
49304933
PARTITION_FOR_VALUES_FROM
49314934
} else {
49324935
p.error("expected partition option");

‎crates/squawk_parser/tests/data/err/create_table.sql‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -46,6 +46,14 @@ create unlogged table t (
4646
)
4747
);
4848

49+
create table t partition of u
50+
-- missing some commas
51+
for values from ('2024-01-01' 1) to ('2024-04-01' 5);
52+
53+
create table t partition of u
54+
-- missing some commas
55+
for values in (1 2 3);
56+
4957
-- exclude missing a comma
5058
create table t (
5159
a int,

‎crates/squawk_parser/tests/data/err/select.sql‎

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,17 @@ select a, b c d, e from t;
1313
-- ^ ^ comma missing
1414
-- \-- this is a label
1515

16+
-- distinct on missing a comma
17+
SELECT DISTINCT ON (a b) a, b, c
18+
FROM t
19+
order by a, b desc;
20+
21+
-- group bys with missing commas
22+
select * from t group by rollup (1 2 3);
23+
select * from t group by cube (1 2 3);
24+
select * from u
25+
group by grouping sets((1 2) grouping sets((), grouping sets(())));
26+
1627
-- trailing comma in args
1728
select f(1,);
1829

‎crates/squawk_parser/tests/snapshots/tests__create_table_err.snap‎

Lines changed: 95 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -422,6 +422,95 @@ SOURCE_FILE
422422
R_PAREN ")"
423423
SEMICOLON ";"
424424
WHITESPACE "\n\n"
425+
CREATE_TABLE
426+
CREATE_KW "create"
427+
WHITESPACE " "
428+
TABLE_KW "table"
429+
WHITESPACE " "
430+
PATH
431+
PATH_SEGMENT
432+
NAME
433+
IDENT "t"
434+
WHITESPACE " "
435+
PARTITION_OF
436+
PARTITION_KW "partition"
437+
WHITESPACE " "
438+
OF_KW "of"
439+
WHITESPACE " "
440+
PATH
441+
PATH_SEGMENT
442+
NAME_REF
443+
IDENT "u"
444+
WHITESPACE "\n "
445+
COMMENT "-- missing some commas"
446+
WHITESPACE "\n "
447+
PARTITION_FOR_VALUES_FROM
448+
FOR_KW "for"
449+
WHITESPACE " "
450+
VALUES_KW "values"
451+
WHITESPACE " "
452+
FROM_KW "from"
453+
WHITESPACE " "
454+
L_PAREN "("
455+
LITERAL
456+
STRING "'2024-01-01'"
457+
WHITESPACE " "
458+
LITERAL
459+
INT_NUMBER "1"
460+
R_PAREN ")"
461+
WHITESPACE " "
462+
TO_KW "to"
463+
WHITESPACE " "
464+
L_PAREN "("
465+
LITERAL
466+
STRING "'2024-04-01'"
467+
WHITESPACE " "
468+
LITERAL
469+
INT_NUMBER "5"
470+
R_PAREN ")"
471+
SEMICOLON ";"
472+
WHITESPACE "\n\n"
473+
CREATE_TABLE
474+
CREATE_KW "create"
475+
WHITESPACE " "
476+
TABLE_KW "table"
477+
WHITESPACE " "
478+
PATH
479+
PATH_SEGMENT
480+
NAME
481+
IDENT "t"
482+
WHITESPACE " "
483+
PARTITION_OF
484+
PARTITION_KW "partition"
485+
WHITESPACE " "
486+
OF_KW "of"
487+
WHITESPACE " "
488+
PATH
489+
PATH_SEGMENT
490+
NAME_REF
491+
IDENT "u"
492+
WHITESPACE "\n "
493+
COMMENT "-- missing some commas"
494+
WHITESPACE "\n "
495+
PARTITION_FOR_VALUES_IN
496+
FOR_KW "for"
497+
WHITESPACE " "
498+
VALUES_KW "values"
499+
WHITESPACE " "
500+
IN_KW "in"
501+
WHITESPACE " "
502+
L_PAREN "("
503+
LITERAL
504+
INT_NUMBER "1"
505+
WHITESPACE " "
506+
LITERAL
507+
INT_NUMBER "2"
508+
WHITESPACE " "
509+
LITERAL
510+
INT_NUMBER "3"
511+
R_PAREN ")"
512+
SEMICOLON ";"
513+
WHITESPACE "\n\n"
425514
CREATE_TABLE
426515
COMMENT "-- exclude missing a comma"
427516
WHITESPACE "\n"
@@ -571,5 +660,9 @@ ERROR@198: unexpected comma
571660
ERROR@199: unexpected comma
572661
ERROR@200: unexpected comma
573662
ERROR@201: unexpected comma
574-
ERROR@1021: expected COMMA
575-
ERROR@1063: expected SEMICOLON
663+
ERROR@1011: expected COMMA
664+
ERROR@1032: expected COMMA
665+
ERROR@1116: expected COMMA
666+
ERROR@1119: expected COMMA
667+
ERROR@1226: expected COMMA
668+
ERROR@1268: expected SEMICOLON

0 commit comments

Comments
 (0)