fix: recognise set operation quantifiers written in any case - #2614
Merged
manticore-projects merged 1 commit intoSep 12, 2026
Merged
Conversation
The grammar records a set operation's modifier verbatim from the source, so `union all` arrives as "all" while `isAll()` asked whether the modifier contained "ALL". Every spelling but upper case therefore read as an unqualified UNION, silently dropping ALL semantics — and the same for DISTINCT, across UNION, EXCEPT, INTERSECT and MINUS alike. A regression from 5fe938b, which replaced the boolean `all` and `distinct` fields with a single verbatim modifier string. The round-trip tests all spell their SQL in upper case, so none of them noticed. Match the leading keyword case-insensitively instead. The match is anchored at the start of the modifier and must end on a word boundary, so a `CORRESPONDING BY NAME MATCHING(all)` column list is no longer mistaken for the ALL quantifier the way a bare `contains` would. `setAll` and `setDistinct` wrote their keyword whatever they were passed, so `setAll(false)` left an ALL modifier behind. They now honour the argument.
Contributor
|
Thank you much for cleaning this up! |
Contributor
Author
|
I did some testing with 5.4 and noticed this regression as well as another one that I've been working on and am about to wrap up. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
union allparses to aUnionOpthat says it is not an ALL union:Write the same statement in upper case and it answers
true. Every set operation is affected, andDISTINCTthe same way:isAll()isDistinct()... union all ..."all"... UNION ALL ..."ALL"... Union All ..."All"... union distinct ..."distinct"... except all ..."all"... intersect all ..."all"On 5.3 all of these answered correctly, so a consumer reading
isAll()to tellUNIONfromUNION ALLsilently reclassifies half its input after upgrading. I hit this through a static-analysis rule that flagsUNIONwhereUNION ALLwould do: it started flagging statements that already saidall.Where it comes from
5fe938b replaced the
boolean allandboolean distinctfields with a single modifier string, so thatCORRESPONDING BY NAME MATCHING(...)could be carried alongside the quantifier. The grammar records that string verbatim:tk.imagekeeps the source's own casing, while the predicates ask a case-sensitive question:Nothing in the suite spells a set operation in lower case —
SetOperationModifierTest, added for #2419, covers all four operators but every fixture is upper case — so the change read as a pure refactor.The fix
Match the leading keyword case-insensitively. The match is anchored at the start and has to end on a word boundary, which makes it stricter than the
containsit replaces: a column list such asCORRESPONDING BY NAME MATCHING(all)can no longer be mistaken for the quantifier, whichcontains("ALL")would have got wrong too had it been written in upper case.While in there, the setters ignored their argument and wrote the keyword they are named after regardless:
They now honour it.
Round-tripping is left alone
toString()still emits the modifier as written, soselect ... union all ...deparses toSELECT ... UNION all ...where 5.3 producedUNION ALL. That is a visible change from 5.3 and arguably wants normalising too, but it changes output for everyone rather than fixing a wrong answer, so I left it out. Happy to add it here if you would rather the two land together.Testing
14 cases added to
SetOperationModifierTestcovering each quantifier in mixed case, each operator, the unqualified operation, and the setters. Full suite: 6618 tests, 0 failures.