Repository navigation
Fuzz expression grouping against PostgreSQL and SQLite - #2839
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2839 +/- ##
=======================================
Coverage 83.16% 83.16%
=======================================
Files 42 42
Lines 36420 36420
Branches 36420 36420
=======================================
Hits 30287 30287
Misses 2636 2636
Partials 3497 3497 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
| tree | ||
| } | ||
|
|
||
| fuzz_target!(|case: Case| { |
There was a problem hiding this comment.
I'd propose this check: without it, Display bugs are reported as grouping errors.
let Ok(original) = pg_query::parse(&sql) else { return };
let Ok(mut statements) = Parser::parse_sql(&PostgreSqlDialect {}, &sql) else { return };
let [statement] = statements.as_mut_slice() else { return };
let printed = statement.to_string();
let Ok(baseline) = pg_query::parse(&printed) else {
panic!("PostgreSQL rejects sqlparser's output\n sql: {sql}\n printed: {printed}");
};
if tree(&original) != tree(&baseline) {
// The tokens changed: a Display bug, or sqlparser parsed a different construct (e.g. a keyword taken as an alias).
panic!("sqlparser reads or prints the statement differently\n sql: {sql}\n printed: {printed}");
}
parenthesize(statement); // Only wraps in `Nested`, no rewrites.
let grouped = statement.to_string();
let regrouped = pg_query::parse(&grouped);
if !regrouped.as_ref().is_ok_and(|r| tree(&baseline) == tree(r)) {
// Same tokens, only the parentheses differ, so this is a real grouping bug.
panic!("sqlparser groups the expression differently\n sql: {sql}\n grouped: {grouped}");
}
There was a problem hiding this comment.
Done as proposed, plus some additional tweaking. I must admit this thing feels like the Pozzo di San Patrizio of bugs
| /// | ||
| /// A custom operator prints as `OPERATOR(name)`, so its name reaches the engine even when the bare text would lex differently. | ||
| /// Booleans print as `TRUE` and `FALSE` like the generated text, since SQLite's constant reuse compares token text. | ||
| pub fn parenthesize(statement: &mut Statement) { |
There was a problem hiding this comment.
It rewrites the statement tree, so let's cover it with tests.
| static DB: Db = Db::open(); | ||
| } | ||
|
|
||
| fn check(db: &Db, case: &Case) { |
There was a problem hiding this comment.
Same as the PostgreSQL target: bytecode depends on how constants are spelled, so 0x1F printing as X'1F' shows up as a grouping difference. The two-step check would help here too: explain(original) vs explain(printed), then explain(printed) vs explain(grouped).
There was a problem hiding this comment.
0x1F printed as X'1F' now fails. TRUE and true compile to different bytecode, so every text has its booleans lowercased before EXPLAIN. The first step also found a real printing bug, 0 NOT NULL + 1 printed as 0 IS NOT NULL + 1.
I'll keep an eye out for others.
|
Thanks @s5dsn-eqee for the PR, I will be merging this now. cc @alamb I might also do a separate PR shutting down the cluster fuzz lite job, when I proposed it I had not realized the state of the crate and I expected bugs to be an exception and not the rule. As it stands, it is functionally useless as it gets stuck in the sea of bugs in main and only burns CI time. |
Several recent PRs, mine and other contributors', showed that the expression tree is often built with wrong operator priorities. I hit the same problem writing a semantic expression evaluator, which kept disagreeing with the engine because of wrong ASTs.
The existing oracle targets only check that sqlparser accepts what PostgreSQL and SQLite accept.
fuzz_postgres_exprandfuzz_sqlite_expralso check the tree: they print it fully parenthesized and fail when the engine reads that text differently from the original (pg_queryparse trees for PostgreSQL,EXPLAINbytecode for SQLite).The inputs are operator chains built from a small hand-written
Grammarof tokens per dialect, so nearly every input is valid SQL and the fuzzing goes into grouping. The flip side is that only those tokens, onlySELECT <expr> FROM tand only four levels of nesting are covered. We will expand it over time, given this is already quite enough for now.This already found many AST errors, so I will open a tracking issue for them, populated automatically with Opus, and plan the fixes once the fuzzer converges.