Skip to content

Commit e7a2693

Browse files
def-claude
andcommitted
sql: parenthesize BETWEEN bounds by their left edge, not right edge
`Parser::parse_between` parses both bounds with `parse_subexpr(Precedence::Like)`, starting fresh with nothing to the bound's left. It therefore walks the bound's *left* spine and stops at the first operator binding at or below `Like`, leaving that operator outside the bound: `x BETWEEN 1 IS NULL AND y` parses `1` as the low bound and then expects `AND` but finds `IS`. `write_between_bound` decided parenthesization with `right_edge`, which ranks the right-closing forms (`IS NULL`, `= ANY (…)`, `IN (…)`, `a <op> ANY/ALL`) as `ATOM` because nothing binds into them *from the right* — exactly the forms whose looseness lives on the left spine. So the printer emitted them bare and the round trip broke (or silently re-associated, e.g. a comparison bound rebinding the whole `BETWEEN` as its left operand). Switch the bound to `left_edge`, the mirror that walks the left spine, which is the edge the parser actually consumes here. The `BETWEEN` subject keeps `right_edge` (the `BETWEEN` keyword prints to its right). As with the rest of the precedence model, parser-produced bounds are wrapped in `Expr::Nested` (ranked `ATOM` on both edges), so the parser and pretty-printer datadriven suites are unchanged — the new parens only appear for ASTs that lost their `Nested`. Tests: extends `between_bound_reparenthesized_after_nested_stripped` in `sqlparser_common.rs` with the right-closing bound forms (`IS NULL`, `= ANY`, `IN`, a boolean bound, and a left-spine operator buried under a tighter top). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
1 parent 05643d5 commit e7a2693

2 files changed

Lines changed: 45 additions & 27 deletions

File tree

src/sql-parser/src/ast/defs/expr.rs

Lines changed: 10 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -607,16 +607,19 @@ fn write_quantified_left<W: fmt::Write, T: AstInfo>(f: &mut AstFormatter<W>, exp
607607
}
608608
}
609609

610-
/// Write `bound` as a `BETWEEN … AND …` bound. The parser parses both bounds at
611-
/// `Precedence::Like`, so a bound exposing a spine at or below `Like` on its
612-
/// right would re-associate out of the `BETWEEN` on reparse (`x BETWEEN a AND b =
613-
/// c` parses as `(x BETWEEN a AND b) = c`); such a bound must be parenthesized.
614-
/// Use [`right_edge`] so the decision follows right-transparent prefixes and
615-
/// nested infixes, not just the top operator. The parser wraps these bounds in
610+
/// Write `bound` as a `BETWEEN … AND …` bound. The parser parses both bounds with
611+
/// `parse_subexpr(Precedence::Like)` (see `Parser::parse_between`), starting fresh
612+
/// with nothing to the bound's left, so it walks the bound's *left spine* and
613+
/// stops at the first operator binding at or below `Like` — leaving that operator
614+
/// outside the bound (`x BETWEEN 1 IS NULL AND y` parses `1` as the bound, then
615+
/// expects `AND` but finds `IS`). A bound is therefore safe bare only when its
616+
/// left edge binds strictly above `Like`; use [`left_edge`] (not [`right_edge`],
617+
/// which closes at `ATOM` for the right-closing `IS NULL`/`= ANY (…)`/`IN (…)`
618+
/// forms whose looseness is on the left). The parser wraps these bounds in
616619
/// `Expr::Nested` (which is `ATOM`, so it prints bare); this re-adds the parens
617620
/// for ASTs where that wrapper is absent.
618621
fn write_between_bound<W: fmt::Write, T: AstInfo>(f: &mut AstFormatter<W>, bound: &Expr<T>) {
619-
let needs_parens = right_edge(bound) <= prec::LIKE;
622+
let needs_parens = left_edge(bound) <= prec::LIKE;
620623
if needs_parens {
621624
f.write_str("(");
622625
f.write_node(bound);

src/sql-parser/tests/sqlparser_common.rs

Lines changed: 35 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -796,26 +796,41 @@ fn between_bound_reparenthesized_after_nested_stripped() {
796796
}
797797
}
798798

799-
// `false BETWEEN x AND (0 = a)` parses with the comparison high bound wrapped
800-
// in `Expr::Nested` (the bound parses at `Like`, above the comparison's
801-
// `Cmp`). Once the `Nested` is stripped, the printer must re-add the parens —
802-
// otherwise it prints `... AND 0 = a`, which reparses as
803-
// `(false BETWEEN x AND 0) = a`.
804-
let mut ast = mz_sql_parser::parser::parse_statements("SELECT false BETWEEN x AND (0 = a)")
805-
.unwrap()
806-
.remove(0)
807-
.ast;
808-
StripNested.visit_statement_mut(&mut ast);
809-
let displayed = ast.to_ast_string_simple();
810-
let mut reparsed = mz_sql_parser::parser::parse_statements(&displayed)
811-
.unwrap()
812-
.remove(0)
813-
.ast;
814-
StripNested.visit_statement_mut(&mut reparsed);
815-
assert_eq!(
816-
ast, reparsed,
817-
"BETWEEN bound display did not round-trip after Nested was stripped; displayed = {displayed:?}"
818-
);
799+
// A `BETWEEN` bound parses with `parse_subexpr(Precedence::Like)`, so a bound
800+
// whose left spine binds at or below `Like` is wrapped in `Expr::Nested` by
801+
// the parser. Once the `Nested` is stripped, the printer must re-add the
802+
// parens, or the bound's leading operator re-associates out of the `BETWEEN`
803+
// (`... AND 0 = a` reparses as `(... BETWEEN ... AND 0) = a`, and
804+
// `... 1 IS NULL AND ...` makes the parser expect `AND` but find `IS`). The
805+
// right-closing forms (`IS NULL`, `= ANY (…)`, `IN (…)`) are the interesting
806+
// cases: their looseness is on the *left* spine, so the decision must use the
807+
// left edge, not the right edge (which closes at `ATOM`).
808+
for sql in [
809+
"SELECT false BETWEEN x AND (0 = a)",
810+
"SELECT y BETWEEN (1 IS NULL) AND z",
811+
"SELECT y BETWEEN x AND (1 IS NULL)",
812+
"SELECT y BETWEEN (a = ANY (ARRAY[b])) AND z",
813+
"SELECT y BETWEEN (a IN (SELECT c FROM t)) AND z",
814+
"SELECT y BETWEEN (a OR b) AND z",
815+
// The left-spine operator is buried under a tighter top (`IN`, `Like`).
816+
"SELECT y BETWEEN (a = ANY (ARRAY[b]) IN (SELECT c FROM t)) AND z",
817+
] {
818+
let mut ast = mz_sql_parser::parser::parse_statements(sql)
819+
.unwrap()
820+
.remove(0)
821+
.ast;
822+
StripNested.visit_statement_mut(&mut ast);
823+
let displayed = ast.to_ast_string_simple();
824+
let mut reparsed = mz_sql_parser::parser::parse_statements(&displayed)
825+
.unwrap()
826+
.remove(0)
827+
.ast;
828+
StripNested.visit_statement_mut(&mut reparsed);
829+
assert_eq!(
830+
ast, reparsed,
831+
"BETWEEN bound display did not round-trip after Nested was stripped: {sql:?} -> {displayed:?}"
832+
);
833+
}
819834
}
820835

821836
#[mz_ore::test]

0 commit comments

Comments
 (0)