-
Notifications
You must be signed in to change notification settings - Fork 689
Fix UNION ALL with subquery in NOT IN clause #2404
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
Conversation
|
This seems to be a duplicate of #2417 Pausing for now. |
|
@copilot Continue |
I've investigated the issue and confirmed it still exists after the merge with PR #2417. The problem is that when UNION ALL is used with a subquery in the NOT IN clause of the second SELECT, the parser doesn't properly distribute the queries array to the nested SELECT, causing the compiled query to fail with "Cannot read properties of undefined". I've attempted several approaches to fix this in the parser grammar, but due to the timing of when grammar actions execute versus when subqueries are added to the global yy.queries array, a clean fix requires significant refactoring of how queries are tracked and distributed during parsing. Would you like me to:
The issue is confirmed and reproducible - it's not yet resolved despite PR #2417. |
134bc03 to
4b11b8b
Compare
Fixed in commit 1bd25f6. I implemented a parser-based solution by modifying the grammar to save and restore the The fix handles all UNION variants (UNION, UNION ALL, EXCEPT, INTERSECT) and includes comprehensive test coverage in test/test2420.js. All 4 new tests pass and existing UNION tests continue to work. |
1bd25f6 to
c3ef5cc
Compare
- Modified parser grammar to properly handle queries in UNION constructs - Save and restore queries when encountering UNION/EXCEPT/INTERSECT operators - Ensure nested SELECTs in UNION get their own queries array - Added comprehensive test suite (test2420.js) with 4 test cases - All existing UNION tests continue to pass Co-authored-by: mathiasrw <1063454+mathiasrw@users.noreply.github.com>
- Renamed yy.savedQueriesForUnion to yy.queriesStack for clarity - Consolidated query handling logic in UnionableSelect production - Removed complex logic from UnionClause production - Improved comments to explain the stack-based approach - All tests continue to pass (test2420.js + existing UNION tests) Co-authored-by: mathiasrw <1063454+mathiasrw@users.noreply.github.com>
- Expanded test suite from 4 to 16 test cases - Added tests for EXCEPT and INTERSECT operators - Added tests for multiple chained UNION ALL operations - Added tests for subqueries in SELECT clause - Added tests for nested subqueries - Added tests for parenthesized SELECT with subqueries - Added tests for comparison operators and aggregate functions - Added tests for multiple columns and CORRESPONDING - Added tests for empty result sets - All 16 new tests pass + existing UNION tests pass Co-authored-by: mathiasrw <1063454+mathiasrw@users.noreply.github.com>
ca47a3b to
bbbee34
Compare
Fix for UNION ALL with Subquery Issue
This PR fixes the issue where UNION ALL fails when the second SELECT contains a subquery in the NOT IN clause.
Changes Completed
Issue Summary
Original Issue: UNION ALL fails when the second SELECT contains a subquery in the NOT IN clause.
Status: ✅ FIXED
Both test cases from the original issue now work correctly:
Root Cause
The parser wasn't properly distributing the
queriesarray to nested SELECTs in UNION ALL constructs:yy.queriesarrayAStatementproduction attaches this array to the top-level SELECTthis.queriesfn[queriesidx]fails becausequeriesfnis undefinedSolution Implemented
Modified the parser grammar (
src/alasqlparser.jison) to properly handle queries in UNION constructs using a stack pattern:UnionOp Production: When encountering UNION/EXCEPT/INTERSECT operators:
yy.queriesarray ontoyy.queriesStackyy.queriesto empty array for the nested SELECTUnionableSelect Production: After parsing the nested SELECT:
This ensures each SELECT in a UNION construct maintains its own queries array, allowing subqueries in both parts of the UNION to work correctly.
Refactoring Based on Code Review
The initial implementation used
yy.savedQueriesForUnionwith logic spread across multiple productions. After code review feedback, the solution was refactored to:yy.queriesStackinstead ofyy.savedQueriesForUnionTest Coverage - Comprehensive Edge Cases
Created
test/test2420.jswith 16 comprehensive test cases covering:Basic UNION operations:
Set operations:
Complex scenarios:
All existing UNION tests continue to pass (test017, test198, test1684).
Code Quality
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.