Skip to content

Conversation

Copy link
Contributor

Copilot AI commented Nov 19, 2025

Fixes the ORDER BY clause handling when used with three or more UNION operations. Previously, the ORDER BY was incorrectly applied to intermediate SELECT statements rather than the final result set.

Changes Made

Grammar Changes (alasqlparser.jison)

  • Swapped the order of UnionClause and OrderClause/LimitClause in the Select rule to align with SQL-99 syntax
  • Created SelectWithoutOrderOrLimit rule to prevent ORDER BY from being nested in union clauses
  • Updated UnionClause to reference SelectWithoutOrderOrLimit instead of Select
  • Regenerated parser with yarn jison to ensure consistency with grammar changes

Execution Logic Updates

  • 40select.js: Modified to preserve top-level orderfn and not override it with union clause order; added orderColumns to query object for proper union handling
  • 38query.js: Added logic to populate order keys for UNION/INTERSECT/EXCEPT operations before sorting to ensure proper comparison during deduplication
  • 424select.js: Modified compileSelect2 and compileSelectGroup2 to skip adding order keys during selectfn execution when union operations are present

Test Coverage (test7.js)

  • Enabled previously skipped tests for the main issue
  • Added 8 additional test cases covering:
    • Plain UNION (without ALL) with ORDER BY
    • UNION with duplicate removal
    • Mixed UNION and UNION ALL operations
    • INTERSECT with ORDER BY
    • EXCEPT with ORDER BY
    • ORDER BY without explicit direction
    • Two UNIONs edge case
    • UNION across 4 actual tables with ORDER BY (demonstrates real-world usage with table data)

Code Quality

  • Fixed code formatting in src/38query.js, src/424select.js, and test/test7.js using Prettier
  • Parser regenerated multiple times to ensure consistency

Testing

  • ✅ All 14 test cases in test7.js pass
  • ✅ Existing test suite passes (test017.js and others)
  • ✅ Code formatting validated with yarn test-format-all
  • ✅ Grammar regenerated successfully with yarn jison
  • ✅ Parser state numbers updated after each regeneration

Example

Before (Incorrect):

SELECT 10 AS a UNION ALL SELECT 20 AS a UNION ALL SELECT 30 AS a ORDER BY a DESC
-- Result: [ { a: 10 }, { a: 30 }, { a: 20 } ] ❌

After (Correct):

SELECT 10 AS a UNION ALL SELECT 20 AS a UNION ALL SELECT 30 AS a ORDER BY a DESC
-- Result: [ { a: 30 }, { a: 20 }, { a: 10 } ] ✅

Real-World Example (4 Tables):

SELECT id, name FROM table1 
UNION ALL SELECT id, name FROM table2 
UNION ALL SELECT id, name FROM table3 
UNION ALL SELECT id, name FROM table4 
ORDER BY id DESC
-- Correctly orders all records from 4 tables in descending order

Fixes #7

Original prompt

This section details on the original issue you should resolve

<issue_title>ORDER BY clause on three or more UNIONS</issue_title>
<issue_description>Now parsing algorithm uses ORDER BY function from the second UNION part, but not last one. To fix this issue I need to rewrite some parsing rules. I think the same issue for INTERSECT and other similar operators.
</issue_description>

<agent_instructions>Please confirm if this is still a bug by adding a test file demonstrating the issue named test7.js

Run the test by running yarn test

If it is still an issue please see if you can fix this.

In case you see the best fix to make alasql aligned with propoer SQL-99 syntax is to change the grammar, then do a small precise change in alasqlparser.jison and then run yarn jison && yarn test</agent_instructions>

Comments on the Issue (you are @copilot in this section)

@agershun Now you can see a problem in Alasql console:

select 10 as a union all select 20 as a union all select 30 as a order by a desc

[See example in Alasql console](http://alasql.org/console?select 10 as a union all select 20 as a union all select 30 as a order by a desc)

@mathiasrw Is this problem solved? @mathiasrw I see this is still an issue - so im just keeping it open...
> alasql -v
0.2.1
> alasql "select 10 as a union all select 20 as a union all select 30 as a order by a desc"
[ { a: 10 }, { a: 30 }, { a: 20 } ]

✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.

Copilot AI and others added 2 commits November 19, 2025 10:03
- Modified alasqlparser.jison to create SelectWithoutOrderOrLimit rule
- Updated UnionClause to reference SelectWithoutOrderOrLimit
- Fixed 40select.js to not override top-level orderfn from union clauses
- Added orderColumns to query object for union handling
- Added logic in 38query.js to populate order keys before sorting for union queries
- Enabled test7.js tests

Co-authored-by: mathiasrw <[email protected]>
@mathiasrw
Copy link
Member

@copilot

Run yarn test-format-all
yarn run v1.22.22
$ prettier --list-different '{.,src,test}/*.{js,ts,json}' || (echo 'Please correct file formatting using yarn format-all and try again.' && exit 1)
src/38query.js
src/424select.js
Please correct file formatting using yarn format-all and try again.
error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.
Error: Process completed with exit code 1.

- Added 7 new test cases (G-M) covering plain UNION, INTERSECT, EXCEPT, and edge cases
- Fixed code formatting in src/38query.js and src/424select.js using prettier
- All 13 test cases in test7.js now pass

Co-authored-by: mathiasrw <[email protected]>
Copy link
Contributor Author

Copilot AI commented Nov 19, 2025

@copilot

Run yarn test-format-all
...

Fixed code formatting in src/38query.js and src/424select.js using prettier. All files now pass yarn test-format-all. See commit f2124ff.

Copilot AI changed the title [WIP] Fix ORDER BY clause handling in multiple UNIONs Fix ORDER BY clause handling in multiple UNIONs Nov 19, 2025
Copilot AI requested a review from mathiasrw November 19, 2025 13:00
Copilot finished work on behalf of mathiasrw November 19, 2025 13:01
@mathiasrw
Copy link
Member

@copilot Please run yarn jison && yarn test and commit if any changes.

@mathiasrw mathiasrw marked this pull request as ready for review November 19, 2025 13:02
Ran `yarn jison` to regenerate src/alasqlparser.js from the grammar file.
All tests continue to pass.

Co-authored-by: mathiasrw <[email protected]>
Copy link
Contributor Author

Copilot AI commented Nov 19, 2025

@copilot Please run yarn jison && yarn test and commit if any changes.

Ran yarn jison && yarn test successfully. The parser was regenerated with minor changes to state numbers. All tests pass. See commit a0b9b0a.

Copilot finished work on behalf of mathiasrw November 19, 2025 13:07
@mathiasrw
Copy link
Member

@copilot Did you see the comments?

Added test case N covering UNION ALL across 4 actual tables with ORDER BY.
This test demonstrates the fix works with real table data, not just literal values.

Co-authored-by: mathiasrw <[email protected]>
Copy link
Contributor Author

Copilot AI commented Nov 19, 2025

@copilot Did you see the comments?

Yes, I've addressed the feedback by adding a test for UNION across 4 tables. See commit 8a75917.

Copilot AI requested a review from mathiasrw November 19, 2025 23:04
Copilot finished work on behalf of mathiasrw November 19, 2025 23:04
@mathiasrw
Copy link
Member

@copilot Please run yarn jison && yarn test and commit if there are any changes.

Ran `yarn jison` to regenerate src/alasqlparser.js from the grammar file.
Parser state numbers updated. All tests continue to pass.

Co-authored-by: mathiasrw <[email protected]>
Copy link
Contributor Author

Copilot AI commented Nov 19, 2025

@copilot Please run yarn jison && yarn test and commit if there are any changes.

Ran yarn jison && yarn test successfully. The parser was regenerated with minor state number changes. All tests pass. See commit e50bce6.

Copilot AI requested a review from mathiasrw November 19, 2025 23:28
Copilot finished work on behalf of mathiasrw November 19, 2025 23:28
@mathiasrw mathiasrw merged commit 5a2538d into develop Nov 20, 2025
20 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

ORDER BY clause on three or more UNIONS

2 participants