Fix unparse union all nested distinct - #24455
Conversation
The Union unparser chose its set quantifier from a single query-global QueryBuilder::distinct_union flag. A distinct UNION nested as an input (Distinct(Union)) set that flag and was flattened inline, so the flag leaked to the enclosing UNION ALL and the nested union rendered without parentheses. Both silently rewrote `a UNION ALL (b UNION c)` into a flat, fully-distinct `a UNION b UNION c`. Emit a nested distinct UNION as its own parenthesized subquery, which isolates the flag and forces parentheses so the outer ALL is preserved.
Unparse each UNION branch in its own QueryBuilder and inline it only when it is a bare SELECT; otherwise wrap it in a parenthesized subquery. This also keeps a branch's operand-scoped clauses (ORDER BY / LIMIT / OFFSET / FETCH / WITH) bound to the branch instead of leaking to the enclosing set operation, e.g. 'a UNION ALL (b UNION c LIMIT 1)' no longer collapses to 'a UNION b UNION c LIMIT 1'. Subsumes the adjacent-distinct-union fix.
Replace the panicking expect on the branch QueryBuilder with a match that inlines a branch (Ok(body)) unless it needs its own parenthesized subquery (a set operation, or a branch carrying operand-scoped clauses).
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #24455 +/- ##
==========================================
- Coverage 81.26% 81.25% -0.01%
==========================================
Files 1112 1112
Lines 391578 391598 +20
Branches 391578 391598 +20
==========================================
+ Hits 318197 318201 +4
- Misses 54677 54685 +8
- Partials 18704 18712 +8 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
cc @goldmedal @phillipleblanc wonder if you guys can help look at this since you worked/reviewed the original PR |
neilconway
left a comment
neilconway
left a comment
There was a problem hiding this comment.
Thanks for the bug report and the PR, @Jeadie!
I think this fix is okay as-is, but the approach of using a distinct_union flag on the builder in the first place seems pretty fragile to me. What do you think of this approach instead? https://gist.github.com/neilconway/f956e8ecec36a2979fdb799036dd1ea4
There was a problem hiding this comment.
Thanks for working on this.
This does fix the nested UNION quantifier and branch-scoped clause issues I tested. The relevant DataFusion SQL and unparser tests are also green.
I found one blocking regression around parenthesization and SQLite compatibility. There are also two smaller test/comment suggestions below.
| // clauses stay bound to the branch. | ||
| match branch_query { | ||
| Some(mut branch_query) | ||
| if !matches!(body, SetExpr::Select(_)) |
There was a problem hiding this comment.
I think this wrapping condition is a little too broad. It wraps any non-SetExpr::Select body, including a nested UNION with the same operator and set quantifier as its parent. In that case the parentheses do not change the meaning, and they cause a regression for SQLite.
For example, DataFusion plans a flat query like a UNION ALL b UNION ALL c as a nested Union(Union(a, b), c). Before this change we emit the flat form, but with this change we emit (a UNION ALL b) UNION ALL c. SQLite does not accept a parenthesized compound SELECT in that position. I verified this with SELECT 1 UNION ALL (SELECT 2 UNION ALL SELECT 3), which fails with a syntax error in SQLite.
Could we wrap only when the parentheses are actually needed? Now that each branch has its own QueryBuilder, I think we can safely compute the parent's set_quantifier before the branch loop and treat a nested union with the same operator and quantifier as associative with the parent. Something along these lines:
let associative_with_parent = matches!(
&body,
SetExpr::SetOperation {
op: ast::SetOperator::Union,
set_quantifier: sq,
..
} if *sq == set_quantifier
);
let needs_wrap = !(matches!(body, SetExpr::Select(_)) || associative_with_parent)
|| branch_query
.as_ref()
.is_some_and(|q| q.has_operand_scoped_clauses());That should keep the cases this PR fixes parenthesized, such as a UNION ALL (b UNION c) and branches with their own LIMIT or ORDER BY, while allowing same-op/same-quantifier unions to stay flat. Could we also add a snapshot for the flat a UNION ALL b UNION ALL c case? The current round-trip tests compare plans, so they do not catch this SQL rendering regression.
| ); | ||
| // A branch's own LIMIT must remain bound to that branch, inside the parens. | ||
| roundtrip_statement_with_dialect_helper!( | ||
| sql: "SELECT j1_string FROM j1 UNION ALL (SELECT j2_string FROM j2 UNION SELECT j1_string FROM j1 LIMIT 5)", |
There was a problem hiding this comment.
Optional test hardening: we already test the branch-scoped ORDER BY case semantically through roundtrip_statement, but that does not pin the generated SQL text. It might be useful to add a roundtrip_statement_with_dialect_helper! case for something like ... UNION ALL (b UNION c ORDER BY 1) so we explicitly protect the required parentheses. An OFFSET case could also cover another operand-scoped clause.
| let mut branch_query = Some(QueryBuilder::default()); | ||
| let body = self.select_to_sql_expr(input, &mut branch_query)?; | ||
|
|
||
| // Inline a branch only when it is a plain SELECT with no |
There was a problem hiding this comment.
Minor suggestion: I think this comment repeats quite a bit of the larger comment just above it, as well as part of the has_operand_scoped_clauses doc comment. Could we drop this shorter comment and keep the explanation in one place? The part explaining why sharing the builder is unsound is especially useful and worth keeping.
5 participants
Which issue does this PR close?
What changes are included in this PR?
Changes to address issue & roundtrip regression testing.
Are these changes tested?
In PR.
Are there any user-facing changes?
No.