-
Notifications
You must be signed in to change notification settings - Fork 690
Add SQL-99 compliant parenthesized SELECT support in UNION operations #2395
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop
Are you sure you want to change the base?
Conversation
mathiasrw
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pattern 1 regression: SELECT ... UNION ALL SELECT ... ORDER BY ... (ORDER BY after all UNIONs) produces incorrect sort order. Test7 shows [10,20,30] instead of expected [30,20,10] for DESC ordering. Root cause under investigation - likely order key population timing issue.
Is this still an issue?
Yes, this is still an issue. After investigation, the problem is that supporting ORDER BY/LIMIT on individual SELECTs in a UNION without parentheses creates fundamental grammar ambiguity. The parser can't distinguish between:
Standard SQL requires parentheses for the second pattern: I recommend either:
The current implementation has 140+ lines of grammar and execution changes, which is indeed complex for this problem. |
lets provide the ability to do these in parenthesis and adhire to SQL-99. But we also want to provide backwards compatability. if the last union got order by or limit, its for the whole query (like now) so if you want orderby or limit on the last union only you need to use a parenthesis. |
|
@copilot Continue |
Implementation complete (commit b5ef978). Added SQL-99 compliant parenthesized SELECT support:
The original issue can be solved by using parentheses on UNION branches or separating queries. |
Co-authored-by: mathiasrw <[email protected]>
Modified grammar to allow ORDER BY and LIMIT before UNION operations. Added execution logic to apply ORDER BY/LIMIT to first SELECT before combining with UNION. Added comprehensive tests for UNION ALL and UNION with ORDER BY/LIMIT. Co-authored-by: mathiasrw <[email protected]>
…, execution logic needs refinement Grammar changes: - Created SelectForUnion rule for use in pattern 2 (ORDER BY before UNION) - Created UnionClauseWithOrder for pattern 2 - Added second Select alternative supporting pattern 2 - SelectWithoutOrderOrLimit reverted to original (no ORDER BY) for pattern 1 Execution logic: - Added check to detect pattern 2 (unionBranchHasOrder) - Apply ORDER BY/LIMIT before UNION only in pattern 2 - Pattern 1 regression: ORDER BY after UNION not working correctly yet Tests: - test_union_order_limit.js passes for pattern 2 - test7.js fails - ORDER BY after multiple UNIONs not sorting correctly - Need to investigate why ORDER BY DESC produces ASC results in pattern 1 Co-authored-by: mathiasrw <[email protected]>
Co-authored-by: mathiasrw <[email protected]>
b5ef978 to
999cb25
Compare
Replaced repetitive conditional checks for union types (unionallfn, unionfn, exceptfn, intersectfn) with a more elegant Array.some() approach. This reduces code duplication and improves maintainability while preserving the exact same functionality. Before: 13 lines of repetitive OR conditions After: 3 lines using array iteration Co-authored-by: mathiasrw <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel like we should cover more edge cases
Also please make sure to always do a deepstrictequal to the full object you are expecting to be returned.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Enhanced in commit f0e7a23.
Added 6 new comprehensive test cases covering:
- Both SELECTs parenthesized with ORDER BY/LIMIT
- ORDER BY DESC scenarios
- Multiple UNION branches
- EXCEPT and INTERSECT operations
- Backwards compatibility verification
All tests now use assert.deepStrictEqual() to verify complete result objects instead of checking individual properties. All 8 tests pass along with 14 backwards compatibility tests.
- Added 6 new test cases covering more scenarios: * Both SELECTs parenthesized (C) * ORDER BY DESC with LIMIT (D) * Multiple branches with ORDER BY (E) * EXCEPT with parenthesized SELECT (F) * INTERSECT with parenthesized SELECT (G) * Backwards compatibility for ORDER BY after UNION (H) - Replaced partial assertions with assert.deepStrictEqual() for all tests - Tests now verify complete result objects, not just individual properties - Fixed test B to handle UNION's unordered result set - All 8 tests pass, covering various SQL-99 compliant scenarios - Backwards compatibility maintained (test7.js: 14/14 passing) Co-authored-by: mathiasrw <[email protected]>
Support ORDER BY and LIMIT on individual SELECTs in UNION operations
This PR implements SQL-99 compliant syntax for ORDER BY and LIMIT on individual SELECT statements within UNION operations, while maintaining full backwards compatibility.
Recent Changes
Enhanced test coverage in
test/test1965.js(latest commit)assert.deepStrictEqual()to verify complete result objectsRefactored repetitive code in
src/38query.jsunionBranchHasOrdercheck from 13 lines to 3 linesArray.some()to eliminate repetitive conditional patternsParser Grammar (
src/alasqlparser.jison)ParenthesizedSelectstatement type for top-level parenthesized SELECT supportLPAR Select RPARalternatives toUnionClausefor all UNION operationsQuery Execution (
src/38query.js)Backwards Compatibility
✅ All backwards compatibility maintained
SELECT ... UNION ALL SELECT ... ORDER BY ...works exactly as beforeSQL-99 Compliant Feature
✅ Use parentheses for ORDER BY/LIMIT on individual SELECTs in UNION branches
SELECT ... UNION ALL (SELECT ... ORDER BY ... LIMIT ...)Solution for Original Issue
The original issue query can now be written using parentheses on UNION branches:
Test Results
Summary: Clean, maintainable implementation with SQL-99 standard parenthesized SELECT support, comprehensive test coverage, and 100% backwards compatibility.
Original prompt
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.