Skip to content

Commit 3f73b8a

Browse files
def-claude
andcommitted
sql: parenthesize the IS DISTINCT FROM right-hand side by its left edge
`Parser::parse_is` parses the right-hand side of `IS DISTINCT FROM` with `parse_subexpr(<IS precedence>)`, so a RHS whose left spine binds at or below `IS` (`OR`/`AND`/another `IS`) re-associates out of the `IS` on reparse: `a IS DISTINCT FROM b OR c` parses as `(a IS DISTINCT FROM b) OR c`. The printer emitted the RHS bare through `IsExprConstruct`'s display, with no precedence handling, so a `Nested`-stripped AST drifted. This drift is invisible to a stable-string round trip — `IsExpr(a, DistinctFrom(Or(b,c)))` and `Or(IsExpr(a, DistinctFrom(b)), c)` print to the same string `a IS DISTINCT FROM b OR c` — which is why the round-trip fuzzer never flagged it despite generating the construct. The bug is real: the two ASTs are distinct and the first does not reparse to itself. Handle `DistinctFrom` in the `IsExpr` print arm (where the precedence helpers are in scope) and parenthesize its RHS with `left_edge` (the RHS is parsed fresh at `IS`, walking its left spine), matching the rest of the model. The bare-keyword constructs (`NULL`/`TRUE`/`FALSE`/`UNKNOWN`) still go through `IsExprConstruct`'s display. Parser-produced RHSs are wrapped in `Expr::Nested` (`ATOM`), so the datadriven suites are unchanged. Tests: adds `is_distinct_from_rhs_reparenthesized_after_nested_stripped` to `sqlparser_common.rs`, which checks the round trip structurally (the stable-string collision makes a string check insufficient). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
1 parent e7a2693 commit 3f73b8a

2 files changed

Lines changed: 60 additions & 1 deletion

File tree

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

Lines changed: 11 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -246,7 +246,17 @@ impl<T: AstInfo> AstDisplay for Expr<T> {
246246
if *negated {
247247
f.write_str("NOT ");
248248
}
249-
f.write_node(construct);
249+
// `IS DISTINCT FROM <rhs>` parses the RHS at the `IS` precedence
250+
// (see `Parser::parse_is`), so a RHS whose left spine binds at or
251+
// below `IS` re-associates out of the `IS` unless parenthesized
252+
// (`a IS DISTINCT FROM b OR c` is `(a IS DISTINCT FROM b) OR c`).
253+
// The other constructs (`NULL`/`TRUE`/…) are bare keywords.
254+
if let IsExprConstruct::DistinctFrom(rhs) = construct {
255+
f.write_str("DISTINCT FROM ");
256+
write_binary_operand(f, rhs, left_edge(rhs) <= prec::IS);
257+
} else {
258+
f.write_node(construct);
259+
}
250260
}
251261
Expr::InList {
252262
expr,

src/sql-parser/tests/sqlparser_common.rs

Lines changed: 49 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -891,3 +891,52 @@ fn binary_op_operand_reparenthesized_after_nested_stripped() {
891891
);
892892
}
893893
}
894+
895+
#[mz_ore::test]
896+
fn is_distinct_from_rhs_reparenthesized_after_nested_stripped() {
897+
use mz_sql_parser::ast::display::AstDisplay;
898+
use mz_sql_parser::ast::visit_mut::{self, VisitMut};
899+
use mz_sql_parser::ast::{AstInfo, Expr};
900+
901+
struct StripNested;
902+
impl<'a, T: AstInfo> VisitMut<'a, T> for StripNested {
903+
fn visit_expr_mut(&mut self, e: &'a mut Expr<T>) {
904+
visit_mut::visit_expr_mut(self, e);
905+
if let Expr::Nested(inner) = e {
906+
*e = (**inner).clone();
907+
}
908+
}
909+
}
910+
911+
// `IS DISTINCT FROM` parses its right-hand side at the `IS` precedence (see
912+
// `Parser::parse_is`), so a RHS whose left spine binds at or below `IS`
913+
// (`OR`/`AND`/`IS`) is wrapped in `Expr::Nested`. Once stripped, the printer
914+
// must re-add the parens — otherwise `a IS DISTINCT FROM b OR c` reparses as
915+
// `(a IS DISTINCT FROM b) OR c`. This drift is invisible to a stable-string
916+
// round trip (both ASTs print to the same string), so it is checked
917+
// structurally here.
918+
for sql in [
919+
"SELECT a IS DISTINCT FROM (b OR c)",
920+
"SELECT a IS DISTINCT FROM (b AND c)",
921+
"SELECT a IS DISTINCT FROM (b IS DISTINCT FROM c)",
922+
"SELECT a IS DISTINCT FROM (b IS NULL)",
923+
// A comparison RHS binds tighter than `IS`, so it stays bare.
924+
"SELECT a IS DISTINCT FROM b = c",
925+
] {
926+
let mut ast = mz_sql_parser::parser::parse_statements(sql)
927+
.unwrap()
928+
.remove(0)
929+
.ast;
930+
StripNested.visit_statement_mut(&mut ast);
931+
let displayed = ast.to_ast_string_simple();
932+
let mut reparsed = mz_sql_parser::parser::parse_statements(&displayed)
933+
.unwrap()
934+
.remove(0)
935+
.ast;
936+
StripNested.visit_statement_mut(&mut reparsed);
937+
assert_eq!(
938+
ast, reparsed,
939+
"IS DISTINCT FROM RHS display did not round-trip after Nested was stripped: {sql:?} -> {displayed:?}"
940+
);
941+
}
942+
}

0 commit comments

Comments
 (0)