fix(parser): pass modifier to ExceptOp/MinusOp constructors (#2419)#2420
Conversation
… between iterations EXCEPT ALL/DISTINCT and MINUS ALL/DISTINCT modifiers were silently dropped during parsing because the grammar captured the modifier via SetOperationModifier() but constructed ExceptOp and MinusOp with their no-arg constructors (defaulting to empty string), unlike UnionOp and IntersectOp which correctly received the modifier. Additionally, the modifier variable was not reset between iterations of the set-operation loop, causing modifiers to leak from one operator to the next (e.g., UNION ALL ... EXCEPT would incorrectly make the EXCEPT inherit ALL). Fixes JSQLParser#2419 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Thank you very much for finding and fixing this. Since you are using a LLM assistant: please use Parametrized Unit Tests where possible. And run './gradlew spotlessApply' to fix all violations. Thanks! |
There was a problem hiding this comment.
Pull request overview
This PR fixes parsing/deparsing fidelity for set-operation modifiers by ensuring ALL/DISTINCT captured by the grammar are preserved for EXCEPT and MINUS, and by preventing modifier state from leaking across chained set operations.
Changes:
- Pass the parsed
modifierintoMinusOp(modifier)andExceptOp(modifier)constructors. - Reset
modifierat the start of each set-operation loop iteration to prevent modifier leakage. - Add regression tests covering round-trip behavior and object state for
EXCEPT/MINUSmodifiers, plus leak-prevention cases.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
src/main/jjtree/net/sf/jsqlparser/parser/JSqlParserCC.jjt |
Ensures EXCEPT/MINUS retain ALL/DISTINCT modifiers and avoids modifier leakage between operations. |
src/test/java/net/sf/jsqlparser/statement/select/SetOperationModifierTest.java |
Adds regression coverage for modifier preservation and leakage prevention across chained set operations. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| * parse-toString round-trips for all set operation types: UNION, INTERSECT, | ||
| * EXCEPT, and MINUS. | ||
| * | ||
| * @see <a href="https://github.com/JSQLParser/JSqlParser/issues/2080">#2080</a> |
There was a problem hiding this comment.
The Javadoc references issue #2080, but this PR (and the described regression) is for #2419. Please update the link so future readers can trace the correct bug report.
| * @see <a href="https://github.com/JSQLParser/JSqlParser/issues/2080">#2080</a> | |
| * @see <a href="https://github.com/JSQLParser/JSqlParser/issues/2419">#2419</a> |
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Converted to parameterized tests and ran spotlessApply. Thanks for the quick feedback. |
Summary
modifiervariable between set-operation loop iterations to prevent modifier leaking from one operator to the nextProblem
The grammar in
JSqlParserCC.jjtcaptures the set operation modifier viaSetOperationModifier()but constructsExceptOpandMinusOpwith their no-arg constructors (which default to empty string), discarding the captured modifier.UnionOpandIntersectOpcorrectly receivemodifieras a constructor argument.Additionally, the
modifiervariable was not reset between iterations of the(LOOKAHEAD(2) ...)+loop, so aUNION ALL ... EXCEPTwould incorrectly leak theALLmodifier to theEXCEPT.Fix
modifiertoMinusOp(modifier)andExceptOp(modifier)constructors{ modifier = null; }at the start of each loop iterationTest plan
testExceptAllRoundTrip- verifiesEXCEPT ALLround-trips correctlytestExceptDistinctRoundTrip- verifiesEXCEPT DISTINCTround-trips correctlytestMinusAllRoundTrip- verifiesMINUS ALLround-trips correctlytestMinusDistinctRoundTrip- verifiesMINUS DISTINCTround-trips correctlytestModifierDoesNotLeakFromUnionAllToExcept- verifies modifier doesn't leak across operatorstestModifierDoesNotLeakFromIntersectAllToUnion- verifies modifier doesn't leak across operatorstestMixedModifiersPreserved- verifies mixed modifiers in chained set opstestExceptAllSetOperationObject/testMinusAllSetOperationObject- verifies SetOperation stateFixes #2419
🤖 Generated with Claude Code