Skip to content
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
45 changes: 45 additions & 0 deletions src/38query.js
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems like wayyy too much code for this problem. Is there a better way to solve this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right - this is too complex. The root issue is that the grammar doesn't support ORDER BY/LIMIT on individual SELECTs in a UNION without parentheses due to parsing ambiguity.

The standard SQL solution is to use parentheses: (SELECT ... ORDER BY ... LIMIT ...) UNION ALL (SELECT ... ORDER BY ... LIMIT ...), but AlaSQL doesn't support parenthesized SELECT statements yet.

A simpler approach would be:

  1. Add support for parenthesized SELECT statements (requires grammar changes but cleaner)
  2. Document that this syntax requires parentheses
  3. Remove the complex two-alternative approach and detection logic

The current ~40 lines in src/38query.js could be reduced to just handling the parenthesized case. Should I implement the parentheses-based solution instead?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should work on supporting parenthesized SELECT statements yet.

Run yarn jison before yarn test when you update the alasqlgrammar.jison

Original file line number Diff line number Diff line change
Expand Up @@ -168,6 +168,51 @@ function queryfn3(query) {
// Remove distinct values
doDistinct(query);

// If we have UNION/UNION ALL/EXCEPT/INTERSECT with ORDER BY/LIMIT before it,
// apply ORDER BY and LIMIT to the first SELECT before combining.
// This handles the pattern: SELECT ... ORDER BY ... LIMIT ... UNION ALL SELECT ... ORDER BY ... LIMIT ...
// We only do this if the UNION branch also has ORDER BY/LIMIT (pattern 2), not if ORDER BY is at the end (pattern 1).
var unionBranchHasOrder = (query.unionallfn && query.unionallfn.query && (query.unionallfn.query.orderfn || query.unionallfn.query.limit)) ||
(query.unionfn && query.unionfn.query && (query.unionfn.query.orderfn || query.unionfn.query.limit)) ||
(query.exceptfn && query.exceptfn.query && (query.exceptfn.query.orderfn || query.exceptfn.query.limit)) ||
(query.intersectfn && query.intersectfn.query && (query.intersectfn.query.orderfn || query.intersectfn.query.limit));

if (unionBranchHasOrder && (query.orderfn || query.limit)) {
// Apply ordering to first SELECT's data
if (query.orderfn) {
// Populate order keys before sorting (needed for UNION queries)
if (query.orderColumns) {
for (var i = 0, ilen = query.data.length; i < ilen; i++) {
for (var idx = 0; idx < query.orderColumns.length; idx++) {
var v = query.orderColumns[idx];
var key = '$$$' + idx;
var r = query.data[i];
if (v instanceof yy.Column && r[v.columnid] !== undefined) {
r[key] = r[v.columnid];
} else if (v instanceof yy.Column) {
r[key] = undefined;
} else {
r[key] = undefined;
}
if (i === 0 && query.removeKeys.indexOf(key) === -1) {
query.removeKeys.push(key);
}
}
}
}
query.data = query.data.sort(query.orderfn);
// Clear orderfn so it doesn't get applied again after UNION
query.orderfn = null;
}
// Apply limit to first SELECT's data
if (query.limit) {
doLimit(query);
// Clear limit so it doesn't get applied again after UNION
query.limit = null;
query.offset = null;
}
}

// UNION / UNION ALL
if (query.unionallfn) {
// TODO Simplify this part of program
Expand Down
29 changes: 29 additions & 0 deletions src/alasqlparser.jison
Original file line number Diff line number Diff line change
Expand Up @@ -431,6 +431,7 @@ Statement
| Reindex
| RenameTable
| Select
| ParenthesizedSelect
| ShowCreateTable
| ShowColumns
| ShowDatabases
Expand Down Expand Up @@ -534,6 +535,13 @@ Select
/* if(yy.queries) $$.queries = yy.queries;
delete yy.queries;
*/ }
| LPAR Select RPAR UnionClause OrderClause LimitClause
{
$$ = $2;
yy.extend($$,$4);
yy.extend($$,$5); yy.extend($$,$6);
if(yy.exists) $$.exists = yy.exists.slice();
}
| SEARCH SearchSelector* IntoClause SearchFrom?
/* SearchLimit? SearchStrategy? SearchTimeout? */
{
Expand All @@ -542,6 +550,11 @@ Select
}
;

ParenthesizedSelect
: LPAR Select RPAR
{ $$ = $2; }
;

SelectWithoutOrderOrLimit
: SelectClause RemoveClause? IntoClause FromClause PivotClause? WhereClause GroupClause UnionClause
{
Expand Down Expand Up @@ -1155,6 +1168,22 @@ UnionClause
{ $$ = {except: $3, corresponding:true} ; }
| INTERSECT CORRESPONDING SelectWithoutOrderOrLimit
{ $$ = {intersect: $3, corresponding:true} ; }
| UNION LPAR Select RPAR
{ $$ = {union: $3} ; }
| UNION ALL LPAR Select RPAR
{ $$ = {unionall: $4} ; }
| EXCEPT LPAR Select RPAR
{ $$ = {except: $3} ; }
| INTERSECT LPAR Select RPAR
{ $$ = {intersect: $3} ; }
| UNION CORRESPONDING LPAR Select RPAR
{ $$ = {union: $4, corresponding:true} ; }
| UNION ALL CORRESPONDING LPAR Select RPAR
{ $$ = {unionall: $5, corresponding:true} ; }
| EXCEPT CORRESPONDING LPAR Select RPAR
{ $$ = {except: $4, corresponding:true} ; }
| INTERSECT CORRESPONDING LPAR Select RPAR
{ $$ = {intersect: $4, corresponding:true} ; }
;

OrderClause
Expand Down
Loading